Add enable flag for pointer events in about://flags

Add enable flag for pointer events in about://flags

Add a flag in about://flags which enables "PointerEvent" blink feature

BUG=575239
Committed: https://crrev.com/2d8d3f92673829a3db08adc6303a89eb36948ef0
Cr-Commit-Position: refs/heads/master@{#388524}

[Navid Zolghadr, 2016-01-07 19:05:58]

Description was changed from

==========
Add enable flag for pointer events in about://flags

Add a flag in about://flags which "PointerEvent" enable blink feature

BUG=575239
==========

to

==========
Add enable flag for pointer events in about://flags

Add a flag in about://flags which enables "PointerEvent" blink feature

BUG=575239
==========

[Navid Zolghadr, 2016-01-07 19:26:00]

nzolghadr@chromium.org changed reviewers:
+ mustaq@chromium.org, rbyers@chromium.org

[Navid Zolghadr, 2016-01-07 19:26:01]

I believe this change is blocked on others as Rick suggested. I just wanted to
have it here until it gets unblocked.

[Rick Byers, 2016-01-07 19:49:02]

https://codereview.chromium.org/1568913003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1568913003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5876: +        Enables the experimental
pointer events.
Let's try to be clear here about the risk of this.  Maybe something like:
"Enables partial experimental support for the Pointer Events API.  This is
intended only for testing by web developers, it will cause some websites to be
subtly broken."

https://codereview.chromium.org/1568913003/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1568913003/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2122: switches::kEnableBlinkFeatures,
I got some flack for using this cheapo approach to enabling specific blink
features by flags - see crbug.com/536913.  The problem is that
--enable-blink-features causes a scary "potential unsafe flag" warning and users
are confused about what triggered it.

An infobar itself isn't bad - we don't want users to forget they have an
incomplete feature enabled.  But if there's going to be an infobar, the text
should direct them to the actual flag they enabled.

So I think either we need to:
 1) just do the plumbing to add another switch (content_switches.cc) which
ultimately has the same effect as --enable-blink-features=PointerEvent, or
 2) Add some mechanism for customizing the text displayed when a flag is
enabled.

[mustaq, 2016-01-07 20:13:06]

https://codereview.chromium.org/1568913003/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1568913003/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2122: switches::kEnableBlinkFeatures,
On 2016/01/07 19:49:02, Rick Byers wrote:
> I got some flack for using this cheapo approach to enabling specific blink
> features by flags - see crbug.com/536913.  The problem is that
> --enable-blink-features causes a scary "potential unsafe flag" warning and
users
> are confused about what triggered it.
> 
> An infobar itself isn't bad - we don't want users to forget they have an
> incomplete feature enabled.  But if there's going to be an infobar, the text
> should direct them to the actual flag they enabled.
> 
> So I think either we need to:
>  1) just do the plumbing to add another switch (content_switches.cc) which
> ultimately has the same effect as --enable-blink-features=PointerEvent, or
>  2) Add some mechanism for customizing the text displayed when a flag is
> enabled.

I think a separate entry in content_switches is better. Customizing the infobar
text only for this one seems a bit overkill to me:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Navid Zolghadr, 2016-01-08 16:50:09]

ptal.

https://codereview.chromium.org/1568913003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1568913003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5876: +        Enables the experimental
pointer events.
On 2016/01/07 19:49:02, Rick Byers wrote:
> Let's try to be clear here about the risk of this.  Maybe something like:
> "Enables partial experimental support for the Pointer Events API.  This is
> intended only for testing by web developers, it will cause some websites to be
> subtly broken."

Done.

https://codereview.chromium.org/1568913003/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1568913003/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2122: switches::kEnableBlinkFeatures,
On 2016/01/07 20:13:06, mustaq wrote:
> On 2016/01/07 19:49:02, Rick Byers wrote:
> > I got some flack for using this cheapo approach to enabling specific blink
> > features by flags - see crbug.com/536913.  The problem is that
> > --enable-blink-features causes a scary "potential unsafe flag" warning and
> users
> > are confused about what triggered it.
> > 
> > An infobar itself isn't bad - we don't want users to forget they have an
> > incomplete feature enabled.  But if there's going to be an infobar, the text
> > should direct them to the actual flag they enabled.
> > 
> > So I think either we need to:
> >  1) just do the plumbing to add another switch (content_switches.cc) which
> > ultimately has the same effect as --enable-blink-features=PointerEvent, or
> >  2) Add some mechanism for customizing the text displayed when a flag is
> > enabled.
> 
> I think a separate entry in content_switches is better. Customizing the
infobar
> text only for this one seems a bit overkill to me:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Done.

