Convert SiteSettingsCategory to use the HostContentSettingsMap instead of

Convert SiteSettingsCategory to use the HostContentSettingsMap instead of
prefs directly.

BUG=543635
Committed: https://crrev.com/d418af7217ad7650923e7480dbb82b07e7b3254d
Cr-Commit-Position: refs/heads/master@{#376497}

[Finnur, 2016-02-16 15:34:53]

Patchset #1 (id:1) has been deleted

[Finnur, 2016-02-16 15:37:59]

Description was changed from

==========
Convert SiteSettingsCategory to use the HostContentSettingsMap instead of
prefs directly.

BUG=543635
==========

to

==========
Convert SiteSettingsCategory to use the HostContentSettingsMap instead of
prefs directly.

BUG=543635
==========

[Finnur, 2016-02-16 15:37:59]

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

[Finnur, 2016-02-16 15:41:04]

This is as per email discussion from Jan 14th-Jan19th subject "Material design
settings + content settings".

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/prefs_private_api.js
(right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/prefs_private_api.js:29: })();
At first, I had a corresponding cr singleton object, much like:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

But, then I realized it looked redundant with the two functions above being so
simple, so I removed it. It may make a comeback in a future CL if there is need,
or if you think it is warranted.

[Finnur, 2016-02-17 16:14:07]

Friendly ping...

[tommycli, 2016-02-17 18:18:06]

finnur: here you go, thanks!

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/prefs_private_api.js
(right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/prefs_private_api.js:29: })();
On 2016/02/16 15:41:04, Finnur wrote:
> At first, I had a corresponding cr singleton object, much like:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> But, then I realized it looked redundant with the two functions above being so
> simple, so I removed it. It may make a comeback in a future CL if there is
need,
> or if you think it is warranted.

After a long discussion with dpapad, we agreed that the best way to do this type
of pattern is to make a FooBrowserProxy object like this:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...


There is no need to make a Polymer element, because the Polymer element that
calls it can just call: FooBrowserProxy.getInstance().foo().

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:48:
notify: true,
nit: Is notify = true only for testing? If so, can we enable that optionally via
a test-overridden global?

See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
for an example.

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:126:
self.categoryEnabled = enabled;
nit: Does it work if you use function() { .. }.bind(this) instead of declaring
the self var?

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:72:
var self = this;
nit: same comment. Does .bind(this) do the trick instead of var self?

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:112: void
SiteSettingsHandler::HandleSetDefaultValueForContentType(
What was the motivation for changing this from using prefs directly to using the
HostContentSettingsMap?

Is the plan to convert all of site settings to use this new mechanism?

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

https://codereview.chromium.org/1698093007/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:86:
testElement.categoryEnabled = true;  // Ensure change event fires.
Is this really to ensure the change event fires? Or to set up the initial
condition of your test to force it to be initially enabled? Comment may need
update

[Finnur, 2016-02-18 14:58:55]

Patchset #2 (id:40001) has been deleted

[Finnur, 2016-02-18 20:38:18]

PTAL

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/prefs_private_api.js
(right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/prefs_private_api.js:29: })();
Done.

I'm fine with that, as long as this doesn't conflict with the consensus on the
email thread about chrome.send wrappers (meaning I'd have to rip it out and do
it differently at a later point). :)

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:48:
notify: true,
Correct. Done.

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:126:
self.categoryEnabled = enabled;
On 2016/02/17 18:18:06, tommycli wrote:
> nit: Does it work if you use function() { .. }.bind(this) instead of declaring
> the self var?

Done.

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:72:
var self = this;
On 2016/02/17 18:18:06, tommycli wrote:
> nit: same comment. Does .bind(this) do the trick instead of var self?

Done.

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:112: void
SiteSettingsHandler::HandleSetDefaultValueForContentType(
That's in the email I referred to but in short: prefs direct give you an
incomplete picture of the state of the content settings.

The plan is indeed to convert all of site settings to use this new mechanism, I
just needed a simple thing to start with to flush out things like the fact that
a proxy object should be used instead (before I go too far down a rabbit hole).
:)

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

