Flesh out the location-page class to make it more general.

Flesh out the location-page class to make it more general.

Name retained for easier comparing.

BUG=TBD

[Dan Beam, 2015-09-29 16:38:55]

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

[Dan Beam, 2015-09-29 16:38:55]

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/advanced_page/advanced_page.html (right):

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/advanced_page.html:25:
<cr-settings-location-page prefs="{{prefs}}" contentSettingsType="[[5]]">
i'm pretty sure this binds to this['5'] (which is undefined)

I think what you want is to create:

  properties: {
    readOnly: true,
    type: Object,
    contentSettingsTypes_: {
      COOKIES: 0,
      IMAGES: 1,
      ...
    },
  },

and then

  contentSettingsType="[[contentSettingsType_.COOKIES]]"

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/advanced_page.html:25:
<cr-settings-location-page prefs="{{prefs}}" contentSettingsType="[[5]]">
why are you binding something to the location page?

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/location_page/location_page.html (right):

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/location_page/location_page.html:18:
<iron-icon icon="{{computeIcon_(contentSettingsType)}}" item-icon></iron-icon>
{{ -> [[

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

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/location_page/location_page.js:103:
chrome.send('fetchContentSettingsData', [this.contentSettingsType]);
michaelpg@: how do you feel about this?

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/location_page/location_page.js:174: default:
assertNotReached();

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/location_page/location_page.js:181: case 2: 
// JAVASCRIPT.
make an enum in the JS

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/location_page/location_page.js:213: default:
assertNotReached();

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/location_page/location_page.js:236: 
all these need /** @private */ and maybe to document their @params

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

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:35:
AddSettingsPageUIHandler(new ContentSettingsHandler());
nit: alpha

[Finnur, 2015-09-30 10:26:05]

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

[Finnur, 2015-09-30 10:26:06]

Good points. Will address them.

Wanted to get a quick answer to your question, and in doing so I realized there
was one question that didn't go out last round.

https://codereview.chromium.org/1368973003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/1368973003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:36:
this.$.pages.setSubpageChain(['search-engines']);
This doesn't work since this.$.pages isn't defined. I need to investigate how to
show my page and set the contentSettingsType property. Any tips?

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/advanced_page/advanced_page.html (right):

https://codereview.chromium.org/1368973003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/advanced_page.html:25:
<cr-settings-location-page prefs="{{prefs}}" contentSettingsType="[[5]]">
On 2015/09/29 16:38:55, Dan Beam wrote:
> why are you binding something to the location page?

Because I envision this to be the page for all categories, not just location,
and I mentioned I didn't want to rename the page yet (to facilitate easier
review).

[Dan Beam, 2015-09-30 17:29:46]

https://codereview.chromium.org/1368973003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/1368973003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:36:
this.$.pages.setSubpageChain(['search-engines']);
On 2015/09/30 10:26:06, Finnur wrote:
> This doesn't work since this.$.pages isn't defined. I need to investigate how
to
> show my page and set the contentSettingsType property. Any tips?

this.$.pages means "a local DOM node with id=pages", so find out how this is
working for other things