Plugin Power Saver: Implement option in Settings.

Plugin Power Saver: Implement option in Settings.

Relevant review here: https://groups.google.com/a/google.com/d/msg/chrome-ui-review/i5ii03kq8sg/LlItQiRRmjkJ

BUG=403800
Committed: https://crrev.com/a23713b67ee741e5bb026d99e86b8c97cc9ab84d
Cr-Commit-Position: refs/heads/master@{#308876}

[tommycli, 2014-12-10 21:31:44]

tommycli@chromium.org changed reviewers:
+ thestig@chromium.org

[tommycli, 2014-12-10 21:31:44]

thestig: May I have a chrome/ review?
groby: +cc

https://codereview.chromium.org/796543002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (left):

https://codereview.chromium.org/796543002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:687: status_value ==
ChromeViewHostMsg_GetPluginInfo_Status::kClickToPlay ||
I don't think the temporary exemption whitelist should apply to Plugin Power
Saver.

https://codereview.chromium.org/796543002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/796543002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:808: show_poster ?
IDR_PLUGIN_POSTER_HTML : IDR_BLOCKED_PLUGIN_HTML,
I think this is fine because the actual HTML is not shown during pre-rendering.
You could use anything.

[Lei Zhang, 2014-12-10 21:51:01]

thestig@chromium.org changed reviewers:
+ bauerb@chromium.org

[Lei Zhang, 2014-12-10 21:51:01]

Fine by me, but bauerb probably should have a look too.

https://codereview.chromium.org/796543002/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/796543002/diff/1/chrome/common/render_message...
chrome/common/render_messages.h:53: kPlayImportantContent,
nit: alphabetical order

[tommycli, 2014-12-10 21:52:55]

thanks. I see you have already included bauerb. cool.

https://codereview.chromium.org/796543002/diff/1/chrome/common/render_messages.h
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/796543002/diff/1/chrome/common/render_message...
chrome/common/render_messages.h:53: kPlayImportantContent,
On 2014/12/10 21:51:01, Lei Zhang wrote:
> nit: alphabetical order

Done.

[Bernhard Bauer, 2014-12-11 14:42:21]

+palmer, pkasting

https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:8030: -        Click to play
Hm... this changes the semantics of click-to-play, which might not be what
existing users expect.

Also, have these strings been reviewed?

[tommycli, 2014-12-11 17:11:19]

bauerb:

I should have linked this originally. This change in behavior has been reviewed
by both laforge, palmer, and UI people. See:

https://groups.google.com/a/google.com/d/msg/chrome-ui-review/i5ii03kq8sg/LlI...

https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:8030: -        Click to play
On 2014/12/11 14:42:21, Bernhard Bauer wrote:
> Hm... this changes the semantics of click-to-play, which might not be what
> existing users expect.
> 
> Also, have these strings been reviewed?

Yes. The behavior is going to change.

[tommycli, 2014-12-11 17:12:09]

tommycli@chromium.org changed reviewers:
+ isherman@chromium.org

[tommycli, 2014-12-11 17:12:10]

isherman: May I have a tools/ review?

[Bernhard Bauer, 2014-12-11 17:28:29]

LGTM, although I'm still somewhat reluctant about changing the behavior for
users who have previously selected click-to-play. But we can see what happens.

Also, what does this mean for non-Flash plugins?

https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:8030: -        Click to play
On 2014/12/11 17:11:19, tommycli wrote:
> On 2014/12/11 14:42:21, Bernhard Bauer wrote:
> > Hm... this changes the semantics of click-to-play, which might not be what
> > existing users expect.
> > 
> > Also, have these strings been reviewed?
> 
> Yes. The behavior is going to change.

OK... Given that this uses content settings behind the scenes and therefore can
have site-specific exceptions, have we thought about integrating with that?

[tommycli, 2014-12-11 18:00:44]

Hey Bernhard,

We plan to migrate users who have already chosen "Click to Play" to "Block". I
haven't figured out how to do that yet, but it's on the plan.

For non-Flash plugins, the throttling mechanism never triggers, so it should
behave the same as 'Allow'.

Site level exceptions still work. It remains a 'janky' feature though, since the
middle option is always labeled as 'Ask'. This change doesn't make it any
jankier than it already was though.

Cheers,

Tommy

On 2014/12/11 17:28:29, Bernhard Bauer wrote:
> LGTM, although I'm still somewhat reluctant about changing the behavior for
> users who have previously selected click-to-play. But we can see what happens.
> 
> Also, what does this mean for non-Flash plugins?
> 
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> File chrome/app/generated_resources.grd (left):
> 
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> chrome/app/generated_resources.grd:8030: -        Click to play
> On 2014/12/11 17:11:19, tommycli wrote:
> > On 2014/12/11 14:42:21, Bernhard Bauer wrote:
> > > Hm... this changes the semantics of click-to-play, which might not be what
> > > existing users expect.
> > > 
> > > Also, have these strings been reviewed?
> > 
> > Yes. The behavior is going to change.
> 
> OK... Given that this uses content settings behind the scenes and therefore
can
> have site-specific exceptions, have we thought about integrating with that?

[tommycli, 2014-12-11 18:06:12]

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

[tommycli, 2014-12-11 18:06:12]

creis: May i have a content/ review? thanks

[Bernhard Bauer, 2014-12-11 18:15:21]

Thanks for the explanations! Some comments inline:

On 2014/12/11 18:00:44, tommycli wrote:
> Hey Bernhard,
> 
> We plan to migrate users who have already chosen "Click to Play" to "Block". I
> haven't figured out how to do that yet, but it's on the plan.

In that case, it might be easier to add a new setting that is not ASK (content
settings can be arbitrary values), so there is no confusion between
pre-migration ASK (i.e. click-to-play which should be migrated to BLOCK) and
post-migration ASK (i.e. power saver). 

> For non-Flash plugins, the throttling mechanism never triggers, so it should
> behave the same as 'Allow'.

OK... I guess that's acceptable? Flash is probably the only thing people really
care about.

> Site level exceptions still work. It remains a 'janky' feature though, since
the
> middle option is always labeled as 'Ask'. This change doesn't make it any
> jankier than it already was though.

Yes, I think the "Ask" thing is an old UI bug. What I was thinking about though
was that we could persist the setting if the user decides to run the peripheral
content, by setting the content setting to ALLOW.

> Cheers,
> 
> Tommy
> 
> On 2014/12/11 17:28:29, Bernhard Bauer wrote:
> > LGTM, although I'm still somewhat reluctant about changing the behavior for
> > users who have previously selected click-to-play. But we can see what
happens.
> > 
> > Also, what does this mean for non-Flash plugins?
> > 
> >
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> > File chrome/app/generated_resources.grd (left):
> > 
> >
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> > chrome/app/generated_resources.grd:8030: -        Click to play
> > On 2014/12/11 17:11:19, tommycli wrote:
> > > On 2014/12/11 14:42:21, Bernhard Bauer wrote:
> > > > Hm... this changes the semantics of click-to-play, which might not be
what
> > > > existing users expect.
> > > > 
> > > > Also, have these strings been reviewed?
> > > 
> > > Yes. The behavior is going to change.
> > 
> > OK... Given that this uses content settings behind the scenes and therefore
> can
> > have site-specific exceptions, have we thought about integrating with that?

[groby-ooo-7-16, 2014-12-11 19:14:11]

More comments inline...

On 2014/12/11 18:15:21, Bernhard Bauer wrote:
> Thanks for the explanations! Some comments inline:
> 
> On 2014/12/11 18:00:44, tommycli wrote:
> > Hey Bernhard,
> > 
> > We plan to migrate users who have already chosen "Click to Play" to "Block".
I
> > haven't figured out how to do that yet, but it's on the plan.

We probably should only change the settings strings once we have the migration
in place.  Just in case there's a hiccup with the migration, and we don't finish
it by branch cut.

> 
> In that case, it might be easier to add a new setting that is not ASK (content
> settings can be arbitrary values), so there is no confusion between
> pre-migration ASK (i.e. click-to-play which should be migrated to BLOCK) and
> post-migration ASK (i.e. power saver). 

Difficult, since it shares a common settings mechanism with many other settings
that are all ALLOW/ASK/BLOCK.

> 
> > For non-Flash plugins, the throttling mechanism never triggers, so it should
> > behave the same as 'Allow'.

I'm not sure it should. Do we know what other plugins are most common?

> 
> OK... I guess that's acceptable? Flash is probably the only thing people
really
> care about.
> 
> > Site level exceptions still work. It remains a 'janky' feature though, since
> the
> > middle option is always labeled as 'Ask'. This change doesn't make it any
> > jankier than it already was though.
> 
> Yes, I think the "Ask" thing is an old UI bug. What I was thinking about
though
> was that we could persist the setting if the user decides to run the
peripheral
> content, by setting the content setting to ALLOW.

You mean on a per-site basis? Again, reluctant. A single click-through does not
necessarily constitute intent to always run peripheral content.

> 
> > Cheers,
> > 
> > Tommy
> > 
> > On 2014/12/11 17:28:29, Bernhard Bauer wrote:
> > > LGTM, although I'm still somewhat reluctant about changing the behavior
for
> > > users who have previously selected click-to-play. But we can see what
> happens.
> > > 
> > > Also, what does this mean for non-Flash plugins?
> > > 
> > >
> >
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> > > File chrome/app/generated_resources.grd (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> > > chrome/app/generated_resources.grd:8030: -        Click to play
> > > On 2014/12/11 17:11:19, tommycli wrote:
> > > > On 2014/12/11 14:42:21, Bernhard Bauer wrote:
> > > > > Hm... this changes the semantics of click-to-play, which might not be
> what
> > > > > existing users expect.
> > > > > 
> > > > > Also, have these strings been reviewed?
> > > > 
> > > > Yes. The behavior is going to change.
> > > 
> > > OK... Given that this uses content settings behind the scenes and
therefore
> > can
> > > have site-specific exceptions, have we thought about integrating with
that?

[groby-ooo-7-16, 2014-12-11 19:22:04]

Sorry, a few more comments

https://codereview.chromium.org/796543002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/796543002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2016: },
We shouldn't remove the flag. If we did, it'd remove our ability to run this as
an experiment.

https://codereview.chromium.org/796543002/diff/20001/chrome/common/render_mes...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/796543002/diff/20001/chrome/common/render_mes...
chrome/common/render_messages.h:58: kPlayImportantContent,
Alphabetically ordering an enum is ... weird :) But I suppose that's the way it
is. 

I hope it's OK for constants in here to change their value? (I.e. this is never
persisted?)

