Bug 103561 - Allow client to monitor event listeners added and removed of a certain type
Summary: Allow client to monitor event listeners added and removed of a certain type
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-28 14:00 PST by Fady Samuel
Modified: 2017-07-18 08:29 PDT (History)
16 users (show)

See Also:


Attachments
Patch (13.70 KB, patch)
2012-11-28 14:01 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (13.69 KB, patch)
2012-11-28 14:58 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (14.07 KB, patch)
2013-01-30 15:31 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (15.45 KB, patch)
2013-01-30 16:33 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (15.45 KB, patch)
2013-01-30 16:38 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2013-01-31 08:39 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2013-01-31 10:26 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2013-02-01 13:23 PST, Fady Samuel
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2012-11-28 14:00:00 PST
[Chromium] Allow client to monitor event listeners added and removed of a certain type
Comment 1 Fady Samuel 2012-11-28 14:01:51 PST
Created attachment 176571 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-28 14:08:21 PST
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.
Comment 3 Darin Fisher (:fishd, Google) 2012-11-28 14:12:59 PST
Why is this needed?
Comment 4 Build Bot 2012-11-28 14:41:50 PST
Comment on attachment 176571 [details]
Patch

Attachment 176571 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15028573
Comment 5 Fady Samuel 2012-11-28 14:46:44 PST
(In reply to comment #3)
> Why is this needed?
Comment 6 Fady Samuel 2012-11-28 14:47:05 PST
(In reply to comment #5)
> (In reply to comment #3)
> > Why is this needed?

Removed [chromium] in title as this touches WebCore.
Comment 7 Fady Samuel 2012-11-28 14:58:16 PST
Created attachment 176584 [details]
Patch
Comment 8 Alexey Proskuryakov 2012-11-28 17:00:59 PST
Adding a virtual client call in Node.cpp when adding and removing any event listeners seems costly. How did you assess performance impact?
Comment 9 Adam Barth 2012-11-29 23:28:20 PST
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.
Comment 10 Fady Samuel 2013-01-30 15:31:27 PST
Reopening to attach new patch.
Comment 11 Fady Samuel 2013-01-30 15:31:30 PST
Created attachment 185596 [details]
Patch
Comment 12 Fady Samuel 2013-01-30 15:32:33 PST
(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 13 Darin Fisher (:fishd, Google) 2013-01-30 15:42:55 PST
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.
Comment 14 Fady Samuel 2013-01-30 16:33:34 PST
Created attachment 185617 [details]
Patch
Comment 15 Fady Samuel 2013-01-30 16:38:15 PST
Created attachment 185619 [details]
Patch
Comment 16 Fady Samuel 2013-01-30 16:38:54 PST
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 17 Darin Fisher (:fishd, Google) 2013-01-30 16:44:01 PST
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.
Comment 18 Fady Samuel 2013-01-31 08:39:43 PST
Created attachment 185790 [details]
Patch
Comment 19 Fady Samuel 2013-01-31 08:40:05 PST
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.
Comment 20 Fady Samuel 2013-01-31 10:26:19 PST
Created attachment 185807 [details]
Patch
Comment 21 Adam Barth 2013-01-31 11:05:36 PST
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.
Comment 22 Fady Samuel 2013-02-01 13:23:08 PST
Created attachment 186122 [details]
Patch
Comment 23 Fady Samuel 2013-02-01 13:23:37 PST
(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 24 Adam Barth 2013-02-02 00:02:12 PST
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.
Comment 25 Darin Fisher (:fishd, Google) 2013-02-02 22:08:21 PST
(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.