Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

chakrashim: Enable new Get/Has/Set etc. Property from String #422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chakrashim: Enable new Get/Has/Set etc. Property from String #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deps/chakrashim/src/jsrtcontextshim.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ bool ContextShim::InitializeBuiltIns() {
return false;
}

if (DefineProperty(globalObject,
if (DefinePropertyById(globalObject,
GetIsolateShim()->GetKeepAliveObjectSymbolPropertyIdRef(),
PropertyDescriptorOptionValues::False,
PropertyDescriptorOptionValues::False,
Expand Down
59 changes: 32 additions & 27 deletions deps/chakrashim/src/jsrtutils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ JsErrorCode UintToValue(uint32_t value, JsValueRef* result) {
JsErrorCode GetProperty(JsValueRef ref,
JsValueRef propName,
JsValueRef *result) {
JsPropertyIdRef idRef = JS_INVALID_REFERENCE;
IfJsErrorRet(GetPropertyIdFromName(propName, &idRef));
IfJsErrorRet(JsGetProperty(ref, idRef, result));
IfJsErrorRet(JsObjectGetProperty(ref, propName, result));

return JsNoError;
}
Expand Down Expand Up @@ -113,9 +111,7 @@ JsErrorCode SetProperty(JsValueRef ref,
JsErrorCode SetProperty(JsValueRef ref,
JsValueRef propName,
JsValueRef propValue) {
JsPropertyIdRef idRef = JS_INVALID_REFERENCE;
IfJsErrorRet(GetPropertyIdFromName(propName, &idRef));
IfJsErrorRet(JsSetProperty(ref, idRef, propValue, false));
IfJsErrorRet(JsObjectSetProperty(ref, propName, propValue, false));

return JsNoError;
}
Expand Down Expand Up @@ -171,9 +167,7 @@ JsErrorCode SetProperty(JsValueRef ref,
JsErrorCode DeleteProperty(JsValueRef ref,
JsValueRef propName,
JsValueRef* result) {
JsPropertyIdRef idRef = JS_INVALID_REFERENCE;
IfJsErrorRet(GetPropertyIdFromName(propName, &idRef));
IfJsErrorRet(JsDeleteProperty(ref, idRef, false, result));
IfJsErrorRet(JsObjectDeleteProperty(ref, propName, false, result));

return JsNoError;
}
Expand Down Expand Up @@ -292,11 +286,8 @@ JsErrorCode HasOwnProperty(JsValueRef object,

*result = JS_INVALID_REFERENCE;

JsPropertyIdRef propId = JS_INVALID_REFERENCE;
IfJsErrorRet(GetPropertyIdFromValue(prop, &propId));

bool hasOwnProperty = false;
IfJsErrorRet(JsHasOwnProperty(object, propId, &hasOwnProperty));
IfJsErrorRet(JsObjectHasOwnProperty(object, prop, &hasOwnProperty));
IfJsErrorRet(JsBoolToBoolean(hasOwnProperty, result));

return JsNoError;
Expand All @@ -305,10 +296,7 @@ JsErrorCode HasOwnProperty(JsValueRef object,
JsErrorCode GetOwnPropertyDescriptor(JsValueRef ref,
JsValueRef prop,
JsValueRef* result) {
JsPropertyIdRef idRef = JS_INVALID_REFERENCE;
IfJsErrorRet(GetPropertyIdFromName(prop, &idRef));

return JsGetOwnPropertyDescriptor(ref, idRef, result);
return JsObjectGetOwnPropertyDescriptor(ref, prop, result);
}

JsErrorCode IsZero(JsValueRef value,
Expand Down Expand Up @@ -399,7 +387,7 @@ JsErrorCode AddExternalData(JsValueRef ref,
IfJsErrorRet(JsCreateExternalObject(data, onObjectFinalize,
&externalObjectRef));

IfJsErrorRet(DefineProperty(ref,
IfJsErrorRet(DefinePropertyById(ref,
externalDataPropertyId,
PropertyDescriptorOptionValues::False,
PropertyDescriptorOptionValues::False,
Expand Down Expand Up @@ -655,7 +643,7 @@ JsErrorCode CreateV8PropertyDescriptor(
return JsNoError;
}

JsErrorCode DefineProperty(JsValueRef object,
JsErrorCode DefinePropertyById(JsValueRef object,
JsPropertyIdRef propertyIdRef,
PropertyDescriptorOptionValues writable,
PropertyDescriptorOptionValues enumerable,
Expand All @@ -677,6 +665,28 @@ JsErrorCode DefineProperty(JsValueRef object,
return JsNoError;
}

JsErrorCode DefinePropertyByName(JsValueRef object,
JsValueRef propertyName,
PropertyDescriptorOptionValues writable,
PropertyDescriptorOptionValues enumerable,
PropertyDescriptorOptionValues configurable,
JsValueRef value,
JsValueRef getter,
JsValueRef setter) {
JsValueRef descriptor = JS_INVALID_REFERENCE;
IfJsErrorRet(CreatePropertyDescriptor(writable, enumerable, configurable,
value, getter, setter, &descriptor));

bool result = false;
IfJsErrorRet(JsObjectDefineProperty(object, propertyName, descriptor, &result));

if (!result) {
return JsErrorInvalidArgument;
}

return JsNoError;
}

JsErrorCode GetPropertyIdFromName(JsValueRef nameRef,
JsPropertyIdRef *idRef) {
StringUtf8 str;
Expand Down Expand Up @@ -751,9 +761,7 @@ JsErrorCode DeleteIndexedProperty(JsValueRef object,
JsErrorCode HasProperty(JsValueRef object,
JsValueRef propName,
bool *result) {
JsPropertyIdRef idRef = JS_INVALID_REFERENCE;
IfJsErrorRet(GetPropertyIdFromName(propName, &idRef));
IfJsErrorRet(JsHasProperty(object, idRef, result));
IfJsErrorRet(JsObjectHasProperty(object, propName, result));

return JsNoError;
}
Expand Down Expand Up @@ -884,9 +892,6 @@ JsErrorCode GetPrivate(JsValueRef object, JsValueRef key,
return JsNoError;
}

JsPropertyIdRef keyIdRef = JS_INVALID_REFERENCE;
IfJsErrorRet(GetPropertyIdFromName(key, &keyIdRef));

// Is 'key' present in hiddenValuesTable? If not, return undefined
JsValueRef hasPropertyRef = JS_INVALID_REFERENCE;
IfJsErrorRet(HasOwnProperty(hiddenValuesTable, key, &hasPropertyRef));
Expand All @@ -899,7 +904,7 @@ JsErrorCode GetPrivate(JsValueRef object, JsValueRef key,
return JsNoError;
}

IfJsErrorRet(JsGetProperty(hiddenValuesTable, keyIdRef, result));
IfJsErrorRet(JsObjectGetProperty(hiddenValuesTable, key, result));

return JsNoError;
}
Expand All @@ -917,7 +922,7 @@ JsErrorCode SetPrivate(JsValueRef object, JsValueRef key,
if (isUndefined) {
IfJsErrorRet(JsCreateObject(&hiddenValuesTable));

IfJsErrorRet(DefineProperty(object, hiddenValuesIdRef,
IfJsErrorRet(DefinePropertyById(object, hiddenValuesIdRef,
PropertyDescriptorOptionValues::False,
PropertyDescriptorOptionValues::False,
PropertyDescriptorOptionValues::False,
Expand Down
8 changes: 4 additions & 4 deletions deps/chakrashim/src/jsrtutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,17 +351,17 @@ JsErrorCode CreatePropertyDescriptor(v8::PropertyAttribute attributes,
JsErrorCode CreateV8PropertyDescriptor(JsValueRef descriptor,
v8::PropertyDescriptor* result);

JsErrorCode DefineProperty(JsValueRef object,
const char * propertyName,
JsErrorCode DefinePropertyById(JsValueRef object,
JsPropertyIdRef propertyIdRef,
PropertyDescriptorOptionValues writable,
PropertyDescriptorOptionValues enumerable,
PropertyDescriptorOptionValues configurable,
JsValueRef value,
JsValueRef getter,
JsValueRef setter);

JsErrorCode DefineProperty(JsValueRef object,
JsPropertyIdRef propertyIdRef,
JsErrorCode DefinePropertyByName(JsValueRef object,
JsValueRef propertyName,
PropertyDescriptorOptionValues writable,
PropertyDescriptorOptionValues enumerable,
PropertyDescriptorOptionValues configurable,
Expand Down
2 changes: 1 addition & 1 deletion deps/chakrashim/src/v8external.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ Local<External> External::New(Isolate* isolate, void* value) {

IsolateShim* iso = IsolateShim::FromIsolate(isolate);
JsValueRef trueRef = iso->GetCurrentContextShim()->GetTrue();
if (jsrt::DefineProperty(externalRef,
if (jsrt::DefinePropertyById(externalRef,
iso->GetCachedSymbolPropertyIdRef(
jsrt::CachedSymbolPropertyIdRef::__isexternal__),
jsrt::PropertyDescriptorOptionValues::False,
Expand Down
4 changes: 2 additions & 2 deletions deps/chakrashim/src/v8function.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace v8 {
using jsrt::IsolateShim;
using jsrt::ContextShim;
using jsrt::PropertyDescriptorOptionValues;
using jsrt::DefineProperty;
using jsrt::DefinePropertyById;

MaybeLocal<Function> Function::New(Local<Context> context,
FunctionCallback callback,
Expand Down Expand Up @@ -106,7 +106,7 @@ Local<Value> Function::Call(Handle<Value> recv,
}

void Function::SetName(Handle<String> name) {
JsErrorCode error = jsrt::DefineProperty((JsValueRef)this,
JsErrorCode error = jsrt::DefinePropertyById((JsValueRef)this,
IsolateShim::GetCurrent()->GetCachedPropertyIdRef(
jsrt::CachedPropertyIdRef::name),
PropertyDescriptorOptionValues::False,
Expand Down
4 changes: 2 additions & 2 deletions deps/chakrashim/src/v8functiontemplate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ class FunctionTemplateData : public TemplateData {
}

if (!className.IsEmpty()) {
if (jsrt::DefineProperty(*prototype,
if (jsrt::DefinePropertyById(*prototype,
iso->GetToStringTagSymbolPropertyIdRef(),
jsrt::PropertyDescriptorOptionValues::True, /* writable */
jsrt::PropertyDescriptorOptionValues::False, /* enumerable */
Expand All @@ -289,7 +289,7 @@ class FunctionTemplateData : public TemplateData {
}
}

if (jsrt::DefineProperty(*prototype,
if (jsrt::DefinePropertyById(*prototype,
iso->GetCachedPropertyIdRef(
jsrt::CachedPropertyIdRef::constructor),
jsrt::PropertyDescriptorOptionValues::True, /* writable */
Expand Down
61 changes: 16 additions & 45 deletions deps/chakrashim/src/v8object.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,11 @@ bool Object::Set(Handle<Value> key, Handle<Value> value) {

Maybe<bool> Object::Set(Handle<Value> key, Handle<Value> value,
PropertyAttribute attribs, bool force) {
JsPropertyIdRef idRef;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this idRef is used twice, is the overhead of invoking GetOrAddPropertyRecord twice (once per JsObject*Property method) smaller than the additional JSRT calls involved in fetching the id ref up front?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, not if it's literal or prop string. considering createString will create literalstring and presumably user will call set via a string variable that was also created on native land...


if (jsrt::GetPropertyIdFromValue((JsValueRef)*key, &idRef) != JsNoError) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this code path worked for Symbols as well as strings; do you know if that is ever used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it also tried to work for all values, and would stringify things that weren't strings or symbols.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider symbol is there. I'm not sure if we should! support other types though.

return Nothing<bool>();
}

// Do it faster if there are no property attributes
if (!force && attribs == None) {
if (JsSetProperty((JsValueRef)this,
idRef, (JsValueRef)*value, false) != JsNoError) {
if (JsObjectSetProperty((JsValueRef)this,
(JsValueRef)*key, (JsValueRef)*value,
false) != JsNoError) {
return Nothing<bool>();
}
} else { // we have attributes just use it
Expand All @@ -100,8 +95,8 @@ Maybe<bool> Object::Set(Handle<Value> key, Handle<Value> value,
configurable = jsrt::PropertyDescriptorOptionValues::True;
}

if (jsrt::DefineProperty((JsValueRef)this,
idRef,
if (jsrt::DefinePropertyByName((JsValueRef)this,
(JsValueRef)*key,
writable,
enumerable,
configurable,
Expand Down Expand Up @@ -134,12 +129,6 @@ Maybe<bool> Object::DefineOwnProperty(
Maybe<bool> Object::DefineProperty(Local<v8::Context> context,
Local<Name> key,
PropertyDescriptor& descriptor) {
JsPropertyIdRef idRef;

if (jsrt::GetPropertyIdFromValue((JsValueRef)*key, &idRef) != JsNoError) {
return Nothing<bool>();
}

Local<Value> value = descriptor.value();
Local<Value> get = descriptor.get();
Local<Value> set = descriptor.set();
Expand All @@ -149,8 +138,9 @@ Maybe<bool> Object::DefineProperty(Local<v8::Context> context,
!descriptor.has_enumerable() &&
!descriptor.has_configurable() &&
!descriptor.has_writable()) {
if (JsSetProperty((JsValueRef)this,
idRef, (JsValueRef)*value, false) != JsNoError) {
if (JsObjectSetProperty((JsValueRef)this,
(JsValueRef)*key, (JsValueRef)*value,
false) != JsNoError) {
return Nothing<bool>();
}
} else { // we have attributes just use it
Expand Down Expand Up @@ -179,8 +169,8 @@ Maybe<bool> Object::DefineProperty(Local<v8::Context> context,
jsrt::PropertyDescriptorOptionValues::False;
}

if (jsrt::DefineProperty((JsValueRef)this,
idRef,
if (jsrt::DefinePropertyByName((JsValueRef)this,
(JsValueRef)*key,
writable,
enumerable,
configurable,
Expand Down Expand Up @@ -280,13 +270,8 @@ Local<Value> Object::GetOwnPropertyDescriptor(Local<Name> key) {
}

Maybe<bool> Object::Has(Local<Context> context, Local<Value> key) {
JsPropertyIdRef idRef;
if (jsrt::GetPropertyIdFromValue((JsValueRef)*key, &idRef) != JsNoError) {
return Nothing<bool>();
}

bool result;
if (JsHasProperty(this, idRef, &result) != JsNoError) {
if (JsHasProperty(this, (JsValueRef)*key, &result) != JsNoError) {
return Nothing<bool>();
}

Expand All @@ -298,13 +283,8 @@ bool Object::Has(Handle<Value> key) {
}

Maybe<bool> Object::Delete(Local<Context> context, Local<Value> key) {
JsPropertyIdRef idRef;
if (jsrt::GetPropertyIdFromValue((JsValueRef)*key, &idRef) != JsNoError) {
return Nothing<bool>();
}

JsValueRef result;
if (JsDeleteProperty(this, idRef, false, &result) != JsNoError) {
if (JsDeleteProperty(this, (JsValueRef)*key, false, &result) != JsNoError) {
return Nothing<bool>();
}

Expand Down Expand Up @@ -356,11 +336,6 @@ Maybe<bool> Object::SetAccessor(Handle<Name> name,
JsValueRef getterRef = JS_INVALID_REFERENCE;
JsValueRef setterRef = JS_INVALID_REFERENCE;

JsPropertyIdRef idRef;
if (jsrt::GetPropertyIdFromName((JsValueRef)*name, &idRef) != JsNoError) {
return Nothing<bool>();
}

if (getter != nullptr) {
AccessorExternalData *externalData = new AccessorExternalData();
externalData->type = Getter;
Expand Down Expand Up @@ -400,8 +375,8 @@ Maybe<bool> Object::SetAccessor(Handle<Name> name,

// CHAKRA-TODO: we ignore AccessControl for now..

if (jsrt::DefineProperty((JsValueRef)this,
idRef,
if (jsrt::DefinePropertyByName((JsValueRef)this,
(JsValueRef)*name,
jsrt::PropertyDescriptorOptionValues::None,
enumerable,
configurable,
Expand Down Expand Up @@ -828,10 +803,6 @@ void Object::SetAccessorProperty(Local<Name> name, Local<Function> getter,
Local<Function> setter,
PropertyAttribute attribute,
AccessControl settings) {
JsPropertyIdRef idRef;
if (jsrt::GetPropertyIdFromName((JsValueRef)*name, &idRef) != JsNoError) {
return;
}

jsrt::PropertyDescriptorOptionValues enumerable =
jsrt::GetPropertyDescriptorOptionValue(!(attribute & DontEnum));
Expand All @@ -840,8 +811,8 @@ void Object::SetAccessorProperty(Local<Name> name, Local<Function> getter,

// CHAKRA-TODO: we ignore AccessControl for now..

if (jsrt::DefineProperty((JsValueRef)this,
idRef,
if (jsrt::DefinePropertyByName((JsValueRef)this,
(JsValueRef)*name,
jsrt::PropertyDescriptorOptionValues::None,
enumerable,
configurable,
Expand Down
8 changes: 1 addition & 7 deletions deps/chakrashim/src/v8objecttemplate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,14 +780,8 @@ JsValueRef CHAKRA_CALLBACK Utils::DefinePropertyCallback(
}

if (result == JS_INVALID_REFERENCE) {
// No interception took place; fall back to default behavior
JsPropertyIdRef propertyIdRef;
if (jsrt::GetPropertyIdFromName(prop, &propertyIdRef) != JsNoError) {
return jsrt::GetFalse();
}

bool result = false;
if (JsDefineProperty(object, propertyIdRef, descriptor, &result)
if (JsObjectDefineProperty(object, prop, descriptor, &result)
Copy link
Contributor

@MSLaguana MSLaguana Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this isn't quite right; Right now this code path isn't going to be used until we fix our handling of vm.runInContext, but when it is used it is perfectly allowable for prop to be a Symbol, which would not be handled with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, Symbol support should be in master no time.

!= JsNoError || !result) {
return jsrt::GetFalse();
}
Expand Down