Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1371073003:
Display material design policies grouped by tags. (Closed)
|
Created:
5 years, 2 months ago by fhorschig Modified:
4 years, 3 months ago CC:
arv+watch_chromium.org, chromium-reviews, michaelpg+watch-polymer_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisplay material design policies grouped by tags.
Tags of policies in chrome://md-policy let policies appear in a group with the tag's name.
Policies without tags won't be displayed.
Policies with multiple tags will appear in every group.
BUG=134849
Patch Set 1 #Patch Set 2 : Policies and groups are sent and displayed in a simple way. #Patch Set 3 : Cleaning and Refactoring. #
Total comments: 8
Patch Set 4 : Extracting PolicyUi + Renaming. #
Total comments: 59
Patch Set 5 : Closure Annotations. #
Total comments: 6
Patch Set 6 : Moved component changes to different CL. #
Total comments: 2
Patch Set 7 : Typing properties. #
Total comments: 2
Patch Set 8 : Remove typing for string property. #
Total comments: 87
Patch Set 9 : Replaced loops with dom-repeat. #Patch Set 10 : Changed readonly to private properties. #
Total comments: 34
Patch Set 11 : Tags as string array. Removed listOf. #
Total comments: 16
Patch Set 12 : Test corrections. #
Total comments: 2
Messages
Total messages: 64 (23 generated)
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
fhorschig@chromium.org changed reviewers: + tnagel@chromium.org
Please mention that this is for chrome://md-policy in the CL description.
I don't think I'm a good reviewer for this. I'd suggest to ask stevenjb@.
fhorschig@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven, Could you please have a look at this CL (especially the polymer parts). Cheers, Friedrich
Might I suggest separating out the addition of policy_resources.grd from the rest of the changes? Thanks!
I only did a partial review. In addition to the suggestions below, please add closure compilation to this: For details see https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... * Add a compiled_resources.gyp file to chrome/browser/resources/md_policy/ * Include entries for policy_group and policy_ui * Add the new compiled_resources.gyp to third_party/closure_compiler/compiled_resources.gyp Thanks! https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_policy/cr_policy_group.html (right): https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/cr_policy_group.html:11: <dom-module id="cr-policy-group"> nit: We've been moving towards not using a cr- prefix for elements that are page specific and have a descriptive prefix (e.g. policy-). Also, even where we still have the cr- prefix we generally don't name the file with cr_ since it just adds noise and length. So, I would suggest policy_group.html and <policy-group> https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/cr_policy_group.html:15: aria-label="[[riskTag]]"> Use one line when everything fits. When it doesn't, event the following line 4 spaces. See https://www.chromium.org/developers/web-development-style-guide#Body https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_policy/cr_policy_group.js (right): https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/cr_policy_group.js:36: * @param {string} tag Name of the policy. s/tag/policy/ https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:24: <dom-module id="cr-policy-ui"> Polymer elements and their scripts should be placed in their own files.
https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_policy/cr_policy_group.html (right): https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/cr_policy_group.html:11: <dom-module id="cr-policy-group"> On 2015/10/06 22:42:03, stevenjb wrote: > nit: We've been moving towards not using a cr- prefix for elements that are page > specific and have a descriptive prefix (e.g. policy-). Also, even where we still > have the cr- prefix we generally don't name the file with cr_ since it just adds > noise and length. So, I would suggest policy_group.html and <policy-group> Done. https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/cr_policy_group.html:15: aria-label="[[riskTag]]"> On 2015/10/06 22:42:03, stevenjb wrote: > Use one line when everything fits. > When it doesn't, event the following line 4 spaces. > See https://www.chromium.org/developers/web-development-style-guide#Body Done. https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_policy/cr_policy_group.js (right): https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/cr_policy_group.js:36: * @param {string} tag Name of the policy. On 2015/10/06 22:42:03, stevenjb wrote: > s/tag/policy/ Done. https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/180001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:24: <dom-module id="cr-policy-ui"> On 2015/10/06 22:42:03, stevenjb wrote: > Polymer elements and their scripts should be placed in their own files. Done.
https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.css (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.css:6: background: #DDD; Prefer RGB (See https://www.chromium.org/developers/web-development-style-guide#Color) https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:22: <script src="chrome://resources/js/i18n_template.js"></script> Any reason this can't be above with the other script includes? https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.html (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:11: <div id="content" class="card-content"></div> Is class needed here? https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:26: factoryImpl: function(riskTag) { @param for riskTag Also, declare (and type) riskTag in the object. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.html (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:13: aria-label="[[introduction.title]]"> Fix indent. (4 spaces from tag) https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:21: this.tagGroups = []; declare tagGroups as a typed property or variable in the object. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:27: this.introduction = { Declare and type introduction https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:42: chrome.send('initialized'); Prefer a more specifically named identifier, e.g. policyUiInitialized. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:56: ++pos; Use {} when the condition or statement is more than 1 line. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:67: this.$.groups.removeChild(this.$.groups.firstChild); 2 spaces here, not 4. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:72: * @private @private last https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:85: * Receives an list of known tags and creates an empty policy list for each. a list https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:86: * @param {Array} tags List of tags. {!Array<string>} (Always use ! or ? before Object or object typedefs and Array) https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:95: * @param {{chromePolicyNames: Object<string, Array<string>>}} namesWithTags !Object<!Array<string>> (keys are always strings so we omit the type) https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:100: this.addNameToTagGroup_.bind(this, name)); {} https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:105: * @param {Object} values Dictionary containing the current policy values. {!Object} or {?Object} https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:113: * @param {Object} status Dictionary containing meta data about set policies. ! or ? https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:122: cr.define('policy', function() { Can we put this above the Polymer object so that it is visibly declared before it is used? Also, type the return value. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:111: void AddLocalizedPoilcyStrings(content::WebUIDataSource* source, Poilcy -> Policy https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:112: const PolicyStringMap* map, nit: maps or map_list https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:494: const std::string& name) const override; Mutable arguments should be declared last, e.g. AddPolicyName(name, name_dictionary) https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:927: scoped_ptr<base::ListValue> list = make_scoped_ptr(new base::ListValue()); scoped_ptr<base::ListValue> list(new base::ListValue); https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:931: list->AppendString(kPolicyRiskTags[tags[i]].key); {} https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:107: result.push(root); {} https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:109: root.id == selector.slice(1)) one line https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:116: selector, root.shadowRoot.children[i])); {} https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:162: return ui.tagGroups[i].element; P{ https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:170: * @param {text} text This is confusing... where is |text| typedef'd? If it is a real type, maybe name the variable something else... that's kind of like having a variable in C++ named 'string' :)
Patchset #5 (id:220001) has been deleted
https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.css (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.css:6: background: #DDD; On 2015/10/07 17:09:32, stevenjb wrote: > Prefer RGB (See > https://www.chromium.org/developers/web-development-style-guide#Color) Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:22: <script src="chrome://resources/js/i18n_template.js"></script> On 2015/10/07 17:09:32, stevenjb wrote: > Any reason this can't be above with the other script includes? No relevant one. Moved. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.html (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:11: <div id="content" class="card-content"></div> On 2015/10/07 17:09:32, stevenjb wrote: > Is class needed here? It is. Otherwise, the paper-card won't recognize the divs as as its contents. This causes (at least) the style to be broken (in terms of margin, padding, ...) https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:26: factoryImpl: function(riskTag) { On 2015/10/07 17:09:32, stevenjb wrote: > @param for riskTag > Also, declare (and type) riskTag in the object. Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.html (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:13: aria-label="[[introduction.title]]"> On 2015/10/07 17:09:32, stevenjb wrote: > Fix indent. (4 spaces from tag) Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:21: this.tagGroups = []; On 2015/10/07 17:09:33, stevenjb wrote: > declare tagGroups as a typed property or variable in the object. Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:27: this.introduction = { On 2015/10/07 17:09:33, stevenjb wrote: > Declare and type introduction Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:42: chrome.send('initialized'); On 2015/10/07 17:09:32, stevenjb wrote: > Prefer a more specifically named identifier, e.g. policyUiInitialized. We are using the same signal for capturing if chrome://policy or chrome://md-policy is initialized. Chrome reacts the same way in both cases and changing the signal name would introduce more complexity than necessary. Any identifier named specifically for this element would conflict with the old code, so I would like to keep the old identifier. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:56: ++pos; On 2015/10/07 17:09:32, stevenjb wrote: > Use {} when the condition or statement is more than 1 line. Done. Thanks, didn't know this rule. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:67: this.$.groups.removeChild(this.$.groups.firstChild); On 2015/10/07 17:09:32, stevenjb wrote: > 2 spaces here, not 4. Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:72: * @private On 2015/10/07 17:09:32, stevenjb wrote: > @private last Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:85: * Receives an list of known tags and creates an empty policy list for each. On 2015/10/07 17:09:32, stevenjb wrote: > a list Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:86: * @param {Array} tags List of tags. On 2015/10/07 17:09:32, stevenjb wrote: > {!Array<string>} (Always use ! or ? before Object or object typedefs and Array) > Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:95: * @param {{chromePolicyNames: Object<string, Array<string>>}} namesWithTags On 2015/10/07 17:09:32, stevenjb wrote: > !Object<!Array<string>> (keys are always strings so we omit the type) Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:100: this.addNameToTagGroup_.bind(this, name)); On 2015/10/07 17:09:33, stevenjb wrote: > {} Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:105: * @param {Object} values Dictionary containing the current policy values. On 2015/10/07 17:09:33, stevenjb wrote: > {!Object} or {?Object} Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:113: * @param {Object} status Dictionary containing meta data about set policies. On 2015/10/07 17:09:33, stevenjb wrote: > ! or ? Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:122: cr.define('policy', function() { On 2015/10/07 17:09:33, stevenjb wrote: > Can we put this above the Polymer object so that it is visibly declared before > it is used? Also, type the return value. Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:111: void AddLocalizedPoilcyStrings(content::WebUIDataSource* source, On 2015/10/07 17:09:33, stevenjb wrote: > Poilcy -> Policy Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:112: const PolicyStringMap* map, On 2015/10/07 17:09:33, stevenjb wrote: > nit: maps or map_list It's a single map. And this map is basically just an array. I will go with strings because its serves both purposes and uses the plural. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:494: const std::string& name) const override; On 2015/10/07 17:09:33, stevenjb wrote: > Mutable arguments should be declared last, e.g. AddPolicyName(name, > name_dictionary) Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:927: scoped_ptr<base::ListValue> list = make_scoped_ptr(new base::ListValue()); On 2015/10/07 17:09:33, stevenjb wrote: > scoped_ptr<base::ListValue> list(new base::ListValue); Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:931: list->AppendString(kPolicyRiskTags[tags[i]].key); On 2015/10/07 17:09:33, stevenjb wrote: > {} Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:107: result.push(root); On 2015/10/07 17:09:33, stevenjb wrote: > {} Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:109: root.id == selector.slice(1)) On 2015/10/07 17:09:33, stevenjb wrote: > one line Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:116: selector, root.shadowRoot.children[i])); On 2015/10/07 17:09:33, stevenjb wrote: > {} Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:162: return ui.tagGroups[i].element; On 2015/10/07 17:09:33, stevenjb wrote: > P{ Done. https://codereview.chromium.org/1371073003/diff/200001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:170: * @param {text} text On 2015/10/07 17:09:33, stevenjb wrote: > This is confusing... where is |text| typedef'd? If it is a real type, maybe name > the variable something else... that's kind of like having a variable in C++ > named 'string' :) Sorry, I meant string.
Please make sure that someone more familiar with the policy code takes a look, but from a webui perspective this lgtm. Also, please do separate out the policy_resources changes into a separate CL. I would like dbeam@ to take a quick look at this since it is new webui and he is much more familiar with webui in general than I am, but I know he will balk at a combined CL like this :) (Also, I am not an owner of c/b/resources but Dan is). https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.html (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:11: <div id="content" class="card-content"></div> On 2015/10/08 09:35:45, fhorschig wrote: > On 2015/10/07 17:09:32, stevenjb wrote: > > Is class needed here? > > It is. Otherwise, the paper-card won't recognize the divs as as its contents. > This causes (at least) the style to be broken (in terms of margin, padding, ...) Oh, I see, that's a Polymer class. Got it. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:42: chrome.send('initialized'); On 2015/10/08 09:35:45, fhorschig wrote: > On 2015/10/07 17:09:32, stevenjb wrote: > > Prefer a more specifically named identifier, e.g. policyUiInitialized. > > We are using the same signal for capturing if chrome://policy or > chrome://md-policy is initialized. Chrome reacts the same way in both cases and > changing the signal name would introduce more complexity than necessary. Any > identifier named specifically for this element would conflict with the old code, > so I would like to keep the old identifier. I see, OK. https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/1371073003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/policy_ui.cc:112: const PolicyStringMap* map, On 2015/10/08 09:35:46, fhorschig wrote: > On 2015/10/07 17:09:33, stevenjb wrote: > > nit: maps or map_list > > It's a single map. And this map is basically just an array. I will go with > strings because its serves both purposes and uses the plural. I see, I was confused by the struct name which makes me think of <map>, not an array of "mappings". I'd suggest renaming the struct, but "strings" is indeed less confusing and acceptable. https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:30: /** @type {string} */ What I meant to suggest here was to declare this as aproperty: Polymer({ is: 'policy-group', properties: { /** * ID used as title for the group card. */ riskTag: { type: String }, } https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:29: /** @type {!Array<{name: string, element: !PolicyGroup}>} */ Make this a property https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:36: /** @type {!{title: string, text: string}} */ This too
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
Patchset #6 (id:340001) has been deleted
Patchset #6 (id:360001) has been deleted
fhorschig@chromium.org changed reviewers: + dbeam@chromium.org
Hi Dan, would you mind to have a look to this CL? Cheers, Friedrich https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:30: /** @type {string} */ On 2015/10/08 17:12:39, stevenjb wrote: > What I meant to suggest here was to declare this as aproperty: > > Polymer({ > is: 'policy-group', > > properties: { > /** > * ID used as title for the group card. > */ > riskTag: { > type: String > }, > } Done. https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:29: /** @type {!Array<{name: string, element: !PolicyGroup}>} */ On 2015/10/08 17:12:39, stevenjb wrote: > Make this a property Removed. Figured out it's redundant to this.$.groups.children. https://codereview.chromium.org/1371073003/diff/240001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:36: /** @type {!{title: string, text: string}} */ On 2015/10/08 17:12:39, stevenjb wrote: > This too Done.
https://codereview.chromium.org/1371073003/diff/380001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/380001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:28: /** Object containing title and text of the introduction card. */ You can (and should) type Object properties: /** * Object containing title and text of the introduction card. * @type { !{ title: string, text: string }|undefined } */
https://codereview.chromium.org/1371073003/diff/380001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/380001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:28: /** Object containing title and text of the introduction card. */ On 2015/10/09 16:10:14, stevenjb wrote: > You can (and should) type Object properties: > > /** > * Object containing title and text of the introduction card. > * @type { !{ title: string, text: string }|undefined } > */ Nice to know, done for both classes.
https://codereview.chromium.org/1371073003/diff/400001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/400001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:31: * @type {string} For "simple" types like String, Closure can infer the type so this is not needed. It's just Object and Array that need additional @type info when available.
https://codereview.chromium.org/1371073003/diff/400001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/400001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:31: * @type {string} On 2015/10/09 16:39:27, stevenjb wrote: > For "simple" types like String, Closure can infer the type so this is not > needed. It's just Object and Array that need additional @type info when > available. Okay, removed.
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:12: <script src="chrome://md-policy/strings.js"></script> if we're not using this on iOS, can you use imports for these instead? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:22: why this \n? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.html (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:1: why \n? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:3: no \n https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:11: aria-label="[[translate(riskTag)]]"> can you compute this property and share it rather than computing it twice? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:20: * and Security'. New Policies are added with |addPolicy|. indent off, should be * Example: * * <policy-group></policy-group> * * By inserting https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:43: this._setRiskTag(riskTag); is this calling a private polymer method? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:51: translate: function(key) { is this used outside of this function? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:51: translate: function(key) { can you use I18nBehavior for this (and maybe move it to shared location)? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.html (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:5: <link rel="import" href="chrome://md-policy/policy_group.html"> why \n's between imports? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:16: </div> <div id="groups"></div> https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:10: return { Page: null }; {Page: null} https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:19: * <policy-ui></policy-ui> 1 less space https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:30: * @type {!{ title: string, text: string }|undefined} {title: string, text: string} https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ don't call _ methods outside of your class https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:69: for (var pos = 0; pos < this.$.groups.children.length; ++pos) curlies around for () { } https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:78: clearGroups_: function() { this.$.groups.innerHTML = '';? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:96: setPolicyGroups: function(tags) { can we use a dom-repeat or iron-list? https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.cc (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. remove (c) https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:13: 1 less \n https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:17: InProcessBrowserTest::SetUpCommandLine(command_line); WebUIBrowserTest::SetUpCommandLine(command_line); https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.h (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:7: #include "base/macros.h" https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:11: one less \n https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:24: SAMLE_A: { is this supposed to be SAMPLE_{A,B}? https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:76: * @param {Array.Object=} policies Defaults to all policies. opt_policies https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:81: var data = { chromePolicyNames : {} }; {chromePolicyNames: {}} (no spaces) https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:84: }; }; -> } https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:94: * @param {Element=} root Defaults to document opt_root https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:97: queryObjectDeep_: function(selector, root) { why not just actually use /deep/? https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:101: } no curlies https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:108: root.classList.contains(selector.slice(1))) { root.matches && root.matches(selector)? https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:112: result.push(root); would also be handled by matches https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:114: result = result.concat(this.queryObjectDeep_(selector, root.children[i])); nit: \n https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:135: while (selector = selectors.shift()) { nit: selector.split(' ').forEach(function() { ... });? https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:149: * @param {Element=} root Defaults to document opt_root https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:164: if (ui.$.groups.children[i].riskTag == name) if you use reflectToAttribute, you can just querySelector('[risk-tag=' + name + ']') maybe? https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:186: GEN('#include "chrome/test/data/webui/policy_ui_material.h"'); move to top https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:192: var exists = this.exists.bind(PolicyUiMaterialWebUITest.prototype); can't you just bind(this)?
tnagel@chromium.org changed reviewers: - tnagel@chromium.org
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:51: translate: function(key) { On 2015/10/10 01:04:18, Dan Beam wrote: > can you use I18nBehavior for this (and maybe move it to shared location)? Dan: I just noticed that we put i18n_behavior in c/b/resources/settings/i18n_behavior/. Should we move it to ui/webui/cr_resources ? https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ On 2015/10/10 01:04:18, Dan Beam wrote: > don't call _ methods outside of your class You can just use: this.introduction = { title: ... }
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:43: this._setRiskTag(riskTag); On 2015/10/10 01:04:18, Dan Beam wrote: > is this calling a private polymer method? I don't actually see this documented anywhere, and it doesn't appear to be a method on the Polymer objects I tested. Any reason why this shouldn't be just 'this.riskTag = riskTag' ?
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:51: translate: function(key) { On 2015/10/12 16:55:37, stevenjb wrote: > On 2015/10/10 01:04:18, Dan Beam wrote: > > can you use I18nBehavior for this (and maybe move it to shared location)? > > Dan: I just noticed that we put i18n_behavior in > c/b/resources/settings/i18n_behavior/. Should we move it to > ui/webui/cr_resources ? yes, now that there's a need for it from other places
A way to use normal assignments (and have notifications working) would be to make my readonly properties non-readonly. I wouldn't like this but if you say this is compliant to some styleguide or the general style we use here, I will change it. Because someone requested documentation for the usage of this._set* methods: https://www.polymer-project.org/1.0/docs/devguide/properties.html#read-only https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:12: <script src="chrome://md-policy/strings.js"></script> On 2015/10/10 01:04:18, Dan Beam wrote: > if we're not using this on iOS, can you use imports for these instead? Done. Also distributed dependencies to the files that actually depend on them. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:22: On 2015/10/10 01:04:18, Dan Beam wrote: > why this \n? Deleted. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.html (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:1: On 2015/10/10 01:04:18, Dan Beam wrote: > why \n? Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:3: On 2015/10/10 01:04:18, Dan Beam wrote: > no \n Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:11: aria-label="[[translate(riskTag)]]"> On 2015/10/10 01:04:18, Dan Beam wrote: > can you compute this property and share it rather than computing it twice? Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:20: * and Security'. New Policies are added with |addPolicy|. On 2015/10/10 01:04:18, Dan Beam wrote: > indent off, should be > > * Example: > * > * <policy-group></policy-group> > * > * By inserting Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:20: * and Security'. New Policies are added with |addPolicy|. On 2015/10/10 01:04:18, Dan Beam wrote: > indent off, should be > > * Example: > * > * <policy-group></policy-group> > * > * By inserting Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:43: this._setRiskTag(riskTag); On 2015/10/10 01:04:18, Dan Beam wrote: > is this calling a private polymer method? Isn't this a private method from this very class? I thought this way to go after reading: https://www.polymer-project.org/1.0/docs/devguide/properties.html#read-only Another reason is notification of change for readonly methods. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.js:51: translate: function(key) { On 2015/10/10 01:04:18, Dan Beam wrote: > is this used outside of this function? It is not used outside this function. As proposed, I moved the behavior to resources and included it from there. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.html (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:5: <link rel="import" href="chrome://md-policy/policy_group.html"> On 2015/10/10 01:04:18, Dan Beam wrote: > why \n's between imports? Reduced. The \n should separate different origins. I also made polymer.html standing out to show that it has to be included first. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:16: </div> On 2015/10/10 01:04:18, Dan Beam wrote: > <div id="groups"></div> Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:10: return { Page: null }; On 2015/10/10 01:04:18, Dan Beam wrote: > {Page: null} Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:19: * <policy-ui></policy-ui> On 2015/10/10 01:04:18, Dan Beam wrote: > 1 less space Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:30: * @type {!{ title: string, text: string }|undefined} On 2015/10/10 01:04:18, Dan Beam wrote: > {title: string, text: string} Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ On 2015/10/10 01:04:18, Dan Beam wrote: > don't call _ methods outside of your class Isn't this "inside" my class? Isn't this setter explicitly defined for this class? My reasoning so far (apart from the earlier shared link) was: if you can call it on a unmodified "this", it okay to do so. Also the compiler doesn't warn me about a private function call. Could you please show me the right way to do this for this as an example? :) https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ On 2015/10/12 16:55:37, stevenjb wrote: > On 2015/10/10 01:04:18, Dan Beam wrote: > > don't call _ methods outside of your class > > You can just use: > > this.introduction = { title: ... } Apparently, setting the object like this doesn't trigger an update to the DOM. I had to trigger a notification myself. Maybe the approach is different when the attribute is complex (Array/Object instead of string/int) https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:69: for (var pos = 0; pos < this.$.groups.children.length; ++pos) On 2015/10/10 01:04:18, Dan Beam wrote: > curlies around for () { } Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:78: clearGroups_: function() { On 2015/10/10 01:04:18, Dan Beam wrote: > this.$.groups.innerHTML = '';? Nice! https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:96: setPolicyGroups: function(tags) { On 2015/10/10 01:04:18, Dan Beam wrote: > can we use a dom-repeat or iron-list? dom-repeat is awesome! Thanks for showing. (iron-list doesn't load data synchronously and only renders elements that are visible in the viewport --> no reliable access to groups when adding policy names.) https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.cc (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/10/10 01:04:18, Dan Beam wrote: > remove (c) Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:13: On 2015/10/10 01:04:18, Dan Beam wrote: > 1 less \n Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:17: InProcessBrowserTest::SetUpCommandLine(command_line); On 2015/10/10 01:04:18, Dan Beam wrote: > WebUIBrowserTest::SetUpCommandLine(command_line); Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.h (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/10/10 01:04:18, Dan Beam wrote: > no (c) Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:7: On 2015/10/10 01:04:19, Dan Beam wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:11: On 2015/10/10 01:04:19, Dan Beam wrote: > one less \n Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:24: SAMLE_A: { On 2015/10/10 01:04:19, Dan Beam wrote: > is this supposed to be SAMPLE_{A,B}? Thanks! https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:76: * @param {Array.Object=} policies Defaults to all policies. On 2015/10/10 01:04:19, Dan Beam wrote: > opt_policies Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:81: var data = { chromePolicyNames : {} }; On 2015/10/10 01:04:19, Dan Beam wrote: > {chromePolicyNames: {}} (no spaces) Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:84: }; On 2015/10/10 01:04:19, Dan Beam wrote: > }; -> } Removed. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:94: * @param {Element=} root Defaults to document On 2015/10/10 01:04:19, Dan Beam wrote: > opt_root Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:97: queryObjectDeep_: function(selector, root) { On 2015/10/10 01:04:19, Dan Beam wrote: > why not just actually use /deep/? It's deprecated [1] and searched DOM-MODULES (which was a problem only initially). Others are already mention that their tests will break. If you know a good method, please let me know. (My intention is, not to enforce an exact HTML structure, just to check for existence and functionality) [1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/68qSZM5QMRQ https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:101: } On 2015/10/10 01:04:19, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:108: root.classList.contains(selector.slice(1))) { On 2015/10/10 01:04:19, Dan Beam wrote: > root.matches && root.matches(selector)? Nice! https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:112: result.push(root); On 2015/10/10 01:04:19, Dan Beam wrote: > would also be handled by matches Exactly what I was looking for. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:114: result = result.concat(this.queryObjectDeep_(selector, root.children[i])); On 2015/10/10 01:04:19, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:135: while (selector = selectors.shift()) { On 2015/10/10 01:04:19, Dan Beam wrote: > nit: selector.split(' ').forEach(function() { ... });? Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:149: * @param {Element=} root Defaults to document On 2015/10/10 01:04:19, Dan Beam wrote: > opt_root Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:164: if (ui.$.groups.children[i].riskTag == name) On 2015/10/10 01:04:19, Dan Beam wrote: > if you use reflectToAttribute, you can just > > querySelector('[risk-tag=' + name + ']') > > maybe? Good idea. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:186: GEN('#include "chrome/test/data/webui/policy_ui_material.h"'); On 2015/10/10 01:04:19, Dan Beam wrote: > move to top Done. https://codereview.chromium.org/1371073003/diff/420001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:192: var exists = this.exists.bind(PolicyUiMaterialWebUITest.prototype); On 2015/10/10 01:04:19, Dan Beam wrote: > can't you just bind(this)? Done.
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ On 2015/10/13 16:29:11, fhorschig wrote: > On 2015/10/12 16:55:37, stevenjb wrote: > > On 2015/10/10 01:04:18, Dan Beam wrote: > > > don't call _ methods outside of your class > > > > You can just use: > > > > this.introduction = { title: ... } > > Apparently, setting the object like this doesn't trigger an update to the DOM. I > had to trigger a notification myself. Maybe the approach is different when the > attribute is complex (Array/Object instead of string/int) this.set('introduction', {...});
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ On 2015/10/13 16:56:09, Dan Beam wrote: > On 2015/10/13 16:29:11, fhorschig wrote: > > On 2015/10/12 16:55:37, stevenjb wrote: > > > On 2015/10/10 01:04:18, Dan Beam wrote: > > > > don't call _ methods outside of your class > > > > > > You can just use: > > > > > > this.introduction = { title: ... } > > > > Apparently, setting the object like this doesn't trigger an update to the DOM. > I > > had to trigger a notification myself. Maybe the approach is different when the > > attribute is complex (Array/Object instead of string/int) > > this.set('introduction', {...}); Polymer is a little funny regarding object notifications: this.introduction = { ... }; // This *should* fire a notification this.introduction.title = '...'; // This does *not* fire a notification this.set('introduction', {...}); // This will fire a notification this.set('introduction.title', '...'); // This will also fire a notification
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ On 2015/10/13 17:30:15, stevenjb wrote: > On 2015/10/13 16:56:09, Dan Beam wrote: > > On 2015/10/13 16:29:11, fhorschig wrote: > > > On 2015/10/12 16:55:37, stevenjb wrote: > > > > On 2015/10/10 01:04:18, Dan Beam wrote: > > > > > don't call _ methods outside of your class > > > > > > > > You can just use: > > > > > > > > this.introduction = { title: ... } > > > > > > Apparently, setting the object like this doesn't trigger an update to the > DOM. > > I > > > had to trigger a notification myself. Maybe the approach is different when > the > > > attribute is complex (Array/Object instead of string/int) > > > > this.set('introduction', {...}); > > Polymer is a little funny regarding object notifications: > this.introduction = { ... }; // This *should* fire a notification > this.introduction.title = '...'; // This does *not* fire a notification > this.set('introduction', {...}); // This will fire a notification > this.set('introduction.title', '...'); // This will also fire a notification I messed the 'readOnly' comment earlier. If you want to make |introduction| readOnly, set it in |properties|: /** * Object containing title and text of the introduction card. * @type {!{ title: string, text: string }|undefined} * @const */ introduction: { type: Object, value: { title: '...', text: '...' }, readOnly: true, } (BTW, we don't usually specify reflectToAttribute, notify, or readOnly when they have the default 'false' value). Note: const / readOnly objects can be set this way, but if you want to initialize other Objects inside |properties|, you need to use the pattern: value: function() { return {...} }, Otherwise every instance of the Polymer object would reference the same object which (for non const objects) would be bad.
https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/420001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:43: this._setIntroduction({ On 2015/10/13 18:36:50, stevenjb wrote: > On 2015/10/13 17:30:15, stevenjb wrote: > > On 2015/10/13 16:56:09, Dan Beam wrote: > > > On 2015/10/13 16:29:11, fhorschig wrote: > > > > On 2015/10/12 16:55:37, stevenjb wrote: > > > > > On 2015/10/10 01:04:18, Dan Beam wrote: > > > > > > don't call _ methods outside of your class > > > > > > > > > > You can just use: > > > > > > > > > > this.introduction = { title: ... } > > > > > > > > Apparently, setting the object like this doesn't trigger an update to the > > DOM. > > > I > > > > had to trigger a notification myself. Maybe the approach is different when > > the > > > > attribute is complex (Array/Object instead of string/int) > > > > > > this.set('introduction', {...}); > > > > Polymer is a little funny regarding object notifications: > > this.introduction = { ... }; // This *should* fire a notification > > this.introduction.title = '...'; // This does *not* fire a notification > > this.set('introduction', {...}); // This will fire a notification > > this.set('introduction.title', '...'); // This will also fire a notification > > I messed the 'readOnly' comment earlier. If you want to make |introduction| > readOnly, set it in |properties|: > > /** > * Object containing title and text of the introduction card. > * @type {!{ title: string, text: string }|undefined} > * @const > */ > introduction: { > type: Object, > value: { > title: '...', > text: '...' > }, > readOnly: true, > } > > (BTW, we don't usually specify reflectToAttribute, notify, or readOnly when they > have the default 'false' value). > > Note: const / readOnly objects can be set this way, but if you want to > initialize other Objects inside |properties|, you need to use the pattern: > > value: function() { return {...} }, > > Otherwise every instance of the Polymer object would reference the same object > which (for non const objects) would be bad. This works good for the introduction. The other properties are obviously not readonly (the implications of this keyword are now clear) and are marked private non-readonly now.
https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:11: <policy-ui></policy-ui> what's the point of this element/file? you should inline contents of policy_ui.html here instead the only reason you'd need to wrap in a custom-element for this is if you do --css-variable-expansion
https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:11: <policy-ui></policy-ui> On 2015/10/14 21:05:06, Dan Beam wrote: > what's the point of this element/file? you should inline contents of > policy_ui.html here instead > > the only reason you'd need to wrap in a custom-element for this is if you do > --css-variable-expansion Do you mean like in Patchset #3 [1]? If I wouldn't use a custom element, I would need something else to handle the data from chrome and fill a repeat-dom with it. Is that possible (or even more elegant) without a custom-element? [1] https://codereview.chromium.org/1371073003/patch/180001/190009
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:99: if (opt_root.matches && opt_root.matches(selector)) This is going to be massively slower than just calling querySelectorAll() at each shadowRoot, then iterating every descendant looking for more ShadowRoots. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:106: for (var i = 0; i < opt_root.shadowRoot.children.length; ++i) { don't use .children (ever), use .firstElementChild and .nextElementSibling https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:124: selector.split(' ').forEach((function(sel) { This doesn't make sense, selectors aren't separated by spaces, "#a .b" means .b inside #a for all web apis, they should be separated by commas, also this breaks if you write [attr='value with space'] I guess you're trying to match across boundaries? This is going to match all kinds of crazy weird things, and doesn't work for .a + .b etc. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:142: return this.listOf(selector, opt_root).length > 0; this is super slow, you're querying for *all* to check for *one*, you need a different early out code path at the first match https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:151: var ui = this.listOf('policy-ui')[0]; again you query for *all* to get the first one, don't do that.
https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.html (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:19: <policy-group risk-tag="[[tag.name]]"></policy-group> why can't this just be [[tag]] and avoid the tags.map(tag => {name: tag}) ? https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:51: value: function() { return []; } does this need an initial value? https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:71: this.set('tags_', tags.map(function(tag) { can we get away without this .map()? https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/strings.html (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/strings.html:2: <link rel="import" href="chrome://resources/html/i18n_template.html"> doesn't this technically need to go after strings.js? https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... strings.js actually sets loadTimeData.data_ = {...} so if process runs before that, translations won't work https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.h (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:7: #include "base/macros.h" https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:1: /** @const {string} Path to source root. */ needs a copyright https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:8: GEN('#include "base/macros.h"'); why is this needed here? https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:17: /** @type {Array.string} @const */ !Array<string> https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:53: * Generate a real C++ class; don't typedef. the CppFixture is always a C++ class, no? I think all of this should probably just be /** @override */ https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:67: * @type {Array.string} !Array<string> https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:70: IGNORED_TAGS: ['DOM-MODULE', 'SCRIPT', 'STYLE', 'TEMPLATE', 'HEAD'], can you make this private and static (i.e. PolicyUiMaterialWebUITest.IGNORED_TAGS_) or just local static (var IGNORED_TAGS)?
Patchset #11 (id:480001) has been deleted
https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.html (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.html:19: <policy-group risk-tag="[[tag.name]]"></policy-group> On 2015/10/14 21:44:00, Dan Beam wrote: > why can't this just be [[tag]] and avoid the tags.map(tag => {name: tag}) ? Done. (I tried this before and it only worked with objects. Maybe due to wrong initilization) https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_ui.js (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:51: value: function() { return []; } On 2015/10/14 21:44:00, Dan Beam wrote: > does this need an initial value? No. Deleted. https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_ui.js:71: this.set('tags_', tags.map(function(tag) { On 2015/10/14 21:44:00, Dan Beam wrote: > can we get away without this .map()? Yes, we can. Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/md_policy/strings.html (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/browser/resourc... chrome/browser/resources/md_policy/strings.html:2: <link rel="import" href="chrome://resources/html/i18n_template.html"> On 2015/10/14 21:44:00, Dan Beam wrote: > doesn't this technically need to go after strings.js? > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > strings.js actually sets loadTimeData.data_ = {...} > > so if process runs before that, translations won't work Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.h (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:7: On 2015/10/14 21:44:00, Dan Beam wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:1: /** @const {string} Path to source root. */ On 2015/10/14 21:44:00, Dan Beam wrote: > needs a copyright Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:8: GEN('#include "base/macros.h"'); On 2015/10/14 21:44:00, Dan Beam wrote: > why is this needed here? Sorry, just added this on the wrong place. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:17: /** @type {Array.string} @const */ On 2015/10/14 21:44:00, Dan Beam wrote: > !Array<string> Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:53: * Generate a real C++ class; don't typedef. On 2015/10/14 21:44:00, Dan Beam wrote: > the CppFixture is always a C++ class, no? > > I think all of this should probably just be > > /** @override */ Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:67: * @type {Array.string} On 2015/10/14 21:44:00, Dan Beam wrote: > !Array<string> Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:70: IGNORED_TAGS: ['DOM-MODULE', 'SCRIPT', 'STYLE', 'TEMPLATE', 'HEAD'], On 2015/10/14 21:44:00, Dan Beam wrote: > can you make this private and static (i.e. > PolicyUiMaterialWebUITest.IGNORED_TAGS_) or just local static (var > IGNORED_TAGS)? Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:99: if (opt_root.matches && opt_root.matches(selector)) On 2015/10/14 21:35:34, esprehn wrote: > This is going to be massively slower than just calling querySelectorAll() at > each shadowRoot, then iterating every descendant looking for more ShadowRoots. Changed the way I test, so I don't need to deep search shadow roots anymore. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:106: for (var i = 0; i < opt_root.shadowRoot.children.length; ++i) { On 2015/10/14 21:35:34, esprehn wrote: > don't use .children (ever), use .firstElementChild and .nextElementSibling Done. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:124: selector.split(' ').forEach((function(sel) { On 2015/10/14 21:35:34, esprehn wrote: > This doesn't make sense, selectors aren't separated by spaces, "#a .b" means .b > inside #a for all web apis, they should be separated by commas, also this breaks > if you write [attr='value with space'] I guess you're trying to match across > boundaries? This is going to match all kinds of crazy weird things, and doesn't > work for .a + .b etc. This is absolutely true. Deleted. Currently, I only have a handful of use cases so solving these problems would be an overkill. The small and countable set of shadow roots will be handled manually, by querying selectors from the known shadow root. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:142: return this.listOf(selector, opt_root).length > 0; On 2015/10/14 21:35:34, esprehn wrote: > this is super slow, you're querying for *all* to check for *one*, you need a > different early out code path at the first match querySelector handles this now. Should be fine. https://codereview.chromium.org/1371073003/diff/460001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:151: var ui = this.listOf('policy-ui')[0]; On 2015/10/14 21:35:34, esprehn wrote: > again you query for *all* to get the first one, don't do that. Now it's querySelector, again. Thanks for pointing out those obvious flaws.
Patchset #11 (id:500001) has been deleted
Friendly ping. What is this CL blocked on?
https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:11: <policy-ui></policy-ui> can you just inline <policy-ui>'s contents here? you're not using CSS variables in the stylesheet, so there's really no reason it has to be it's own element, AFAICT
Friedrich, I have the impression that this CL could use a rebase. With today's ToT (and CL 1395073002 on top of it), the CL doesn't apply cleanly. Could you please take a look?
https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.html (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:17: <template is="dom-repeat" items="[[policies_]]" as="policy"> should this be an iron-list? is the size of policies_ bounded in practice? https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.cc (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:13: command_line->AppendSwitch(switches::kEnableMaterialDesignPolicyPage); why not just use commandLineSwitches in PolymerTest? https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:40: function PolicyUiMaterialWebUITest() { arguable nit: file name should match this class name https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:62: extraLibraries: PolymerTest.getLibraries(ROOT_PATH), does this actually override anything or is the same as the default? https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:85: return opt_root.querySelector(selector) !== undefined; this is always true (querySelector never returns undefined) https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:134: expectTrue(exists('policy-ui')); if the test is "has one PolictyUi", why not expectEquals(count('policy-ui'), 1); why do you need exists if you have count? https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:155: expectTrue(exists('policy-group', this.policyUi)); nit: this.policyUi.$$('policy-group')
Patchset #12 (id:540001) has been deleted
Patchset #12 (id:560001) has been deleted
Sorry it took me forever to go over the comments [1]. I will need another Patchset for Async tests. [1] I had trouble applying the patch on a new machine due to merge conflicts with grit_whitelist (that happens to be uploaded as diff only) https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... File chrome/browser/resources/md_policy/md_policy.html (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... chrome/browser/resources/md_policy/md_policy.html:11: <policy-ui></policy-ui> On 2015/11/03 17:52:55, Dan Beam wrote: > can you just inline <policy-ui>'s contents here? you're not using CSS variables > in the stylesheet, so there's really no reason it has to be it's own element, > AFAICT What do you mean by "inline <policy-ui>'s contents"? The component itself is receiver of messages and generates groups from them, so I would use scripts that are not tied to an Polymer object. Is that the right way? After all, I will definitely need an object that will receive the messages and generates groups. If you mean with "inline <policy-ui>'s contents" just moving the definition of the DOM to this file, I can do this. Please correct me if I am missing the point, but I guess this would be a similar situation to Patchset 3 where this was critizised: https://codereview.chromium.org/1371073003/patch/180001/190009 https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... File chrome/browser/resources/md_policy/policy_group.html (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/browser/resourc... chrome/browser/resources/md_policy/policy_group.html:17: <template is="dom-repeat" items="[[policies_]]" as="policy"> On 2015/11/03 18:22:31, Dan Beam wrote: > should this be an iron-list? is the size of policies_ bounded in practice? The iron-list usually has the advantage of rendering items when they would be visible on screen. This behavior causes trouble for me because I need (reliable and constant) access to all DOM elements that are created. The size of policies_ is not technically but conceptually bounded. A group will contain a very small subset of the chromium policies [1]. I don't think there would be a recognizable impact when switching to iron-list. [1] For further reference or if you are interested in the numbers: There are ~300 policies which only contain ~50-60 policies that may influence a user's privacy/security. Even if a group would contain nearly all of them, every single one would have to be active to be displayed. Last time I checked the list of policies, there was not a single group that contains 20 or more policies. https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.cc (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.cc:13: command_line->AppendSwitch(switches::kEnableMaterialDesignPolicyPage); On 2015/11/03 18:22:31, Dan Beam wrote: > why not just use commandLineSwitches in PolymerTest? I tried doing that (like it was done here: [1]) and it didn't have an effect. I will look up the reason as it seems nicer but the current version works. https://code.google.com/p/chromium/codesearch#chromium/src[1] /chrome/test/data/webui/settings/cr_settings_browsertest.js&q=commandLineSwitches&sq=package:chromium&type=cs&l=27 https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.js (right): https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:40: function PolicyUiMaterialWebUITest() { On 2015/11/03 18:22:31, Dan Beam wrote: > arguable nit: file name should match this class name Sounds good. (Will come within the next Patchset) https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:62: extraLibraries: PolymerTest.getLibraries(ROOT_PATH), On 2015/11/03 18:22:32, Dan Beam wrote: > does this actually override anything or is the same as the default? The test cases cannot be instantiated without setting the path in this line, so it seems to be different from the default. https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:85: return opt_root.querySelector(selector) !== undefined; On 2015/11/03 18:22:32, Dan Beam wrote: > this is always true (querySelector never returns undefined) Function deleted. https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:134: expectTrue(exists('policy-ui')); On 2015/11/03 18:22:31, Dan Beam wrote: > if the test is "has one PolictyUi", why not expectEquals(count('policy-ui'), 1); > > why do you need exists if you have count? Good point, exists doesn't exist anymore. https://codereview.chromium.org/1371073003/diff/520001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.js:155: expectTrue(exists('policy-group', this.policyUi)); On 2015/11/03 18:22:32, Dan Beam wrote: > nit: this.policyUi.$$('policy-group') Done.
lgtm https://codereview.chromium.org/1371073003/diff/580001/chrome/test/data/webui... File chrome/test/data/webui/policy_ui_material.h (right): https://codereview.chromium.org/1371073003/diff/580001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1371073003/diff/580001/chrome/test/data/webui... chrome/test/data/webui/policy_ui_material.h:15: nit: // WebUIBrowserTest:
what's the status of this CL? can we close it?
On 2016/08/12 04:38:47, Dan Beam wrote: > what's the status of this CL? can we close it? Friedrich will return to Google real soon, maybe we wait whether he'd like to pick it up?
On 2016/08/12 13:43:39, Thiemo Nagel wrote: > On 2016/08/12 04:38:47, Dan Beam wrote: > > what's the status of this CL? can we close it? > > Friedrich will return to Google real soon, maybe we wait whether he'd like to > pick it up? ok, cool with me
On 2016/08/12 at 16:41:50, dbeam wrote: > On 2016/08/12 13:43:39, Thiemo Nagel wrote: > > On 2016/08/12 04:38:47, Dan Beam wrote: > > > what's the status of this CL? can we close it? > > > > Friedrich will return to Google real soon, maybe we wait whether he'd like to > > pick it up? > > ok, cool with me Can we close this and have him re-open it when ready? Having a patch from 2015 hanging around in the review
On 2016/08/30 at 23:36:54, esprehn wrote: > On 2016/08/12 at 16:41:50, dbeam wrote: > > On 2016/08/12 13:43:39, Thiemo Nagel wrote: > > > On 2016/08/12 04:38:47, Dan Beam wrote: > > > > what's the status of this CL? can we close it? > > > > > > Friedrich will return to Google real soon, maybe we wait whether he'd like to > > > pick it up? > > > > ok, cool with me > > Can we close this and have him re-open it when ready? Having a patch from 2015 hanging around in the review err... review queue for a year and then waiting seems a bit silly. There's nothing wrong with reuploading or reopening. :)
Message was sent while issue was closed.
I'm going to close this, feel free to reopen if you want to land it. :) |
