Migrate enabled state of plugins into specialized Flash and PDF prefs.

Migrate enabled state of plugins into specialized Flash and PDF prefs.

For PDF and Flash the enabled state is defined by the
ManagedDefaultPluginsSetting and PluginsAlwaysOpenPdfExternally prefs.
All other plugins should never be disabled in the first place.

BUG=662002
TEST=manual

Committed: https://crrev.com/9823e58abc9fc8de91f26a31dad7a2d24a8b7727
Cr-Commit-Position: refs/heads/master@{#432477}

[pastarmovj, 2016-11-04 16:22:52]

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

[pastarmovj, 2016-11-04 16:22:53]

Hi Bernhard, can you please have a look at this change?
Thanks,
Julian

[Bernhard Bauer, 2016-11-11 12:15:25]

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:76: plugin_name ==
base::ASCIIToUTF16("Adobe Flash Player"));
Would it make sense to move this to a constant in chrome/ rather than
duplicating it over and over?

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:381: // this time but we'd rather not
CHECK on that.
Couple of things:
1) This explanation seems a bit too detailed (and really, subjective), in
particular given that it's not clear who "I" is.
2) The code doesn't actually do anything apart from logging, so I don't see how
it would prevent a CHECK.
3) If you say "X should not happen", don't add a check for X. In this particular
case, we either make it a precondition that the name is non-empty, in which case
we DCHECK it, and accept crashes in case that turns out to be untrue (which is
fine, because that's actually the only way we'll learn about it!), or we
outright accept any name, but then we shouldn't spam the log about it either.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
has some good guidance there.

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:390: // TODO(pastarmovj): crbug/662006:
Remove migration eventually.
Nit: For ideal clickability, use a full URL: https://crbug/com/662006.

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:402: plugins_migrated = true;
So, this would set the flag even if the migrated plugin is neither the PDF
viewer nor Flash. Is that really what we want?

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:451: plugin_state_.Set(plugin_path,
enabled);
So, if we enable all plugins anyway, why do we even need this state?

[pastarmovj, 2016-11-14 13:57:59]

pastarmovj@chromium.org changed reviewers:
+ clamy@chromium.org, holte@chromium.org

[pastarmovj, 2016-11-14 13:58:00]

bauerb: PTAL

clamy: Please review changes in content/public/common.

holte: Please review changes to histograms.xml.


Thanks!
Julian

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:76: plugin_name ==
base::ASCIIToUTF16("Adobe Flash Player"));
On 2016/11/11 12:15:24, Bernhard Bauer wrote:
> Would it make sense to move this to a constant in chrome/ rather than
> duplicating it over and over?

Done.

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:381: // this time but we'd rather not
CHECK on that.
On 2016/11/11 12:15:24, Bernhard Bauer wrote:
> Couple of things:
> 1) This explanation seems a bit too detailed (and really, subjective), in
> particular given that it's not clear who "I" is.
> 2) The code doesn't actually do anything apart from logging, so I don't see
how
> it would prevent a CHECK.
> 3) If you say "X should not happen", don't add a check for X. In this
particular
> case, we either make it a precondition that the name is non-empty, in which
case
> we DCHECK it, and accept crashes in case that turns out to be untrue (which is
> fine, because that's actually the only way we'll learn about it!), or we
> outright accept any name, but then we shouldn't spam the log about it either.
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> has some good guidance there.
 Restructued this code. It doesn't matter anymore just like it didn't before.

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:390: // TODO(pastarmovj): crbug/662006:
Remove migration eventually.
On 2016/11/11 12:15:24, Bernhard Bauer wrote:
> Nit: For ideal clickability, use a full URL: https://crbug/com/662006.

Was trying to save a line :) changed.

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:402: plugins_migrated = true;
On 2016/11/11 12:15:24, Bernhard Bauer wrote:
> So, this would set the flag even if the migrated plugin is neither the PDF
> viewer nor Flash. Is that really what we want?

Yes. I want to know that any plugin was user-disabled (which is going to be
undone so once per-user, provided that the about:plugins page is gone). This way
we can measure when we have reset [almost]all visible in uma instances.

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:451: plugin_state_.Set(plugin_path,
enabled);
On 2016/11/11 12:15:24, Bernhard Bauer wrote:
> So, if we enable all plugins anyway, why do we even need this state?

Not anymore but untangling this is going to make this CL too complex. This is
the plan for http://crbug.com/662002 anyways.

[Bernhard Bauer, 2016-11-14 14:35:38]

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2474713003/diff/40001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:390: // TODO(pastarmovj): crbug/662006:
Remove migration eventually.
On 2016/11/14 13:58:00, pastarmovj wrote:
> On 2016/11/11 12:15:24, Bernhard Bauer wrote:
> > Nit: For ideal clickability, use a full URL: https://crbug/com/662006.
> 
> Was trying to save a line :) changed.