https://codereview.chromium.org/796543002/diff/20001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (left):

https://codereview.chromium.org/796543002/diff/20001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:898:
l10n_util::GetStringFUTF16(IDS_PLUGIN_LOAD, group_name), GURL());
Do we still need IDS_PLUGIN_LOAD?

https://codereview.chromium.org/796543002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/796543002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10007: <obsolete>Left-click to play plugins
has been removed.</obsolete>
Do we need a new metric?

[Bernhard Bauer, 2014-12-11 19:41:23]

On 2014/12/11 19:14:11, groby wrote:
> More comments inline...
> 
> On 2014/12/11 18:15:21, Bernhard Bauer wrote:
> > Thanks for the explanations! Some comments inline:
> > 
> > On 2014/12/11 18:00:44, tommycli wrote:
> > > Hey Bernhard,
> > > 
> > > We plan to migrate users who have already chosen "Click to Play" to
"Block".
> I
> > > haven't figured out how to do that yet, but it's on the plan.
> 
> We probably should only change the settings strings once we have the migration
> in place.  Just in case there's a hiccup with the migration, and we don't
finish
> it by branch cut.
> 
> > 
> > In that case, it might be easier to add a new setting that is not ASK
(content
> > settings can be arbitrary values), so there is no confusion between
> > pre-migration ASK (i.e. click-to-play which should be migrated to BLOCK) and
> > post-migration ASK (i.e. power saver). 
> 
> Difficult, since it shares a common settings mechanism with many other
settings
> that are all ALLOW/ASK/BLOCK.

