Add runtime setting to force passive event listeners.

Add runtime setting to force passive event listeners.

Add the ability to set the default value for the 'passive' field
in AddEventListenerOptions.

The value can take on 4 values; 'false', 'true', 'documentonlytrue',
'forcetrue'. It can be set from the command line or by adjusting
the chrome://flags.

BUG=599611
Committed: https://crrev.com/f65650b0471dfd3d5fda67ae634a986148ad2d3e
Cr-Commit-Position: refs/heads/master@{#393565}

[dtapuska, 2016-05-09 19:07:11]

Description was changed from

==========
Add runtime setting to force passive event listeners.

Add the ability to set the default value for the 'passive' field
in AddEventListenerOptions.

The value can take on 4 values; 'false', 'true', 'documentonlytrue',
'forcetrue'. It can be set from the command line or by adjusting
the chrome://flags.

BUG=599611
==========

to

==========
Add runtime setting to force passive event listeners.

Add the ability to set the default value for the 'passive' field
in AddEventListenerOptions.

The value can take on 4 values; 'false', 'true', 'documentonlytrue',
'forcetrue'. It can be set from the command line or by adjusting
the chrome://flags.

BUG=599611
==========

[dtapuska, 2016-05-09 19:07:11]

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

[dtapuska, 2016-05-09 19:07:34]

PTAL; as both of you requested this feature. Hopefully it manages to achieve
what you guys wanted.

[Rick Byers, 2016-05-09 21:48:48]

Thanks, looks good - just a couple small suggestions.

https://codereview.chromium.org/1965493002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1965493002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5653: +        Passive Event Listeners
Maybe "Passive Event Listener Override"?

https://codereview.chromium.org/1965493002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5656: +        Adjust the default 'passive'
value when registering a TouchStart, TouchMove, MouseWheel and Wheel.
Since users go searching chrome://flags for interesting things to try we should
try to make the impact of this more clear.  Eg. maybe: "Forces touchstart,
touchmove, mousewheel and wheel event listeners (which haven't requested
otherwise) to be treated as passive.  This will break touch/wheel behavior on
some websites but is useful for demonstrating the potential performance benefits
of adopting passive event listeners."?

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/AddEventListenerOptionsDefaults.h
(right):

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/AddEventListenerOptionsDefaults.h:11:
False, // Default of false.
Nit: make it clear this is what's required by spec.  Anything else is an
experiment which likely breaks websites.

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventTarget.cpp:136: if
(node->isDocumentNode() ||
static_cast<Node*>(node->document().documentElement()) == node ||
static_cast<Node*>(node->document().body()) == node) {
the static_casts aren't actually necessary here, are they?

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Settings.in:404: passiveListenerDefault
type=PassiveListenerDefault, initial=PassiveListenerDefault::False
nit: please add a comment.  There's an effort to reduce the flags/settings, so
it's valuable to explain what they're for / link to details.  Maybe we'd say we
can remove this once our intervention (and associated devtools features) ships?

[aelias_OOO_until_Jul13, 2016-05-10 01:22:59]

Could you write a unit test loading in some HTML with a touch listener and
checking the GraphicsLayer has the expected EventListenerOptions? 
WebFrameTest.cpp or environs might be the easiest place to write such a test. 
(Although it might seem like overkill to unit test an experiment, I'm concerned
this is exactly the kind of thing that might regress without anyone noticing and
then we would go and ship an intervention that actually does nothing, or
something like that.)

[dtapuska, 2016-05-11 00:32:04]

On 2016/05/10 01:22:59, aelias wrote:
> Could you write a unit test loading in some HTML with a touch listener and
> checking the GraphicsLayer has the expected EventListenerOptions? 
> WebFrameTest.cpp or environs might be the easiest place to write such a test. 
> (Although it might seem like overkill to unit test an experiment, I'm
concerned
> this is exactly the kind of thing that might regress without anyone noticing
and
> then we would go and ship an intervention that actually does nothing, or
> something like that.)

I do think adding a unit test for this is probably overkill since this code
wouldn't
be used in an intervention. The compositor is likely the case where an
intervention
will take place instead of changing the default of how an event listener is
registered
as that can be dynamic. This is a relatively easy mechanism to change the
default
behaviour of adding an event listener.

None the less it might be relatively easy to write a layout test to test this.
Which I will do.

[aelias_OOO_until_Jul13, 2016-05-11 02:30:46]

OK, makes sense, lgtm even without the test in that case.

BTW, unit tests are also able to load HTML and I have found them strictly easier
to write than layout tests (as you can better control ordering and don't need to
add plumbing hooks to JS).

[dtapuska, 2016-05-11 20:01:11]

https://codereview.chromium.org/1965493002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1965493002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5653: +        Passive Event Listeners
On 2016/05/09 21:48:48, Rick Byers wrote:
> Maybe "Passive Event Listener Override"?

Done.

https://codereview.chromium.org/1965493002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5656: +        Adjust the default 'passive'
value when registering a TouchStart, TouchMove, MouseWheel and Wheel.
On 2016/05/09 21:48:48, Rick Byers wrote:
> Since users go searching chrome://flags for interesting things to try we
should
> try to make the impact of this more clear.  Eg. maybe: "Forces touchstart,
> touchmove, mousewheel and wheel event listeners (which haven't requested
> otherwise) to be treated as passive.  This will break touch/wheel behavior on
> some websites but is useful for demonstrating the potential performance
benefits
> of adopting passive event listeners."?

Done.

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/AddEventListenerOptionsDefaults.h
(right):

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/AddEventListenerOptionsDefaults.h:11:
False, // Default of false.
On 2016/05/09 21:48:48, Rick Byers wrote:
> Nit: make it clear this is what's required by spec.  Anything else is an
> experiment which likely breaks websites.

Done.

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventTarget.cpp:136: if
(node->isDocumentNode() ||
static_cast<Node*>(node->document().documentElement()) == node ||
static_cast<Node*>(node->document().body()) == node) {
On 2016/05/09 21:48:48, Rick Byers wrote:
> the static_casts aren't actually necessary here, are they?

I've tried it; I do recall one build bot complaining about this in the past. But
I've sent it for the try jobs without it to see.

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/Settings.in (right):

https://codereview.chromium.org/1965493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/Settings.in:404: passiveListenerDefault
type=PassiveListenerDefault, initial=PassiveListenerDefault::False
On 2016/05/09 21:48:48, Rick Byers wrote:
> nit: please add a comment.  There's an effort to reduce the flags/settings, so
> it's valuable to explain what they're for / link to details.  Maybe we'd say
we
> can remove this once our intervention (and associated devtools features)
ships?

Done.

[Rick Byers, 2016-05-12 01:19:04]

LGTM

[dtapuska, 2016-05-12 01:47:31]

dtapuska@chromium.org changed reviewers:
+ asvitkine@chromium.org, creis@chromium.org, thestig@chromium.org

[dtapuska, 2016-05-12 19:59:53]

On 2016/05/12 01:19:04, Rick Byers wrote:
> LGTM

ping.

[Lei Zhang, 2016-05-12 20:15:23]

On 2016/05/12 19:59:53, dtapuska wrote:
> On 2016/05/12 01:19:04, Rick Byers wrote:
> > LGTM
> 
> ping.

There are multiple reviewers. Who are you pinging?

This is the first time I'm getting notified about this CL. Did you want me to
review chrome/ ?

[dtapuska, 2016-05-12 20:20:47]

On 2016/05/12 20:15:23, Lei Zhang wrote:
> On 2016/05/12 19:59:53, dtapuska wrote:
> > On 2016/05/12 01:19:04, Rick Byers wrote:
> > > LGTM
> > 
> > ping.
> 
> There are multiple reviewers. Who are you pinging?
> 
> This is the first time I'm getting notified about this CL. Did you want me to
> review chrome/ ?

hmm; the additional reviewers were added once Rick gave his l-g-t-m looks like I
didn't do a publish so you didn't get a email notification. I was just going to
leave for the day soon so was hoping it would get looked at by the time I come
in tomorrow.

[Charlie Reis, 2016-05-12 20:30:16]

content/ LGTM with nit.

https://codereview.chromium.org/1965493002/diff/40001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1965493002/diff/40001/content/public/common/c...
content/public/common/content_switches.cc:678: // Default for passive listeners
being added. Values are defined as:
Can you give some context for what passive listeners are in the comment?  It's
not clear from context.

[Alexei Svitkine (slow), 2016-05-12 21:32:23]

lgtm

[Lei Zhang, 2016-05-12 22:10:30]

lgtm

[dtapuska, 2016-05-13 15:52:11]

https://codereview.chromium.org/1965493002/diff/40001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1965493002/diff/40001/content/public/common/c...
content/public/common/content_switches.cc:678: // Default for passive listeners
being added. Values are defined as:
On 2016/05/12 20:30:16, Charlie Reis (slow to reply) wrote:
> Can you give some context for what passive listeners are in the comment?  It's
> not clear from context.

Done.

[dtapuska, 2016-05-13 15:52:16]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-05-13 15:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
thestig@chromium.org, asvitkine@chromium.org, creis@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1965493002/#ps60001
(title: "Fix comments")

[commit-bot: I haz the power, 2016-05-13 15:52:29]

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

[commit-bot: I haz the power, 2016-05-13 17:21:45]

Description was changed from

==========
Add runtime setting to force passive event listeners.

Add the ability to set the default value for the 'passive' field
in AddEventListenerOptions.

The value can take on 4 values; 'false', 'true', 'documentonlytrue',
'forcetrue'. It can be set from the command line or by adjusting
the chrome://flags.

BUG=599611
==========

to

==========
Add runtime setting to force passive event listeners.

Add the ability to set the default value for the 'passive' field
in AddEventListenerOptions.

The value can take on 4 values; 'false', 'true', 'documentonlytrue',
'forcetrue'. It can be set from the command line or by adjusting
the chrome://flags.

BUG=599611
==========

[commit-bot: I haz the power, 2016-05-13 17:21:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-13 17:24:07]

Description was changed from

==========
Add runtime setting to force passive event listeners.

Add the ability to set the default value for the 'passive' field
in AddEventListenerOptions.

The value can take on 4 values; 'false', 'true', 'documentonlytrue',
'forcetrue'. It can be set from the command line or by adjusting
the chrome://flags.

BUG=599611
==========

to

==========
Add runtime setting to force passive event listeners.

Add the ability to set the default value for the 'passive' field
in AddEventListenerOptions.

The value can take on 4 values; 'false', 'true', 'documentonlytrue',
'forcetrue'. It can be set from the command line or by adjusting
the chrome://flags.

BUG=599611

Committed: https://crrev.com/f65650b0471dfd3d5fda67ae634a986148ad2d3e
Cr-Commit-Position: refs/heads/master@{#393565}
==========

[commit-bot: I haz the power, 2016-05-13 17:24:09]

Patchset 4 (id:??) landed as
https://crrev.com/f65650b0471dfd3d5fda67ae634a986148ad2d3e
Cr-Commit-Position: refs/heads/master@{#393565}