Bug 77295

Created attachment 124462 [details] Proof of concept patch

Comment on attachment 124462 [details] Proof of concept patch Came up while looking at the history.state stuff, the JSC generator generates code for [CachedAttribute] SerializedScriptValue attributes. Rough patch as a possible solution, it probably needs more work to handle more [InitializedByConstructor] attributes (and i'm not sure hijacking that extended-attribute is even a good idea), and i haven't looked into the setters case. I tried going with ForceSet() instead of using hidden attributes but i didn't find a way to reset the cached value in that case (would probably need to set the accessor function again, but it's internal to the v8 generated file), and a way to invalidate the value is needed for the history.state thing. Ideas/comments welcome.

(In reply to comment #2) > (From update of attachment 124462 [details]) > [...] > I tried going with ForceSet() instead of using hidden attributes but i didn't find a way to reset the cached value in that case (would probably need to set the accessor function again, but it's internal to the v8 generated file), and a way to invalidate the value is needed for the history.state thing. > s/hidden attributes/hidden values/

Comment on attachment 124462 [details] Proof of concept patch Attachment 124462 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11369253

Comment on attachment 124462 [details] Proof of concept patch View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review > Source/WebCore/Modules/intents/Intent.idl:34 > + readonly attribute [InitializedByConstructor] SerializedScriptValue data; You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later. Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler.

(In reply to comment #5) > (From update of attachment 124462 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review > > > Source/WebCore/Modules/intents/Intent.idl:34 > > + readonly attribute [InitializedByConstructor] SerializedScriptValue data; > > You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later. > > Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler. Yeah, [InitializedByConstructor] is wrongly used here, that's what i meant when i said i was hijacking the meaning, but i was somehow trying to preserve the behavior introduced by https://bugs.webkit.org/show_bug.cgi?id=36892 and https://bugs.webkit.org/show_bug.cgi?id=75641 What i'm trying to do is something more akin to what the JSC generator does for SerializedScriptValue attributes. It generates getters/setters that deserialize the value when they're called (and cache the deserialized value in a member in the object if [CachedAttribute] is specified), which is not done in the v8 generator. Instead there's code to deserialize one (and only one) SerializedScriptValue attribute upfront in the constructor and ForceSet:tting it in the object, avoiding the generation and use of getters/setters. If i understood things correctly this was initially set up like that because MessageEvent needed the data deserialized in the right context (which presumably would not be the correct one if deserialized lazily in a getter?). It looks like the webintents case would be handled correctly. Do you think we can do something to not special case what MessageEvent needs so that we can generate the getter/setter functions and do everything the same way?

Comment on attachment 124462 [details] Proof of concept patch View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review >>> Source/WebCore/Modules/intents/Intent.idl:34 >>> + readonly attribute [InitializedByConstructor] SerializedScriptValue data; >> >> You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later. >> >> Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler. > > Yeah, [InitializedByConstructor] is wrongly used here, that's what i meant when i said i was hijacking the meaning, but i was somehow trying to preserve the behavior introduced by https://bugs.webkit.org/show_bug.cgi?id=36892 and https://bugs.webkit.org/show_bug.cgi?id=75641 > > What i'm trying to do is something more akin to what the JSC generator does for SerializedScriptValue attributes. It generates getters/setters that deserialize the value when they're called (and cache the deserialized value in a member in the object if [CachedAttribute] is specified), which is not done in the v8 generator. > Instead there's code to deserialize one (and only one) SerializedScriptValue attribute upfront in the constructor and ForceSet:tting it in the object, avoiding the generation and use of getters/setters. > If i understood things correctly this was initially set up like that because MessageEvent needed the data deserialized in the right context (which presumably would not be the correct one if deserialized lazily in a getter?). It looks like the webintents case would be handled correctly. > > Do you think we can do something to not special case what MessageEvent needs so that we can generate the getter/setter functions and do everything the same way? Ah, I got it. But still I have some questions. Why is this line not "readonly attribute [CachedAttribute] SerializedScriptValue data"? What you want to do is to make it work, isn't it? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:869 > + if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"CachedAttribute"}) { > + push(@implContentDecls, <<END); > + v8::Handle<v8::String> propertyName = v8::String::NewSymbol("${attrName}"); > + v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(propertyName); > + if (!value.IsEmpty()) > + return value; > +END > + } This looks fine. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1025 > + if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"CachedAttribute"}) { > + my $getterFunc = $codeGenerator->WK_lcfirst($attribute->signature->name); > + push(@implContentDecls, <<END); > + SerializedScriptValue* serialized = imp->${getterFunc}(); > + value = serialized ? serialized->deserialize() : v8::Handle<v8::Value>(v8::Null()); > + info.Holder()->SetHiddenValue(propertyName, value); > + return value; > +END > + } else { > + push(@implContentDecls, " " . ReturnNativeToJSValue($attribute->signature, $result, " ").";\n"); > + } This also looks fine. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1962 > - # Attributes of type SerializedScriptValue are set in the > + # Attributes of type SerializedScriptValue marked as [InitializedByConstructor] are set in the > # constructor and don't require callbacks. > - return if ($attribute->signature->type eq "SerializedScriptValue"); > + return if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"InitializedByConstructor"}); But I am not still sure why we need these changes about [InitializedByConstructor]. I am expecting that the changes of two "if ($attribute->signature->type eq "SerializedScriptValue" && $attrExt->{"CachedAttribute"}) { ... }" (i.e. line 862 -- 869 and 1015 -- 1025) would be enough to cache attributes. With the two changes, the attribute will be cached when the getter/setter is called at the first time. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2325 > + if ($attribute->signature->extendedAttributes->{"InitializedByConstructor"}) { > + die "Only one attribute of type SerializedScriptValue [InitializedByConstructor] supported" if $serializedAttribute; > + $serializedAttribute = $attribute; > + next; > + } Ditto. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:136 > + { "serializedScriptValueAttr", DontDelete | ReadOnly, (intptr_t)static_cast<PropertySlot::GetValueFunc>(jsTestObjSerializedScriptValueAttr), (intptr_t)0, NoIntrinsic }, > + { "cachedSerializedScriptValueAttr", DontDelete | ReadOnly, (intptr_t)static_cast<PropertySlot::GetValueFunc>(jsTestObjCachedSerializedScriptValueAttr), (intptr_t)0, NoIntrinsic }, Would you please make a patch with the "right" run-bindings-tests results? You didn't touch CodeGeneratorJS.pm and thus JSTestObj.cpp should not be changed. Maybe you need to rebaseline run-bindings-tests before you make a patch. The run-bindings-tests results in WebKit trunk are sometimes wrong, since people are likely to forget to update the results:-) (The run-bindings-tests results are just for reviews and the build bots do not check their failures.) > Source/WebCore/bindings/scripts/test/TestObj.idl:155 > + > + readonly attribute SerializedScriptValue serializedScriptValueAttr; > + readonly attribute [CachedAttribute] SerializedScriptValue cachedSerializedScriptValueAttr; It might be better to put these tests in TestSerializedScriptValueInterface.idl. Also, please remove "readonly" in order to test its setter. > Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl:35 > - readonly attribute SerializedScriptValue value; > + readonly attribute [InitializedByConstructor] SerializedScriptValue value; Where are the test results (i.e. test/V8/V8TestSerializedScriptValueInterface.cpp)?

