Support registering and dispatching passive event listeners.

Support registering and dispatching passive event listeners.

Add support for being able to register passive event listeners and
being able to dispatch content to them.

BUG=489802
Committed: https://crrev.com/89ef8848791589f46db76d1d6ad7fa32d73895e1
Cr-Commit-Position: refs/heads/master@{#368907}

[dtapuska, 2016-01-05 22:06:35]

Description was changed from

==========
Support registering and dispatching passive event listeners.

Add support for being able to register passive event listeners and
being able to dispatch content to them.

BUG=489802
==========

to

==========
Support registering and dispatching passive event listeners.

Add support for being able to register passive event listeners and
being able to dispatch content to them.

BUG=489802
==========

[dtapuska, 2016-01-05 22:06:35]

dtapuska@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2016-01-08 17:55:27]

Sorry for the delay, looking now.  But you haven't done an intent-to-implement
for this yet, right?  We should probably do that before landing this.

[dtapuska, 2016-01-08 17:59:11]

On 2016/01/08 17:55:27, Rick Byers wrote:
> Sorry for the delay, looking now.  But you haven't done an intent-to-implement
> for this yet, right?  We should probably do that before landing this.

I'll send one today. Also this likely needs to change as I wasn't planning on
exposing EventListenerOptions separately but with my Intent to Ship on that this
morning it changes this.

[Rick Byers, 2016-01-08 18:02:24]

On 2016/01/08 17:59:11, dtapuska wrote:
> On 2016/01/08 17:55:27, Rick Byers wrote:
> > Sorry for the delay, looking now.  But you haven't done an
intent-to-implement
> > for this yet, right?  We should probably do that before landing this.
> 
> I'll send one today. Also this likely needs to change as I wasn't planning on
> exposing EventListenerOptions separately but with my Intent to Ship on that
this
> morning it changes this.

Ah yeah, I had just written a comment about that.  I guess it's possible there
will be pushback on the intent - I could be convinced that it's better to wait
and ship both options together (especially now that the UMA metrics give us more
confidence of the safety).

[Rick Byers, 2016-01-08 19:32:56]

Looks good to me other than the RuntimeEnabledFeature change and a few minor
test suggestions.

I was assuming we wouldn't ship this (or perhaps even promote it to
'experimental') until it was wired to have at least some of the desired
performance improvement.  What do you think?  Maybe starting in "experimental"
is fine - it's still useful for testing.  The main reason not to ship this until
it has real value is probably to avoid developer confusion and also give us some
protection in case I've screwed up the design and we decide we want something
different after all.

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html
(right):

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html:33:
document.body.dispatchEvent(new Event('test', {'bubbles': true, 'cancelable':
true}));
perhaps you should also verify that the blocking handle can successfully call
preventDefault in this case?

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html:40:
test(function(t) { testPassiveValue({}, false, t); }, "Prevent default with
empty object");
Unless you're getting coverage elsewhere, you should probably add test cases for
the legacy forms of the API (3rd arg omitted, 3rd arg a boolean), right?

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html:42:
test(function(t) { testPassiveValue({passive: true},
internals.runtimeFlags.eventListenerOptionsEnabled ? true : false, t); },
"Prevent default with passive true ");
nit: Unless you're planning on explicitly running the tests with the feature
disabled, you might as well make the expected value 'true' here.  Testing at
least one manually that nothing has changed when the feature is disabled is good
(our virtual/stable/webexposed tests won't help in detecting this type of API
exposure), but an automated test for that is probably overkill unless there's
non-trivial code to support it.

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_query.html
(right):

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_query.html:22:
assert_equals(supportsPassive,
internals.runtimeFlags.eventListenerOptionsEnabled);
I guess this could be a good place to verify that the runtime enabled feature is
correctly supporting the feature detection.  I.e. instead of just testing
whatever the default state is, assert that it's enabled (I don't think we'd ever
put the feature below "test" status), verify the feature detection passes, then
disable it and verify the feature detection fails.  WDYT?

[dtapuska, 2016-01-11 18:56:28]

PTAL

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html
(right):

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html:33:
document.body.dispatchEvent(new Event('test', {'bubbles': true, 'cancelable':
true}));
On 2016/01/08 19:32:56, Rick Byers wrote:
> perhaps you should also verify that the blocking handle can successfully call
> preventDefault in this case?

