[HBD] Update MD Site Settings Plugins section to show 3rd mode checkbox.

[HBD] Update MD Site Settings Plugins section to show 3rd mode checkbox.

Also updates a bunch of strings.

BUG=622922
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/744a17d56bde3a6113040c5d398effab4748f44f
Cr-Commit-Position: refs/heads/master@{#415394}

[tommycli, 2016-08-26 21:55:27]

Description was changed from

==========
[HBD] Update MD Site Settings Plugins section to show 3rd mode checkbox.

Also updates a bunch of strings.

BUG=622922
==========

to

==========
[HBD] Update MD Site Settings Plugins section to show 3rd mode checkbox.

Also updates a bunch of strings.

BUG=622922
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2016-08-26 22:33:10]

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

[tommycli, 2016-08-26 22:33:11]

finnur: PTAL, thanks! Sorry this CL is big. Wasn't sure how to divide up into
smaller ones.

[Finnur, 2016-08-29 11:26:28]

Big?   oO

:)

Not sure I'd use that word for this CL (nor how you could have trimmed it down
further). :)

https://codereview.chromium.org/2280233002/diff/1/chrome/app/settings_strings...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2280233002/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:1266: <message
name="IDS_SETTINGS_SITE_SETTINGS_FLASH_DETECT_IMPORTANT_RECOMMENDED" desc="The
'detect and run important content' label in site settings -- the recommended
setting for when sites want to run plugins.">
s/plugins/Flash/

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(left):

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:94: //
"Detect important" vs "Let me choose".
Nit: These comments have been very helpful in the past to figure out if the
labeling, for a new category that one is adding, has already been set up. I'd
keep it, but augment it to say something like:

// This category is tri-state: "Allow", "Detect", "Ask before running".

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:135:
// is explicitly ALLOW.
nit: s/explicitly/explicitly set to/

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1281:
if (base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)) {
MD-Settings uses only IMPORTANT_CONTENT. I presume that setting is also
overloaded elsewhere in the plugins c++ code (so this just works)?

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_settings_category_tests.js (left):

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:132: '',
testElement.computeCategoryDesc(category, false, false));
I don't think we need to remove these... They catch when we fail to add a
category description for a new category.

But we should change them to:

assertNotEquals('', testElement.computeCategoryDesc(category,
settings.PermissionValues.ALLOW, true));	
assertNotEquals('', testElement.computeCategoryDesc(category,
settings.PermissionValues.ALLOW, false));	
assertNotEquals('', testElement.computeCategoryDesc(category,
settings.PermissionValues.BLOCK, true));	
assertNotEquals('', testElement.computeCategoryDesc(category,
settings.PermissionValues.BLOCK, false));

... and probably, to complete the coverage, add this to line 134: 

// Test additional tri-state values:
if (category == settings.ContentSettingsTypes.PLUGINS) {
  assertNotEquals('', testElement.computeCategoryDesc(category,
settings.PermissionValues.IMPORTANT_CONTENT, true));
  assertNotEquals('', testElement.computeCategoryDesc(category,
settings.PermissionValues.IMPORTANT_CONTENT, false));
}

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_settings_category_tests.js (right):

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:137: plugins:
'detect',
Wait, should this string not match this one?

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:142: });
nit: I've made it a habit to group these at the top of each test...

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:171:
assertTrue(askCheckbox.checked);
This is a bit non-intuitive (to me, at least). If we've selected Blocked, why is
the checkbox in checked-disabled state? Should it not be in unchecked-disabled
state, like when you move from Allow->Block (line 215 below)?

[tommycli, 2016-08-29 18:31:47]

finnur: addressed your comments in PS2.

PS3 has some extra string changes as requested by UI peeps. Thanks!

https://codereview.chromium.org/2280233002/diff/1/chrome/app/settings_strings...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2280233002/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:1266: <message
name="IDS_SETTINGS_SITE_SETTINGS_FLASH_DETECT_IMPORTANT_RECOMMENDED" desc="The
'detect and run important content' label in site settings -- the recommended
setting for when sites want to run plugins.">
On 2016/08/29 11:26:27, Finnur wrote:
> s/plugins/Flash/

Done.

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(left):

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:94: //
"Detect important" vs "Let me choose".
On 2016/08/29 11:26:27, Finnur wrote:
> Nit: These comments have been very helpful in the past to figure out if the
> labeling, for a new category that one is adding, has already been set up. I'd
> keep it, but augment it to say something like:
> 
> // This category is tri-state: "Allow", "Detect", "Ask before running".

Done. Thanks good suggestion.

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.js:135:
// is explicitly ALLOW.
On 2016/08/29 11:26:27, Finnur wrote:
> nit: s/explicitly/explicitly set to/

