Bug 101348 – CustomEvent: Allow taking in a serialized value during initialization.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sadrul Habib Chowdhury]

CustomEvent: Allow taking in a serialized value during initialization.

[Sadrul Habib Chowdhury]

Created attachment 172582 [details]
Patch

[Sadrul Habib Chowdhury]

Hi Adam. I am addressing your comment from https://bugs.webkit.org/show_bug.cgi?id=101293 here. Please take a look and see if this makes sense. I will add a test if it does.

One note is that CustomEvent::detail() is no longer a const method.

[Early Warning System Bot]

Comment on attachment 172582 [details]
Patch

Attachment 172582 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14733743

[Adam Barth]

Comment on attachment 172582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172582&action=review

> Source/WebCore/dom/CustomEvent.cpp:83
> +const ScriptValue& CustomEvent::detail()
> +{
> +    if (m_serializedScriptValue.get())
> +        m_detail = ScriptValue::deserialize(ScriptState::current(), m_serializedScriptValue.get());
> +    return m_detail;
> +}

This patch is almost right.  The one tweak is that we need to do this work in in the bindings layer and cache the result on the JavaScript wrapper (i.e., JSCustomEvent or V8CustomEvent).  The way you've written this, the m_detail is shared by all wrappers.  We want different instances of the details object for each wrapper (i.e., for each isolated world that might receive the event).

Also, rather than using ScriptState::current(), we'll want to use the CreationContext of the JavaScript wrapper as the context for the deserialization.

[Sadrul Habib Chowdhury]

Created attachment 172585 [details]
Patch

[Early Warning System Bot]

Comment on attachment 172585 [details]
Patch

Attachment 172585 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14731768

[Adam Barth]

Comment on attachment 172585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172585&action=review

> Source/WebCore/ChangeLog:10
> +        If a CustomEvent is initialized using a serialized value, then for each access
> +        to |detail|, the value is deserialized first. This way, each world gets a different
> +        deserialization.

In your patch, you're only calling deserialize once per CustomEvent object though.

[Sadrul Habib Chowdhury]

Created attachment 172601 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 172585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172585&action=review

>> Source/WebCore/ChangeLog:10
>> +        deserialization.
> 
> In your patch, you're only calling deserialize once per CustomEvent object though.

The serialized script-value doesn't get reset once it has been serialized. So we end up deserializing for each call.

[Sadrul Habib Chowdhury]

Comment on attachment 172601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172601&action=review

I looked at the slides from Haraken to do the custom bindings. But it is likely that I missed a few things, or misunderstood a few things. Please let me know if things look reasonable (and links to reading material would be helpful too :) )

> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:61
> +        return ScriptValue::deserialize(ScriptState::forContext(info.Holder()->CreationContext()), serialized.get()).v8Value();

I am not sure how we can cache the deserialized value here. Suggestions?

[Early Warning System Bot]

Comment on attachment 172601 [details]
Patch

Attachment 172601 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14760019

[EFL EWS Bot]

Comment on attachment 172601 [details]
Patch

Attachment 172601 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14755170

[Early Warning System Bot]

Comment on attachment 172601 [details]
Patch

Attachment 172601 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14747468

[Adam Barth]

Comment on attachment 172601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172601&action=review

> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:59
> +    RefPtr<SerializedScriptValue> serialized;
> +    serialized = imp->serializedScriptValue();

THese lines can be combined.

>> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:61
>> +        return ScriptValue::deserialize(ScriptState::forContext(info.Holder()->CreationContext()), serialized.get()).v8Value();
> 
> I am not sure how we can cache the deserialized value here. Suggestions?

You can add a property to V8HiddenPropertyName and store get/set the deserialized value on info.Holder().  Take a look at the generated code for normal SerializedScriptValue attributes.

> Source/WebCore/dom/CustomEvent.idl:29
> +    CustomToJSObject,

Why do we need a CustomToJSObject?

> Source/WebCore/dom/CustomEvent.idl:32
> -    [InitializedByEventConstructor] readonly attribute DOMObject detail;
> +    [Custom] readonly attribute DOMObject detail;