Done.

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html:40:
test(function(t) { testPassiveValue({}, false, t); }, "Prevent default with
empty object");
On 2016/01/08 19:32:56, Rick Byers wrote:
> Unless you're getting coverage elsewhere, you should probably add test cases
for
> the legacy forms of the API (3rd arg omitted, 3rd arg a boolean), right?

Done.

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_dispatch.html:42:
test(function(t) { testPassiveValue({passive: true},
internals.runtimeFlags.eventListenerOptionsEnabled ? true : false, t); },
"Prevent default with passive true ");
On 2016/01/08 19:32:56, Rick Byers wrote:
> nit: Unless you're planning on explicitly running the tests with the feature
> disabled, you might as well make the expected value 'true' here.  Testing at
> least one manually that nothing has changed when the feature is disabled is
good
> (our virtual/stable/webexposed tests won't help in detecting this type of API
> exposure), but an automated test for that is probably overkill unless there's
> non-trivial code to support it.

The feature is manually run with it disabled via the stable tests. Since this is
a webplatform test the test needs to by dynamic under the conditions rather than
changing the expected output (which don't exist for webplatform tests)

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_query.html
(right):

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_query.html:22:
assert_equals(supportsPassive,
internals.runtimeFlags.eventListenerOptionsEnabled);
On 2016/01/08 19:32:56, Rick Byers wrote:
> I guess this could be a good place to verify that the runtime enabled feature
is
> correctly supporting the feature detection.  I.e. instead of just testing
> whatever the default state is, assert that it's enabled (I don't think we'd
ever
> put the feature below "test" status), verify the feature detection passes,
then
> disable it and verify the feature detection fails.  WDYT?

Is this not doing it when it runs via the stable test?

[Rick Byers, 2016-01-11 22:37:05]

LGTM with nit

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_query.html
(right):

https://codereview.chromium.org/1563623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/eventlisteneroptions/passive_query.html:22:
assert_equals(supportsPassive,
internals.runtimeFlags.eventListenerOptionsEnabled);
On 2016/01/11 18:56:28, dtapuska wrote:
> On 2016/01/08 19:32:56, Rick Byers wrote:
> > I guess this could be a good place to verify that the runtime enabled
feature
> is
> > correctly supporting the feature detection.  I.e. instead of just testing
> > whatever the default state is, assert that it's enabled (I don't think we'd
> ever
> > put the feature below "test" status), verify the feature detection passes,
> then
> > disable it and verify the feature detection fails.  WDYT?
> 
> Is this not doing it when it runs via the stable test?

Ah, I forgot this entire directly is enabled for virtual/stable
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

The more common pattern I think is to explicitly toggle the
RuntimeEnabledFeature state, but this is fine too.

Maybe add a comment here (and in the other test) just mentioning that this tests
expects to be run with and without the feature enabled via virtual/stable.

[dtapuska, 2016-01-12 17:28:25]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-01-12 17:28:26]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1563623002/#ps120001
(title: "Adjust comments in layout tests")

[commit-bot: I haz the power, 2016-01-12 17:29:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1563623002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1563623002/120001

[commit-bot: I haz the power, 2016-01-12 17:36:58]

Description was changed from

==========
Support registering and dispatching passive event listeners.

Add support for being able to register passive event listeners and
being able to dispatch content to them.

BUG=489802
==========

to

==========
Support registering and dispatching passive event listeners.

Add support for being able to register passive event listeners and
being able to dispatch content to them.

BUG=489802
==========

[commit-bot: I haz the power, 2016-01-12 17:36:59]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-12 17:37:38]

Description was changed from

==========
Support registering and dispatching passive event listeners.

Add support for being able to register passive event listeners and
being able to dispatch content to them.

BUG=489802
==========

to

==========
Support registering and dispatching passive event listeners.

Add support for being able to register passive event listeners and
being able to dispatch content to them.

BUG=489802

Committed: https://crrev.com/89ef8848791589f46db76d1d6ad7fa32d73895e1
Cr-Commit-Position: refs/heads/master@{#368907}
==========

[commit-bot: I haz the power, 2016-01-12 17:37:38]

Patchset 7 (id:??) landed as
https://crrev.com/89ef8848791589f46db76d1d6ad7fa32d73895e1
Cr-Commit-Position: refs/heads/master@{#368907}