Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1372053002:
Flesh out the location-page class to make it more general. (Closed)
|
Created:
5 years, 2 months ago by Finnur Modified:
5 years, 1 month ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert the location-page class into a general site-settings-category class that can handle showing any category.
Also wire the toggle up to actual prefs and implement the UI for the site list below it.
BUG=543635
Committed: https://crrev.com/1c386ff87977f702a5260314ff78dab1143304d5
Cr-Commit-Position: refs/heads/master@{#357346}
Patch Set 1 #Patch Set 2 : Split list into two #
Total comments: 28
Patch Set 3 : Address feedback #
Total comments: 56
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 6
Patch Set 7 : Address feedback: CLs 4 through 7 (inclusive) #
Total comments: 9
Patch Set 8 : Polish #
Total comments: 40
Patch Set 9 : Address feedback #Patch Set 10 : List population #
Total comments: 47
Patch Set 11 : Sync to head (no meaningful changes) #Patch Set 12 : Address feedback #
Total comments: 6
Patch Set 13 : Address feedback from Michael #
Total comments: 33
Patch Set 14 : Address feedback from Michael #
Total comments: 28
Patch Set 15 : Address feedback from Dan and Michael #
Total comments: 4
Patch Set 16 : Address feedback #Patch Set 17 : Sync to head #Patch Set 18 #Patch Set 19 : #
Total comments: 3
Messages
Total messages: 60 (13 generated)
finnur@chromium.org changed reviewers: + dbeam@chromium.org
I've split the Allowed/Blocked list into a separate element. Would really appreciate you taking a look. I believe all previous comments were addressed (in the old closed CL. See diff between patchset 1 and 2 for what has changed since you last reviewed. I think this is a sizable chunk worthy of a single check-in (after review). It is not fully implemented, but compared to what's checked in should be a big improvement, functionality-wise. Can polish and add to it in a follow-up.
Ping... I'm getting a little worried about review turn-around time, and suspect it could prove to be my biggest productivity brake. But I'm not sure how to tackle it... :s Any ideas?
was busy wasn't doing reviews
https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/location_page.html (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.html:2: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/classes/iron-flex-layout.html"> why do you need this? https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:69: toggleState: { categoryEnabled or typeEnabled https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:77: siteSelected: { selectedSite or selectedOrigin https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:115: }, all of these should be private where possible (i.e. just _ at end, you can't currently use @private afaik) https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:120: this.hidden = true; please do this in css instead https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:124: [this.category, this.categorySubtype == this.categorySubtypes.ALLOW]); why can't we get these prefs from |this.prefs| and prefs.js? https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:178: * A handler for selecting a site (by double clicking on the name). why are we handling double click? do the mocks say this? https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:191: console.log('action ' + event.target.selectedItems[0].textContent); debugging code https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:226: if (this.categorySubtype == this.categorySubtypes.ALLOW) { pull this.categorySubtype == this.categorySubtypes.ALLOW into a method https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:230: } no curlies https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/content_settings_handler.cc (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:31: base::Unretained(this))); why are any of these needed? why can't we use prefs.js? https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:36: CHECK_EQ(1U, args->GetSize()); change to DCHECKs https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:84: */ remove
what progress did this make today?
I've done most of what you said. I've been testing it lately and getting ready to upload (not done yet). I converted the global toggle to prefs (removing 2 out of 3 calls into native code) and wanted to get your input before converting the datalist also (in site.js).
finnur@chromium.org changed reviewers: + michaelpg@chromium.org
finnur@chromium.org changed reviewers: - michaelpg@chromium.org
Please take another look. I've converted the toggle to use prefs, so now that should work end-to-end. Tested it for all categories I've implemented and it flips the right value and an update through either old settings pages or the new is reflected on both sides. I'll convert the rest tomorrow (site_list.js), would be good to get your input to see if I'm doing it right. Thanks! https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/location_page.html (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.html:2: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/classes/iron-flex-layout.html"> I don't think I do. Must have been something I was trying out and then reverted (and forgot to delete this). https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:69: toggleState: { On 2015/10/05 16:52:57, Dan Beam wrote: > categoryEnabled or typeEnabled Done. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:77: siteSelected: { On 2015/10/05 16:52:57, Dan Beam wrote: > selectedSite or selectedOrigin Done. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:115: }, On 2015/10/05 16:52:57, Dan Beam wrote: > all of these should be private where possible (i.e. just _ at end, you can't > currently use @private afaik) Done. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:120: this.hidden = true; Do you mean something like this? (first answer) http://stackoverflow.com/questions/20816163/how-can-i-make-custom-polymer-ele... https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:124: [this.category, this.categorySubtype == this.categorySubtypes.ALLOW]); I've converted the other file to use prefs, I don't want to go too far down the rabbit hole in case my approach needs altering. If you are OK with my use of prefs in the other file, I'll work on converting this to prefs also. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:178: * A handler for selecting a site (by double clicking on the name). Hmm... I guess not. Click is more appropriate, I presume? https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:191: console.log('action ' + event.target.selectedItems[0].textContent); On 2015/10/05 16:52:58, Dan Beam wrote: > debugging code Done. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:226: if (this.categorySubtype == this.categorySubtypes.ALLOW) { On 2015/10/05 16:52:57, Dan Beam wrote: > pull this.categorySubtype == this.categorySubtypes.ALLOW into a method Done. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:230: } On 2015/10/05 16:52:57, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/content_settings_handler.cc (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:31: base::Unretained(this))); We can. I just threw this up to mock the data to get the UI right. But, I don't mind taking it a little further, I just thought this was enough for a first pass and it would evolve in a later changelist. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:36: CHECK_EQ(1U, args->GetSize()); This is going away soon. https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:84: */ Ditto.
finnur@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb for OWNERS check on settings_strings.grdp
jlklein@chromium.org changed reviewers: + jlklein@chromium.org
https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:115: }, On 2015/10/05 16:52:57, Dan Beam wrote: > all of these should be private where possible (i.e. just _ at end, you can't > currently use @private afaik) I believe this was fixed in mid-August here: https://github.com/google/closure-compiler/commit/8c959cd226cef914cc502b72b98... let me know if you still have issues with @private.
Dan, I've got a question on using prefs for the list of sites, which I started
looking into this morning...
As you know, we expose prefs through prefs_util.cc, where we whitelist certain
prefs for .js use (see PrefsUtil::GetWhitelistedKeys) and we specify their type
at the same time. These types are supported:
enum PrefType {
PREF_TYPE_NONE,
PREF_TYPE_BOOLEAN,
PREF_TYPE_NUMBER,
PREF_TYPE_STRING,
PREF_TYPE_URL,
PREF_TYPE_LIST,
PREF_TYPE_LAST = PREF_TYPE_LIST,
};
However, the relevant preference that we want to expose is, for example:
"profile": {
"content_settings": {
"exceptions": {
"notifications": {
"https:\/\/foo.com,*": {
"last_used": 1442486005.4385,
"setting": 1
},
"https:\/\/bar.com,*": {
"last_used": 1442486005.4385,
"setting": 1
},
}
}
}
}
In other words, the list we need to show in the UI is under
profile.content_settings.exceptions.notifications, which is a dictionary, not a
list. Two problems there:
1) Dictionary is not a type supported by the prefs_util.cc.
2) This makes it hard to white-list each pref that we want to show and be able
to update, because we'd have to do that dynamically somehow. No?
As a test, I already tried fooling the system by specifying STRING as a pref
type (hoping to create a dictionary out of it on the .js side), but that got
flagged in the schema validation (Invalid value for argument 1. Property
'.20.type': Value must be one of: [BOOLEAN, NUMBER, STRING, URL, LIST]).
Any suggestions?
+dschuyler, michaelpg: who's working on a list of dictionaries? is it done yet? if not: why?
https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:115: }, On 2015/10/06 17:07:21, Jeremy Klein wrote: > On 2015/10/05 16:52:57, Dan Beam wrote: > > all of these should be private where possible (i.e. just _ at end, you can't > > currently use @private afaik) > > I believe this was fixed in mid-August here: > > https://github.com/google/closure-compiler/commit/8c959cd226cef914cc502b72b98... > > let me know if you still have issues with @private. our version of the compiler is older than that
On 2015/10/07 15:20:32, Dan Beam wrote: > +dschuyler, michaelpg: who's working on a list of dictionaries? is it done yet? > if not: why? I believe Michael has done that work. I have not used it yet. On Sept 18, Michael said in a email, "We don't support DictionaryPrefs yet (I don't see why not, though) but ListPrefs that have DictionaryValues should be fine." Michael would know better, but that sounds like we'd want to whitelist the list itself as a list (the values within the list would not be in the whitelist type). If I'm right, a list of Numbers or Strings or dictionaries would all be of type List. Michael, is that roughly right?
Ping...
On 2015/10/09 15:05:20, Finnur wrote: > Ping... what are you pinging? have you contact michael?
My hope was that we (as in you and I) could make some progress on the CL, even if Michael (who is already cc-ed) hasn't responded to the question posed. I feel we are at a good junction point to review already and his answers don't block reviewing the current state of the CL, because his answers affect a future version of this CL or even the next CL. I also feel I can learn a great deal from your comments on what |this| CL contains, as I have with your previous comments. My hope was also that I could deliver this solution to you piece-meal, in part to facilitate a faster and easier review for you (not too much to review at a time) and in part so as for me not to go too deep down a rabbit hole in the wrong direction. I feel that that's a more reasonable approach (a recommended approach even). I'm fully aware of the fact that it may not be complete and it may not get there until much later, but to swallow an elephant we should take it a spoon at a time, right?
https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/advanced_page/advanced_page.html:27: <cr-settings-location-page prefs="{{prefs}}" why would a location-page need to know that it's category is location? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/advanced_page/advanced_page.js:44: contentSettingsTypes: { why is this here? this should be in some constants file or in some base class https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/location_page.html (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.html:23: on-change="handleToggleChange_"></paper-toggle-button> handleToggleChange_ -> onToggleChange_ https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/location_page.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:80: * the values found in the ContentSettingsType. we shouldn't have to copy this or make new references, I wouldn't think. ContentSettingsType should just live in global code https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:96: ASK: 3, this should probably be in the same constants file (assuming this is just an enum you copied from C++) https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:117: 'default_content_setting_values.media_stream_mic.value)', 'categoryPrefChanged_(prefs.profile.default_content_setting_values.*)' https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:127: * Gets the pref at the given key. Asserts if the pref is not found. nit: Asserts -> Throws https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:128: * @param {string} key key -> prefPath https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:145: this.getPref_(key); why are you calling getPref_ here? just for the assert()? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.html (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:8: <dom-module id="cr-settings-site-list"> remove cr- https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:23: item-icon></iron-icon></div> wrapped weirdly. does <div> <iron-icon icon="[[computeSiteIcon_(item.url)]]" item-icon></iron-icon> </div> work? also, why is the outer div needed? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:29: alt="menu"></paper-icon-button> alt="menu"? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:31: on-iron-select="selectAction_"> onActionMenuIronSelect_ https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:39: </template> for these maybe consider hidden="[[!showBlockAction_]]" instead of <template is="dom-if" if="[[showBlockAction_]]"> https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:17: * ... other pages ... indent weird https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:47: }, can this be removed? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:63: }, do we need this? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:66: * What the main toggle for the category is set to (the global default if i still have no idea what this means https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:77: selectedOrigin: { siteOrigin? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:115: }, for all these Label_*, can we do 1 of 2 things: a) use I18nBehavior b) create a private i18n_ object, like this: i18n_: { readOnly: true, type: Object, value: function() { return { blockAction: loadTimeData.getString('siteSettingsActionBlock'), ... }, }, }, which can be used from HTML as [[i18n_.blockAction]] https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:120: this.hidden = true; move this to the template's HTML https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:165: * @param {array} siteList The list of all sites to display for this category Array preferably with types, i.e. Array<string> also consider closure compiling your code: https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:245: return 'communication:message'; // TODO(finnur): Implement actual favicons. favicons are the icons you see next to the title of the page in the tab handle https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings_page/site_settings_page.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:37: }, wat https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/content_settings_handler.cc (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:31: bool allowedList = false; cpp_var_names_like_this https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:72: : "Settings.receiveBlockedData", why does it matter which one is called currently? they both do the same thing without differentiating https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/content_settings_handler.h (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.h:2: // Use of this source code is governed by a BSD-style license that can be no (c) https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.h:8: #include <vector> why? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.h:9: #include "base/macros.h"
overall this is looking better, but: a) we should mirror C++ constants to somewhere it's more apparent, like this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... b) I understand a lot of this isn't working yet, but try your best to not leave debug code: this will be in chrome primetime eventually
Thanks for the review comments! > b) I understand a lot of this isn't working yet, but try your best to not leave > debug code: this will be in chrome primetime eventually Without context I'm not sure what this applies to, but in general I appreciate your push to get rid of debug code. I do believe, though, that in *some* cases debug code is justified. One example: The console.log function you had me remove from the click handler for the dropdown menu for each site. Here's my reasoning: - It helped my productivity greatly by speeding up my end-to-end test. - It didn't contribute to log spew, because it requires a startup flag and a user gesture to activate. - For each cycle of feedback I am spending time re-adding this code in order to validate my changes and subsequently removing it again before uploading. And that's especially frustrating in light of the fact that... - The log function is the first to go once the functionality is implemented, so it shouldn't see the light of day unless we flip the md-settings flag on a half-finished solution. :) But anyway, not a biggie. Happy to report that I've addressed your comments, hopefully to your satisfaction. PTAL. New code is starting from CL4. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/advanced_page/advanced_page.html:27: <cr-settings-location-page prefs="{{prefs}}" In email and in the CL description (a reference I've now deleted, since it no longer applies), I mentioned that I was not renaming this class in *this* CL. The purpose was mostly to facilitate easier comparing for you. I see now that this is causing confusion (which in turn is probably not reflecting well on my CL, I suspect) so I decided to take the plunge and rename this class to alleviate that confusion. I also moved this (and other classes) out of the location_page folder and into the site_settings folder, which is probably the natural place for this code. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/advanced_page/advanced_page.js:44: contentSettingsTypes: { Good point. Moved to constants.js. Thanks for the link to the existing const file, it was much appreciated. As you can see, I'm setting the .category property in the ready function now. I'd like to use the const in .html instead, but I don't see an easy way of doing that (a number of Internet searches on how to do that with Polymer came up empty). If you know of a way, I'd be all ears. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/location_page.html (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.html:23: on-change="handleToggleChange_"></paper-toggle-button> On 2015/10/10 01:27:39, Dan Beam wrote: > handleToggleChange_ -> onToggleChange_ Done (in site_settings_category.html). https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/location_page.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:80: * the values found in the ContentSettingsType. Moved to constants.js. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:96: ASK: 3, Ditto. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:117: 'default_content_setting_values.media_stream_mic.value)', On 2015/10/10 01:27:40, Dan Beam wrote: > 'categoryPrefChanged_(prefs.profile.default_content_setting_values.*)' Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:127: * Gets the pref at the given key. Asserts if the pref is not found. On 2015/10/10 01:27:40, Dan Beam wrote: > nit: Asserts -> Throws Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:128: * @param {string} key On 2015/10/10 01:27:40, Dan Beam wrote: > key -> prefPath Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/location_page.js:145: this.getPref_(key); Hmm... redundant. Removed. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.html (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:8: <dom-module id="cr-settings-site-list"> On 2015/10/10 01:27:40, Dan Beam wrote: > remove cr- Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:23: item-icon></iron-icon></div> It isn't. Removed. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:29: alt="menu"></paper-icon-button> Huh. Removed. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:31: on-iron-select="selectAction_"> On 2015/10/10 01:27:40, Dan Beam wrote: > onActionMenuIronSelect_ Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.html:39: </template> Done. My thinking here was that this list could get really long, and dom-if would make sure un-necessary menu items are not added to the DOM. But, if you feel there's a benefit to doing it the other way, then I don't feel that strongly about it. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:17: * ... other pages ... On 2015/10/10 01:27:40, Dan Beam wrote: > indent weird Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:47: }, Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:63: }, Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:66: * What the main toggle for the category is set to (the global default if Sorry, forgot to update this comment, only did so in the other file (which also keeps track of this var). https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:77: selectedOrigin: { I kind of like your previous suggestion better (selectedOrigin). It conveys to the outside (of the polymer object) that that's the origin the user selected, which is what I was going for. Or is there a polymer convention not to convey such facts to the outside? https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:115: }, Oooh, I like that idea. Converted to b). https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:120: this.hidden = true; On 2015/10/10 01:27:40, Dan Beam wrote: > move this to the template's HTML Done. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:165: * @param {array} siteList The list of all sites to display for this category On 2015/10/10 01:27:40, Dan Beam wrote: > Array preferably with types, i.e. Array<string> > > also consider closure compiling your code: > https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... Done. Need to figure out the closure compiling thing. I get a NOTIMPL thrown in landmines.py when I try to use it on Windows... https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:245: return 'communication:message'; // TODO(finnur): Implement actual favicons. I am aware of that, as evident by the fact that I implemented the async fetching of favicons for this very same screen on Android. :) Favicon fetching is probably worthy of its own CL, so this icon is simply a placeholder (so we can verify that an icon appears for sites). https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings_page/site_settings_page.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:37: }, Removed. (This is here because I was asking you about this in my email on Sept 25th, hoping to facilitate some discussion or get some guidance on how to structure and navigate these elements). https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/content_settings_handler.cc (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/content_settings_handler.cc:72: : "Settings.receiveBlockedData", I found that if two polymer objects try to setup a receive function with the same name for native code to call, then only the latter object will receive both messages. However, we've already established that this file isn't needed (well, as soon as we can work with dictionaries in prefs), so I've simply removed it. https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/compiled_resources.gyp (right): https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/compiled_resources.gyp:16: 'site_settings/compiled_resources.gyp:*', This is here to facilitate discussion. Should it be here? Is more required? Incidentally, I have been seeing dependency problems, where my changes to some .js files (I think it was site_list.js, not entirely sure) have not been picked up unless I also modify some other pre-existing .js file... https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:87: this.$.allowList.categorySubtype = siteSettings.DefaultValues.ALLOW; If there's a way to use the consts in html, then this goes away too. https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:103: If you are interested in testing this locally, add the following lines here: if (this.categorySubtype == siteSettings.DefaultValues.ALLOW) { this.siteList_ = [ {url: "http://foo.com"}, {url: "http://bar.com"} ]; } else if (this.categorySubtype == siteSettings.DefaultValues.BLOCK) { this.siteList_ = [ {url: "http://foo2.com"}, {url: "http://bar2.com"} ]; } else { assertNotReached(); }
finnur@chromium.org changed reviewers: + michaelpg@chromium.org
Dan: I'd really appreciate hearing some feedback before EOD (your timezone) since that will ensure I can hit the ground running when I come in on Monday morning -- it is now nearing EOD (and EOW) here. Michael: There's also a question for you a bit further up the thread here (a few comments above starting with 2015-10-07 12:10:48 UTC -- Note: if Rietveld is not showing UTC time for you, just search for :48 and start reading).
https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:43: this.$.locationCategory.category = why wouldn't the location category itself do this? https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/location_page/site_list.html (right): https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:8: <dom-module id="settings-site-list"> this should be in a site_settings_list or site_settings directory, not location_page https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:10: href="chrome://md-settings/settings_page/settings_page.css"> nit: don't wrap, also don't use chrome://md-setting/ IMO https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/constants.js (right): https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:25: * Containins the possible default values for a given contentSettingsType. Contains? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:29: </cr-site-settings-category> rebase and/or remove "cr-" https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:44: siteSettings.ContentSettingsTypes.GEOLOCATION; why can't this be done from within the page itself. this should be a property somewhere. as in: Polymer ({ properties: { type: siteSettings.ContentSettingsTypes.GEOLOCATION, }, }) https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/location_page/site_list.html (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:10: href="chrome://md-settings/settings_page/settings_page.css"> unwrap and/or remove chrome://md-settings/ https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:15: <paper-item class="menu-trigger"> why is it a menu with only a submenu? probably only need one or the other, no? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:19: <template is="dom-repeat" items="{{siteList_}}"> can we use iron-list instead? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:21: <div><iron-icon don't put two tags on the same line unless the close on the same line https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:34: value="Allow">[[allowActionLabel_]]</paper-item> don't use the literal "Allow" here. instead use a constant so we can't misspell it https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:29: siteList_: { why not sites_? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:63: }, don't make variables names that are only separated by an s and are copies of other state https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:69: toggleState_: { this is a bad name why not something involving default and our global (like your description). https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:81: group by access (like in c++) https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:113: resetActionLabel_: { group all the labels together (and/or check out I18nBehavior) https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:120: this.hidden = true; move to the markup or CSS https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:124: [this.category, this.categorySubtype == this.categorySubtypes.ALLOW]); why are we not just sending the enum value? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:133: return self.receiveSiteList_.apply(self, arguments); what's different? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:160: this.resetActionLabel_ = loadTimeData.getString('siteSettingsActionReset'); why is this being done here? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:247: remove newline https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/constants.js (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:5: cr.define('siteSettings', function() { why siteSettings instead of settings? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:6: why this \n? https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:25: * Containins the possible default values for a given contentSettingsType. contains
https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:245: return 'communication:message'; // TODO(finnur): Implement actual favicons. On 2015/10/15 15:46:33, Finnur wrote: > I am aware of that, as evident by the fact that I implemented the async fetching > of favicons for this very same screen on Android. :) > > Favicon fetching is probably worthy of its own CL, so this icon is simply a > placeholder (so we can verify that an icon appears for sites). what I'm saying is that this comment seems as if this code will affect the favicon of this page rather than get the icon associated with the site we're showing. that was my impression when first reading your comment. I understand I misread it. hopefully it won't be there to long for others to make these same mistake i did. https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/compiled_resources.gyp (right): https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/compiled_resources.gyp:16: 'site_settings/compiled_resources.gyp:*', On 2015/10/15 15:46:33, Finnur wrote: > This is here to facilitate discussion. > > Should it be here? Is more required? > > Incidentally, I have been seeing dependency problems, where my changes to some > .js files (I think it was site_list.js, not entirely sure) have not been picked > up unless I also modify some other pre-existing .js file... you probably shouldn't add it if you can't compile your code https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:87: this.$.allowList.categorySubtype = siteSettings.DefaultValues.ALLOW; On 2015/10/15 15:46:34, Finnur wrote: > If there's a way to use the consts in html, then this goes away too. I can see the benefit of this, though I'm not sure how much namespace resolution polymer does. this seems like it might be common (binding outside of |this|) so I'll ask there polymer team whether this is a bug or a feature :)
Thanks for the review feedback. Really appreciated it. https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/location_page/site_list.js:245: return 'communication:message'; // TODO(finnur): Implement actual favicons. Oh, I see. Good point. Changed. https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/compiled_resources.gyp (right): https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/compiled_resources.gyp:16: 'site_settings/compiled_resources.gyp:*', You mean if I can't run the closure compiler due to hitting the NOTREACHED? OK, removed. https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:87: this.$.allowList.categorySubtype = siteSettings.DefaultValues.ALLOW; Much appreciated. Look forward to hearing their answer. https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:43: this.$.locationCategory.category = I'm not sure I follow. Remember... there is no location category anymore, only a general purpose class that can display any category we want, and it needs to be told which category to display. I would prefer not to do it exactly like this, and instead tell it in HTML, through a property on the HTML, such as: category="{{siteSettings.ContentSettingsTypes.GEOLOCATION}}" ... but as we've established, it is not clear how to do that. https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/location_page/site_list.html (right): https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:8: <dom-module id="settings-site-list"> Oh, noes. My bad. :( These two files (location_page/site_list.html/js) are redundant. When I moved them to the site_settings folder I must have forgotten to close them, so my editor decided to preserve the old copies and I simply missed that when I uploaded (I generally review my changes before publishing my updates, but new files I prefer to review in my editor with syntax highlighting and all). I'll make the suggested changes (those that still apply) but to the right site_list.html and .js files, of course. My apologies for the confusion! https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:10: href="chrome://md-settings/settings_page/settings_page.css"> Not a problem. FWIW the chrome://md-setting/ convention came from the original (location_page.html). :) https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/constants.js (right): https://codereview.chromium.org/1372053002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:25: * Containins the possible default values for a given contentSettingsType. Done (as mentioned elsewhere). https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:29: </cr-site-settings-category> I'm not sure what rebase means in this context, but I'm fine with removing the "cr-" prefix. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:44: siteSettings.ContentSettingsTypes.GEOLOCATION; See answer elsewhere. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/location_page/site_list.html (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:10: href="chrome://md-settings/settings_page/settings_page.css"> Done. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:15: <paper-item class="menu-trigger"> Sure. A sole <paper-submenu> seems to work just fine. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:19: <template is="dom-repeat" items="{{siteList_}}"> Hmm... I'm able to construct a free-standing iron-list showing my data, but simply the act of moving it inside my paper-menu causes it to refuse to draw any content. I tried sending the iron-resize event, but that seemed to do nothing and despite playing with it for a while I was not able to get it to do what I wanted... :/ https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:21: <div><iron-icon Done (in other file). https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.html:34: value="Allow">[[allowActionLabel_]]</paper-item> Done. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/location_page/site_list.js (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:29: siteList_: { Yup. That works too. Done. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:63: }, This has already been removed from the more up-to-date file. Again, my apologies for the confusion. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:69: toggleState_: { Already fixed (see previous comment, one up). https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:81: Moved alongside |prefs| (in the file that matters). https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:113: resetActionLabel_: { Already fixed. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:120: this.hidden = true; Already fixed. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:124: [this.category, this.categorySubtype == this.categorySubtypes.ALLOW]); Already removed. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:133: return self.receiveSiteList_.apply(self, arguments); Already removed. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:160: this.resetActionLabel_ = loadTimeData.getString('siteSettingsActionReset'); Already removed. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/location_page/site_list.js:247: Already removed. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/constants.js (right): https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:5: cr.define('siteSettings', function() { Sure, 'settings' also works for me. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:6: On 2015/10/19 07:18:07, Dan Beam wrote: > why this \n? Done. https://codereview.chromium.org/1372053002/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:25: * Containins the possible default values for a given contentSettingsType. On 2015/10/19 07:18:07, Dan Beam wrote: > contains Done.
Just getting to this after two weeks in India :-)
On 2015/10/07 12:10:48, Finnur wrote:
> Dan, I've got a question on using prefs for the list of sites, which I started
> looking into this morning...
>
> As you know, we expose prefs through prefs_util.cc, where we whitelist certain
> prefs for .js use (see PrefsUtil::GetWhitelistedKeys) and we specify their
type
> at the same time. These types are supported:
>
> enum PrefType {
> PREF_TYPE_NONE,
> PREF_TYPE_BOOLEAN,
> PREF_TYPE_NUMBER,
> PREF_TYPE_STRING,
> PREF_TYPE_URL,
> PREF_TYPE_LIST,
> PREF_TYPE_LAST = PREF_TYPE_LIST,
> };
>
> However, the relevant preference that we want to expose is, for example:
>
> "profile": {
> "content_settings": {
> "exceptions": {
> "notifications": {
> "https:\/\/foo.com,*": {
> "last_used": 1442486005.4385,
> "setting": 1
> },
> "https:\/\/bar.com,*": {
> "last_used": 1442486005.4385,
> "setting": 1
> },
>
> }
> }
> }
> }
>
> In other words, the list we need to show in the UI is under
> profile.content_settings.exceptions.notifications, which is a dictionary, not
a
> list. Two problems there:
>
> 1) Dictionary is not a type supported by the prefs_util.cc.
I'm uploading a patch to add the dictionary type to prefs_util and the
settingsPrivate IDL.
Only reason we haven't yet is because we haven't needed to.
c/b/resources/settings/prefs/prefs.js already supports dictionary types, so
after updating the
API everything should just work.
> 2) This makes it hard to white-list each pref that we want to show and be able
> to update, because we'd have to do that dynamically somehow. No?
>
> As a test, I already tried fooling the system by specifying STRING as a pref
> type (hoping to create a dictionary out of it on the .js side), but that got
> flagged in the schema validation (Invalid value for argument 1. Property
> '.20.type': Value must be one of: [BOOLEAN, NUMBER, STRING, URL, LIST]).
>
> Any suggestions?
Thanks, Michael. These changes seem to work well and allow me to show the site list, which I did to avoid sitting idle. Those changes are in this CL (not a lot of code) plus some polish. Dan: ping (PTAL).
Clarification: https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/settings_private/prefs_util.cc:126: (*s_whitelist)["profile.default_content_setting_values.popups"] = The changes above this line are purely alphabetical ordering.
Some comments. Still need to look over site_settings_category.* https://codereview.chromium.org/1372053002/diff/180001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/app/settings_st... chrome/app/settings_strings.grdp:426: <message name="IDS_SETTINGS_SITE_SETTINGS_ASK_FIRST_RECOMMENDED" desc="Label for ask first option in site settings."> all with "(recommended)": provide more context in desc https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/constants.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:8: * configuring in the UI). comment on where these numbers come from/what file this should be in sync with https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:20: MIC: 14, 12? https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icons/communication-icons.html"> add iron-icon/iron-icon.html https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:5: <link rel="import" href="chrome://resources/polymer/v1_0/paper-menu/paper-menu.html"> add paper-icon-button, paper-menu-button https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:7: <link rel="import" href="chrome://md-settings/prefs/prefs.html"> s/prefs.html/prefs_types.html https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:12: <link rel="import" type="css" href="../settings_page/settings_page.css"> o_O we don't use "../" relative paths elsewhere, I'd say make this absolute (chrome://md-settings/) but I'm not sure if it matters. dbeam? https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:30: on-click="onOriginClick_">[[item.url]]</paper-item></div> on-tap https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:7: * 'settings-site-list' is the widget that shows a list of Allowed and nit: "is the widget that shows" --> "shows" https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:40: * Array of sites to display in the widget. /** @type {Array<{url: String}>} */ https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:44: value: [], function() { return []; }, ^ so multiple <settings-site-list>s don't refer to the same sites_ array https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:49: * See |categories| for possible values. update reference https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:51: category: { category: Number, &c for all following properties https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:91: type: Object, readOnly: true, https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:104: allowAction: loadTimeData.getString('siteSettingsActionAllow'), 2-space indent https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:141: var sites = pref['value']; nit: pref.value https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:156: setupActionMenu_: function() { ubernit: setUpActionMenu_ https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:168: onOriginClick_: function(event) { Tap https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:17: getPref_: function(prefPath) { extract into separate behavior for this and settings-languages-singleton https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:31: setPrefValue_: function(prefPath, value) { same https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:32: this.set('prefs.' + prefPath + '.value', value); contradictory to the comment, this doesn't throw if the pref is not found https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:58: // Wonder if any of these enum values are directly accessible from .js? make a TODO or remove comment (i'm not sure what this comment means)
Patchset #11 (id:200001) has been deleted
Patchset #11 (id:220001) has been deleted
Feedback addressed. PTAL. https://codereview.chromium.org/1372053002/diff/180001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/app/settings_st... chrome/app/settings_strings.grdp:426: <message name="IDS_SETTINGS_SITE_SETTINGS_ASK_FIRST_RECOMMENDED" desc="Label for ask first option in site settings."> On 2015/10/22 16:11:33, michaelpg wrote: > all with "(recommended)": provide more context in desc Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/constants.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:8: * configuring in the UI). On 2015/10/22 16:11:33, michaelpg wrote: > comment on where these numbers come from/what file this should be in sync with Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/constants.js:20: MIC: 14, Good catch! https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icons/communication-icons.html"> On 2015/10/22 16:11:33, michaelpg wrote: > add iron-icon/iron-icon.html Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:5: <link rel="import" href="chrome://resources/polymer/v1_0/paper-menu/paper-menu.html"> On 2015/10/22 16:11:33, michaelpg wrote: > add paper-icon-button, paper-menu-button Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:7: <link rel="import" href="chrome://md-settings/prefs/prefs.html"> On 2015/10/22 16:11:33, michaelpg wrote: > s/prefs.html/prefs_types.html Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:12: <link rel="import" type="css" href="../settings_page/settings_page.css"> dbeam is the one who requested the relative paths... :) https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:30: on-click="onOriginClick_">[[item.url]]</paper-item></div> On 2015/10/22 16:11:33, michaelpg wrote: > on-tap Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:7: * 'settings-site-list' is the widget that shows a list of Allowed and On 2015/10/22 16:11:34, michaelpg wrote: > nit: "is the widget that shows" --> "shows" Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:40: * Array of sites to display in the widget. Doesn't work. Not sure what the error means... line 41: Don't use wrapper types (i.e. new String() or @type {String}) * @type {Array<{url: String}>} ^^^^^^ https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:44: value: [], On 2015/10/22 16:11:34, michaelpg wrote: > function() { return []; }, > > ^ so multiple <settings-site-list>s don't refer to the same sites_ array Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:49: * See |categories| for possible values. On 2015/10/22 16:11:33, michaelpg wrote: > update reference Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:51: category: { On 2015/10/22 16:11:34, michaelpg wrote: > category: Number, > > &c for all following properties Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:91: type: Object, On 2015/10/22 16:11:33, michaelpg wrote: > readOnly: true, Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:104: allowAction: loadTimeData.getString('siteSettingsActionAllow'), On 2015/10/22 16:11:34, michaelpg wrote: > 2-space indent Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:141: var sites = pref['value']; On 2015/10/22 16:11:33, michaelpg wrote: > nit: pref.value Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:156: setupActionMenu_: function() { uberdone. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:168: onOriginClick_: function(event) { On 2015/10/22 16:11:34, michaelpg wrote: > Tap Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:17: getPref_: function(prefPath) { It is a bit unfortunate that behaviors can't inherit behaviors from other behaviors, since this causes a bit of a cascading effect on isPrefEnabled... https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:31: setPrefValue_: function(prefPath, value) { On 2015/10/22 16:11:34, michaelpg wrote: > same Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:32: this.set('prefs.' + prefPath + '.value', value); On 2015/10/22 16:11:34, michaelpg wrote: > contradictory to the comment, this doesn't throw if the pref is not found Done. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:58: // Wonder if any of these enum values are directly accessible from .js? Removed (the todo is kind of covered already in constants.js).
https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:17: getPref_: function(prefPath) { On 2015/10/26 14:38:05, Finnur wrote: > It is a bit unfortunate that behaviors can't inherit behaviors from other > behaviors, since this causes a bit of a cascading effect on isPrefEnabled... You could override getPref_. Each behavior overrides any similarly named behaviors before it in the behaviors list. https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_behavior.js (right): https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_behavior.js:31: this.set('prefs.' + prefPath + '.value', value); call this.getPref_ so the assert runs: setPrefValue_: function(prefPath, value) { this.getPref_(prefPath); this.set('prefs.' + prefPath + '.value', value); }, https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:8: <link rel="import" href="chrome://resources/polymer/v1_0/paper-menu/paper-menu.html"> nit: alphabetize the icon and paper element imports https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:25: <paper-toggle-button id="toggle" checked="{{categoryEnabled}}" The current Options implementation isn't just binary allow/block; some content settings, including Location have an "Ask when a website tries to [x]" option that can be applied to the entire category. Is that something you plan to implement eventually? I'm worried trying to support that (some categories have 2 options, some have 3) for each category and site list using the model in this CL will be complicated.
https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:25: <paper-toggle-button id="toggle" checked="{{categoryEnabled}}" I haven't updated the code, just wanted to get a quick word in... My answer to your question is: I think we are moving away from the tri-state altogether and when categories need more complex settings we'll add a check-box (or something) below the toggle, such as with the cookies category and 3rd party cookies. Rebecca, or someone can probably chime in with a more authoritative answer. I'll try to loop her in.
Addressed. PTAL. https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:17: getPref_: function(prefPath) { I suspect you are suggestion a solution to a different problem than I'm having. My problem is that I wanted to use getPref_ from within SiteSettingsBehavior, and your suggestion provided a hint that made it possible (by defining the PrefsBehavior *ahead* of the SiteSettingsBehavior). So, after all my problem wasn't that I needed inheritance, although that would also have been a solution. https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_behavior.js (right): https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_behavior.js:31: this.set('prefs.' + prefPath + '.value', value); On 2015/10/26 22:15:22, michaelpg wrote: > call this.getPref_ so the assert runs: > > setPrefValue_: function(prefPath, value) { > this.getPref_(prefPath); > this.set('prefs.' + prefPath + '.value', value); > }, Done. https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:8: <link rel="import" href="chrome://resources/polymer/v1_0/paper-menu/paper-menu.html"> On 2015/10/26 22:15:22, michaelpg wrote: > nit: alphabetize the icon and paper element imports Done (I believe).
Ping...
Suggesting some name changes, and a couple things that confuse me or might be bugs. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:34: selectedOrigin: { nit: selectedSite? https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:214: return siteList.length > 0 && toggleState; If this is a BLOCK list, and the category is set to ALLOW, then we never show the list? https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:7: * NOTE: This behavior depends on PrefsHehavior so classes that need this PrefsBehavior https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:8: * behavior must first list the PrefsBehavior (ahead of this behavior). You could rename this to SiteSettingsBehaviorImpl, and then define: SiteSettingsBehavior = [PrefsBehavior, SiteSettingsBehaviorImpl] But this could lead to ugly multiple inheritance. I'll talk to the Polymer team and see what they think; it's fine for now. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:19: isPrefEnabled_: function(category) { isCategoryAllowed_ https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:24: return pref.value != settings.DefaultValues.ALLOW; should this be ASK? https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:35: computeIcon_: function(category) { computeIconForContentCategory_ (overly generic names are at risk of being overridden by other behaviors) https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:65: computeTitle_: function(category) { computeTitleForContentCategory_ https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:95: computeTogglePrefName_: function(category) { computeCategoryPrefName_ https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:107: computeExceptionsPrefName_: function(category) { computeCategoryExceptionsPrefName_ https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:150: computeDesc_: function(category, categoryEnabled) { computeCategoryDesc_ https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:35: route: String, unused https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:40: subpage: { unused https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:49: PAGE_ID: { unused https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:58: pageTitle: { unused
All addressed/commented on. PTAL. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:34: selectedOrigin: { Technically, Origin is more accurate than Site, since this is a list of origins and it can contain multiple entries for the same site, such as: http://foo.com:123 http://foo.com:234 I've fixed the comment, though. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:214: return siteList.length > 0 && toggleState; At first I thought you'd spotted a bug, but on closer reading I see that this function is correct. Here is how it is supposed to work: If this is a BLOCK list we only show it if it contains any entries *and* the category is set to ALLOW. The reasoning is that when the whole category is set to BLOCK, we don't need to list the individual sites that are also blocked (it would be redundant). This matches what we did for Chrome on Android, which is what we are trying to get parity with. But I'll reorder this function to try to clarify and add a comment. Your comment, however, exposed a bug in that the categories are not changing anymore when the data changes, but I've now rectified that. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:7: * NOTE: This behavior depends on PrefsHehavior so classes that need this On 2015/10/28 18:02:21, michaelpg wrote: > PrefsBehavior Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:8: * behavior must first list the PrefsBehavior (ahead of this behavior). On 2015/10/28 18:02:21, michaelpg wrote: > You could rename this to SiteSettingsBehaviorImpl, and then define: > > SiteSettingsBehavior = [PrefsBehavior, SiteSettingsBehaviorImpl] > > But this could lead to ugly multiple inheritance. I'll talk to the Polymer team > and see what they think; it's fine for now. Acknowledged. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:19: isPrefEnabled_: function(category) { On 2015/10/28 18:02:21, michaelpg wrote: > isCategoryAllowed_ Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:24: return pref.value != settings.DefaultValues.ALLOW; Ooops! Yes. Good catch. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:35: computeIcon_: function(category) { On 2015/10/28 18:02:21, michaelpg wrote: > computeIconForContentCategory_ > > > (overly generic names are at risk of being overridden by other behaviors) Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:65: computeTitle_: function(category) { On 2015/10/28 18:02:21, michaelpg wrote: > computeTitleForContentCategory_ Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:95: computeTogglePrefName_: function(category) { On 2015/10/28 18:02:21, michaelpg wrote: > computeCategoryPrefName_ Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:107: computeExceptionsPrefName_: function(category) { On 2015/10/28 18:02:21, michaelpg wrote: > computeCategoryExceptionsPrefName_ Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:150: computeDesc_: function(category, categoryEnabled) { On 2015/10/28 18:02:21, michaelpg wrote: > computeCategoryDesc_ Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:35: route: String, Yeah. These I are from the skeleton location page that I inherited. Wasn't sure what they did or what to do with them. Should have just deleted them, I guess. Removed. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:40: subpage: { On 2015/10/28 18:02:21, michaelpg wrote: > unused Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:49: PAGE_ID: { On 2015/10/28 18:02:21, michaelpg wrote: > unused Done. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:58: pageTitle: { On 2015/10/28 18:02:21, michaelpg wrote: > unused Done.
LGTM, modulo my question about the ALLOW list and 2 indent nits. Thanks! https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:214: return siteList.length > 0 && toggleState; On 2015/10/29 12:21:21, Finnur wrote: > At first I thought you'd spotted a bug, but on closer reading I see that this > function is correct. > > Here is how it is supposed to work: > If this is a BLOCK list we only show it if it contains any entries *and* the > category is set to ALLOW. > > The reasoning is that when the whole category is set to BLOCK, we don't need to > list the individual sites that are also blocked (it would be redundant). This > matches what we did for Chrome on Android, which is what we are trying to get > parity with. If this is an ALLOW list, we always show it if it contains any entries, even if the category is set to ALLOW. Is that right? > > But I'll reorder this function to try to clarify and add a comment. > > Your comment, however, exposed a bug in that the categories are not changing > anymore when the data changes, but I've now rectified that. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:35: route: String, On 2015/10/29 12:21:22, Finnur wrote: > Yeah. These I are from the skeleton location page that I inherited. Wasn't sure > what they did or what to do with them. Should have just deleted them, I guess. Yup those are the last vestiges of a previous routing system. Thanks! > > Removed. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:31: category-enabled="[[categoryEnabled]]"></settings-site-list> nit: use same indent as previous line https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:34: category-enabled="[[categoryEnabled]]"></settings-site-list> nit: use same indent as previous line
I don't understand why we'd make @polymerBehaviors full of @private methods. these should probably be public or go back to just being private instance methods. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:193: is: 'settings-languages-singleton', nit: \n https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_behavior.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_behavior.js:31: setPrefValue_: function(prefPath, value) { these should be @protected or public https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:29: <template is="dom-repeat" items="{{sites_}}"> i think this will eventually need to be an iron-list instead https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:145: if (sites[origin]['setting'] == this.categorySubtype) { nit: sites[origin].setting https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:147: newList.push({ url: tokens[0] }); push({url: tokens[0]}); ^ no spaces ^ https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:228: else nit: no else after a return https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.html:1: <link rel="import" href="chrome://md-settings/site_settings/constants.html"> this also depends on [at least] load_time_data and assert https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.html:2: nit: remove \n https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:12: var SiteSettingsBehavior = { if this is going to call getPref_, it should have: behaviors: [PrefsBehavior], as a dependency https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:20: is: 'site-settings-category', \n
Addressed comments from both Dan and Michael. PTAL. https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:214: return siteList.length > 0 && toggleState; Correct. That's how it works on Chrome for Android today. By default, the category is set to ALLOW, and we show both BLOCK and ALLOW. Once the user flips the category default to BLOCK, we show the ALLOW list, but change the header from 'Allowed' to 'Exceptions'. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/languages_page/languages.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/languages_page/languages.js:193: is: 'settings-languages-singleton', On 2015/10/29 21:10:39, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_behavior.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_behavior.js:31: setPrefValue_: function(prefPath, value) { Sure. Protected kind of makes sense in this context, I guess. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:29: <template is="dom-repeat" items="{{sites_}}"> I addressed this previously. I'm happy to change this to an iron-list, if there's a good reason and I can get it to work, which hasn't been the case. As a test, I constructed a free-standing iron-list showing my data, but simply the act of moving it inside my paper-menu causes it to refuse to draw any content. I tried sending the iron-resize event, but that seemed to do nothing and despite playing with it for a while I was not able to get it to do what I wanted... :/ To see what I mean, you can add the following code snippet first to lines 19 and then 29 and only the former case will show the data: <iron-list items="[[sites_]]" as="item"> <template><div>url: <span>[[item.url]]</span></div></template> </iron-list> https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:145: if (sites[origin]['setting'] == this.categorySubtype) { On 2015/10/29 21:10:39, Dan Beam wrote: > nit: sites[origin].setting Done. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:147: newList.push({ url: tokens[0] }); On 2015/10/29 21:10:39, Dan Beam wrote: > push({url: tokens[0]}); > ^ no spaces ^ Done. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:228: else On 2015/10/29 21:10:39, Dan Beam wrote: > nit: no else after a return Done. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.html:1: <link rel="import" href="chrome://md-settings/site_settings/constants.html"> Hmmm... It seems to be fine to include... <link rel="import" href="chrome://resources/html/assert.html"> ... but if I also add this ... <link rel="import" href="chrome://resources/html/load_time_data.html"> I get: "Unexpected condition on chrome://md-settings/advanced: should only include this file once", source: chrome://resources/js/load_time_data.js (139) Curiously, there is currently no md-settings page that includes load_time_data.html, despite many pages the use loadTimeData.getString. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.html:2: On 2015/10/29 21:10:39, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:12: var SiteSettingsBehavior = { I experimented with making behaviors inherit from behaviors and could neither get it to work nor find any reference material on it on the web. But Michael's suggestion of using: var SiteSettingsBehavior = [PrefsBehavior, SiteSettingsBehaviorImpl]; ... does seem to work. I have switched to that instead. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:31: category-enabled="[[categoryEnabled]]"></settings-site-list> On 2015/10/29 19:41:58, michaelpg wrote: > nit: use same indent as previous line Done. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:34: category-enabled="[[categoryEnabled]]"></settings-site-list> On 2015/10/29 19:41:58, michaelpg wrote: > nit: use same indent as previous line Done. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:20: is: 'site-settings-category', On 2015/10/29 21:10:39, Dan Beam wrote: > \n Done.
lgtm I may have more nits in the future but let's land this puppy, it's been too long https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:29: <template is="dom-repeat" items="{{sites_}}"> On 2015/10/30 12:13:46, Finnur wrote: > I addressed this previously. I'm happy to change this to an iron-list, if > there's a good reason and I can get it to work, which hasn't been the case. > > As a test, I constructed a free-standing iron-list showing my data, but simply > the act of moving it inside my paper-menu causes it to refuse to draw any > content. I tried sending the iron-resize event, but that seemed to do nothing > and despite playing with it for a while I was not able to get it to do what I > wanted... :/ > > To see what I mean, you can add the following code snippet first to lines 19 and > then 29 and only the former case will show the data: > > <iron-list items="[[sites_]]" as="item"> > <template><div>url: <span>[[item.url]]</span></div></template> > </iron-list> it needs an explicit height: """ Important: iron-list must ether be explicitly sized, or delegate scrolling to an explicitly sized parent. By "explicitly sized", we mean it either has an explicit CSS height property set via a class or inline style, or else is sized by other layout means (e.g. the flex or fit classes). """ https://elements.polymer-project.org/elements/iron-list https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.html:1: <link rel="import" href="chrome://md-settings/site_settings/constants.html"> On 2015/10/30 12:13:47, Finnur wrote: > Hmmm... > > It seems to be fine to include... > <link rel="import" href="chrome://resources/html/assert.html"> > ... but if I also add this ... > <link rel="import" href="chrome://resources/html/load_time_data.html"> > > I get: > "Unexpected condition on chrome://md-settings/advanced: should only include > this file once", source: chrome://resources/js/load_time_data.js (139) > > Curiously, there is currently no md-settings page that includes > load_time_data.html, despite many pages the use loadTimeData.getString. imports should be de-duped automatically. just ignore this for now https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:12: var SiteSettingsBehavior = { On 2015/10/30 12:13:47, Finnur wrote: > I experimented with making behaviors inherit from behaviors and could neither > get it to work nor find any reference material on it on the web. > > But Michael's suggestion of using: > var SiteSettingsBehavior = [PrefsBehavior, SiteSettingsBehaviorImpl]; > > ... does seem to work. I have switched to that instead. cool with me https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_behavior.js (right): https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_behavior.js:31: setPrefValue_: function(prefPath, value) { _ is only used for @private. these should be getPref and setPrefValue https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:145: * @protected drop all the _s
Thanks for the feedback. https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/1372053002/diff/290001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.html:29: <template is="dom-repeat" items="{{sites_}}"> Ah. OK. Will check in as-is and try this in a followup. https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_behavior.js (right): https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_behavior.js:31: setPrefValue_: function(prefPath, value) { On 2015/10/30 20:23:53, Dan Beam wrote: > _ is only used for @private. these should be getPref and setPrefValue Done. https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_behavior.js (right): https://codereview.chromium.org/1372053002/diff/310001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_behavior.js:145: * @protected On 2015/10/30 20:23:54, Dan Beam wrote: > drop all the _s Done.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1372053002/#ps330001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372053002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372053002/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1372053002/#ps390001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1372053002/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1372053002/390001
Message was sent while issue was closed.
Committed patchset #19 (id:390001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/1c386ff87977f702a5260314ff78dab1143304d5 Cr-Commit-Position: refs/heads/master@{#357346}
Message was sent while issue was closed.
https://codereview.chromium.org/1372053002/diff/390001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1372053002/diff/390001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:30: </cr-settings-section> wrong closing tag -- now everything is a child of this section :-)
Message was sent while issue was closed.
https://codereview.chromium.org/1372053002/diff/390001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1372053002/diff/390001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:30: </cr-settings-section> On 2015/11/02 23:25:56, michaelpg wrote: > wrong closing tag -- now everything is a child of this section :-) Good find. BTW, this broke the paper-dialog rendering in reset-page/reset-page.html.
Message was sent while issue was closed.
https://codereview.chromium.org/1372053002/diff/390001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.html (right): https://codereview.chromium.org/1372053002/diff/390001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.html:30: </cr-settings-section> On 2015/11/04 01:58:43, dpapad wrote: > On 2015/11/02 23:25:56, michaelpg wrote: > > wrong closing tag -- now everything is a child of this section :-) > > Good find. BTW, this broke the paper-dialog rendering in > reset-page/reset-page.html. to be fixed with https://codereview.chromium.org/1415693011 |