Done.

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2280233002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1281:
if (base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)) {
On 2016/08/29 11:26:27, Finnur wrote:
> MD-Settings uses only IMPORTANT_CONTENT. I presume that setting is also
> overloaded elsewhere in the plugins c++ code (so this just works)? 

Arg.... this is really annoying, but "Ask before running" will still be stored
on disk as the DETECT_IMPORTANT_CONTENT content setting for now.

In the future, I will move it back to the ASK setting, but I will have to figure
out how to do it on a separate CL.

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_settings_category_tests.js (left):

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:132: '',
testElement.computeCategoryDesc(category, false, false));
On 2016/08/29 11:26:27, Finnur wrote:
> I don't think we need to remove these... They catch when we fail to add a
> category description for a new category.
> 
> But we should change them to:
> 
> assertNotEquals('', testElement.computeCategoryDesc(category,
> settings.PermissionValues.ALLOW, true));	
> assertNotEquals('', testElement.computeCategoryDesc(category,
> settings.PermissionValues.ALLOW, false));	
> assertNotEquals('', testElement.computeCategoryDesc(category,
> settings.PermissionValues.BLOCK, true));	
> assertNotEquals('', testElement.computeCategoryDesc(category,
> settings.PermissionValues.BLOCK, false));
> 
> ... and probably, to complete the coverage, add this to line 134: 
> 
> // Test additional tri-state values:
> if (category == settings.ContentSettingsTypes.PLUGINS) {
>   assertNotEquals('', testElement.computeCategoryDesc(category,
> settings.PermissionValues.IMPORTANT_CONTENT, true));
>   assertNotEquals('', testElement.computeCategoryDesc(category,
> settings.PermissionValues.IMPORTANT_CONTENT, false));
> }

Done.

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/site_settings_category_tests.js (right):

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:137: plugins:
'detect',
On 2016/08/29 11:26:27, Finnur wrote:
> Wait, should this string not match this one?
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...

Done. Oops.

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:142: });
On 2016/08/29 11:26:27, Finnur wrote:
> nit: I've made it a habit to group these at the top of each test...

Done.

https://codereview.chromium.org/2280233002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/site_settings_category_tests.js:171:
assertTrue(askCheckbox.checked);
On 2016/08/29 11:26:27, Finnur wrote:
> This is a bit non-intuitive (to me, at least). If we've selected Blocked, why
is
> the checkbox in checked-disabled state? Should it not be in unchecked-disabled
> state, like when you move from Allow->Block (line 215 below)?

Basically - we don't want the checkbox unchecked unless the user has explicitly
unchecked it.

If the user starts on DETECT (toggle on, checkbox checked), and goes to BLOCK
(toggle off, checkbox can be anything), and then hits the toggle again, we want
to go to DETECT.

If we unchecked the checkbox, we would go to ALLOW.

[Finnur, 2016-08-30 10:58:11]

LGTM.

https://codereview.chromium.org/2280233002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2280233002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:323:
return loadTimeData.getString('siteSettingsFlashAskBefore');
Hmmm... I just noticed that when one presses Back (when on the Plugins  page),
the label under the category doesn't reflect the changes one has made on the
Site Category page. Meaning: if you change from Allow to Block, the label still
reads 'Allow flash' (paraphrasing).

This seems to be a problem for all categories and I feel this worked at one
point. Is it possible the routing changes caused this to happen, or am I
remembering incorrectly and this never worked?

[tommycli, 2016-08-30 16:53:43]

https://codereview.chromium.org/2280233002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2280233002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:323:
return loadTimeData.getString('siteSettingsFlashAskBefore');
On 2016/08/30 10:58:11, Finnur wrote:
> Hmmm... I just noticed that when one presses Back (when on the Plugins  page),
> the label under the category doesn't reflect the changes one has made on the
> Site Category page. Meaning: if you change from Allow to Block, the label
still
> reads 'Allow flash' (paraphrasing).
> 
> This seems to be a problem for all categories and I feel this worked at one
> point. Is it possible the routing changes caused this to happen, or am I
> remembering incorrectly and this never worked?

I noticed the same thing. Looking at the contents of site_settings_page.js, I'd
assume it never worked heh

[tommycli, 2016-08-30 16:53:50]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-08-30 16:54:05]

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

[tommycli, 2016-08-30 16:54:49]

On 2016/08/30 16:54:05, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Thanks for your help on this Finnur. I think we are almost there, I think I have
to write another followup patch to rename all the instances of "Plugins" to
"Flash".

[commit-bot: I haz the power, 2016-08-30 16:57:00]

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

[commit-bot: I haz the power, 2016-08-30 16:57:01]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-08-30 17:59:37]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-08-30 17:59:37]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/2280233002/#ps60001
(title: "merge origin/master")

[commit-bot: I haz the power, 2016-08-30 18:00:20]

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-08-30 18:06:53]

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

[commit-bot: I haz the power, 2016-08-30 18:06:54]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tommycli, 2016-08-30 18:07:23]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-08-30 18:08:26]

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-08-30 19:27:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-30 19:30:38]

Description was changed from

==========
[HBD] Update MD Site Settings Plugins section to show 3rd mode checkbox.

Also updates a bunch of strings.

BUG=622922
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[HBD] Update MD Site Settings Plugins section to show 3rd mode checkbox.

Also updates a bunch of strings.

BUG=622922
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/744a17d56bde3a6113040c5d398effab4748f44f
Cr-Commit-Position: refs/heads/master@{#415394}
==========

[commit-bot: I haz the power, 2016-08-30 19:30:40]

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

[Finnur, 2016-09-29 11:19:03]

https://codereview.chromium.org/2280233002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_settings_category_tests.js (right):

https://codereview.chromium.org/2280233002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:185:
}).then(function(arguments) {
Apologies for raising an old thread here, but Dave made a comment about my test
(another CL), and his comment applies here also, I believe, because I used this
test as a base for mine.

See:
https://codereview.chromium.org/2381583002/diff/20001/chrome/test/data/webui/...