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
Changes from all commits
Commits
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
chakrashim: Enable new Get/Has/Set etc. Property from String
obastemur committed Nov 7, 2017
commit 7a4ba202e8062766506f30afd83345678accf6f0
2 changes: 1 addition & 1 deletion deps/chakrashim/src/jsrtcontextshim.cc
Original file line number Diff line number Diff line change
@@ -292,7 +292,7 @@ bool ContextShim::InitializeBuiltIns() {
return false;
}

if (DefineProperty(globalObject,
if (DefinePropertyById(globalObject,
GetIsolateShim()->GetKeepAliveObjectSymbolPropertyIdRef(),
PropertyDescriptorOptionValues::False,
PropertyDescriptorOptionValues::False,
59 changes: 32 additions & 27 deletions deps/chakrashim/src/jsrtutils.cc
Original file line number Diff line number Diff line change
@@ -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;
}
@@ -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;
}
@@ -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;
}
@@ -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;
@@ -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,
@@ -399,7 +387,7 @@ JsErrorCode AddExternalData(JsValueRef ref,
IfJsErrorRet(JsCreateExternalObject(data, onObjectFinalize,
&externalObjectRef));

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

JsErrorCode DefineProperty(JsValueRef object,
JsErrorCode DefinePropertyById(JsValueRef object,
JsPropertyIdRef propertyIdRef,
PropertyDescriptorOptionValues writable,
PropertyDescriptorOptionValues enumerable,
@@ -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;
@@ -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;
}
@@ -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));
@@ -899,7 +904,7 @@ JsErrorCode GetPrivate(JsValueRef object, JsValueRef key,
return JsNoError;
}

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

return JsNoError;
}
@@ -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,
8 changes: 4 additions & 4 deletions deps/chakrashim/src/jsrtutils.h
Original file line number Diff line number Diff line change
@@ -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,
2 changes: 1 addition & 1 deletion deps/chakrashim/src/v8external.cc
Original file line number Diff line number Diff line change
@@ -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,
4 changes: 2 additions & 2 deletions deps/chakrashim/src/v8function.cc
Original file line number Diff line number Diff line change
@@ -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,
@@ -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,
4 changes: 2 additions & 2 deletions deps/chakrashim/src/v8functiontemplate.cc
Original file line number Diff line number Diff line change
@@ -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 */
@@ -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 */
61 changes: 16 additions & 45 deletions deps/chakrashim/src/v8object.cc
Original file line number Diff line number Diff line change
@@ -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
@@ -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,
@@ -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();
@@ -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
@@ -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,
@@ -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>();
}

@@ -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>();
}

@@ -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;
@@ -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,
@@ -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));
@@ -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,
8 changes: 1 addition & 7 deletions deps/chakrashim/src/v8objecttemplate.cc
Original file line number Diff line number Diff line change
@@ -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

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();
}