[Rick Byers, 2016-01-08 19:38:24]

Looks great, thanks.  It's a little sad that so much plumbing is required
(compared to just passing --enable-blink-features=PointerEvent) but oh well...

https://codereview.chromium.org/1568913003/diff/60001/content/child/runtime_f...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/1568913003/diff/60001/content/child/runtime_f...
content/child/runtime_features.cc:211:
WebRuntimeFeatures::enableFeatureFromString(
nit: Might as well add a enablePointerEvent method to avoid going via a (more
error-prone) string.  Looks trivial to do.  Most of the RuntimeEnabledFeatures
machinery is auto-generated from RuntimeEnabledFeatures.in but apparently not
this little piece of glue :-(

[Alexei Svitkine (slow), 2016-01-08 19:50:44]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-01-08 19:50:45]

You can avoid a lot of this plumbing by using the base/feature_list.h API, which
does a lot of stuff for you. See:

https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/featur...

[Rick Byers, 2016-01-08 20:00:51]

Cool!  But that's basically what we were initially doing by relying on
--enable-blink-features=PointerEvent.  But the problem with that was the
warning we get for using the --enable-blink-features switch.  We probably
still want that warning, so we probably need a dedicated switch.  I guess
if we decided it was OK not to have a warning at all in this case, we could
replace --enable-blink-features with just --enable-feature (or, as I
mentioned earlier, we could improve the UI for the warning when
--enable-blink-features is used).

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2016-01-08 20:08:48]

Well, --enable-features doesn't trigger any warning - nor is it planned to.
There's special plumbing for it for about:flags that handles it correctly
via the FEATURE_VALUE_TYPE() macro.

On Fri, Jan 8, 2016 at 3:00 PM, Rick Byers <rbyers@chromium.org> wrote:

> Cool!  But that's basically what we were initially doing by relying on
> --enable-blink-features=PointerEvent.  But the problem with that was the
> warning we get for using the --enable-blink-features switch.  We probably
> still want that warning, so we probably need a dedicated switch.  I guess
> if we decided it was OK not to have a warning at all in this case, we could
> replace --enable-blink-features with just --enable-feature (or, as I
> mentioned earlier, we could improve the UI for the warning when
> --enable-blink-features is used).
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Navid Zolghadr, 2016-01-08 20:47:26]

I was wondering why some of the existing ones using "feature" instead of
"switch". I will take a closer look and post another patch with this new API.

[Rick Byers, 2016-01-08 20:53:28]

On 2016/01/08 20:47:26, Navid Zolghadr wrote:
> I was wondering why some of the existing ones using "feature" instead of
> "switch". I will take a closer look and post another patch with this new API.

But you explicitly want the warning, right (security is irrelevant, but
"stability will suffer" is accurate if you consider reliable web page behavior
as a type of "stability")?  If so you should probably just keep the CL you have.
 I'm happy either way though - shouldn't be a big deal and this is all temporary
anyway.

[Navid Zolghadr, 2016-01-08 21:30:01]

On 2016/01/08 20:53:28, Rick Byers wrote:
> On 2016/01/08 20:47:26, Navid Zolghadr wrote:
> > I was wondering why some of the existing ones using "feature" instead of
> > "switch". I will take a closer look and post another patch with this new
API.
> 
> But you explicitly want the warning, right (security is irrelevant, but
> "stability will suffer" is accurate if you consider reliable web page behavior
> as a type of "stability")?  If so you should probably just keep the CL you
have.
>  I'm happy either way though - shouldn't be a big deal and this is all
temporary
> anyway.

Oh. I forgot that this is temporary anyway too.
Btw, regarding the warning not sure how to test this. Although I added this new
flag to bad_flag_prompt file not sure why I don't get a particular warning for
this. I only see --no-sandbox warning which I usually run my Chrome with.

[Navid Zolghadr, 2016-01-11 18:14:30]