Which mechanism are you talking about? Content settings can use arbitrary
values, it just happens that most of them use ALLOW and BLOCK (and some of them
ASK). There are other settings though that don't even use numeric values. In
this particular case, we could extend the ContentSetting enum with more values.

https://codereview.chromium.org/796543002/diff/20001/chrome/common/render_mes...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/796543002/diff/20001/chrome/common/render_mes...
chrome/common/render_messages.h:58: kPlayImportantContent,
On 2014/12/11 19:22:03, groby wrote:
> Alphabetically ordering an enum is ... weird :) But I suppose that's the way
it
> is. 
> 
> I hope it's OK for constants in here to change their value? (I.e. this is
never
> persisted?)

Correct, this is only for messages between browser and renderer.

[Peter Kasting, 2014-12-11 19:52:44]

Since I got CCed on this review, my only comment is that we should ensure (in
this and subsequent reviews) that either:
* Click-to-play users stay in some mode where no plugins run by default, but
they can be clicked in-page to run (you don't have to mess with the content
settings icon in the omnibox); or
* We make click-to-play be as above but with autorunning things like 1x1
plugins, and we actively message to users that the behavior has changed, and
what their options are to act on it.

Bernhard is capable of handling all content settings review issues here.

[groby-ooo-7-16, 2014-12-11 21:45:18]

Rietveld chokes on the comment, so let's try e-mail....

Bernhard: I was under the impression ContentSettingsBubbleModel & WebUI are
kind-of tied to ContentSettings, but on closer reading, it seems we might
be able to add the type _relatively_ easily.
But given that ContentSettings is shared with web permissions (GeoLocation,
etc.) I'm not sure adding an additional value to the  enum would be the
best approach. Maybe map in the bubble model, similar to RPH_IGNORE?

Peter: All existing users with click-to-play will be migrated to BLOCK.
(i.e. right-click to play). That's the migration mentioned above, which I
think needs to land before this CL lands.
The new setting is strictly opt-in.


