Make DETECT_IMPORTANT_CONTENT the default Plugins content setting based

Make DETECT_IMPORTANT_CONTENT the default Plugins content setting based
on the results of a FieldTrial (also flags).

Still respect the user's choice when they've explicitly chosen an
alternative (e.g. they chose to BLOCK all plugins).

R=tommycli@chromium.org,bauerb@chromium.org,jochen@chromium.org
TBR=markusheintz@chromium.org
BUG=476742
Committed: https://crrev.com/155ac97a453b6faf109949ed5319a90e759af9d2
Cr-Commit-Position: refs/heads/master@{#325846}

[Dan Beam, 2015-04-15 03:32:23]

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

[Dan Beam, 2015-04-15 03:36:23]

still WIP, will add reviewer when ready

https://codereview.chromium.org/1084123002/diff/1/components/content_settings...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1084123002/diff/1/components/content_settings...
components/content_settings/core/browser/content_settings_default_provider.cc:44:
PluginsFieldTrial::GetDefaultPluginsContentSetting()},
for this to be called, PluginsFieldTrial needed to be moved to components/ and
out of chrome/.

because PluginsFieldTrial had a dependency on chrome/, I had to move those to
components/ (fortunately just switches that are only used in 2 places:
PluginsFieldTrial, about:flags).

unfortunately this code (as is) would probably create static initializers... so
we'll have to shuffle things around to not call this so early.  but if it
doesn't: great!

[Dan Beam, 2015-04-16 17:29:04]

Patchset #5 (id:80001) has been deleted

[Dan Beam, 2015-04-16 17:29:17]

Patchset #3 (id:40001) has been deleted

[Dan Beam, 2015-04-16 17:29:24]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2015-04-16 17:29:37]

Patchset #2 (id:60001) has been deleted

[Dan Beam, 2015-04-16 18:37:51]

dbeam@chromium.org changed reviewers:
+ bauerb@chromium.org, markusheintz@chromium.org, tommycli@chromium.org

[Dan Beam, 2015-04-16 18:39:51]

Patchset #3 (id:120001) has been deleted

[Dan Beam, 2015-04-16 18:58:16]

Hey all,

Markus/Bernhard: you both own lots of this code, but this diverges for small
bits (e.g. website_settings for Markus, plugins for Bernhard).  You can both
review applicable stuff or discuss amongst yourselves[1] what to do.  Also
please my first comment[2] as to why things needed to be moved to
components/content_settings.

Tommy is a stakeholder in plugins power saver, so should also review
(specifically the field trial/prefs logic and tests).

[1] http://bit.ly/1J80wk8
[2] https://codereview.chromium.org/1084123002/#msg2

https://codereview.chromium.org/1084123002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugins_field_trial_browsertest.cc (right):

https://codereview.chromium.org/1084123002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugins_field_trial_browsertest.cc:10: #include
"chrome/browser/profiles/profile.h"
afaik, this file needs to live here because its dependency on Browser and
Profile

[Bernhard Bauer, 2015-04-17 15:59:35]

LGTM w/ nits:

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
components/content_settings/core/browser/content_settings_default_provider.cc:45:
{prefs::kDefaultPluginsSetting, CONTENT_SETTING_NUM_SETTINGS},  // Dynamic.
Nit: We usually use DEFAULT for the absence of a content setting value (and
there is some code that will check that we don't ever actually return DEFAULT).

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
components/content_settings/core/browser/content_settings_default_provider.cc:326:
if (!value) return true;
Return statement comes on a new line.

(Have you been writing too much Android code? ;-) )

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
File components/content_settings/core/browser/plugins_field_trial_unittest.cc
(right):

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
components/content_settings/core/browser/plugins_field_trial_unittest.cc:18:
const auto& kFieldTrialName = PluginsFieldTrial::kFieldTrialName;
This seems like you could also do it with `using`?

[tommycli, 2015-04-17 17:03:57]

lgtm sans some nits below:

https://codereview.chromium.org/1084123002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugins_field_trial_browsertest.cc (right):