On 2016/01/08 21:30:01, Navid Zolghadr wrote:
> On 2016/01/08 20:53:28, Rick Byers wrote:
> > On 2016/01/08 20:47:26, Navid Zolghadr wrote:
> > > I was wondering why some of the existing ones using "feature" instead of
> > > "switch". I will take a closer look and post another patch with this new
> API.
> > 
> > But you explicitly want the warning, right (security is irrelevant, but
> > "stability will suffer" is accurate if you consider reliable web page
behavior
> > as a type of "stability")?  If so you should probably just keep the CL you
> have.
> >  I'm happy either way though - shouldn't be a big deal and this is all
> temporary
> > anyway.
> 
> Oh. I forgot that this is temporary anyway too.
> Btw, regarding the warning not sure how to test this. Although I added this
new
> flag to bad_flag_prompt file not sure why I don't get a particular warning for
> this. I only see --no-sandbox warning which I usually run my Chrome with.

I realized to be able to use the new way of adding Chrome feature I need to not
use some machinery we have for Blink features. However, since pointer event is a
Blink feature and we would like to take advantage of state of the features in
blink (e.g. experimental) we better stick with what Blink has for adding
features which the last CL is already using. So after a quick discussion with
Rick we decided to keep the CL as is.

[Alexei Svitkine (slow), 2016-01-11 18:58:36]

On 2016/01/11 18:14:30, Navid Zolghadr wrote:
> On 2016/01/08 21:30:01, Navid Zolghadr wrote:
> > On 2016/01/08 20:53:28, Rick Byers wrote:
> > > On 2016/01/08 20:47:26, Navid Zolghadr wrote:
> > > > I was wondering why some of the existing ones using "feature" instead of
> > > > "switch". I will take a closer look and post another patch with this new
> > API.
> > > 
> > > But you explicitly want the warning, right (security is irrelevant, but
> > > "stability will suffer" is accurate if you consider reliable web page
> behavior
> > > as a type of "stability")?  If so you should probably just keep the CL you
> > have.
> > >  I'm happy either way though - shouldn't be a big deal and this is all
> > temporary
> > > anyway.
> > 
> > Oh. I forgot that this is temporary anyway too.
> > Btw, regarding the warning not sure how to test this. Although I added this
> new
> > flag to bad_flag_prompt file not sure why I don't get a particular warning
for
> > this. I only see --no-sandbox warning which I usually run my Chrome with.
> 
> I realized to be able to use the new way of adding Chrome feature I need to
not
> use some machinery we have for Blink features. However, since pointer event is
a
> Blink feature and we would like to take advantage of state of the features in
> blink (e.g. experimental) we better stick with what Blink has for adding
> features which the last CL is already using. So after a quick discussion with
> Rick we decided to keep the CL as is.

I don't think the two are incompatible.

I think if you use FeatureList API, you can still use the Blink feature system.
You just need to change:

 if (command_line.HasSwitch(switches::kEnablePointerEvents))
  WebRuntimeFeatures::enableFeatureFromString(...);

to

if (base::FeatureList::IsEnabled(features::kEnablePointerEvents))
  WebRuntimeFeatures::enableFeatureFromString(...);

So, this still uses the Blink feature system but eliminates the need for extra
plumbing at the Chrome/ level.

[Rick Byers, 2016-01-12 16:41:48]