Did we lose InitializedByEventConstructor?

[Sadrul Habib Chowdhury]

Created attachment 172604 [details]
Patch

[Adam Barth]

Comment on attachment 172604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172604&action=review

> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:56
> +        return ScriptValue::deserialize(ScriptState::forContext(info.Holder()->CreationContext()), serialized.get()).v8Value();

There's no reason to use the ScriptValue helpers when you're inside the bindings.  You can just call SerialziedScriptValue::deserialize directly.  ScriptValue is an abstraction that lets you share code between V8 and JSC.

[Sadrul Habib Chowdhury]

Created attachment 172608 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 172601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172601&action=review

>> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:59
>> +    serialized = imp->serializedScriptValue();
> 
> THese lines can be combined.

Done.

>>> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:61
>>> +        return ScriptValue::deserialize(ScriptState::forContext(info.Holder()->CreationContext()), serialized.get()).v8Value();
>> 
>> I am not sure how we can cache the deserialized value here. Suggestions?
> 
> You can add a property to V8HiddenPropertyName and store get/set the deserialized value on info.Holder().  Take a look at the generated code for normal SerializedScriptValue attributes.

Nice! Done.

>> Source/WebCore/dom/CustomEvent.idl:29
>> +    CustomToJSObject,
> 
> Why do we need a CustomToJSObject?

We don't. I misunderstood. Removed.

>> Source/WebCore/dom/CustomEvent.idl:32
>> +    [Custom] readonly attribute DOMObject detail;
> 
> Did we lose InitializedByEventConstructor?

We did. Another misunderstanding. Fixed.

[Sadrul Habib Chowdhury]

Comment on attachment 172604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172604&action=review

>> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:56
>> +        return ScriptValue::deserialize(ScriptState::forContext(info.Holder()->CreationContext()), serialized.get()).v8Value();
> 
> There's no reason to use the ScriptValue helpers when you're inside the bindings.  You can just call SerialziedScriptValue::deserialize directly.  ScriptValue is an abstraction that lets you share code between V8 and JSC.

Cool! Done.

[Early Warning System Bot]

Comment on attachment 172608 [details]
Patch

Attachment 172608 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14745411

[Adam Barth]

Comment on attachment 172608 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172608&action=review

> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:53
> +    RefPtr<SerializedScriptValue> serialized = imp->serializedScriptValue();

RefPtr<SerializedScriptValue> -> SerializedScriptValue*

The CustomEvent will hold a reference to SerializedScriptValue  There's no need to hold another one.

> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:55
> +        v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(V8HiddenPropertyName::state());

Can we create a new V8HiddenPropertyName for use here?  Perhaps "detail" ?  It's just a matter of adding it to the macro in the header.

> Source/WebCore/dom/CustomEvent.h:56
> +    void initCustomEvent(const AtomicString& type, bool canBubble, bool cancelable, PassRefPtr<SerializedScriptValue>);

If you're not going to implement this feature for JSC, can you wrap this function in a USE(V8) #if and add a comment that this isn't implemented for JSC yet?

> Source/WebCore/dom/CustomEvent.h:61
> +    PassRefPtr<SerializedScriptValue> serializedScriptValue() { return m_serializedScriptValue; }

PassRefPtr<SerializedScriptValue>  ->  SerializedScriptValue*

This function isn't transferring ownership of the SerializedScriptValue.  There's no reason to use a PassRefPtr.  See http://www.webkit.org/coding/RefPtr.html for more information about how RefPtr works.

> Source/WebCore/dom/CustomEvent.idl:31
> -    [InitializedByEventConstructor] readonly attribute DOMObject detail;
> +    [InitializedByEventConstructor, Custom] readonly attribute DOMObject detail;