On Thu, Dec 11, 2014 at 11:52 AM, <pkasting@chromium.org> wrote:

> Since I got CCed on this review, my only comment is that we should ensure
> (in
> this and subsequent reviews) that either:
> * Click-to-play users stay in some mode where no plugins run by default,
> but
> they can be clicked in-page to run (you don't have to mess with the content
> settings icon in the omnibox); or
> * We make click-to-play be as above but with autorunning things like 1x1
> plugins, and we actively message to users that the behavior has changed,
> and
> what their options are to act on it.
>
> Bernhard is capable of handling all content settings review issues here.
>
> https://codereview.chromium.org/796543002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-12-11 21:52:26]

On Thu, Dec 11, 2014 at 1:44 PM, Rachel Blum <groby@chromium.org> wrote:

> Rietveld chokes on the comment, so let's try e-mail....
>

Argh, Rietveld ate my reply too.

Peter: All existing users with click-to-play will be migrated to BLOCK.
> (i.e. right-click to play). That's the migration mentioned above, which I
> think needs to land before this CL lands.
> The new setting is strictly opt-in.
>

As an existing click-to-play user and advocate, I think that's a mistake.

The general reason for using C2P is "block annoyances but still make the
web easy to use".  From what I understand of this new mode, it's much
closer to that spirit than BLOCK is.  And BLOCK will feel broken to users
(they'll click things and it won't work, and they won't think to
right-click), and even when they figure it out, right-clicking is
significantly less convenient when you enable plugins frequently (which
many of us do).

So I think if you're not going to preserve an actual click-to-play setting
(the value of doing so depends on the precise details of this new mode, but
it's probably not worth doing), it would be much better to migrate users to
the new mode, instead of migrating them to BLOCK.

If I've misunderstood something about the new mode that invalidates this
argument, please let me know.

PK

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bernhard Bauer, 2014-12-11 23:00:34]

On Thu Dec 11 2014 at 9:45:18 PM Rachel Blum <groby@chromium.org> wrote:

> Rietveld chokes on the comment, so let's try e-mail....
>
> Bernhard: I was under the impression ContentSettingsBubbleModel & WebUI
> are kind-of tied to ContentSettings, but on closer reading, it seems we
> might be able to add the type _relatively_ easily.
> But given that ContentSettings is shared with web permissions
> (GeoLocation, etc.) I'm not sure adding an additional value to the  enum
> would be the best approach. Maybe map in the bubble model, similar to
> RPH_IGNORE?
>

There shouldn't be a problem with adding an additional value to the
ContentSetting enum. There is no code that depends on the full range of all
possible ContentSetting values (if there was, it would already have
problems with the setting SESSION_ONLY, which is only used for cookies).

All of that being said, I'm starting to come around to the idea of
migrating from c2p to power saver, in which case we wouldn't need any
migration anyway (but we could still do it, seeing as ASK doesn't really
reflect either c2p nor power saver).


> Peter: All existing users with click-to-play will be migrated to BLOCK.
> (i.e. right-click to play). That's the migration mentioned above, which I
> think needs to land before this CL lands.
> The new setting is strictly opt-in.
>
>
> On Thu, Dec 11, 2014 at 11:52 AM, <pkasting@chromium.org> wrote:
>
>> Since I got CCed on this review, my only comment is that we should ensure
>> (in
>> this and subsequent reviews) that either:
>> * Click-to-play users stay in some mode where no plugins run by default,
>> but
>> they can be clicked in-page to run (you don't have to mess with the
>> content
>> settings icon in the omnibox); or
>> * We make click-to-play be as above but with autorunning things like 1x1
>> plugins, and we actively message to users that the behavior has changed,
>> and
>> what their options are to act on it.
>>
>> Bernhard is capable of handling all content settings review issues here.
>>
>> https://codereview.chromium.org/796543002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-12-11 23:07:35]

actions.xml lgtm -- thanks

[tommycli, 2014-12-12 00:45:23]

groby: addressed your in-code comments below

https://codereview.chromium.org/796543002/diff/20001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/796543002/diff/20001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2016: },
On 2014/12/11 19:22:03, groby wrote:
> We shouldn't remove the flag. If we did, it'd remove our ability to run this
as
> an experiment.

Done.

https://codereview.chromium.org/796543002/diff/20001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (left):

https://codereview.chromium.org/796543002/diff/20001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:898:
l10n_util::GetStringFUTF16(IDS_PLUGIN_LOAD, group_name), GURL());
On 2014/12/11 19:22:03, groby wrote:
> Do we still need IDS_PLUGIN_LOAD?

Done. You are correct. This is now extraneous.

https://codereview.chromium.org/796543002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/796543002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:10007: <obsolete>Left-click to play plugins
has been removed.</obsolete>
On 2014/12/11 19:22:04, groby wrote:
> Do we need a new metric?