On 2016/01/11 18:58:36, Alexei Svitkine wrote:
> On 2016/01/11 18:14:30, Navid Zolghadr wrote:
> > On 2016/01/08 21:30:01, Navid Zolghadr wrote:
> > > On 2016/01/08 20:53:28, Rick Byers wrote:
> > > > On 2016/01/08 20:47:26, Navid Zolghadr wrote:
> > > > > I was wondering why some of the existing ones using "feature" instead
of
> > > > > "switch". I will take a closer look and post another patch with this
new
> > > API.
> > > > 
> > > > But you explicitly want the warning, right (security is irrelevant, but
> > > > "stability will suffer" is accurate if you consider reliable web page
> > behavior
> > > > as a type of "stability")?  If so you should probably just keep the CL
you
> > > have.
> > > >  I'm happy either way though - shouldn't be a big deal and this is all
> > > temporary
> > > > anyway.
> > > 
> > > Oh. I forgot that this is temporary anyway too.
> > > Btw, regarding the warning not sure how to test this. Although I added
this
> > new
> > > flag to bad_flag_prompt file not sure why I don't get a particular warning
> for
> > > this. I only see --no-sandbox warning which I usually run my Chrome with.
> > 
> > I realized to be able to use the new way of adding Chrome feature I need to
> not
> > use some machinery we have for Blink features. However, since pointer event
is
> a
> > Blink feature and we would like to take advantage of state of the features
in
> > blink (e.g. experimental) we better stick with what Blink has for adding
> > features which the last CL is already using. So after a quick discussion
with
> > Rick we decided to keep the CL as is.
> 
> I don't think the two are incompatible.
> 
> I think if you use FeatureList API, you can still use the Blink feature
system.
> You just need to change:
> 
>  if (command_line.HasSwitch(switches::kEnablePointerEvents))
>   WebRuntimeFeatures::enableFeatureFromString(...);
> 
> to
> 
> if (base::FeatureList::IsEnabled(features::kEnablePointerEvents))
>   WebRuntimeFeatures::enableFeatureFromString(...);
> 
> So, this still uses the Blink feature system but eliminates the need for extra
> plumbing at the Chrome/ level.

Either way is fine with me, this will all go away once we promote pointer events
to "experimental" status (hopefully sometime in early Q2).  So it's not worth
worrying about too much IMHO.

[Navid Zolghadr, 2016-01-13 16:33:54]

I made use of Chrome feature as per Alexei's suggestion to make this CL and the
consequent CL which gets rid of these changes smaller :). ptal.

[Alexei Svitkine (slow), 2016-01-13 17:17:58]

lgtm, thanks

https://codereview.chromium.org/1568913003/diff/100001/content/public/common/...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/1568913003/diff/100001/content/public/common/...
content/public/common/content_features.cc:26: // Partial support for pointer
event feature
Nit: add a . at the end

[Navid Zolghadr, 2016-04-20 15:12:48]

ptal

https://codereview.chromium.org/1568913003/diff/100001/content/public/common/...
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/1568913003/diff/100001/content/public/common/...
content/public/common/content_features.cc:26: // Partial support for pointer
event feature
On 2016/01/13 17:17:58, Alexei Svitkine wrote:
> Nit: add a . at the end

Done.

[Rick Byers, 2016-04-20 15:25:03]

LGTM

[Navid Zolghadr, 2016-04-20 16:08:57]

nzolghadr@chromium.org changed reviewers:
+ creis@chromium.org

[Navid Zolghadr, 2016-04-20 16:08:58]

creis@chromium.org: Please review changes in 

content/*

[Charlie Reis, 2016-04-20 16:42:32]

content/ LGTM.

Bug 567740 is fixed now, right?  (I'm looking at https://crbug.com/575239#c1.)

[Navid Zolghadr, 2016-04-20 17:10:05]

On 2016/04/20 16:42:32, Charlie Reis wrote:
> content/ LGTM.
> 
> Bug 567740 is fixed now, right?  (I'm looking at https://crbug.com/575239#c1.)

Yes. The CL landed yesterday.

[Navid Zolghadr, 2016-04-20 17:10:42]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-04-20 17:10:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1568913003/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-04-20 17:10:51]

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

[commit-bot: I haz the power, 2016-04-20 17:14:47]

Description was changed from

==========
Add enable flag for pointer events in about://flags

Add a flag in about://flags which enables "PointerEvent" blink feature

BUG=575239
==========

to

==========
Add enable flag for pointer events in about://flags

Add a flag in about://flags which enables "PointerEvent" blink feature

BUG=575239
==========

[commit-bot: I haz the power, 2016-04-20 17:14:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-22 19:24:31]

Description was changed from

==========
Add enable flag for pointer events in about://flags

Add a flag in about://flags which enables "PointerEvent" blink feature

BUG=575239
==========

to

==========
Add enable flag for pointer events in about://flags

Add a flag in about://flags which enables "PointerEvent" blink feature

BUG=575239

Committed: https://crrev.com/2d8d3f92673829a3db08adc6303a89eb36948ef0
Cr-Commit-Position: refs/heads/master@{#388524}
==========

[commit-bot: I haz the power, 2016-04-22 19:24:32]

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