[Chromium] Allow client to monitor event listeners added and removed of a certain type
Created attachment 176571 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Why is this needed?
Comment on attachment 176571 [details] Patch Attachment 176571 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15028573
(In reply to comment #3) > Why is this needed?
(In reply to comment #5) > (In reply to comment #3) > > Why is this needed? Removed [chromium] in title as this touches WebCore.
Created attachment 176584 [details] Patch
Adding a virtual client call in Node.cpp when adding and removing any event listeners seems costly. How did you assess performance impact?
Comment on attachment 176584 [details] Patch There's some further discussion about this patch in https://code.google.com/p/chromium/issues/detail?id=140316. It sounds like we should think more about the design.
Reopening to attach new patch.
Created attachment 185596 [details] Patch
(In reply to comment #11) > Created an attachment (id=185596) [details] > Patch I've added an optimization where it calls out to ChromeClient only when transitioning from 0 to 1 event listeners or 1 to 0 event listeners.
Comment on attachment 185596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185596&action=review > Source/WebCore/page/ChromeClient.h:325 > + virtual void didRemoveEventListener(Node*, const AtomicString&) { } nit: It might be better to give these names like didAddFirstEventListener and didRemoveLastEventListener, so that the transition between some and none is more apparent.
Created attachment 185617 [details] Patch
Created attachment 185619 [details] Patch
Comment on attachment 185596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185596&action=review >> Source/WebCore/page/ChromeClient.h:325 >> + virtual void didRemoveEventListener(Node*, const AtomicString&) { } > > nit: It might be better to give these names like didAddFirstEventListener and didRemoveLastEventListener, > so that the transition between some and none is more apparent. Done.
Comment on attachment 185619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185619&action=review > Source/WebKit/chromium/public/WebEventListenerClient.h:34 > +class WebEventListenerClient { I think you should just add these methods to WebViewClient, and then implement the multi-cast support on the Chromium side.
Created attachment 185790 [details] Patch
Comment on attachment 185619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185619&action=review >> Source/WebKit/chromium/public/WebEventListenerClient.h:34 >> +class WebEventListenerClient { > > I think you should just add these methods to WebViewClient, and then implement > the multi-cast support on the Chromium side. Done.
Created attachment 185807 [details] Patch
Comment on attachment 185807 [details] Patch These ChangeLogs contain no information about why we're making this change. That's important information to communicate, especially for a change like this where the reason isn't obvious.
Created attachment 186122 [details] Patch
(In reply to comment #21) > (From update of attachment 185807 [details]) > These ChangeLogs contain no information about why we're making this change. That's important information to communicate, especially for a change like this where the reason isn't obvious. Done. Could you please take another look?
Comment on attachment 186122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186122&action=review > Source/WebCore/ChangeLog:10 > + BrowserPlugin needs to be able to observe event listeners attached to it > + so that it can enable window.open when event listeners are attached to the > + 'newwindow' event. That doesn't sound like a good API. Adding event listeners should not produce side effects.
(In reply to comment #24) > (From update of attachment 186122 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186122&action=review > > > Source/WebCore/ChangeLog:10 > > + BrowserPlugin needs to be able to observe event listeners attached to it > > + so that it can enable window.open when event listeners are attached to the > > + 'newwindow' event. > > That doesn't sound like a good API. Adding event listeners should not produce side effects. Fady, this is a good point. I suspect your goal here was to determine if it was worth allowing window.open(), but in reality, it seems like there could be a race between window.open() executed by JS within the guest page, and code in the embedder calling addEventListener. To avoid that race condition, the proper solution is probably to always let window.open() appear to succeed, and then upon dispatching the 'newwindow' event, if no one handles the event, then you can just close the newly opened window. To the guest it'll look like the window was opened and then closed. It seems like just an optimization to try to block window.open() if there is not a registered event listener. Since the developer has control over both the embedder code and the guest code, we probably don't need to worry that much about optimizing the case where window.open() is called when there is no 'newwindow' event listener.