I don't think so. I think all the cases are now covered by Plugin_Blocked (Right
click to play) and our various throttle metrics

[tommycli, 2014-12-12 00:47:38]

groby: addressed some more comments inline below:

On 2014/12/11 19:14:11, groby wrote:
> More comments inline...
> 
> On 2014/12/11 18:15:21, Bernhard Bauer wrote:
> > Thanks for the explanations! Some comments inline:
> > 
> > On 2014/12/11 18:00:44, tommycli wrote:
> > > Hey Bernhard,
> > > 
> > > We plan to migrate users who have already chosen "Click to Play" to
"Block".
> I
> > > haven't figured out how to do that yet, but it's on the plan.
> 
> We probably should only change the settings strings once we have the migration
> in place.  Just in case there's a hiccup with the migration, and we don't
finish
> it by branch cut.
> 
> > 
> > In that case, it might be easier to add a new setting that is not ASK
(content
> > settings can be arbitrary values), so there is no confusion between
> > pre-migration ASK (i.e. click-to-play which should be migrated to BLOCK) and
> > post-migration ASK (i.e. power saver). 
> 
> Difficult, since it shares a common settings mechanism with many other
settings
> that are all ALLOW/ASK/BLOCK.
> 
> > 
> > > For non-Flash plugins, the throttling mechanism never triggers, so it
should
> > > behave the same as 'Allow'.
> 
> I'm not sure it should. Do we know what other plugins are most common?

No idea. It's pretty trivial to apply this to all Pepper plugins (by removing
the Flash check). That would probably be the most 'fair'. I feel like there are
no other Pepper plugins.

> 
> > 
> > OK... I guess that's acceptable? Flash is probably the only thing people
> really
> > care about.
> > 
> > > Site level exceptions still work. It remains a 'janky' feature though,
since
> > the
> > > middle option is always labeled as 'Ask'. This change doesn't make it any
> > > jankier than it already was though.
> > 
> > Yes, I think the "Ask" thing is an old UI bug. What I was thinking about
> though
> > was that we could persist the setting if the user decides to run the
> peripheral
> > content, by setting the content setting to ALLOW.
> 
> You mean on a per-site basis? Again, reluctant. A single click-through does
not
> necessarily constitute intent to always run peripheral content.
> 