Gotcha. FWIW, TODO(https://crbug.com/662006) is also acceptable (even prefered
by some people) and would also be shorter :)

https://codereview.chromium.org/2474713003/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2474713003/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:380: // Migrate disabled plugins and
re-enabled them all internally.
Nit: "re-enable".

https://codereview.chromium.org/2474713003/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:385: if (plugin->GetString("name",
&name)) {
The check is now probably unnecessary as well -- an empty plugin will just not
match either of the cases below.

https://codereview.chromium.org/2474713003/diff/60001/content/public/common/c...
File content/public/common/content_constants.cc (right):

https://codereview.chromium.org/2474713003/diff/60001/content/public/common/c...
content/public/common/content_constants.cc:17: const char
kAdobeFlashPlayerPluginName[] = "Adobe Flash Player";
It might still make sense to keep this value in chrome/, because it's the name
of the plugin group, which is a chrome/ concept. I'm fairly sure we can find
some other constant file to move it to :)

[clamy, 2016-11-14 14:38:44]

Thanks! One question below.

https://codereview.chromium.org/2474713003/diff/80001/content/public/common/c...
File content/public/common/content_constants.h (right):

https://codereview.chromium.org/2474713003/diff/80001/content/public/common/c...
content/public/common/content_constants.h:28: CONTENT_EXPORT extern const char
kAdobeFlashPlayerPluginName[];
Why do you need to move this to content/? It seems to be only used in Chrome.

[Steven Holte, 2016-11-14 21:17:28]

histograms lgtm

[pastarmovj, 2016-11-15 10:09:18]

Thanks for the reviews!

bauerb, please take another look.

clamy, all content related files were reverted.

https://codereview.chromium.org/2474713003/diff/60001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2474713003/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:380: // Migrate disabled plugins and
re-enabled them all internally.
On 2016/11/14 14:35:38, Bernhard Bauer wrote:
> Nit: "re-enable".

Done.

https://codereview.chromium.org/2474713003/diff/60001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_prefs.cc:385: if (plugin->GetString("name",
&name)) {
On 2016/11/14 14:35:38, Bernhard Bauer wrote:
> The check is now probably unnecessary as well -- an empty plugin will just not
> match either of the cases below.

Done.

https://codereview.chromium.org/2474713003/diff/60001/content/public/common/c...
File content/public/common/content_constants.cc (right):

https://codereview.chromium.org/2474713003/diff/60001/content/public/common/c...
content/public/common/content_constants.cc:17: const char
kAdobeFlashPlayerPluginName[] = "Adobe Flash Player";
On 2016/11/14 14:35:38, Bernhard Bauer wrote:
> It might still make sense to keep this value in chrome/, because it's the name
> of the plugin group, which is a chrome/ concept. I'm fairly sure we can find
> some other constant file to move it to :)

Done.

https://codereview.chromium.org/2474713003/diff/80001/content/public/common/c...
File content/public/common/content_constants.h (right):

https://codereview.chromium.org/2474713003/diff/80001/content/public/common/c...
content/public/common/content_constants.h:28: CONTENT_EXPORT extern const char
kAdobeFlashPlayerPluginName[];
On 2016/11/14 14:38:43, clamy wrote:
> Why do you need to move this to content/? It seems to be only used in Chrome.

Yeah it felt right the right place (provided the rest of the constats here) but
then I found equally suitable place in the chrome/browser/plugins dir.

[Bernhard Bauer, 2016-11-16 09:48:17]

lgtm

[pastarmovj, 2016-11-16 09:51:04]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-11-16 09:51:04]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2474713003/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-11-16 09:51:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 13:52:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 13:52:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pastarmovj, 2016-11-16 13:56:51]

The CQ bit was checked by pastarmovj@chromium.org

[commit-bot: I haz the power, 2016-11-16 13:57:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 14:18:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-16 14:21:14]

Description was changed from

==========
Migrate enabled state of plugins into specialized Flash and PDF prefs.

For PDF and Flash the enabled state is defined by the
ManagedDefaultPluginsSetting and PluginsAlwaysOpenPdfExternally prefs.
All other plugins should never be disabled in the first place.

BUG=662002
TEST=manual
==========

to

==========
Migrate enabled state of plugins into specialized Flash and PDF prefs.

For PDF and Flash the enabled state is defined by the
ManagedDefaultPluginsSetting and PluginsAlwaysOpenPdfExternally prefs.
All other plugins should never be disabled in the first place.

BUG=662002
TEST=manual

Committed: https://crrev.com/9823e58abc9fc8de91f26a31dad7a2d24a8b7727
Cr-Commit-Position: refs/heads/master@{#432477}
==========

[commit-bot: I haz the power, 2016-11-16 14:21:15]

Patchset 6 (id:??) landed as
https://crrev.com/9823e58abc9fc8de91f26a31dad7a2d24a8b7727
Cr-Commit-Position: refs/heads/master@{#432477}