Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7585036:
First CL for the about:policy page. This only implements the policy section of the page. (Closed)
|
Created:
9 years, 4 months ago by simo Modified:
9 years, 3 months ago Reviewers:
Mattias Nissler (ping if slow), joth, Evan Stade, commit-bot: I haz the power, arv (Not doing code reviews), James Hawkins CC:
chromium-reviews, John Knottenbelt, Torne Visibility:
Public. |
DescriptionFirst CL for the about:policy page. This only implements the policy section of the page.
Preliminary design doc:
https://docs.google.com/a/google.com/document/d/1KWsF52ImY4eJbNgizaA6Gw_aMVg24zgZfypKZBs376k/edit?hl=en_US&ndplr=1
TEST=set some policies, start chrome and go to chrome://policy
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98977
Patch Set 1 #
Total comments: 119
Patch Set 2 : Removed the map wrapper and wrapped the javascript functions into an object. #
Total comments: 18
Patch Set 3 : . #
Total comments: 77
Patch Set 4 : . #Patch Set 5 : Please ignore previous patch. #
Total comments: 12
Patch Set 6 : Changes in policy.html, policy.js and policy.css #Patch Set 7 : Change in policy.js #
Total comments: 1
Patch Set 8 : Rebased patch #Patch Set 9 : Fixed a change in policy.html #
Total comments: 30
Patch Set 10 : Added a test case for PolicyStatus #
Total comments: 35
Patch Set 11 : Created MockConfigurationPolicyReader and fixed other nits. #
Total comments: 10
Patch Set 12 : Fixed tests, removed description section from the page. #Patch Set 13 : Fixed valgrind errors. #Patch Set 14 : Fixed valgrind errors #Patch Set 15 : Valgrind fix #Patch Set 16 : . #
Total comments: 3
Messages
Total messages: 26 (0 generated)
first round of comments. http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4141: <message name="IDS_POLICY_TITLE" desc="Title for the about:policies page"> Most of the other desc="" attributes in the file have a full sentence, terminated with a period. http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4145: BLAH BLAH BLAH BLAH BLAH Replace with something informative. The strings will go through UI review anyway, but let's at least put some description what the page is good for. http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4177: <message name="IDS_POLICY_APPLIES_TO" desc="Table header for first column in policy table"> I wouldn't put the column index into the description, since this is fragile w.r.t. reordering columns. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:9: #include <set> alphabetize http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:18: explain what the class is good for. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:21: no newline here http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:22: public: indentation http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:23: explicit ConfigurationPolicyStatusKeeper( explicit only for one-argument constructors. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:58: no newline here. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:242: no newline http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:243: DictionaryValue* mp_policy = managed_platform_.get()->GetPolicyStatus(policy); you are leaking all these DictionaryValue pointers here. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:265: } // policy namespace policy http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:266: remove trailing newline http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:17: Need a class description here. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:19: public: indentation is one space for visibility labels only. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:23: virtual ~ConfigurationPolicyReader(); Move destructor declaration before other members. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:29: // Creates a ConfigurationPolicyPrefStore that reads managed platform It's not a PrefStore. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:32: CreateManagedPlatformPolicyReader(bool managed); What does the managed parameter do? http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:38: // Creates a ConfigurationPolicyPrefStore that reads managed cloud policy. Comments says managed while name says recommended? http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:55: void Refresh(); document http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:72: class comment. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:74: remove newline http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:81: // display missing period. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:102: } // policy should be "namespace policy" http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... File chrome/browser/policy/policy_status_map.cc (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:23: string16 error_message) : name(name), line break before : http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:54: value.get()->Equals(other_info->value.get()) && don't need the .get(), scoped_ptr has an operator-> defined. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:84: only one newline http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:127: } // policy namespace policy http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... File chrome/browser/policy/policy_status_map.h (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:20: // policy wasn't actually set). The description in parentheses is actually redundant, since it just lists the values that are given below. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:28: // recommended or none). same here. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:36: // enforced or undefined). same here http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:44: PolicyStatusInfo(string16 name, you should include the string16 header, since you're using the type and shouldn't rely on some other header to pull it in for you. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:64: // missing comment? http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:86: no newline here http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:86: If you intentionally left out the DISALLOW_COPY_AND_ASSIGN here, then you should provide copy constructor and assignment operator instead of DeepCopy(). This boils down to the question whether you want to pass this by value or by pointer/reference. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:88: only one newline http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:90: // Wrapper class around an std::map<ConfigurationPolicyType, PolicyStatusInfo*> What benefit do we get from using a wrapper? http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:92: no newline http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:93: public: indentation http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:125: } // policy namespace policy http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.css:2: font-family: Arial, sans-serif; James will tell you to sort the declarations alphabetically ;) http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.css:110: remove trailing whitespace http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy.js File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.js:4: I guess all this stuff should go into some object so we don't pollute the global namespace. There are helpers to do so, check out e.g. chrome://settings or chrome://oobe http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.js:128: remove trailing whitespace. http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... File chrome/browser/resources/policy/policy.css (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy/policy.css:1: body { Duplicate file? http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... File chrome/browser/resources/policy/policy.html (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy/policy.html:1: <!DOCTYPE HTML> Duplicate? http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:14: #include "content/browser/tab_contents/tab_contents.h" alphabetize http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:61: only one newline http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:75: NewCallback(this, &PolicyUIHandler::HandleRequestPolicyData)); 4 spaces indentation. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:79: ListValue* list = new ListValue(); You are creating a new ListValue object and leak it by reassigning your pointer 3 lines below. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:80: // policy::PolicyStatus* policy_status = new policy::PolicyStatus(); indentation http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:87: void PolicyUIHandler::Observe(int type, Since you have a reference to your PolicyStatus instance to get the data from, I think you shouldn't use NotificationService here, since you can register an observer directly. Or even simpler, have PolicyStatus accept a delegate pointer at construction time that PolicyUIHandler can implement to receive change notifications. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:109: remove trailing whitespace. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:6: #define CHROME_BROWSER_UI_WEBUI_POLICY_UI_H_ excess space http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:14: onlye one newline. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:16: // The base class handler of Javascript messages of the about:policy page Missing period. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:19: no newline here http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:20: public: indentation http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:28: // Callback for the "requestPolicyData" message. You should make this private, unless you have a good reason to keep it public. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:34: const NotificationDetails& details); Here, and on all other overriden functions: add an OVERRIDE declaration before the ; (see base/compiler_specific.h for the declaration). http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:39: Missing class description http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:41: public: indentation
http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:236: I am leaking the PolicyStatusInfo pointers here, will fix. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:243: DictionaryValue* mp_policy = managed_platform_.get()->GetPolicyStatus(policy); On 2011/08/09 13:20:40, Mattias Nissler wrote: > you are leaking all these DictionaryValue pointers here. I thought I saw in base/values.h that the pointers added to a ListValue are deallocated when the ListValue destructor is called? Won't this take care of the DictionaryValue pointers here? http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... File chrome/browser/policy/policy_status_map.h (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:90: // Wrapper class around an std::map<ConfigurationPolicyType, PolicyStatusInfo*> On 2011/08/09 13:20:40, Mattias Nissler wrote: > What benefit do we get from using a wrapper? I did this because I think it makes the ConfigurationPolicyPrefKeeper less cluttered and to avoid repeating code. I was also thinking that methods to find conflicting policies could fit in here if I were to implement this feature. However, I don't know how exactly this will be done so I could revisit whether I need this wrapper later.
Uploaded a new patch. http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4141: <message name="IDS_POLICY_TITLE" desc="Title for the about:policies page"> On 2011/08/09 13:20:40, Mattias Nissler wrote: > Most of the other desc="" attributes in the file have a full sentence, > terminated with a period. Done. http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4145: BLAH BLAH BLAH BLAH BLAH On 2011/08/09 13:20:40, Mattias Nissler wrote: > Replace with something informative. The strings will go through UI review > anyway, but let's at least put some description what the page is good for. Done. http://codereview.chromium.org/7585036/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4177: <message name="IDS_POLICY_APPLIES_TO" desc="Table header for first column in policy table"> On 2011/08/09 13:20:40, Mattias Nissler wrote: > I wouldn't put the column index into the description, since this is fragile > w.r.t. reordering columns. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:9: #include <set> On 2011/08/09 13:20:40, Mattias Nissler wrote: > alphabetize Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:18: On 2011/08/09 13:20:40, Mattias Nissler wrote: > explain what the class is good for. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:21: On 2011/08/09 13:20:40, Mattias Nissler wrote: > no newline here Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:22: public: On 2011/08/09 13:20:40, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:23: explicit ConfigurationPolicyStatusKeeper( On 2011/08/09 13:20:40, Mattias Nissler wrote: > explicit only for one-argument constructors. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:58: On 2011/08/09 13:20:40, Mattias Nissler wrote: > no newline here. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:242: On 2011/08/09 13:20:40, Mattias Nissler wrote: > no newline Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:265: } // policy On 2011/08/09 13:20:40, Mattias Nissler wrote: > namespace policy Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.cc:266: On 2011/08/09 13:20:40, Mattias Nissler wrote: > remove trailing newline Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:17: On 2011/08/09 13:20:40, Mattias Nissler wrote: > Need a class description here. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:19: public: On 2011/08/09 13:20:40, Mattias Nissler wrote: > indentation is one space for visibility labels only. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:23: virtual ~ConfigurationPolicyReader(); On 2011/08/09 13:20:40, Mattias Nissler wrote: > Move destructor declaration before other members. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:29: // Creates a ConfigurationPolicyPrefStore that reads managed platform On 2011/08/09 13:20:40, Mattias Nissler wrote: > It's not a PrefStore. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:32: CreateManagedPlatformPolicyReader(bool managed); On 2011/08/09 13:20:40, Mattias Nissler wrote: > What does the managed parameter do? I have changed this now to be a PolicyStatusInfo::PolicyLevel enum value. This indicates whether this reader reads managed or recommended policy. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:38: // Creates a ConfigurationPolicyPrefStore that reads managed cloud policy. On 2011/08/09 13:20:40, Mattias Nissler wrote: > Comments says managed while name says recommended? Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:55: void Refresh(); On 2011/08/09 13:20:40, Mattias Nissler wrote: > document Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:72: On 2011/08/09 13:20:40, Mattias Nissler wrote: > class comment. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:74: On 2011/08/09 13:20:40, Mattias Nissler wrote: > remove newline Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:81: // display On 2011/08/09 13:20:40, Mattias Nissler wrote: > missing period. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:102: } // policy On 2011/08/09 13:20:40, Mattias Nissler wrote: > should be "namespace policy" Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... File chrome/browser/policy/policy_status_map.cc (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:23: string16 error_message) : name(name), On 2011/08/09 13:20:40, Mattias Nissler wrote: > line break before : Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:54: value.get()->Equals(other_info->value.get()) && On 2011/08/09 13:20:40, Mattias Nissler wrote: > don't need the .get(), scoped_ptr has an operator-> defined. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:84: On 2011/08/09 13:20:40, Mattias Nissler wrote: > only one newline Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.cc:127: } // policy On 2011/08/09 13:20:40, Mattias Nissler wrote: > namespace policy Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... File chrome/browser/policy/policy_status_map.h (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:20: // policy wasn't actually set). On 2011/08/09 13:20:40, Mattias Nissler wrote: > The description in parentheses is actually redundant, since it just lists the > values that are given below. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:28: // recommended or none). On 2011/08/09 13:20:40, Mattias Nissler wrote: > same here. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:36: // enforced or undefined). On 2011/08/09 13:20:40, Mattias Nissler wrote: > same here Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:44: PolicyStatusInfo(string16 name, On 2011/08/09 13:20:40, Mattias Nissler wrote: > you should include the string16 header, since you're using the type and > shouldn't rely on some other header to pull it in for you. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:64: // On 2011/08/09 13:20:40, Mattias Nissler wrote: > missing comment? Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:86: On 2011/08/09 13:20:40, Mattias Nissler wrote: > If you intentionally left out the DISALLOW_COPY_AND_ASSIGN here, then you should > provide copy constructor and assignment operator instead of DeepCopy(). This > boils down to the question whether you want to pass this by value or by > pointer/reference. I didn't intentionally leave it out but on rethinking this, I decided to pass this by value instead of by pointer/reference like I did before. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:86: On 2011/08/09 13:20:40, Mattias Nissler wrote: > no newline here Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:88: On 2011/08/09 13:20:40, Mattias Nissler wrote: > only one newline Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/policy/policy_st... chrome/browser/policy/policy_status_map.h:125: } // policy On 2011/08/09 13:20:40, Mattias Nissler wrote: > namespace policy Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.css:2: font-family: Arial, sans-serif; On 2011/08/09 13:20:40, Mattias Nissler wrote: > James will tell you to sort the declarations alphabetically ;) Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.css:110: On 2011/08/09 13:20:40, Mattias Nissler wrote: > remove trailing whitespace Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy.js File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.js:4: On 2011/08/09 13:20:40, Mattias Nissler wrote: > I guess all this stuff should go into some object so we don't pollute the global > namespace. There are helpers to do so, check out e.g. chrome://settings or > chrome://oobe Okay, I wasn't sure what the policy on this was because for chrome://plugins and chrome://history this wasn't the case. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:14: #include "content/browser/tab_contents/tab_contents.h" On 2011/08/09 13:20:40, Mattias Nissler wrote: > alphabetize Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:61: On 2011/08/09 13:20:40, Mattias Nissler wrote: > only one newline Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:75: NewCallback(this, &PolicyUIHandler::HandleRequestPolicyData)); On 2011/08/09 13:20:40, Mattias Nissler wrote: > 4 spaces indentation. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:79: ListValue* list = new ListValue(); On 2011/08/09 13:20:40, Mattias Nissler wrote: > You are creating a new ListValue object and leak it by reassigning your pointer > 3 lines below. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:80: // policy::PolicyStatus* policy_status = new policy::PolicyStatus(); On 2011/08/09 13:20:40, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:109: On 2011/08/09 13:20:40, Mattias Nissler wrote: > remove trailing whitespace. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:6: #define CHROME_BROWSER_UI_WEBUI_POLICY_UI_H_ On 2011/08/09 13:20:40, Mattias Nissler wrote: > excess space Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:14: On 2011/08/09 13:20:40, Mattias Nissler wrote: > onlye one newline. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:16: // The base class handler of Javascript messages of the about:policy page On 2011/08/09 13:20:40, Mattias Nissler wrote: > Missing period. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:19: On 2011/08/09 13:20:40, Mattias Nissler wrote: > no newline here Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:20: public: On 2011/08/09 13:20:40, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:28: // Callback for the "requestPolicyData" message. On 2011/08/09 13:20:40, Mattias Nissler wrote: > You should make this private, unless you have a good reason to keep it public. Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:34: const NotificationDetails& details); On 2011/08/09 13:20:40, Mattias Nissler wrote: > Here, and on all other overriden functions: add an OVERRIDE declaration before > the ; (see base/compiler_specific.h for the declaration). Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:39: On 2011/08/09 13:20:40, Mattias Nissler wrote: > Missing class description Done. http://codereview.chromium.org/7585036/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.h:41: public: On 2011/08/09 13:20:40, Mattias Nissler wrote: > indentation Done.
http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:37: #status-title, #policies-title { nit: One selector per line. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:48: border-bottom: 1px solid #EEE; s/EEE/eee/ http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:68: border: 1px solid #D9D9D9; d9d9d9 http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:91: border: 1px solid #FFF; Lower case colors, here and elsewhere. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.html:28: <table class="separator-table"> Is this really tabular data? http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.html:79: Remove blank lines. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.html:82: <div i18n-content="noPoliciesSet">NO_POLICIES_RECEIVED</div> Did you intend to leave this string in the div? http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.js:33: var output = document.getElementById('policiesTemplate'); $('') here and elsewhere. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.js:42: chrome.send('requestPolicyData', []); No need to pass this last (empty) param.
James, I have fixed the issues you pointed out but I have now changed the javascript to be encapsulated in an object following Mattias' comment. Could you have a look at it? http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:37: #status-title, #policies-title { On 2011/08/10 17:38:08, James Hawkins wrote: > nit: One selector per line. Done. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:48: border-bottom: 1px solid #EEE; On 2011/08/10 17:38:08, James Hawkins wrote: > s/EEE/eee/ Done. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:68: border: 1px solid #D9D9D9; On 2011/08/10 17:38:08, James Hawkins wrote: > d9d9d9 Done. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.css:91: border: 1px solid #FFF; On 2011/08/10 17:38:08, James Hawkins wrote: > Lower case colors, here and elsewhere. Done. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.html:28: <table class="separator-table"> On 2011/08/10 17:38:08, James Hawkins wrote: > Is this really tabular data? No, I guess not. I have changed this now. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.html:79: On 2011/08/10 17:38:08, James Hawkins wrote: > Remove blank lines. Done. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.html:82: <div i18n-content="noPoliciesSet">NO_POLICIES_RECEIVED</div> On 2011/08/10 17:38:08, James Hawkins wrote: > Did you intend to leave this string in the div? No. Done. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.js:33: var output = document.getElementById('policiesTemplate'); On 2011/08/10 17:38:08, James Hawkins wrote: > $('') here and elsewhere. Done. http://codereview.chromium.org/7585036/diff/6001/chrome/browser/resources/pol... chrome/browser/resources/policy.js:42: chrome.send('requestPolicyData', []); On 2011/08/10 17:38:08, James Hawkins wrote: > No need to pass this last (empty) param. Done.
http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:74: float: right; RTL http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:78: padding-right: 20px; RTL http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:109: padding-left: 10px; RTL http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:125: text-align: left; RTL
http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:68: placeholder="Filter policies by name"> i18n http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:86: jsvalues=".className:Policy.isPolicySet($this)? ws http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:87: 'policy-set' : 'policy-unset'; no ws before : http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:88: .style.display:Policy.shouldDisplayPolicy($this) ? ws http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:22: 'any_policies_set': true do you have control of the json schema? Can you change to camelCase? http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:23: } missing semicolon http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:35: * True if none of the received policies are actually set, false otherwise. /** * ... */ http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:38: no_active_policies_: false, no underscores in js names http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:62: * @param {term} The search string @param {TypeName} type The search string. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:67: var show_unsent = $('toggle-unsent-policies').checked; no underscores http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:68: for (var r = 1; r < table.rows.length; ++r) { r++ http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:76: var name_cell = row.getElementsByClassName('policy-name')[0]; var nameCell = row.querySelector('.policy-name'); http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:77: var cell_contents = name_cell.innerHTML.replace(/<[^>]+>/g,""); use single quoted strings... but this use of innerHTML is pretty suspicious. Can you just use textContent instead? http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:91: $('policies').style.display = ''; How about using the 'hidden' dom property instead? http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:96: for (var i = 0; i < table_rows.length; ++i) { Prefer post increment in JS for consistency http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:98: table_rows[i].style.display = 'table-row'; here too http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:138: Policy.isPolicySet = function(policy) { This seems pretty useless. Can you inline this? http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:150: $('toggle-unsent-policies').onchange = function (event) { ... = function(event) {
Some more comments. Oh, and unit tests for ConfigurationPolicyReader would be nice to have, too :) http://codereview.chromium.org/7585036/diff/14001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7585036/diff/14001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4149: This page lists the policies that are currently enabled on this client and their values for better no line breaks in the string please. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:24: public: indentation http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:39: PolicyStatusMapType; you can remove the Type suffix, we usually don't put it. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:62: ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList(); I think you should get this pointer from the provider, making the accessor public. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:64: // Create a mapping from ConfigurationPolicyTypes to actual policy names. Hm, so we do this and store the mapping for every keeper? That seems like unneeded overhead. Conceptually, I think there should be a function on PolicyDefinitionList to convert a type into a name. I'm not sure what the best way of implementing that would be though. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:80: no newline http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:112: bool ConfigurationPolicyReader::IsInitializationComplete() const { Do you actually need that whole initialization complete logic? If not, remove it :) http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:128: BrowserPolicyConnector* connector = indentation http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:187: if (!provider_) indentation http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:23: public: only one space of indentation for the visibility label, 2 spaces for the declarations. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:60: PolicyStatusInfo::PolicyLevel policy_level); indentation. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:86: no newline here. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:87: public: indentation as described above. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... chrome/browser/ui/webui/policy_ui.h:17: public: indentation. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... chrome/browser/ui/webui/policy_ui.h:26: no newline http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... chrome/browser/ui/webui/policy_ui.h:37: public: indentation
http://codereview.chromium.org/7585036/diff/14001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7585036/diff/14001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4149: This page lists the policies that are currently enabled on this client and their values for better On 2011/08/12 09:42:05, Mattias Nissler wrote: > no line breaks in the string please. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:24: public: On 2011/08/12 09:42:05, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:39: PolicyStatusMapType; On 2011/08/12 09:42:05, Mattias Nissler wrote: > you can remove the Type suffix, we usually don't put it. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:62: ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList(); On 2011/08/12 09:42:05, Mattias Nissler wrote: > I think you should get this pointer from the provider, making the accessor > public. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:64: // Create a mapping from ConfigurationPolicyTypes to actual policy names. On 2011/08/12 09:42:05, Mattias Nissler wrote: > Hm, so we do this and store the mapping for every keeper? That seems like > unneeded overhead. Conceptually, I think there should be a function on > PolicyDefinitionList to convert a type into a name. I'm not sure what the best > way of implementing that would be though. I have added a static method to PolicyStatus that computes this. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:80: On 2011/08/12 09:42:05, Mattias Nissler wrote: > no newline Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:112: bool ConfigurationPolicyReader::IsInitializationComplete() const { On 2011/08/12 09:42:05, Mattias Nissler wrote: > Do you actually need that whole initialization complete logic? If not, remove it > :) Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:128: BrowserPolicyConnector* connector = On 2011/08/12 09:42:05, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:187: if (!provider_) On 2011/08/12 09:42:05, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:23: public: On 2011/08/12 09:42:05, Mattias Nissler wrote: > only one space of indentation for the visibility label, 2 spaces for the > declarations. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:60: PolicyStatusInfo::PolicyLevel policy_level); On 2011/08/12 09:42:05, Mattias Nissler wrote: > indentation. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:86: On 2011/08/12 09:42:05, Mattias Nissler wrote: > no newline here. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:87: public: On 2011/08/12 09:42:05, Mattias Nissler wrote: > indentation as described above. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:74: float: right; On 2011/08/11 21:16:30, James Hawkins wrote: > RTL Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:78: padding-right: 20px; On 2011/08/11 21:16:30, James Hawkins wrote: > RTL I can't get the checkbox to stay on the right side of the label if I use RTL here. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:109: padding-left: 10px; On 2011/08/11 21:16:30, James Hawkins wrote: > RTL Do you mean RTL in terms of internationalization? Or why should this be right-to-left? http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.css:125: text-align: left; On 2011/08/11 21:16:30, James Hawkins wrote: > RTL Do you mean RTL in terms of internationalization? Or why should this be right-to-left? http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:68: placeholder="Filter policies by name"> On 2011/08/11 23:30:58, arv wrote: > i18n On the chrome://settings page, the placeholder is replaced with i18n content through localStrings.getStrings in the javascript. Is this not the way to do it? http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:86: jsvalues=".className:Policy.isPolicySet($this)? On 2011/08/11 23:30:58, arv wrote: > ws Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:87: 'policy-set' : 'policy-unset'; On 2011/08/11 23:30:58, arv wrote: > no ws before : Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:88: .style.display:Policy.shouldDisplayPolicy($this) ? On 2011/08/11 23:30:58, arv wrote: > ws Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:22: 'any_policies_set': true On 2011/08/11 23:30:58, arv wrote: > do you have control of the json schema? Can you change to camelCase? Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:23: } On 2011/08/11 23:30:58, arv wrote: > missing semicolon Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:35: * True if none of the received policies are actually set, false otherwise. On 2011/08/11 23:30:58, arv wrote: > /** > * ... > */ Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:38: no_active_policies_: false, On 2011/08/11 23:30:58, arv wrote: > no underscores in js names Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:62: * @param {term} The search string On 2011/08/11 23:30:58, arv wrote: > @param {TypeName} type The search string. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:67: var show_unsent = $('toggle-unsent-policies').checked; On 2011/08/11 23:30:58, arv wrote: > no underscores Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:68: for (var r = 1; r < table.rows.length; ++r) { On 2011/08/11 23:30:58, arv wrote: > r++ Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:76: var name_cell = row.getElementsByClassName('policy-name')[0]; On 2011/08/11 23:30:58, arv wrote: > var nameCell = row.querySelector('.policy-name'); Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:77: var cell_contents = name_cell.innerHTML.replace(/<[^>]+>/g,""); On 2011/08/11 23:30:58, arv wrote: > use single quoted strings... but this use of innerHTML is pretty suspicious. Can > you just use textContent instead? Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:91: $('policies').style.display = ''; On 2011/08/11 23:30:58, arv wrote: > How about using the 'hidden' dom property instead? Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:96: for (var i = 0; i < table_rows.length; ++i) { On 2011/08/11 23:30:58, arv wrote: > Prefer post increment in JS for consistency Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:98: table_rows[i].style.display = 'table-row'; On 2011/08/11 23:30:58, arv wrote: > here too Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:138: Policy.isPolicySet = function(policy) { On 2011/08/11 23:30:58, arv wrote: > This seems pretty useless. Can you inline this? Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.js:150: $('toggle-unsent-policies').onchange = function (event) { On 2011/08/11 23:30:58, arv wrote: > ... = function(event) { Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... chrome/browser/ui/webui/policy_ui.h:17: public: On 2011/08/12 09:42:05, Mattias Nissler wrote: > indentation. Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... chrome/browser/ui/webui/policy_ui.h:26: On 2011/08/12 09:42:05, Mattias Nissler wrote: > no newline Done. http://codereview.chromium.org/7585036/diff/14001/chrome/browser/ui/webui/pol... chrome/browser/ui/webui/policy_ui.h:37: public: On 2011/08/12 09:42:05, Mattias Nissler wrote: > indentation Done.
http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7585036/diff/14001/chrome/browser/resources/po... chrome/browser/resources/policy.html:68: placeholder="Filter policies by name"> On 2011/08/16 17:36:34, simo wrote: > On 2011/08/11 23:30:58, arv wrote: > > i18n > On the chrome://settings page, the placeholder is replaced with i18n content > through localStrings.getStrings in the javascript. Is this not the way to do it? Just do it using the i18n template mechanism where possible i18n-values="placeholder: filterPoliciesText" http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.css:73: float: right; why float? http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.css:105: padding-left: 10px; -webkit-padding-start: 10px; Any place you have left/right you need to flip it for RTL. See http://dev.chromium.org/developers/web-development-style-guide http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.html:76: jsvalues=".style.visibility: anyPoliciesSet ? Do you want this to take up space even if hidden? Maybe use jsdisplay here too? http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.html:89: .style.visibility: Policy.shouldDisplayPolicy($this) ? same here? http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.js:25: cr.define('cr.ui', function() { This should not be part of cr.ui. cr.ui is meant for comon ui code located in resources/shared/js/cr/ui/ http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.js:42: * @type {string} @private
http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.css:73: float: right; On 2011/08/16 19:14:44, arv wrote: > why float? Because I need the checkbox and label to go as far right without wrapping the label text as possible so that I have the section title (Policies) on the left side and the checkbox, label and search field on the same line but on the right side. http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.css:105: padding-left: 10px; On 2011/08/16 19:14:44, arv wrote: > -webkit-padding-start: 10px; > > Any place you have left/right you need to flip it for RTL. > > See http://dev.chromium.org/developers/web-development-style-guide Done. http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.html:76: jsvalues=".style.visibility: anyPoliciesSet ? On 2011/08/16 19:14:44, arv wrote: > Do you want this to take up space even if hidden? > > Maybe use jsdisplay here too? If I use jsdiplay, the table isn't generated so when the checkbox is checked (to show unset policies), there is nothing to display. I tried generating the template when the checkbox is checked but I couldn't really get it to work. But I've now changed it to use style.display instead of style.visibility. http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.html:89: .style.visibility: Policy.shouldDisplayPolicy($this) ? On 2011/08/16 19:14:44, arv wrote: > same here? Same answer. http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.js:25: cr.define('cr.ui', function() { On 2011/08/16 19:14:44, arv wrote: > This should not be part of cr.ui. cr.ui is meant for comon ui code located in > resources/shared/js/cr/ui/ Okay, I have put it into 'policies' now. Is that okay? http://codereview.chromium.org/7585036/diff/26001/chrome/browser/resources/po... chrome/browser/resources/policy.js:42: * @type {string} On 2011/08/16 19:14:44, arv wrote: > @private Done.
http://codereview.chromium.org/7585036/diff/31001/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7585036/diff/31001/chrome/browser/resources/po... chrome/browser/resources/policy.js:96: arv: I had to change this back to .style.display because I changed the policies div element in policy.html to use the display attribute and not the visibility attribute.
LGTM
Pretty close, the main comments are testing-related. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:23: public: indentation http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:72: LOG(WARNING) << "Received unknown policy from provider."; If it should never happen, let's make it a NOTREACHED() http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:224: unsent_policies.clear(); this line is not needed. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:248: // This should never happen. Put a NOTREACHED() then. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:19: // order to determine the status of a policy (this includes it's value and s/it's/its/ http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:25: PolicyStatusInfo::PolicyLevel policy_level); indentation http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:79: class PolicyStatus { Do we have a test case for this class? http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:88: ListValue* GetPolicyStatusList(bool& any_policies_sent) const; out parameters should be pointers per the style guide. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader_unittest.cc (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. I don't think it's required to have test cases for all policies here. Having a test case for each type would be enough IMHO. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:30: template<typename TESTBASE> It seems TESTBASE is the same in all instantiations, so we can drop this parameter. If you're changing the tests to be non-parameterized as per my previous comment, you may want to use testing::Test as the base class. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:43: dict->SetString("name", policy.policy_name()); Use string constants declared in PolicyStatusInfo http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... File chrome/browser/policy/policy_status_info.cc (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:54: value.reset(rhs.value->DeepCopy()); AFAICS, you have operator= and the copy constructor so you can put PolicyStatusInfo into STL containers. Since containers copy stuff a lot, we tend to just use pointers for the container contents and make sure to take care we delete the pointers in destruction. Would that work here as well? http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:66: result->SetString("name", name); Declare static class members for these string constants and use them here and also in unit tests. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:89: ASCIIToUTF16("undefined") }; Can you put a DCHECK that makes sure the numeric value of source_type is actually within bounds? http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:97: ASCIIToUTF16("undefined") }; same here.
http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:23: public: On 2011/08/24 11:57:09, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:72: LOG(WARNING) << "Received unknown policy from provider."; On 2011/08/24 11:57:09, Mattias Nissler wrote: > If it should never happen, let's make it a NOTREACHED() Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:224: unsent_policies.clear(); On 2011/08/24 11:57:09, Mattias Nissler wrote: > this line is not needed. Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:248: // This should never happen. On 2011/08/24 11:57:09, Mattias Nissler wrote: > Put a NOTREACHED() then. Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:19: // order to determine the status of a policy (this includes it's value and On 2011/08/24 11:57:09, Mattias Nissler wrote: > s/it's/its/ Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:25: PolicyStatusInfo::PolicyLevel policy_level); On 2011/08/24 11:57:09, Mattias Nissler wrote: > indentation Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:79: class PolicyStatus { On 2011/08/24 11:57:09, Mattias Nissler wrote: > Do we have a test case for this class? Now we do! http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:88: ListValue* GetPolicyStatusList(bool& any_policies_sent) const; On 2011/08/24 11:57:09, Mattias Nissler wrote: > out parameters should be pointers per the style guide. Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader_unittest.cc (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/08/24 11:57:09, Mattias Nissler wrote: > I don't think it's required to have test cases for all policies here. Having a > test case for each type would be enough IMHO. Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:30: template<typename TESTBASE> On 2011/08/24 11:57:09, Mattias Nissler wrote: > It seems TESTBASE is the same in all instantiations, so we can drop this > parameter. If you're changing the tests to be non-parameterized as per my > previous comment, you may want to use testing::Test as the base class. Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:43: dict->SetString("name", policy.policy_name()); On 2011/08/24 11:57:09, Mattias Nissler wrote: > Use string constants declared in PolicyStatusInfo Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... File chrome/browser/policy/policy_status_info.cc (right): http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:54: value.reset(rhs.value->DeepCopy()); Yeah, that works. On 2011/08/24 11:57:09, Mattias Nissler wrote: > AFAICS, you have operator= and the copy constructor so you can put > PolicyStatusInfo into STL containers. Since containers copy stuff a lot, we tend > to just use pointers for the container contents and make sure to take care we > delete the pointers in destruction. Would that work here as well? http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:66: result->SetString("name", name); On 2011/08/24 11:57:09, Mattias Nissler wrote: > Declare static class members for these string constants and use them here and > also in unit tests. Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:89: ASCIIToUTF16("undefined") }; On 2011/08/24 11:57:09, Mattias Nissler wrote: > Can you put a DCHECK that makes sure the numeric value of source_type is > actually within bounds? Done. http://codereview.chromium.org/7585036/diff/43001/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:97: ASCIIToUTF16("undefined") }; On 2011/08/24 11:57:09, Mattias Nissler wrote: > same here. Done.
mostly happy, mainly nits. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:184: : managed_platform_(managed_platform), only 4 spaces of indentation here. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:204: ASCIIToUTF16(policy->name), indentation, how about breaking after the = above? http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:35: PolicyStatusInfo::PolicyLevel policy_level); Another 4 spaces indentation here http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:40: PolicyStatusInfo::PolicyLevel policy_level); same here http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:44: CreateRecommendedPlatformPolicyReader( 4 spaces indentation http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:50: PolicyStatusInfo::PolicyLevel policy_level); same here http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader_unittest.cc (right): http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:26: DictionaryValue* CreateDictionary(const char* policy_name) { How about accepting an additional parameter to set for value_dict_path, so you don't have to do that in all of the tests below? http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:144: : managed_platform_provider_(), 4 spaces of indentation http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:149: new ConfigurationPolicyReader(&managed_platform_provider_, Instead of actual ConfigurationPolicyReaders, can we maybe mock them? http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:161: policy_status_.reset( new PolicyStatus(managed_platform_, no space after ( http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:183: if (defined) need braces here (multi-line conditional block) http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:192: ConfigurationPolicyReader* managed_platform_; I think you're leaking these, you might want to wrap them in scoped_ptrs http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:230: PolicyStatusInfo::LEVEL_UNDEFINED)); +4 spaces identation http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:233: PolicyStatusInfo::SOURCE_TYPE_UNDEFINED)); same here http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:242: PolicyStatusInfo::USER)); same here http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:250: Value* status_dict = Value::CreateNullValue(); Can just use NULL here. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:252: // Every policy in policy_list_ has to appear in the returned ListValue. it should be |policy_list_| (just convention for variable names) http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:266: ASSERT_FALSE(status_dict->Equals(Value::CreateNullValue())); You leak the created null value.
http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:184: : managed_platform_(managed_platform), On 2011/08/25 11:01:02, Mattias Nissler wrote: > only 4 spaces of indentation here. Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:204: ASCIIToUTF16(policy->name), On 2011/08/25 11:01:02, Mattias Nissler wrote: > indentation, how about breaking after the = above? Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:35: PolicyStatusInfo::PolicyLevel policy_level); On 2011/08/25 11:01:02, Mattias Nissler wrote: > Another 4 spaces indentation here Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:40: PolicyStatusInfo::PolicyLevel policy_level); On 2011/08/25 11:01:02, Mattias Nissler wrote: > same here Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:44: CreateRecommendedPlatformPolicyReader( On 2011/08/25 11:01:02, Mattias Nissler wrote: > 4 spaces indentation Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.h:50: PolicyStatusInfo::PolicyLevel policy_level); On 2011/08/25 11:01:02, Mattias Nissler wrote: > same here Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader_unittest.cc (right): http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:26: DictionaryValue* CreateDictionary(const char* policy_name) { On 2011/08/25 11:01:02, Mattias Nissler wrote: > How about accepting an additional parameter to set for value_dict_path, so you > don't have to do that in all of the tests below? Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:144: : managed_platform_provider_(), On 2011/08/25 11:01:02, Mattias Nissler wrote: > 4 spaces of indentation Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:149: new ConfigurationPolicyReader(&managed_platform_provider_, On 2011/08/25 11:01:02, Mattias Nissler wrote: > Instead of actual ConfigurationPolicyReaders, can we maybe mock them? Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:161: policy_status_.reset( new PolicyStatus(managed_platform_, On 2011/08/25 11:01:02, Mattias Nissler wrote: > no space after ( Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:183: if (defined) On 2011/08/25 11:01:02, Mattias Nissler wrote: > need braces here (multi-line conditional block) Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:192: ConfigurationPolicyReader* managed_platform_; On 2011/08/25 11:01:02, Mattias Nissler wrote: > I think you're leaking these, you might want to wrap them in scoped_ptrs They are passed in the the PolicyStatus object in the constructor which wraps them into scoped_ptrs. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:230: PolicyStatusInfo::LEVEL_UNDEFINED)); On 2011/08/25 11:01:02, Mattias Nissler wrote: > +4 spaces identation Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:233: PolicyStatusInfo::SOURCE_TYPE_UNDEFINED)); On 2011/08/25 11:01:02, Mattias Nissler wrote: > same here Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:242: PolicyStatusInfo::USER)); On 2011/08/25 11:01:02, Mattias Nissler wrote: > same here Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:250: Value* status_dict = Value::CreateNullValue(); On 2011/08/25 11:01:02, Mattias Nissler wrote: > Can just use NULL here. Done. http://codereview.chromium.org/7585036/diff/46001/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:252: // Every policy in policy_list_ has to appear in the returned ListValue. On 2011/08/25 11:01:02, Mattias Nissler wrote: > it should be |policy_list_| (just convention for variable names) Done.
Getting there. Can you please also add a BUG= reference to the CL description? http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader.cc (right): http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:64: delete entry->second; Please use STLDeleteContainerPointers from base/stl_util-inl.h instead. http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:114: connector->GetManagedPlatformProvider(), policy_level); I think I said this before, but I cannot seem to remember your answer: Rather than passing in |policy_level|, shouldn't we just put the appropriate policy level (as the name of this function implies) here? Also for the 3 functions below. http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader.cc:184: managed_cloud_(managed_cloud), indentation (line up with managed_platform_ in line above) http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... File chrome/browser/policy/configuration_policy_reader_unittest.cc (right): http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:21: PolicyStatusInfo::MANDATORY)); indentation http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:23: PolicyStatusInfo::RECOMMENDED)); indentation http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:65: GetPolicyStatus(kPolicyRestoreOnStartupURLs))); another 4 spaces indentation (or break after the opening paren) http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/confi... chrome/browser/policy/configuration_policy_reader_unittest.cc:189: MockConfigurationPolicyProvider managed_platform_provider_; Why do you still need providers? I think you can just make the mock readers use gmock and use EXPECT_CALL() to instruct them to return whatever you need. http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/mock_... File chrome/browser/policy/mock_configuration_policy_reader.h (right): http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/mock_... chrome/browser/policy/mock_configuration_policy_reader.h:21: class MockConfigurationPolicyReader : public ConfigurationPolicyReader, Ah, so I probably wasn't clear enough regarding the mock in my previous comments, sorry. We use gmock for generating mocks and handling calls into these. See http://code.google.com/p/googlemock/ The idea is that calls into the mock get caught and you can associate whatever action should be performed on the call. You can also configure the return value. Thus, you don't need the actual policy provider, but you can just instruct the mock to return a suitable DictionaryValue upon calls to GetPolicyStatus(). http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/polic... File chrome/browser/policy/policy_status_info.h (right): http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.h:50: remove extra blank line http://codereview.chromium.org/7585036/diff/46002/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.h:86: static const std::string level_dict_path; These are constants and should thus be name kLevelDictPath etc.
LGTM! I fired try jobs, if they're green we can ship it.
On 2011/08/30 09:25:45, Mattias Nissler wrote: > LGTM! I fired try jobs, if they're green we can ship it. Patch 14 is the fixed one.
Still looking good. Will fire a new round of try jobs.
Change committed as 98977
FYI this broke the configuration_policy=0 build, we're looking into it :) http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Redux/bu... A couple latent drive-by comments while I'm here... Cheers! http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... File chrome/browser/policy/policy_status_info.cc (right): http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.cc:72: ASCIIToUTF16("undefined") }; to avoid having static class instances (against style guide; not thread safe; bloats atexit() work) it would be better to have an array of const char[] here and do the ASCIIToUTF16() call in the return statement. Ditto in the fn below. http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... File chrome/browser/policy/policy_status_info.h (right): http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.h:8: #include <map> not used? http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... chrome/browser/policy/policy_status_info.h:90: static const std::string kValueDictPath; note static class instances are not allowed in style guide. Could these be const char[] ?
On 2011/09/05 11:09:38, joth wrote: > FYI this broke the configuration_policy=0 build, we're looking into it :) > http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Linux%2520Redu... > > A couple latent drive-by comments while I'm here... > > Cheers! > > http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... > File chrome/browser/policy/policy_status_info.cc (right): > > http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... > chrome/browser/policy/policy_status_info.cc:72: ASCIIToUTF16("undefined") }; > to avoid having static class instances (against style guide; not thread safe; > bloats atexit() work) it would be better to have an array of const char[] here > and do the ASCIIToUTF16() call in the return statement. > > Ditto in the fn below. > > http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... > File chrome/browser/policy/policy_status_info.h (right): > > http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... > chrome/browser/policy/policy_status_info.h:8: #include <map> > not used? > > http://codereview.chromium.org/7585036/diff/58013/chrome/browser/policy/polic... > chrome/browser/policy/policy_status_info.h:90: static const std::string > kValueDictPath; > note static class instances are not allowed in style guide. Could these be const > char[] ? Thanks! I will fix these problems in the second CL for this which is currently being reviewed or in an additional one. |
