[Webkit-unassigned] [Bug 77295] V8 idl code generator doesn't handle [CachedAttribute] on SerializedScriptValue attributes.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77295





--- Comment #7 from Kentaro Hara <haraken at chromium.org>  2012-01-29 20:51:38 PST ---
(From update of attachment 124462)
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)?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.