Bug 103561 – Allow client to monitor event listeners added and removed of a certain type

• Component: WebKit Misc.
• Status: REOPENED

[Fady Samuel]

[Chromium] Allow client to monitor event listeners added and removed of a certain type

[Fady Samuel]

Created attachment 176571 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Why is this needed?

[Build Bot]

Comment on attachment 176571 [details]
Patch

Attachment 176571 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15028573

[Fady Samuel]

(In reply to comment #3)
> Why is this needed?

[Fady Samuel]

(In reply to comment #5)
> (In reply to comment #3)
> > Why is this needed?

Removed [chromium] in title as this touches WebCore.

[Fady Samuel]

Created attachment 176584 [details]
Patch

[Alexey Proskuryakov]

Adding a virtual client call in Node.cpp when adding and removing any event listeners seems costly. How did you assess performance impact?

[Adam Barth]

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.

[Fady Samuel]

Reopening to attach new patch.

[Fady Samuel]

Created attachment 185596 [details]
Patch

[Fady Samuel]

(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.

[Darin Fisher (:fishd, Google)]

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.

[Fady Samuel]

Created attachment 185617 [details]
Patch

[Fady Samuel]

Created attachment 185619 [details]
Patch

[Fady Samuel]

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.

[Darin Fisher (:fishd, Google)]

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.

[Fady Samuel]

Created attachment 185790 [details]
Patch

[Fady Samuel]

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.

[Fady Samuel]

Created attachment 185807 [details]
Patch

[Adam Barth]

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.

[Fady Samuel]

Created attachment 186122 [details]
Patch

[Fady Samuel]

(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?

[Adam Barth]

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.

[Darin Fisher (:fishd, Google)]

(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.