https://codereview.chromium.org/1698093007/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:86:
testElement.categoryEnabled = true;  // Ensure change event fires.
The answer is 'both' (unless I'm misunderstanding the question).

I've changed the comment to (hopefully) clarify.

[tommycli, 2016-02-18 22:15:19]

This is lgtm sans these below nits.

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:112: void
SiteSettingsHandler::HandleSetDefaultValueForContentType(
On 2016/02/18 20:38:18, Finnur wrote:
> That's in the email I referred to but in short: prefs direct give you an
> incomplete picture of the state of the content settings.
> 
> The plan is indeed to convert all of site settings to use this new mechanism,
I
> just needed a simple thing to start with to flush out things like the fact
that
> a proxy object should be used instead (before I go too far down a rabbit
hole).
> :)

finnur: I brain farted and neglected to read the email. Thank you for providing
the executive summary in-addition. I've now also read the email and understand
these motivations. Thanks.

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

https://codereview.chromium.org/1698093007/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:86:
testElement.categoryEnabled = true;  // Ensure change event fires.
On 2016/02/18 20:38:18, Finnur wrote:
> The answer is 'both' (unless I'm misunderstanding the question).
> 
> I've changed the comment to (hopefully) clarify. 

Optional nit: Would it make more sense to do something like:

testElement.categoryEnabled = true;
testElement.prefs = prefsLocationDisabled;
return runAndResolveWhenCategoryEnabledChanged(function() {
  MockInteractions.tap(testElement.$.toggle);
}.then(function() {
  expectTrue(testElement.categoryEnabled);
});

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:7: var
changeCategoryOptions = settings_test.changeCategoryOptions || {
Why is this called changeCategoryOptions instead of siteSettingsCategoryOptions
or something?

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_proxy.js
(right):

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_proxy.js:12:
function SiteSettingsPrefsProxy() {};
SiteSettingsPrefsBrowserProxy. I think the pattern is FooBrowserProxy, where
this object is a proxy for the browser

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_proxy.js:14:
// TODOf figure out test.
nit: typo?

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:54:
this.addCategory_(-1);  // Every category (All Sites).
Ahh... the -1 sentinel value seems confusing. What if you just made a new
addAllCategories method?

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

https://codereview.chromium.org/1698093007/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:107:
assertTrue(testElement.categoryEnabled);
These should be expect instead of assert right?

[tommycli, 2016-02-18 23:36:42]

Finnur: 

One more thing - Demetrios has further refined his FooBrowserProxy pattern in a
new CL. See the changes to SearchEnginesBrowserProxy here:
https://codereview.chromium.org/1666623006/

I think this CL is fine as-is, and it's still lgtm. We can take this as an
iterative process.

I'm just saying: If you have problems with the testing, to look at his changes
in the next CL for a potential refactor path.

Cheers,

Tommy

[Finnur, 2016-02-19 10:18:13]

Thanks for the heads-up on the CL.

Comments addressed. Checking in.

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/site_settings_handler.cc (right):

https://codereview.chromium.org/1698093007/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/site_settings_handler.cc:112: void
SiteSettingsHandler::HandleSetDefaultValueForContentType(
No harm, no foul. :)

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

https://codereview.chromium.org/1698093007/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:86:
testElement.categoryEnabled = true;  // Ensure change event fires.
No, at a glance I don't think so. The 'change' event happens when you assign to
the prefs, so we might miss it if we move the assignment out of the block.

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_category.js:7: var
changeCategoryOptions = settings_test.changeCategoryOptions || {
I was struggling to find something that fit the spirit of the Proxy pattern I
was following.

I like siteSettingsCategoryOptions. Changed.

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_proxy.js
(right):

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_proxy.js:12:
function SiteSettingsPrefsProxy() {};
On 2016/02/18 22:15:19, tommycli wrote:
> SiteSettingsPrefsBrowserProxy. I think the pattern is FooBrowserProxy, where
> this object is a proxy for the browser

Done.

https://codereview.chromium.org/1698093007/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_prefs_proxy.js:14:
// TODOf figure out test.
No, these are private messages for me. In this case I forgot to remove it (test
already fixed).

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

https://codereview.chromium.org/1698093007/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_settings_category_tests.js:107:
assertTrue(testElement.categoryEnabled);
To me, 'expect' is appropriate if the test can continue and still give you
meaningful results. The problem here is that if the enabled state is wrong to
begin with, then continuing is not going to be helpful.

I don't feel strongly, though, so feel free to convince me otherwise for a
follow-up CL.

[Finnur, 2016-02-19 10:19:14]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-02-19 10:19:14]

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

[commit-bot: I haz the power, 2016-02-19 10:21:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698093007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698093007/100001

[Finnur, 2016-02-19 13:24:27]

The CQ bit was unchecked by finnur@chromium.org

[Finnur, 2016-02-19 13:24:39]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-02-19 13:27:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698093007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698093007/100001

[commit-bot: I haz the power, 2016-02-19 16:19:05]

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

[commit-bot: I haz the power, 2016-02-19 16:19:06]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Finnur, 2016-02-19 17:03:12]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-02-19 17:08:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698093007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698093007/100001

[commit-bot: I haz the power, 2016-02-19 18:45:12]

Description was changed from

==========
Convert SiteSettingsCategory to use the HostContentSettingsMap instead of
prefs directly.

BUG=543635
==========

to

==========
Convert SiteSettingsCategory to use the HostContentSettingsMap instead of
prefs directly.

BUG=543635
==========

[commit-bot: I haz the power, 2016-02-19 18:45:13]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-02-19 18:46:11]

Description was changed from

==========
Convert SiteSettingsCategory to use the HostContentSettingsMap instead of
prefs directly.

BUG=543635
==========

to

==========
Convert SiteSettingsCategory to use the HostContentSettingsMap instead of
prefs directly.

BUG=543635

Committed: https://crrev.com/d418af7217ad7650923e7480dbb82b07e7b3254d
Cr-Commit-Position: refs/heads/master@{#376497}
==========

[commit-bot: I haz the power, 2016-02-19 18:46:13]

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