CustomEvent: Allow taking in a serialized value during initialization.
Created attachment 172582 [details] Patch
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.
Comment on attachment 172582 [details] Patch Attachment 172582 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14733743
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.
Created attachment 172585 [details] Patch
Comment on attachment 172585 [details] Patch Attachment 172585 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14731768
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.
Created attachment 172601 [details] Patch
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.
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?
Comment on attachment 172601 [details] Patch Attachment 172601 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14760019
Comment on attachment 172601 [details] Patch Attachment 172601 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14755170
Comment on attachment 172601 [details] Patch Attachment 172601 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14747468
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?
Created attachment 172604 [details] Patch
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.
Created attachment 172608 [details] Patch
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.
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.
Comment on attachment 172608 [details] Patch Attachment 172608 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14745411
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.)
Comment on attachment 172608 [details] Patch Attachment 172608 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14726971
Created attachment 172613 [details] Patch
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!
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?
(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>.
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.
Created attachment 173344 [details] Patch
Comment on attachment 173344 [details] Patch This looks good too.
> 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>.
(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.
Comment on attachment 173344 [details] Patch Clearing flags on attachment: 173344 Committed r134120: <http://trac.webkit.org/changeset/134120>
All reviewed patches have been landed. Closing bug.