Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7828042:
Implemented status section functionality for about:policy. (Closed)
|
Created:
9 years, 3 months ago by simo Modified:
9 years, 3 months ago Reviewers:
commit-bot: I haz the power, arv (Not doing code reviews), James Hawkins, Mattias Nissler (ping if slow) CC:
chromium-reviews Visibility:
Public. |
DescriptionImplemented status section functionality for about:policy.
Displaying status information
Fetching of policies on click
Refresh of policy information when it changes
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101061
Patch Set 1 #
Total comments: 20
Patch Set 2 : . #
Total comments: 71
Patch Set 3 : . #
Total comments: 2
Patch Set 4 : . #
Total comments: 12
Patch Set 5 : . #Patch Set 6 : Rebased patch. #
Messages
Total messages: 18 (0 generated)
first round of comments. http://codereview.chromium.org/7828042/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7828042/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4188: <message name="IDS_POLICY_USER_ID" desc="The label for the user id in the user policies status box."> Hm, it seems that we should find a common term for IDS_POLICY_DEVICE_ID and IDS_POLICY_USER_ID, since they're actually the same thing. http://codereview.chromium.org/7828042/diff/1/chrome/app/generated_resources.... chrome/app/generated_resources.grd:4225: <message name="IDS_POLICY_SHOW_MORE" desc="The text for the link that expands a policy value or policy status table cell."> Ah, so here I can see a good cut for splitting the CL, one for the status box, one for the expand values functionality. http://codereview.chromium.org/7828042/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7828042/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:102: // Removes an observer to each one of the ConfigurationPolicyReaders. s/to/from/ http://codereview.chromium.org/7828042/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:108: // it is set tof alse. This is for the about:policy UI to display. tof alse? sounds like a nordic actor. http://codereview.chromium.org/7828042/diff/1/chrome/browser/resources/policy.js File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7828042/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.js:218: Policy.returnData = function(data) { Hm, so we now have multiple entry points to push data to the page. Is that really necessary? Can't we just send a full update every time we need to update something? Data-size wise that shouldn't be a problem. Maybe I'm missing something here. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:16: #include "chrome/browser/profiles/profile_manager.h" What do you need ProfileManager for? http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:115: void PolicyUIHandler::HandleUpdateFetchTimes(const ListValue* args) { I see that you poll for these from the JS side. I don't think that's necessary, people will be smart enough to reload the page. Eventually, we can probably also implement a notification so we can push updates after each reload, but that probably requires some refactoring of the policy code. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:184: string16 user_last_fetch_time = GetLastFetchTime(user_subsystem); No need for all the local variables, just put the CreateStatusMessageString() and GetLastFetchTime() calls as second parameter to SetString() below. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:202: results->SetString("userId", user_id); Looks like for the identifiers, you could also introduce a a helper function as you did for fetch time and status message. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:211: results->SetString("user", username); Again, no need for local variables. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:221: prefs->GetInteger(prefs::kDevicePolicyRefreshRate)); Another good case for a helper function that wraps getting the time and formatting it to a string?
I have removed the polling for the "last policy fetch" times in the javascript for now. http://codereview.chromium.org/7828042/diff/1/chrome/browser/policy/configura... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7828042/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:102: // Removes an observer to each one of the ConfigurationPolicyReaders. On 2011/09/02 11:16:09, Mattias Nissler wrote: > s/to/from/ Done. http://codereview.chromium.org/7828042/diff/1/chrome/browser/policy/configura... chrome/browser/policy/configuration_policy_reader.h:108: // it is set tof alse. This is for the about:policy UI to display. On 2011/09/02 11:16:09, Mattias Nissler wrote: > tof alse? sounds like a nordic actor. Done. http://codereview.chromium.org/7828042/diff/1/chrome/browser/resources/policy.js File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7828042/diff/1/chrome/browser/resources/policy... chrome/browser/resources/policy.js:218: Policy.returnData = function(data) { On 2011/09/02 11:16:09, Mattias Nissler wrote: > Hm, so we now have multiple entry points to push data to the page. Is that > really necessary? Can't we just send a full update every time we need to update > something? Data-size wise that shouldn't be a problem. Maybe I'm missing > something here. Okay, true I can use a single entry point to push data. I have added a boolean to the policyDataFormat so that I know whether an update is happening because I need to call some more functions in that case. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:16: #include "chrome/browser/profiles/profile_manager.h" On 2011/09/02 11:16:09, Mattias Nissler wrote: > What do you need ProfileManager for? Oh, nothing, forgot to take that out. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:115: void PolicyUIHandler::HandleUpdateFetchTimes(const ListValue* args) { On 2011/09/02 11:16:09, Mattias Nissler wrote: > I see that you poll for these from the JS side. I don't think that's necessary, > people will be smart enough to reload the page. Eventually, we can probably also > implement a notification so we can push updates after each reload, but that > probably requires some refactoring of the policy code. Since I can't find any time formatting in javascript, I will just not update the times for now. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:184: string16 user_last_fetch_time = GetLastFetchTime(user_subsystem); On 2011/09/02 11:16:09, Mattias Nissler wrote: > No need for all the local variables, just put the CreateStatusMessageString() > and GetLastFetchTime() calls as second parameter to SetString() below. Done. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:202: results->SetString("userId", user_id); On 2011/09/02 11:16:09, Mattias Nissler wrote: > Looks like for the identifiers, you could also introduce a a helper function as > you did for fetch time and status message. Done. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:211: results->SetString("user", username); On 2011/09/02 11:16:09, Mattias Nissler wrote: > Again, no need for local variables. Done. http://codereview.chromium.org/7828042/diff/1/chrome/browser/ui/webui/policy_... chrome/browser/ui/webui/policy_ui.cc:221: prefs->GetInteger(prefs::kDevicePolicyRefreshRate)); On 2011/09/02 11:16:09, Mattias Nissler wrote: > Another good case for a helper function that wraps getting the time and > formatting it to a string? Done.
http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:25: class Observer { Document this class. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:27: virtual ~Observer() {} Make the destructor protected. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:28: virtual void OnPolicyValuesChanged() = 0; Document this method. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:40: void AddObserver(Observer* observer); Comment on the ownership and NULL-ability of |observer| of these methods. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:80: ObserverList<Observer, true> observers_; Document this var. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:14: <div id="dataTemplate"> IDs should be dash-form. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:16: style="display: none" jsdisplay="displayStatusSection"> Don't specify style in HTML. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:39: jscontent="deviceLastFetchTime"></span> Wrapped lines should be 4 space indented from the start of the previous line. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:95: <div id="no-policies" stylchre="display: none" What is stylechre? http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:126: <span class="cellText" jscontent="value"></span> Classes use dash-form. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:128: <a class="toggler expand" style="display: none" Use hidden instead of display: none. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:151: <script src="chrome://policy/strings.js"></script> Why did you move these? Scripts should be sourced at the top of the file. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:158: <script src="policy.js"></script> Indentation off. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:5: //var localStrings = new LocalStrings(); Deadcode. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:12: 'anyPoliciesSet': true, Document vars. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:58: displayStatusSection_: false, Why not check the visibility of the status on the fly instead of tracking it with a var? http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:69: * expects an object structure like the above. 'the above' is ambiguous. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:70: * @param {Object} policyData Detailed info about policies You changed the parameter but not the documentation. Is there a reason you changed it to data from policyData? The latter seems more descriptive. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:121: tableRows[i].style.display = 'table-row'; Using hidden will make this a lot simpler. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:210: * @param {Object} data The data Period at end of sentence. I'd also elucidate on 'The data' which doesn't add much to the already-stated var. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:251: return (textContainer.offsetWidth < cellText.offsetWidth); Remove parens. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:40: IDS_POLICY_USERNAME); This looks like it doesn't need to be wrapped. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:122: DictionaryValue* dict = GetStatusData(); Leak? http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:190: if (fetch_user_policy) I'd add a blank line here for readability: I initially thought this last block was an 'else if'. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:199: subsystem->GetCloudPolicyCacheBase(); Indentation. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:33: typedef policy::CloudPolicySubsystem CloudPolicySubsystem; I'm not a fan of these typedefs. They're essentialy 'using' declarations, which are forbidden in header files. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:36: void HandleRequestData(const ListValue* args); Document the param. Specify unused if it's unused. Here and below. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:50: // true, if any policy was fetched (device or user), returns true. Returns You specified 'returns true' twice. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:59: // |result|. s/result/results/. Should document whether |results| may be NULL or must not be NULL. Here and below. Also, consider renaming the variable, since the data returned is device IDs, not 'results'.
http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:11: #include "chrome/browser/prefs/pref_service.h" alphabetize http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:18: #include "chrome/common/pref_names.h" alphabetize http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:118: bool any_policies_sent; should this rather be any_policies_set? http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:124: results.SetBoolean("isPolicyUpdate", is_policy_update); do we need to pass this flag explicitly? I think it'd be more natural to only set "policies" if is_policy_update is true. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:169: GetDeviceIds(results); In the previous comments, I was actually referring to making functions for a single data item, so you could write: results->SetString("deviceId", GetDeviceId(g_browser_process->browser_policy_connector()->GetDeviceCloudPolicyDataStore())) http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:170: GetPolicyFetchIntervals(results); Same here.
http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_reader.h (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:25: class Observer { On 2011/09/05 22:10:38, James Hawkins wrote: > Document this class. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:27: virtual ~Observer() {} On 2011/09/05 22:10:38, James Hawkins wrote: > Make the destructor protected. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:28: virtual void OnPolicyValuesChanged() = 0; On 2011/09/05 22:10:38, James Hawkins wrote: > Document this method. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:40: void AddObserver(Observer* observer); On 2011/09/05 22:10:38, James Hawkins wrote: > Comment on the ownership and NULL-ability of |observer| of these methods. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_reader.h:80: ObserverList<Observer, true> observers_; On 2011/09/05 22:10:38, James Hawkins wrote: > Document this var. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:14: <div id="dataTemplate"> On 2011/09/05 22:10:38, James Hawkins wrote: > IDs should be dash-form. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:16: style="display: none" jsdisplay="displayStatusSection"> On 2011/09/05 22:10:38, James Hawkins wrote: > Don't specify style in HTML. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:39: jscontent="deviceLastFetchTime"></span> On 2011/09/05 22:10:38, James Hawkins wrote: > Wrapped lines should be 4 space indented from the start of the previous line. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:95: <div id="no-policies" stylchre="display: none" On 2011/09/05 22:10:38, James Hawkins wrote: > What is stylechre? A typo. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:126: <span class="cellText" jscontent="value"></span> On 2011/09/05 22:10:38, James Hawkins wrote: > Classes use dash-form. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:128: <a class="toggler expand" style="display: none" On 2011/09/05 22:10:38, James Hawkins wrote: > Use hidden instead of display: none. But won't the togglers take up space when they are hidden, then? That's not the intended result. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:151: <script src="chrome://policy/strings.js"></script> On 2011/09/05 22:10:38, James Hawkins wrote: > Why did you move these? Scripts should be sourced at the top of the file. Because I was looking at other html files like plugins.html and noticed that they were all at the bottom and then I went to a js class where I heard that placing them at the bottom can make the page load faster. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.html:158: <script src="policy.js"></script> On 2011/09/05 22:10:38, James Hawkins wrote: > Indentation off. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:5: //var localStrings = new LocalStrings(); On 2011/09/05 22:10:38, James Hawkins wrote: > Deadcode. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:12: 'anyPoliciesSet': true, On 2011/09/05 22:10:38, James Hawkins wrote: > Document vars. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:58: displayStatusSection_: false, On 2011/09/05 22:10:38, James Hawkins wrote: > Why not check the visibility of the status on the fly instead of tracking it > with a var? Done. I had some use for this var before and then forgot to remove it. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:69: * expects an object structure like the above. On 2011/09/05 22:10:38, James Hawkins wrote: > 'the above' is ambiguous. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:70: * @param {Object} policyData Detailed info about policies On 2011/09/05 22:10:38, James Hawkins wrote: > You changed the parameter but not the documentation. Is there a reason you > changed it to data from policyData? The latter seems more descriptive. At some point I distinguished between statusData and policyData and then I didn't know whether I should still call it policyData when both are combined so I changed it to just "data". Now, calling it policyData does make more sense though. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:121: tableRows[i].style.display = 'table-row'; On 2011/09/05 22:10:38, James Hawkins wrote: > Using hidden will make this a lot simpler. I've changed this but I don't really understand how it makes it simpler. Could you explain? http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:210: * @param {Object} data The data On 2011/09/05 22:10:38, James Hawkins wrote: > Period at end of sentence. I'd also elucidate on 'The data' which doesn't add > much to the already-stated var. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/resources/pol... chrome/browser/resources/policy.js:251: return (textContainer.offsetWidth < cellText.offsetWidth); On 2011/09/05 22:10:38, James Hawkins wrote: > Remove parens. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:11: #include "chrome/browser/prefs/pref_service.h" On 2011/09/06 11:19:30, Mattias Nissler wrote: > alphabetize Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:18: #include "chrome/common/pref_names.h" On 2011/09/06 11:19:30, Mattias Nissler wrote: > alphabetize Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:40: IDS_POLICY_USERNAME); On 2011/09/05 22:10:38, James Hawkins wrote: > This looks like it doesn't need to be wrapped. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:118: bool any_policies_sent; On 2011/09/06 11:19:30, Mattias Nissler wrote: > should this rather be any_policies_set? Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:122: DictionaryValue* dict = GetStatusData(); On 2011/09/05 22:10:38, James Hawkins wrote: > Leak? |dict| is added to the |results| DictionaryValue which takes ownership of it. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:124: results.SetBoolean("isPolicyUpdate", is_policy_update); On 2011/09/06 11:19:30, Mattias Nissler wrote: > do we need to pass this flag explicitly? I think it'd be more natural to only > set "policies" if is_policy_update is true. is_policy_update is set to true after an OnPolicyValuesChanged event and it is false in other cases, i.e. when the UI first loads and requests the policy data. In both cases "policies" will be set though. I need this because the javascript needs to take additional actions when the policy values change after having been loaded previously. I could also have the javascript function take an additional parameter and not add it to the dictionary, I don't know which one makes more sense. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:169: GetDeviceIds(results); On 2011/09/06 11:19:30, Mattias Nissler wrote: > In the previous comments, I was actually referring to making functions for a > single data item, so you could write: > > results->SetString("deviceId", > GetDeviceId(g_browser_process->browser_policy_connector()->GetDeviceCloudPolicyDataStore())) Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:170: GetPolicyFetchIntervals(results); On 2011/09/06 11:19:30, Mattias Nissler wrote: > Same here. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:190: if (fetch_user_policy) On 2011/09/05 22:10:38, James Hawkins wrote: > I'd add a blank line here for readability: I initially thought this last block > was an 'else if'. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:199: subsystem->GetCloudPolicyCacheBase(); On 2011/09/05 22:10:38, James Hawkins wrote: > Indentation. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:33: typedef policy::CloudPolicySubsystem CloudPolicySubsystem; On 2011/09/05 22:10:38, James Hawkins wrote: > I'm not a fan of these typedefs. They're essentialy 'using' declarations, which > are forbidden in header files. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:36: void HandleRequestData(const ListValue* args); On 2011/09/05 22:10:38, James Hawkins wrote: > Document the param. Specify unused if it's unused. Here and below. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:50: // true, if any policy was fetched (device or user), returns true. Returns On 2011/09/05 22:10:38, James Hawkins wrote: > You specified 'returns true' twice. Done. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:59: // |result|. On 2011/09/05 22:10:38, James Hawkins wrote: > s/result/results/. Should document whether |results| may be NULL or must not be > NULL. Here and below. Also, consider renaming the variable, since the data > returned is device IDs, not 'results'. I have changed the method signature.
C++ parts LGTM. http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.cc (right): http://codereview.chromium.org/7828042/diff/1013/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.cc:124: results.SetBoolean("isPolicyUpdate", is_policy_update); On 2011/09/07 12:18:16, simo wrote: > On 2011/09/06 11:19:30, Mattias Nissler wrote: > > do we need to pass this flag explicitly? I think it'd be more natural to only > > set "policies" if is_policy_update is true. > is_policy_update is set to true after an OnPolicyValuesChanged event and it is > false in other cases, i.e. when the UI first loads and requests the policy data. > In both cases "policies" will be set though. I need this because the javascript > needs to take additional actions when the policy values change after having been > loaded previously. I could also have the javascript function take an additional > parameter and not add it to the dictionary, I don't know which one makes more > sense. I see. http://codereview.chromium.org/7828042/diff/9001/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7828042/diff/9001/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:46: // of the policy system. Mention ownership transfer.
http://codereview.chromium.org/7828042/diff/9001/chrome/browser/ui/webui/poli... File chrome/browser/ui/webui/policy_ui.h (right): http://codereview.chromium.org/7828042/diff/9001/chrome/browser/ui/webui/poli... chrome/browser/ui/webui/policy_ui.h:46: // of the policy system. On 2011/09/07 12:50:48, Mattias Nissler wrote: > Mention ownership transfer. Done.
On 2011/09/07 14:52:58, simo wrote: > http://codereview.chromium.org/7828042/diff/9001/chrome/browser/ui/webui/poli... > File chrome/browser/ui/webui/policy_ui.h (right): > > http://codereview.chromium.org/7828042/diff/9001/chrome/browser/ui/webui/poli... > chrome/browser/ui/webui/policy_ui.h:46: // of the policy system. > On 2011/09/07 12:50:48, Mattias Nissler wrote: > > Mention ownership transfer. > > Done. ping?
http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.css:159: .hidden { Use the hidden attribute, not a hidden class. Also, you should just include webui.css, which has the hidden attribute defined appropriately. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.html:157: Remove blank line. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.js:10: Remove blank line. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.js:128: * jstemplate. @private here and elsewhere.
FYI: I think it is fine to use jst here http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.css:150: overflow: hidden; I think also need "white-space: nowrap" here unless that gets set somewhere else. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.html:23: <section id="status-section" jsselect="status" class="hidden" use jsdisplay here
arv: Maybe I'm missing something, where is it fine to use jst? Or do you just mean for about:policy in general? http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.css (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.css:150: overflow: hidden; On 2011/09/09 20:59:32, arv wrote: > I think also need "white-space: nowrap" here unless that gets set somewhere > else. Done. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.css:159: .hidden { On 2011/09/09 20:54:04, James Hawkins wrote: > Use the hidden attribute, not a hidden class. Also, you should just include > webui.css, which has the hidden attribute defined appropriately. Done. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.html (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.html:23: <section id="status-section" jsselect="status" class="hidden" On 2011/09/09 20:59:32, arv wrote: > use jsdisplay here Done. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.html:157: On 2011/09/09 20:54:04, James Hawkins wrote: > Remove blank line. Done. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.js:10: On 2011/09/09 20:54:04, James Hawkins wrote: > Remove blank line. Done. http://codereview.chromium.org/7828042/diff/13002/chrome/browser/resources/po... chrome/browser/resources/policy.js:128: * jstemplate. On 2011/09/09 20:54:04, James Hawkins wrote: > @private > > here and elsewhere. Done.
On 2011/09/12 12:46:30, simo wrote: > arv: Maybe I'm missing something, where is it fine to use jst? Or do you just > mean for about:policy in general? > I'll answer for arv. I asked arv (not knowing myself) if it was legit according to our style guide to use jstemplate, and he replied that for the usage you have here, you're good to go.
LGTM
lgtm
I rebased because the tryjobs failed on mac and also my ConfigurationPolicyProvider refactor CL just got committed which made some changes in configuration_policy_reader.cc
I sent new try jobs. On Tue, Sep 13, 2011 at 1:28 PM, <simo@google.com> wrote: > I rebased because the tryjobs failed on mac and also my > ConfigurationPolicyProvider refactor CL just got committed which made some > changes in configuration_policy_reader.cc > > > http://codereview.chromium.**org/7828042/<http://codereview.chromium.org/7828... >
Change committed as 101061 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