https://codereview.chromium.org/1084123002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugins_field_trial_browsertest.cc:36:
IN_PROC_BROWSER_TEST_F(PluginsFieldTrialPrefTest, EnableFieldTrialNoPrefSet) {
It seems tragic to have a test separated from its parent.

Do you think this could work using a user_prefs::TestingPrefServiceSimple like
here:
https://code.google.com/p/chromium/codesearch/#chromium/src/components/sync_d...
?

https://codereview.chromium.org/1084123002/diff/140001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1084123002/diff/140001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:579:
'browser/plugins/plugins_field_trial_browsertest.cc',
It surprises me that this is the only enable_plugins conditioned test in
chrome_tests.gypi. I can't find anything else though.

https://codereview.chromium.org/1084123002/diff/160001/components/content_set...
File components/content_settings/core/common/content_settings_switches.h
(right):

https://codereview.chromium.org/1084123002/diff/160001/components/content_set...
components/content_settings/core/common/content_settings_switches.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
This is a nit: But I think this goes in
components/plugins/common/plugin_switches.h

[Dan Beam, 2015-04-18 19:37:35]

https://codereview.chromium.org/1084123002/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugins_field_trial_browsertest.cc (right):

https://codereview.chromium.org/1084123002/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugins_field_trial_browsertest.cc:36:
IN_PROC_BROWSER_TEST_F(PluginsFieldTrialPrefTest, EnableFieldTrialNoPrefSet) {
On 2015/04/17 17:03:56, tommycli wrote:
> It seems tragic to have a test separated from its parent.
> 
> Do you think this could work using a user_prefs::TestingPrefServiceSimple like
> here:
>
https://code.google.com/p/chromium/codesearch/#chromium/src/components/sync_d...
> ?

Done.

https://codereview.chromium.org/1084123002/diff/140001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1084123002/diff/140001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:579:
'browser/plugins/plugins_field_trial_browsertest.cc',
On 2015/04/17 17:03:56, tommycli wrote:
> It surprises me that this is the only enable_plugins conditioned test in
> chrome_tests.gypi. I can't find anything else though.

Surprise removed.

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
components/content_settings/core/browser/content_settings_default_provider.cc:45:
{prefs::kDefaultPluginsSetting, CONTENT_SETTING_NUM_SETTINGS},  // Dynamic.
On 2015/04/17 15:59:34, Bernhard Bauer wrote:
> Nit: We usually use DEFAULT for the absence of a content setting value (and
> there is some code that will check that we don't ever actually return
DEFAULT).

Done.

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
components/content_settings/core/browser/content_settings_default_provider.cc:326:
if (!value) return true;
On 2015/04/17 15:59:34, Bernhard Bauer wrote:
> Return statement comes on a new line.
> 
> (Have you been writing too much Android code? ;-) )

this is allowed by the style guide:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

and is done 452 times in chrome (afaict):
https://code.google.com/p/chromium/codesearch#search/&q=%5E%5Cs*if%5Cs%5C(.*%...

will change if you want, but I don't necessarily think extra lines == more
readable code here

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
File components/content_settings/core/browser/plugins_field_trial_unittest.cc
(right):

https://codereview.chromium.org/1084123002/diff/140001/components/content_set...
components/content_settings/core/browser/plugins_field_trial_unittest.cc:18:
const auto& kFieldTrialName = PluginsFieldTrial::kFieldTrialName;
On 2015/04/17 15:59:35, Bernhard Bauer wrote:
> This seems like you could also do it with `using`?

can't use a class member (that's what I originally tried)

https://codereview.chromium.org/1084123002/diff/160001/components/content_set...
File components/content_settings/core/common/content_settings_switches.h
(right):

https://codereview.chromium.org/1084123002/diff/160001/components/content_set...
components/content_settings/core/common/content_settings_switches.h:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/04/17 17:03:56, tommycli wrote:
> This is a nit: But I think this goes in
> components/plugins/common/plugin_switches.h
> 

Done.

[Dan Beam, 2015-04-19 00:42:01]

Patchset #2 (id:100001) has been deleted

[Dan Beam, 2015-04-19 00:42:19]

Patchset #4 (id:180001) has been deleted

[Dan Beam, 2015-04-19 00:42:33]

Patchset #4 (id:200001) has been deleted

[Dan Beam, 2015-04-19 03:52:26]

dbeam@chromium.org changed reviewers:
+ jochen@chromium.org
- markusheintz@chromium.org

[Dan Beam, 2015-04-19 03:53:05]

dbeam@chromium.org changed reviewers:
+ markusheintz@chromium.org

[Dan Beam, 2015-04-19 03:57:51]

+jochen@ for components/components.gyp, chrome/chrome_browser.gypi,
chrome/browser/BUILD.gn

moving markusheintz@ to TBR

[jochen (gone - plz use gerrit), 2015-04-20 06:48:44]

lgtm

[Dan Beam, 2015-04-20 14:20:45]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-04-20 14:20:46]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1084123002/#ps220001
(title: "gyp shuffle for ios")

[commit-bot: I haz the power, 2015-04-20 14:21:11]

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

[Dan Beam, 2015-04-20 14:21:37]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2015-04-20 15:32:58]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-04-20 15:33:41]

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

[commit-bot: I haz the power, 2015-04-20 15:35:10]

Committed patchset #4 (id:220001)

[commit-bot: I haz the power, 2015-04-20 15:36:19]

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