You probably want to mark this V8Custom rather than Custom.  Custom implies that JSC has a custom implementation as well.  (Alternatively, you could create a custom implementation for JSC, but given that this functionality supports a Chromium WebKit API, that's probably not necessary.)

[Early Warning System Bot]

Comment on attachment 172608 [details]
Patch

Attachment 172608 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14726971

[Sadrul Habib Chowdhury]

Created attachment 172613 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 172608 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172608&action=review

>> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:53
>> +    RefPtr<SerializedScriptValue> serialized = imp->serializedScriptValue();
> 
> RefPtr<SerializedScriptValue> -> SerializedScriptValue*
> 
> The CustomEvent will hold a reference to SerializedScriptValue  There's no need to hold another one.

Done.

>> Source/WebCore/bindings/v8/custom/V8CustomEventCustom.cpp:55
>> +        v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(V8HiddenPropertyName::state());
> 
> Can we create a new V8HiddenPropertyName for use here?  Perhaps "detail" ?  It's just a matter of adding it to the macro in the header.

Done.

>> Source/WebCore/dom/CustomEvent.h:56
>> +    void initCustomEvent(const AtomicString& type, bool canBubble, bool cancelable, PassRefPtr<SerializedScriptValue>);
> 
> If you're not going to implement this feature for JSC, can you wrap this function in a USE(V8) #if and add a comment that this isn't implemented for JSC yet?

Done. Thanks. (I almost started looking into a JSC implementation, but this sounds better :) )

>> Source/WebCore/dom/CustomEvent.h:61
>> +    PassRefPtr<SerializedScriptValue> serializedScriptValue() { return m_serializedScriptValue; }
> 
> PassRefPtr<SerializedScriptValue>  ->  SerializedScriptValue*
> 
> This function isn't transferring ownership of the SerializedScriptValue.  There's no reason to use a PassRefPtr.  See http://www.webkit.org/coding/RefPtr.html for more information about how RefPtr works.

Done.

>> Source/WebCore/dom/CustomEvent.idl:31
>> +    [InitializedByEventConstructor, Custom] readonly attribute DOMObject detail;
> 
> You probably want to mark this V8Custom rather than Custom.  Custom implies that JSC has a custom implementation as well.  (Alternatively, you could create a custom implementation for JSC, but given that this functionality supports a Chromium WebKit API, that's probably not necessary.)

I went with marking it as V8Custom. Thanks!

[Adam Barth]

dglazkov just IMed me with some concerns about this approach.  He's worried that using CustomEvents might not make sense because if we were to implement <webview> as a native part of the platform we wouldn't use CustomEvent, we would define a new type of event.

Are these events exposed to the user of <webview>, or are they just consumed internally by the <webview> component?

[Sadrul Habib Chowdhury]

(In reply to comment #25)
> dglazkov just IMed me with some concerns about this approach.  He's worried that using CustomEvents might not make sense because if we were to implement <webview> as a native part of the platform we wouldn't use CustomEvent, we would define a new type of event.
> 
> Are these events exposed to the user of <webview>, or are they just consumed internally by the <webview> component?

The events are indeed exposed to the users of <webview>.

[Adam Barth]

This patch looks great based on the approach we discussed.  I would have added a comment to the USE(V8) ifdef, but that's just a nit.

The remaining question here is whether we should be using CustomEvent or whether we should let the embedder define new kinds of events.

[Sadrul Habib Chowdhury]

Created attachment 173344 [details]
Patch

[Adam Barth]

Comment on attachment 173344 [details]
Patch

This looks good too.

[Adam Barth]

> Are these events exposed to the user of <webview>, or are they just consumed internally by the <webview> component?

@dglazkov: In this new approach, these events are just used internally by the <webview> component.  The <webview> component then translates them into regular-looking events for the user of the <webview>.

[Dimitri Glazkov (Google)]

(In reply to comment #30)
> > Are these events exposed to the user of <webview>, or are they just consumed internally by the <webview> component?
> 
> @dglazkov: In this new approach, these events are just used internally by the <webview> component.  The <webview> component then translates them into regular-looking events for the user of the <webview>.

ookay.

[WebKit Review Bot]

Comment on attachment 173344 [details]
Patch

Clearing flags on attachment: 173344

Committed r134120: <http://trac.webkit.org/changeset/134120>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.