Agreed. A click through should not (and doesn't in this patch) add it to the
per-site whitelist.

> > 
> > > Cheers,
> > > 
> > > Tommy
> > > 
> > > On 2014/12/11 17:28:29, Bernhard Bauer wrote:
> > > > LGTM, although I'm still somewhat reluctant about changing the behavior
> for
> > > > users who have previously selected click-to-play. But we can see what
> > happens.
> > > > 
> > > > Also, what does this mean for non-Flash plugins?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> > > > File chrome/app/generated_resources.grd (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/796543002/diff/20001/chrome/app/generated_res...
> > > > chrome/app/generated_resources.grd:8030: -        Click to play
> > > > On 2014/12/11 17:11:19, tommycli wrote:
> > > > > On 2014/12/11 14:42:21, Bernhard Bauer wrote:
> > > > > > Hm... this changes the semantics of click-to-play, which might not
be
> > what
> > > > > > existing users expect.
> > > > > > 
> > > > > > Also, have these strings been reviewed?
> > > > > 
> > > > > Yes. The behavior is going to change.
> > > > 
> > > > OK... Given that this uses content settings behind the scenes and
> therefore
> > > can
> > > > have site-specific exceptions, have we thought about integrating with
> that?

[Charlie Reis, 2014-12-12 17:28:26]

On 2014/12/11 18:06:12, tommycli wrote:
> creis: May i have a content/ review? thanks

Looks like you don't need me anymore in the latest patches.  Feel free to ping
me if that changes.

[tommycli, 2014-12-12 20:17:50]

bauerb: PTAL. The latest patchset implements the scheme we discussed over email.

generated_resources.grd also changed in the latest patchset btw.

https://codereview.chromium.org/796543002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/796543002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/content_settings_exceptions_area.js:280: else
if (setting == 'ask')
Letting this stick around to support users with legacy exception settings.

[tommycli, 2014-12-15 15:35:10]

bauerb: friendly ping. don't mean to be a nag. london time & PST offset in worst
possible way. thanks

[Bernhard Bauer, 2014-12-15 17:19:31]

LGTM! Sorry for the late reply; this got buried over the weekend.

[tommycli, 2014-12-15 17:25:33]

bauerb: No problemo. Thank you sir1

[tommycli, 2014-12-15 17:26:08]

thestig: May i have a chrome/ OWNER stamp?

[tommycli, 2014-12-15 22:43:42]

tommycli@chromium.org changed reviewers:
+ tsepez@chromium.org
- creis@chromium.org, groby@chromium.org, isherman@chromium.org,
pkasting@chromium.org

[tommycli, 2014-12-15 22:44:05]

tsepez: May I have a review for chrome/common/render_messages.h please?

Thanks.

[Tom Sepez, 2014-12-15 22:52:01]

lgtm

[Lei Zhang, 2014-12-15 22:58:08]

Looking...

Do we need to update the enterprise setting template?
components/policy/resources/policy_templates.json

[tommycli, 2014-12-15 23:00:56]

On 2014/12/15 22:58:08, Lei Zhang wrote:
> Looking...
> 
> Do we need to update the enterprise setting template?
> components/policy/resources/policy_templates.json

You are correct. Yes we do. I will do that now.

[tommycli, 2014-12-15 23:06:32]

On 2014/12/15 23:00:56, tommycli wrote:
> On 2014/12/15 22:58:08, Lei Zhang wrote:
> > Looking...
> > 
> > Do we need to update the enterprise setting template?
> > components/policy/resources/policy_templates.json
> 
> You are correct. Yes we do. I will do that now.

I fixed the policy templates.

http://www.chromium.org/developers/design-documents/ui-localization doesn't seem
to mention policy_templates.grd/json. Do you know if we need to take any further
action to make sure the translations are good?

[tommycli, 2014-12-15 23:12:24]

tommycli@chromium.org changed reviewers:
+ atwilson@chromium.org

[tommycli, 2014-12-15 23:12:25]

atwilson: May I have a components/policy review? Thanks.

Be aware: I am changing one of the enum values. Do we need to take any actions
to gracefully migrate existing enterprise users who are on an obsolete enum
value?

For individual users with the old enum setting, we silently reinterpret the old
value (ASK) as one of the new values (BLOCK). The old ASK has been replaced with
a new DETECT option.

[tommycli, 2014-12-15 23:13:07]

On 2014/12/15 23:12:25, tommycli wrote:
> atwilson: May I have a components/policy review? Thanks.
> 
> Be aware: I am changing one of the enum values. Do we need to take any actions
> to gracefully migrate existing enterprise users who are on an obsolete enum
> value?
> 
> For individual users with the old enum setting, we silently reinterpret the
old
> value (ASK) as one of the new values (BLOCK). The old ASK has been replaced
with
> a new DETECT option.

Some context: We don't actually want to migrate user's settings, as this change
is experimental, and may be reverted at some point.

[Bernhard Bauer, 2014-12-15 23:15:15]

On 2014/12/15 23:13:07, tommycli wrote:
> On 2014/12/15 23:12:25, tommycli wrote:
> > atwilson: May I have a components/policy review? Thanks.
> > 
> > Be aware: I am changing one of the enum values. Do we need to take any
actions
> > to gracefully migrate existing enterprise users who are on an obsolete enum
> > value?
> > 
> > For individual users with the old enum setting, we silently reinterpret the
> old
> > value (ASK) as one of the new values (BLOCK). The old ASK has been replaced
> with
> > a new DETECT option.
> 
> Some context: We don't actually want to migrate user's settings, as this
change
> is experimental, and may be reverted at some point.

Do we want to keep the ASK value in the policy templates for now, and only
remove it when we officially deprecate the option? And/or never remove it from
the template, just mark it as "Don't use this".

[tommycli, 2014-12-15 23:33:05]

On 2014/12/15 23:15:15, Bernhard Bauer wrote:
> On 2014/12/15 23:13:07, tommycli wrote:
> > On 2014/12/15 23:12:25, tommycli wrote:
> > > atwilson: May I have a components/policy review? Thanks.
> > > 
> > > Be aware: I am changing one of the enum values. Do we need to take any
> actions
> > > to gracefully migrate existing enterprise users who are on an obsolete
enum
> > > value?
> > > 
> > > For individual users with the old enum setting, we silently reinterpret
the
> > old
> > > value (ASK) as one of the new values (BLOCK). The old ASK has been
replaced
> > with
> > > a new DETECT option.
> > 
> > Some context: We don't actually want to migrate user's settings, as this
> change
> > is experimental, and may be reverted at some point.
> 
> Do we want to keep the ASK value in the policy templates for now, and only
> remove it when we officially deprecate the option? And/or never remove it from
> the template, just mark it as "Don't use this".

That sounds right. thestig just linked me to this:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add...,
which said that removed policies should be deprecated over a number of releases.

That makes sense. The enterprise policies need to continue to support 'ask' for
a while.

Two open questions:
1. Are we shooting ourselves in the foot by adding 'detect' to the policy
options? (Would we have to do a long deprecation process in the future if it
doesn't work out?)
2. Do we have to do anything to get the new settings translated?

[Lei Zhang, 2014-12-16 00:00:38]

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

https://codereview.chromium.org/796543002/diff/160001/chrome/app/generated_re...
chrome/app/generated_resources.grd:8030: +        Do not run plugins by default
Do we want to keep the "You can manually run a blocked plug-in at any time."
note?

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(left):

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:61:
CONTENT_SETTINGS_TYPE_PLUGINS, CONTENT_SETTING_ASK);
Do you still need to test the ASK case?

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2065:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderClickToPlay) {
Change the name of the test accordingly?

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/resource...
chrome/browser/resources/options/content_settings_exceptions_area.js:280: else
if (setting == 'ask')
WRT to the ask state. Can we make sure if the user has "ask" as the preference
setting, then it shows up in the settings UI as blocked? i.e. Make sure we never
show a UI in the limbo state to the user.

[Bernhard Bauer, 2014-12-16 09:12:36]

On 2014/12/15 23:33:05, tommycli wrote:
> On 2014/12/15 23:15:15, Bernhard Bauer wrote:
> > On 2014/12/15 23:13:07, tommycli wrote:
> > > On 2014/12/15 23:12:25, tommycli wrote:
> > > > atwilson: May I have a components/policy review? Thanks.
> > > > 
> > > > Be aware: I am changing one of the enum values. Do we need to take any
> > actions
> > > > to gracefully migrate existing enterprise users who are on an obsolete
> enum
> > > > value?
> > > > 
> > > > For individual users with the old enum setting, we silently reinterpret
> the
> > > old
> > > > value (ASK) as one of the new values (BLOCK). The old ASK has been
> replaced
> > > with
> > > > a new DETECT option.
> > > 
> > > Some context: We don't actually want to migrate user's settings, as this
> > change
> > > is experimental, and may be reverted at some point.
> > 
> > Do we want to keep the ASK value in the policy templates for now, and only
> > remove it when we officially deprecate the option? And/or never remove it
from
> > the template, just mark it as "Don't use this".
> 
> That sounds right. thestig just linked me to this:
>
https://sites.google.com/a/chromium.org/dev/developers/how-tos/enterprise/add...,
> which said that removed policies should be deprecated over a number of
releases.
> 
> That makes sense. The enterprise policies need to continue to support 'ask'
for
> a while.
> 
> Two open questions:
> 1. Are we shooting ourselves in the foot by adding 'detect' to the policy
> options? (Would we have to do a long deprecation process in the future if it
> doesn't work out?)

Yes, adding a new option usually means committing to supporting it for an almost
indefinite time. In particular if this is an experiment, that does not seem to
me like the thing to do right now.

> 2. Do we have to do anything to get the new settings translated?

I *think* it works automatically, as the policy template file is processed by
Grit just as the .grd files are.

[Andrew T Wilson (Slow), 2014-12-16 09:48:09]

https://codereview.chromium.org/796543002/diff/200001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/796543002/diff/200001/components/policy/resou...
components/policy/resources/policy_templates.json:2514: 'value': 5,
Why is this 5 and not 4?

I would just not add this as a policy setting for now (it isn't likely that any
enterprise will want to enforce "DetectImportantContent" via policy).

So I'd say to revert the changes to this file, and just make sure that
ClickToPlay policy is treated the same as "Right click to play" in settings
(i.e. make sure that you can still right click to play content when this policy
is set).

[tommycli, 2014-12-16 19:55:47]

thestig/bauerb: Addressed your comments.

- Reverted the policy changes.
- Show ASK as BLOCK in the WebUI.
- Fixed some broken tests, see below comment for details.

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

https://codereview.chromium.org/796543002/diff/160001/chrome/app/generated_re...
chrome/app/generated_resources.grd:8030: +        Do not run plugins by default
On 2014/12/16 00:00:38, Lei Zhang wrote:
> Do we want to keep the "You can manually run a blocked plug-in at any time."
> note?

No. The UI folks specifically recommended its removal.

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(left):

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:61:
CONTENT_SETTINGS_TYPE_PLUGINS, CONTENT_SETTING_ASK);
On 2014/12/16 00:00:38, Lei Zhang wrote:
> Do you still need to test the ASK case?

Done.

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/resource...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/796543002/diff/160001/chrome/browser/resource...
chrome/browser/resources/options/content_settings_exceptions_area.js:280: else
if (setting == 'ask')
On 2014/12/16 00:00:38, Lei Zhang wrote:
> WRT to the ask state. Can we make sure if the user has "ask" as the preference
> setting, then it shows up in the settings UI as blocked? i.e. Make sure we
never
> show a UI in the limbo state to the user.

Done. In content_settings_handler.

https://codereview.chromium.org/796543002/diff/200001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/796543002/diff/200001/components/policy/resou...
components/policy/resources/policy_templates.json:2514: 'value': 5,
On 2014/12/16 09:48:09, Andrew T Wilson wrote:
> Why is this 5 and not 4?
> 
> I would just not add this as a policy setting for now (it isn't likely that
any
> enterprise will want to enforce "DetectImportantContent" via policy).
> 
> So I'd say to revert the changes to this file, and just make sure that
> ClickToPlay policy is treated the same as "Right click to play" in settings
> (i.e. make sure that you can still right click to play content when this
policy
> is set).

Done. I reverted this. It was '5' because it's an enum, and '4' was used for
"Allow cookies for Session only"

https://codereview.chromium.org/796543002/diff/240001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (left):

https://codereview.chromium.org/796543002/diff/240001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:1017: { "policy": {
"DefaultPluginsSetting": 3 },
Removed because the 'ask' radio box is gone. There is special code in
content_settings_handler to display 'ask' as 'block' instead.

[Lei Zhang, 2014-12-16 21:20:18]

https://codereview.chromium.org/796543002/diff/240001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/796543002/diff/240001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:599: // For Plugins,
display the obsolete ASK setting as BLOCK.
Do we need to update WebsiteSettingsHandler::GetSettingDefaultFromModel() as
well?

The comment there has a TODO to deduplicate the two, but removing
GetSettingDefaultFromModel() here might make that a bit more confusing. Boo to
code duplication.

[tommycli, 2014-12-16 23:32:29]

thestig: Okay i fixed the lock icon. See below. Thanks!

https://codereview.chromium.org/796543002/diff/240001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/796543002/diff/240001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/content_settings_handler.cc:599: // For Plugins,
display the obsolete ASK setting as BLOCK.
On 2014/12/16 21:20:18, Lei Zhang wrote:
> Do we need to update WebsiteSettingsHandler::GetSettingDefaultFromModel() as
> well?
> 
> The comment there has a TODO to deduplicate the two, but removing
> GetSettingDefaultFromModel() here might make that a bit more confusing. Boo to
> code duplication.

Done.

I couldn't find any breakages as a result of this change in
chrome://settings-frame/websiteSettings. In fact, I was not able to find a way
into that window other than browsing there directly.

Confusingly, the Views interface accessible via the lock icon is also called
WebsiteSettings, and I did fix that.

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/website_settings_handler.cc:511: void
WebsiteSettingsHandler::HandleUpdateDefaultSetting(
I removed the TODO and inlined the call. After inlining, there's not much room
left for deduplication, since they do function somewhat differently.

[tommycli, 2014-12-16 23:32:51]

tommycli@chromium.org changed reviewers:
- atwilson@chromium.org

[Lei Zhang, 2014-12-17 00:36:01]

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2063: // Checks that plugins
are not loaded on prerendering pages when Plugin Power
This contradicts the comment in prerender_plugin_power_saver.html, and the test
failed on some bots.

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/website_settings_handler.cc:513:
base::DictionaryValue filter_settings;
nit: You can leave these where they were, i.e. declare it closer to where it's
actually used.

[Bernhard Bauer, 2014-12-17 17:14:17]

lgtm

[tommycli, 2014-12-17 19:51:06]

thestig: Okay thanks for being patient with me. I think I got it. The Test
plugin only works on Windows, so... yeah see below.

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2063: // Checks that plugins
are not loaded on prerendering pages when Plugin Power
On 2014/12/17 00:36:01, Lei Zhang wrote:
> This contradicts the comment in prerender_plugin_power_saver.html, and the
test
> failed on some bots.

Done. I updated the comment. The correct intent is in the HTML file. This test
fails on Linux and Mac because it relies on a Test Plugin that does not work on
Aura. Or at least: for the same reason as PrerenderDelayLoadPlugin only works on
Windows.

This befuzzled me for days.

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/796543002/diff/300001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/website_settings_handler.cc:513:
base::DictionaryValue filter_settings;
On 2014/12/17 00:36:01, Lei Zhang wrote:
> nit: You can leave these where they were, i.e. declare it closer to where it's
> actually used.

Done.

[Lei Zhang, 2014-12-17 20:46:40]

lgtm

https://codereview.chromium.org/796543002/diff/200002/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/796543002/diff/200002/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:2040: #if defined(USE_AURA) &&
!defined(OS_WIN)
Ouch, this is pretty painful to look at. Essentially the only platform that
still runs this test is 32-bit Windows. It's because the test plugin is a NPAPI
plugin, and there aren't many platforms where NPAPI still works. I'll file a bug
for converting this test to PPAPI, or something.

[tommycli, 2014-12-17 20:55:45]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-12-17 20:57:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796543002/200002

[commit-bot: I haz the power, 2014-12-17 23:17:58]

Committed patchset #18 (id:200002)

[commit-bot: I haz the power, 2014-12-17 23:19:18]

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