(In reply to comment #2) > (From update of attachment 124462 [details]) > I tried going with ForceSet() instead of using hidden attributes but i didn't find a way to reset the cached value in that case (would probably need to set the accessor function again, but it's internal to the v8 generated file), and a way to invalidate the value is needed for the history.state thing. > > Ideas/comments welcome. Using GetHiddenValue()/SetHiddenValue() makes sense to me. (FYI: Rather, I would object to use ForceSet() here. We are planning to move DOM attributes from a DOM object to a JS prototype chain (https://bugs.webkit.org/show_bug.cgi?id=75297), but ForceSet() makes it difficult (or impossible?). I cannot move MessageEvent.data from a DOM object to a JS prototype chain because it uses ForceSet()...)

(In reply to comment #7) > (From update of attachment 124462 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124462&action=review > > >>> Source/WebCore/Modules/intents/Intent.idl:34 > >>> + readonly attribute [InitializedByConstructor] SerializedScriptValue data; > >> > >> You do not need to add [InitializedByConstructor] here. Sorry, [InitializedByConstructor] is mis-renamed and it should be [InitializedByEventConstructor]. It is used by Event constructors only (i.e. [ConstructorTemplate=Event]). I'll rename it later. > >> > >> Would you write a patch, ignoring [InitializedByConstructor] cases? Then, the patch would become much simpler. > > > > Yeah, [InitializedByConstructor] is wrongly used here, that's what i meant when i said i was hijacking the meaning, but i was somehow trying to preserve the behavior introduced by https://bugs.webkit.org/show_bug.cgi?id=36892 and https://bugs.webkit.org/show_bug.cgi?id=75641 > > > > What i'm trying to do is something more akin to what the JSC generator does for SerializedScriptValue attributes. It generates getters/setters that deserialize the value when they're called (and cache the deserialized value in a member in the object if [CachedAttribute] is specified), which is not done in the v8 generator. > > Instead there's code to deserialize one (and only one) SerializedScriptValue attribute upfront in the constructor and ForceSet:tting it in the object, avoiding the generation and use of getters/setters. > > If i understood things correctly this was initially set up like that because MessageEvent needed the data deserialized in the right context (which presumably would not be the correct one if deserialized lazily in a getter?). It looks like the webintents case would be handled correctly. > > > > Do you think we can do something to not special case what MessageEvent needs so that we can generate the getter/setter functions and do everything the same way? > > Ah, I got it. But still I have some questions. > > Why is this line not "readonly attribute [CachedAttribute] SerializedScriptValue data"? What you want to do is to make it work, isn't it? I was trying to preserve a behavior that i thought was introduced just for MessageEvent, but that one is actually using custom getter/setters so i'll just get rid of the whole [InitializedByConstructor] faux handling and make a proper patch. Sorry for the misunderstanding.

Created attachment 124611 [details] Better patch

Attachment 124611 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:76: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:89: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:100: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:113: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:126: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 5 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ping?

Comment on attachment 124611 [details] Better patch View in context: https://bugs.webkit.org/attachment.cgi?id=124611&action=review > Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:88 > + info.Holder()->DeleteHiddenValue(v8::String::NewSymbol("cachedValue")); // Invalidate the cached value. Why don't you set the new value by SetHiddenValue(), instead of invalidating the current value?

Comment on attachment 124611 [details] Better patch View in context: https://bugs.webkit.org/attachment.cgi?id=124611&action=review >> Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:88 >> + info.Holder()->DeleteHiddenValue(v8::String::NewSymbol("cachedValue")); // Invalidate the cached value. > > Why don't you set the new value by SetHiddenValue(), instead of invalidating the current value? We'd need to deserialize it, i figured it would be better to do that lazily when it's requested (i think the JSC generator does it like this as well). Also, (using this case as an example) we'd need to assume that v == imp->getCachedValue(), otherwise we'd need to call imp->getCachedValue() before deserializing, and by that point we might as well just let the getter do its job.

Comment on attachment 124611 [details] Better patch View in context: https://bugs.webkit.org/attachment.cgi?id=124611&action=review Feel free to ping me if I am missing a review. >>> Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:88 >>> + info.Holder()->DeleteHiddenValue(v8::String::NewSymbol("cachedValue")); // Invalidate the cached value. >> >> Why don't you set the new value by SetHiddenValue(), instead of invalidating the current value? > > We'd need to deserialize it, i figured it would be better to do that lazily when it's requested (i think the JSC generator does it like this as well). Also, (using this case as an example) we'd need to assume that v == imp->getCachedValue(), otherwise we'd need to call imp->getCachedValue() before deserializing, and by that point we might as well just let the getter do its job. Makes sense!

Comment on attachment 124611 [details] Better patch Let me land this patch manually, to avoid style check errors.

Thanks!

Committed r106605: <http://trac.webkit.org/changeset/106605>