Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17387002:
Sending known policy names for extensions to chrome://policy page (Closed)
|
Created:
7 years, 6 months ago by anitawoodruff Modified:
7 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@anita-policies Visibility:
Public. |
DescriptionSending known policy names for extensions to chrome://policy page
Currently only the Chrome policy table shows policies which exist
but have not been set. This change allows extension policy tables to
show all known policies defined in their policy schemas, when the
'show unset policies' checkbox is ticked. Hence the Status column has
been resurrected for all extension policy tables, since it now may show
either 'OK' or 'Not set.'
BUG=248533
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208469
Patch Set 1 : Sending known policy names for extensions #
Total comments: 3
Patch Set 2 : After rebasing against patch set 9 of issue 16689004 #
Total comments: 8
Patch Set 3 : Fixing indentation and out-of-date parameter documentation #Patch Set 4 : Use a single for-loop in policy_ui.cc SendPolicyNames #Patch Set 5 : Only resend policy names on extensions loaded/unloaded #Patch Set 6 : After rebasing against patch set 10 of issue 16689004 #
Total comments: 14
Patch Set 7 : Responding to Joao's comments on PS6 #
Total comments: 17
Patch Set 8 : Adding required includes, removing superfluous comments #Patch Set 9 : Renaming SchemaMap iterator to a unique name in local scope #Patch Set 10 : Removed unnecessary comments from SendPolicyValues; rebased against master #Patch Set 11 : ifdef guards to prevent crash on OS_ANDROID and OS_IOS #Patch Set 12 : After rebasing against master #
Total comments: 1
Patch Set 13 : Fixing accidentally-removed line in prev patch #
Messages
Total messages: 16 (0 generated)
Hi Joao, here's my 2nd CL, sorry it took a while to upload. Major changes are: - Send through extension policy names, - Send extension names with the policy names, not the policy values, - Always include the status column in policy tables. Anita https://codereview.chromium.org/17387002/diff/4001/chrome/browser/ui/webui/po... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/4001/chrome/browser/ui/webui/po... chrome/browser/ui/webui/policy_ui.cc:505: SendPolicyNames(); This seemed necessary now that extension names get sent through with policy names, because otherwise if a new extension got added and a policy set on it, the policy table didn't get added to the page. However, this does mean we are sending through all the policy names whenever anything changes, which may be overkill. Thoughts?
https://codereview.chromium.org/17387002/diff/4001/chrome/browser/ui/webui/po... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/4001/chrome/browser/ui/webui/po... chrome/browser/ui/webui/policy_ui.cc:505: SendPolicyNames(); On 2013/06/18 09:14:10, anitawoodruff wrote: > This seemed necessary now that extension names get sent through with policy > names, because otherwise if a new extension got added and a policy set on it, > the policy table didn't get added to the page. However, this does mean we are > sending through all the policy names whenever anything changes, which may be > overkill. Thoughts? Yes, this is overkill. The right way to do it is to make this class a content::NotificationObserver and register to receive NOTIFICATION_EXTENSION_LOADED, and SendPolicyNames() when that notification is received. This class can also observe NOTIFICATION_EXTENSION_UNLOADED to remove a table when an extension is removed. So the javascript side has to drop all tables and create them again whenever SendPolicyNames() is received. SendPolicyValues() should also be sent immediately after to send values for the new tables. https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... chrome/browser/resources/policy.js:332: names.extensionPolicyNames[ext].name, 'ID: ' + ext); nit: indent https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... chrome/browser/resources/policy.js:428: label_content); nit: indent https://codereview.chromium.org/17387002/diff/8001/chrome/browser/ui/webui/po... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/8001/chrome/browser/ui/webui/po... chrome/browser/ui/webui/policy_ui.cc:576: } Have a single for loop: - get the list of all extensions as in lines 526-529; - then get the schema_map as in lines 546-550; - then iterate over all the extensions, skipping non enterprise extensions, as in lines 532-543; - after setting the extension name, get its PolicySchemaMap and add the policy names for that extension: policy::PolicyDomainDescriptor::SchemaMap::const_iterator it = schema_map.find(extension->id()); if (it == schema_map.end()) { NOTREACHED(); continue; } const policy::PolicySchemaMap* policies = it->second->GetProperties(); // loop from lines 567-570
Fixed indentation of parameters & a couple of other things I noticed. https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... chrome/browser/resources/policy.js:332: names.extensionPolicyNames[ext].name, 'ID: ' + ext); On 2013/06/18 13:26:04, Joao da Silva wrote: > nit: indent Done. https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... chrome/browser/resources/policy.js:428: label_content); On 2013/06/18 13:26:04, Joao da Silva wrote: > nit: indent Done. https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... chrome/browser/resources/policy.js:439: * @param {string} id Used as key when storing new table in policyTables. oops, this parameter description should have been removed. Will fix. https://codereview.chromium.org/17387002/diff/8001/chrome/browser/resources/p... chrome/browser/resources/policy.js:475: * @param {boolean} includeStatus Whether to include a status column. this one too.
Hey Joao, thanks for the tip about the for-loop, your way makes way more sense. I've tried to implement it in the latest patch, but not sure if you realised this branch doesn't currently filter non-enterprise extensions. (See my reply to your comment). https://codereview.chromium.org/17387002/diff/8001/chrome/browser/ui/webui/po... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/8001/chrome/browser/ui/webui/po... chrome/browser/ui/webui/policy_ui.cc:576: } On 2013/06/18 13:26:04, Joao da Silva wrote: > Have a single for loop: > > - get the list of all extensions as in lines 526-529; > - then get the schema_map as in lines 546-550; > - then iterate over all the extensions, skipping non enterprise extensions, as > in lines 532-543; > - after setting the extension name, get its PolicySchemaMap and add the policy > names for that extension: > > policy::PolicyDomainDescriptor::SchemaMap::const_iterator it = > schema_map.find(extension->id()); > if (it == schema_map.end()) { > NOTREACHED(); > continue; > } > > const policy::PolicySchemaMap* policies = it->second->GetProperties(); > // loop from lines 567-570 I've combined it into a single for-loop, but that NOTREACHED() check fails, possibly because in this branch I don't filter out non-enterprise extensions (that's in the other review). (Wouldn't enterprise extensions without any policies also cause it to fail??) Anyway, instead of NOTREACHED() here I'm saving the entry to the map before continuing, so the name and 'No policies set' element can be shown for extensions without policies.
Hey Joao, I did as you said and made the class a notification observer so it only resends policy names upon extensions loading/unloading, and made the javascript recreate all tables whenever that happens. https://codereview.chromium.org/17387002/diff/4001/chrome/browser/ui/webui/po... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/4001/chrome/browser/ui/webui/po... chrome/browser/ui/webui/policy_ui.cc:505: SendPolicyNames(); On 2013/06/18 13:26:04, Joao da Silva wrote: > On 2013/06/18 09:14:10, anitawoodruff wrote: > > This seemed necessary now that extension names get sent through with policy > > names, because otherwise if a new extension got added and a policy set on it, > > the policy table didn't get added to the page. However, this does mean we are > > sending through all the policy names whenever anything changes, which may be > > overkill. Thoughts? > > Yes, this is overkill. The right way to do it is to make this class a > content::NotificationObserver and register to receive > NOTIFICATION_EXTENSION_LOADED, and SendPolicyNames() when that notification is > received. > > This class can also observe NOTIFICATION_EXTENSION_UNLOADED to remove a table > when an extension is removed. So the javascript side has to drop all tables and > create them again whenever SendPolicyNames() is received. SendPolicyValues() > should also be sent immediately after to send values for the new tables. Done.
Almost there https://codereview.chromium.org/17387002/diff/24001/chrome/browser/resources/... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/17387002/diff/24001/chrome/browser/resources/... chrome/browser/resources/policy.js:392: var chromeTable = this.appendNewTable('chrome', 'Chrome policies', ''); Creating the chromeTable here is not necessary anymore, since it's now created on setPolicyNames. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:269: class PolicyUIHandler : public content::NotificationObserver, #include "content/public/browser/notification_observer.h" https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:328: DISALLOW_COPY_AND_ASSIGN(PolicyUIHandler); This should be the last statement within the class { } https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:456: content::NotificationService::AllSources()); This is not needed, the registrar_ does it automatically on destruction. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:525: switch (type) { Since this method always takes the same action the switch statement can be removed; just DCHECK that the type is one of the expected ones: DCHECK(type == LOADED || type == UNLOADED); SendPolicyNames(); SendPolicyValues(); https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:561: extensions::ExtensionSystem::Get(Profile::FromWebUI(web_ui())); nit: indent (4 spaces) https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:587: schema_map.find(extension->id()); nit: indent (4 spaces)
Hi Joao/James, Joao said I should add James as another reviewer to this review, now that #16689004 (Adding extension policies to chrome://policy page) is good to go. (This review is branched off that one). Hope that's ok with you James :-) Anita https://codereview.chromium.org/17387002/diff/24001/chrome/browser/resources/... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/17387002/diff/24001/chrome/browser/resources/... chrome/browser/resources/policy.js:392: var chromeTable = this.appendNewTable('chrome', 'Chrome policies', ''); On 2013/06/19 16:35:17, Joao da Silva wrote: > Creating the chromeTable here is not necessary anymore, since it's now created > on setPolicyNames. Done. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:269: class PolicyUIHandler : public content::NotificationObserver, On 2013/06/19 16:35:17, Joao da Silva wrote: > #include "content/public/browser/notification_observer.h" Done. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:328: DISALLOW_COPY_AND_ASSIGN(PolicyUIHandler); On 2013/06/19 16:35:17, Joao da Silva wrote: > This should be the last statement within the class { } Done. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:456: content::NotificationService::AllSources()); On 2013/06/19 16:35:17, Joao da Silva wrote: > This is not needed, the registrar_ does it automatically on destruction. Done. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:525: switch (type) { On 2013/06/19 16:35:17, Joao da Silva wrote: > Since this method always takes the same action the switch statement can be > removed; just DCHECK that the type is one of the expected ones: > > DCHECK(type == LOADED || type == UNLOADED); > SendPolicyNames(); > SendPolicyValues(); Done. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:561: extensions::ExtensionSystem::Get(Profile::FromWebUI(web_ui())); On 2013/06/19 16:35:17, Joao da Silva wrote: > nit: indent (4 spaces) Done. https://codereview.chromium.org/17387002/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:587: schema_map.find(extension->id()); On 2013/06/19 16:35:17, Joao da Silva wrote: > nit: indent (4 spaces) Done.
lgtm
LGTM with nits. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/resources/... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/17387002/diff/31001/chrome/browser/resources/... chrome/browser/resources/policy.js:332: } Optional nit: Blank lines between blocks. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:550: // Get extensions. High-level: Some of your code comments are borderline superfluous. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:556: // Get the policy schema map. Superfluous. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:563: // Add extension names and policies to the map. Superfluous. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:568: // Skip this extension if it's a component extension. Superfluous. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:572: // Add extension name to map. Superfluous. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:576: // Get extension's policy schema. Superfluous. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:592: // Add extension's entry to the map. Superfluous.
Thanks for your comments James. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/resources/... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/17387002/diff/31001/chrome/browser/resources/... chrome/browser/resources/policy.js:332: } On 2013/06/20 18:18:52, James Hawkins wrote: > Optional nit: Blank lines between blocks. Done. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:550: // Get extensions. On 2013/06/20 18:18:52, James Hawkins wrote: > High-level: Some of your code comments are borderline superfluous. I take your point; I initially added these comments as notes to help myself understand the code (since it's my first time coding in C++), with the intention of removing them later, thanks for spotting this. I've removed the ones you mentioned here. What should I do about the other superfluous comments in the SendPolicyValues() method below? (Introduced in issue 16689004 which I have just committed). Should I remove them as part of this CL too? Or start a new issue? https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:556: // Get the policy schema map. On 2013/06/20 18:18:52, James Hawkins wrote: > Superfluous. Done. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:563: // Add extension names and policies to the map. On 2013/06/20 18:18:52, James Hawkins wrote: > Superfluous. Done. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:568: // Skip this extension if it's a component extension. On 2013/06/20 18:18:52, James Hawkins wrote: > Superfluous. Done. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:572: // Add extension name to map. On 2013/06/20 18:18:52, James Hawkins wrote: > Superfluous. Done. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:576: // Get extension's policy schema. On 2013/06/20 18:18:52, James Hawkins wrote: > Superfluous. Done. https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:592: // Add extension's entry to the map. On 2013/06/20 18:18:52, James Hawkins wrote: > Superfluous. Done.
https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/17387002/diff/31001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:550: // Get extensions. On 2013/06/21 10:16:16, anitawoodruff wrote: > On 2013/06/20 18:18:52, James Hawkins wrote: > > High-level: Some of your code comments are borderline superfluous. > > I take your point; I initially added these comments as notes to help myself > understand the code (since it's my first time coding in C++), with the intention > of removing them later, thanks for spotting this. > > I've removed the ones you mentioned here. What should I do about the other > superfluous comments in the SendPolicyValues() method below? (Introduced in > issue 16689004 which I have just committed). Should I remove them as part of > this CL too? Or start a new issue? Up to you.
#ifdefs lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anitawoodruff@chromium.org/17387002/64001
Hopefully I've learnt my lesson and won't do something this stupid again. https://codereview.chromium.org/17387002/diff/64001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/policy_ui.cc (left): https://codereview.chromium.org/17387002/diff/64001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/policy_ui.cc:565: web_ui()->CallJavascriptFunction("policy.Page.setPolicyValues", all_policies); Oops. This line should certainly not have been removed! I was a little too hasty in my rebasing here. Certainly learnt my lesson about trying code out before committing in future..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anitawoodruff@chromium.org/17387002/59004
Message was sent while issue was closed.
Change committed as 208469 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
