Plugin Power Saver: Overhaul plugin-power-saver flags. Show in UI.

Plugin Power Saver: Overhaul plugin-power-saver flags. Show in UI.

1) Combine the existing --enable-plugin-power-saver flag with a Finch experiment.

2) Add a corresponding --disable-plugin-power-saver flag, as that allows users to opt out of a Finch experiment.

3) Shows DETECT in Content Settings UI if flag/Finch is on. Grays out the ALLOW option.

4) Shows DETECT in the Website Settings dropdown if the flag/Finch is on.

BUG=471077
Committed: https://crrev.com/e2b1f1b77d410afd44d4d65f67710fef216f0c78
Cr-Commit-Position: refs/heads/master@{#323138}

[tommycli, 2015-03-26 22:50:53]

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

[tommycli, 2015-03-26 22:50:54]

thestig, piman: PTAL

Thanks!

[piman, 2015-03-26 22:56:42]

lgtm

[tommycli, 2015-03-27 22:09:39]

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

[tommycli, 2015-03-27 22:09:39]

thestig: PTAL. I tacked on more stuff onto this patch to prep for the 100%
Googler experiment.

I have a question about the flag naming. See below.

+cc groby

https://codereview.chromium.org/1033103002/diff/100001/components/plugins/com...
File components/plugins/common/plugins_switches.cc (right):

https://codereview.chromium.org/1033103002/diff/100001/components/plugins/com...
components/plugins/common/plugins_switches.cc:13: const char
kEnableForcePluginPowerSaver[] = "enable-force-plugin-power-saver";
I'm kind of torn on whether or not I should rename this to:

--enable-force-plugin-power-saver

The flag overrides the content setting and turns PPS on. Without the flag, it
just follows the user's content setting.

We have --disable-force-plugin-power-saver because we want to allow Googlers to
opt-out of the experiment if they desire.

Would it be better/simpler if we just called it:

--enable-plugin-power-saver
--disable-plugin-power-saver

?

I worry it may be somewhat confusing either way.

[groby-ooo-7-16, 2015-03-27 23:12:31]

groby@chromium.org changed reviewers:
+ blundell@chromium.org

[groby-ooo-7-16, 2015-03-27 23:12:32]

If you want to keep things in components, you'll need a components owner to
approve. Added blundell@

https://codereview.chromium.org/1033103002/diff/140001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/1033103002/diff/140001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_menu_model.cc:101: if
(permission_.type == CONTENT_SETTINGS_TYPE_PLUGINS) {
That logic is used several places. Worth factoring out?

https://codereview.chromium.org/1033103002/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1033103002/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:765: if ((status_value ==
ChromeViewHostMsg_GetPluginInfo_Status::kAllowed ||
Should we just send the right value from the browser, vs. duplicating logic here
and in browser process?

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
File components/plugins/common/BUILD.gn (right):

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
components/plugins/common/BUILD.gn:6: sources = [
Question: Why make this into a component? There's nothing really componentized?
Or are you planning to move more of this into components/ ?

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
File components/plugins/common/OWNERS (right):

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
components/plugins/common/OWNERS:1: tommycli@chromium.org
Please always add at least one backup owner.

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
File components/plugins/common/plugins_field_trial.h (right):

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
components/plugins/common/plugins_field_trial.h:16: 
 private:, please. Otherwise DISALLOW_ won't disallow :)

[tommycli, 2015-03-28 00:40:04]

groby: thanks!

https://codereview.chromium.org/1033103002/diff/140001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_menu_model.cc (right):

https://codereview.chromium.org/1033103002/diff/140001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_menu_model.cc:101: if
(permission_.type == CONTENT_SETTINGS_TYPE_PLUGINS) {
On 2015/03/27 23:12:32, groby wrote:
> That logic is used several places. Worth factoring out?

Done. Seems better now. I was actually skeptical of this suggestion originally,
but it seems to have worked out.

https://codereview.chromium.org/1033103002/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1033103002/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:765: if ((status_value ==
ChromeViewHostMsg_GetPluginInfo_Status::kAllowed ||
On 2015/03/27 23:12:32, groby wrote:
> Should we just send the right value from the browser, vs. duplicating logic
here
> and in browser process?

In this particular case, we are sending the right value. I'm just fixing an
oversight from before'

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
File components/plugins/common/BUILD.gn (right):

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
components/plugins/common/BUILD.gn:6: sources = [
On 2015/03/27 23:12:32, groby wrote:
> Question: Why make this into a component? There's nothing really
componentized?
> Or are you planning to move more of this into components/ ?

This goes into the existing 'plugins' component, which already contains the
loadable_plugin_placeholder code. It could easily live in chrome/ also, but I'm
mostly just following the omnibox example.

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
File components/plugins/common/OWNERS (right):

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
components/plugins/common/OWNERS:1: tommycli@chromium.org
On 2015/03/27 23:12:32, groby wrote:
> Please always add at least one backup owner.

Done. I just added myself to the components/plugins/OWNER s list instead then.

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
File components/plugins/common/plugins_field_trial.h (right):

https://codereview.chromium.org/1033103002/diff/140001/components/plugins/com...
components/plugins/common/plugins_field_trial.h:16: 
On 2015/03/27 23:12:32, groby wrote:
>  private:, please. Otherwise DISALLOW_ won't disallow :)

Done. doh

[Lei Zhang, 2015-03-28 01:11:58]

https://codereview.chromium.org/1033103002/diff/240001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1033103002/diff/240001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:116: "//components/plugins/common",
Shouldn't this be gated on "if (enable_plugins)" ?

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
File components/plugins/common/BUILD.gn (right):

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
components/plugins/common/BUILD.gn:13: deps = []
This is not in agreement with the gyp version.

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
File components/plugins/common/plugins_field_trial.h (right):

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
components/plugins/common/plugins_field_trial.h:22: static bool
EnableForcePluginPowerSaver();
EnableFoo sounds like a verb. This should be IsFoo.

[blundell, 2015-03-30 07:42:39]

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

[blundell, 2015-03-30 07:42:40]

+bauerb

bauerb is the right reviewer for adding an owner of the plugins component.
tommycli, you should also add yourself as an owner of plugins.gypi in
//components/OWNERS. Once bauerb has given his stamp, I'll approve the
//components/OWNERS change.

[tommycli, 2015-03-30 18:25:16]

thestig/blundell: addressed your comments below. Thanks!

https://codereview.chromium.org/1033103002/diff/240001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1033103002/diff/240001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:116: "//components/plugins/common",
On 2015/03/28 01:11:57, Lei Zhang wrote:
> Shouldn't this be gated on "if (enable_plugins)" ?

Done.

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
File components/plugins/common/BUILD.gn (right):

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
components/plugins/common/BUILD.gn:13: deps = []
On 2015/03/28 01:11:58, Lei Zhang wrote:
> This is not in agreement with the gyp version.

Done.

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
File components/plugins/common/plugins_field_trial.h (right):

https://codereview.chromium.org/1033103002/diff/240001/components/plugins/com...
components/plugins/common/plugins_field_trial.h:22: static bool
EnableForcePluginPowerSaver();
On 2015/03/28 01:11:58, Lei Zhang wrote:
> EnableFoo sounds like a verb. This should be IsFoo.

Done.

[Lei Zhang, 2015-03-30 18:45:31]

chrome/ lgtm with a couple comments:

https://codereview.chromium.org/1033103002/diff/260001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1033103002/diff/260001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2061: "plugin-power-saver",
This change will likely make users who have it enabled lose their setting.
Intentional?

https://codereview.chromium.org/1033103002/diff/260001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1033103002/diff/260001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:767:
ChromeViewHostMsg_GetPluginInfo_Status::kPlayImportantContent) &&
Can you add a comment to explain why this is here?

[tommycli, 2015-03-30 18:57:56]

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

[tommycli, 2015-03-30 18:59:43]

thestig: thanks!

https://codereview.chromium.org/1033103002/diff/260001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1033103002/diff/260001/chrome/browser/about_f...
chrome/browser/about_flags.cc:2061: "plugin-power-saver",
On 2015/03/30 18:45:31, Lei Zhang wrote:
> This change will likely make users who have it enabled lose their setting.
> Intentional?

Done. Not intentional.

https://codereview.chromium.org/1033103002/diff/260001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1033103002/diff/260001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:767:
ChromeViewHostMsg_GetPluginInfo_Status::kPlayImportantContent) &&
On 2015/03/30 18:45:31, Lei Zhang wrote:
> Can you add a comment to explain why this is here?

Done.

[tommycli, 2015-03-30 19:27:00]

isherman: May I have a tools/histograms review?

Thanks!

[Ilya Sherman, 2015-03-30 20:57:01]

histograms.xml lgtm

[tommycli, 2015-03-30 21:58:14]

isherman: thanks!

bauerb: ptal,

[tommycli, 2015-03-30 22:18:57]

blundell:

thestig just pointed out that bauerb is OOO this whole week.

Would you be willing to give provisional approval to the
components/plugins/common additions?

It doesn't modify any existing code, just adds new code. I will be happy to move
it out of components/plugins if bauerb gets back and objects.

thanks

[blundell, 2015-03-31 08:15:37]

On 2015/03/30 22:18:57, tommycli wrote:
> blundell:
> 
> thestig just pointed out that bauerb is OOO this whole week.
> 
> Would you be willing to give provisional approval to the
> components/plugins/common additions?
> 
> It doesn't modify any existing code, just adds new code. I will be happy to
move
> it out of components/plugins if bauerb gets back and objects.
> 
> thanks

I don't feel comfortable providing an owners review of feature-specific code
that I'm not familiar with -- that's why components have individual owners :).

If you want to land this before bauerb gets back, I suggest that you move the
code to //chrome at this time and take out all the changes to //components. You
can then move it into //components and add yourself as an owner of the component
later on. You'll need to find a //chrome owner who's comfortable reviewing that
code then, of course. Alternatively, if this patch could wait until bauerb is
back that seems the simplest. Apologies!

[tommycli, 2015-03-31 16:37:08]

blundell: No problem.

thestig: How do you feel about putting it into chrome/browser/plugins for now? I
think it's only containing NPAPI code right now, but it seems like the most
logical place under chrome/

[tommycli, 2015-03-31 19:07:18]

tommycli@chromium.org changed reviewers:
- bauerb@chromium.org, blundell@chromium.org

[tommycli, 2015-03-31 19:07:19]

thestig: I moved the stuff in components/plugins/common to
chrome/browser/plugins.

Let me know if that seems reasonable until bauerb gets back.

[Lei Zhang, 2015-03-31 20:27:39]

lgtm

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:62: "+components/plugins/common",
no longer needed

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/plugins...
File chrome/browser/plugins/plugins_field_trial.cc (right):

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/plugins...
chrome/browser/plugins/plugins_field_trial.cc:38: "Dogfood";
4 space indent from the front?

Maybe "!= Disabled" ?

https://codereview.chromium.org/1033103002/diff/320001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1033103002/diff/320001/chrome/chrome_browser....
chrome/chrome_browser.gypi:1989: 'browser/plugins/plugins_field_trial.cc',
nit: alphabetical order

[tommycli, 2015-03-31 20:34:56]

thestig: thanks!

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:62: "+components/plugins/common",
On 2015/03/31 20:27:38, Lei Zhang wrote:
> no longer needed

Done.

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/plugins...
File chrome/browser/plugins/plugins_field_trial.cc (right):

https://codereview.chromium.org/1033103002/diff/320001/chrome/browser/plugins...
chrome/browser/plugins/plugins_field_trial.cc:38: "Dogfood";
On 2015/03/31 20:27:38, Lei Zhang wrote:
> 4 space indent from the front?
> 
> Maybe "!= Disabled" ?

Done.

https://codereview.chromium.org/1033103002/diff/320001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1033103002/diff/320001/chrome/chrome_browser....
chrome/chrome_browser.gypi:1989: 'browser/plugins/plugins_field_trial.cc',
On 2015/03/31 20:27:39, Lei Zhang wrote:
> nit: alphabetical order

Done.

[tommycli, 2015-03-31 21:03:48]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2015-03-31 21:03:49]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
isherman@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1033103002/#ps340001
(title: " ")

[commit-bot: I haz the power, 2015-03-31 21:04:24]

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

[commit-bot: I haz the power, 2015-03-31 22:55:17]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-03-31 22:56:06]

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