Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1052943002:
Added device naming information to chrome://policy (Closed)
|
Created:
5 years, 8 months ago by Polina Bondarenko Modified:
5 years, 8 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded device naming information to chrome://policy
Added Asset Id, Location of the device and directory API ID
to the device policy box.
BUG=464184
Committed: https://crrev.com/36d002df861fc6da74af21a1c2e3974f33b1a552
Cr-Commit-Position: refs/heads/master@{#325225}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed comments #
Total comments: 5
Patch Set 3 : Fixed comments #Patch Set 4 : Fixed IOS whitelist #
Messages
Total messages: 32 (12 generated)
pbond@chromium.org changed reviewers: + atwilson@chromium.org, mnissler@chromium.org, nkostylev@chromium.org
atwilson@chromium.org: Please review changes in mnissler@chromium.org: Please review changes in nkostylev@chromium.org: Please review changes in Hi, there is implementation of population of device naming information in the chrome://policy for device policies. The PRD is here https://docs.google.com/document/d/1fqS12910zcPs3OFs77aeQ4yqZ8hqguwNYkU2eeTgS... Thanks!
LGTM w/ nits. https://codereview.chromium.org/1052943002/diff/1/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/1052943002/diff/1/chrome/browser/resources/po... chrome/browser/resources/policy.js:51: // Populate the device naming information Nit (here and below): Sentences in comments should have a period in the end. https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.grdp File components/policy_strings.grdp (right): https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.g... components/policy_strings.grdp:253: Location: Hm, people may think this is the actual geolocation as determined by the device, which may lead to confusion. Maybe we should name this "Assigned Location"?
https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.grdp File components/policy_strings.grdp (right): https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.g... components/policy_strings.grdp:253: Location: On 2015/04/02 12:11:29, Mattias Nissler wrote: > Hm, people may think this is the actual geolocation as determined by the device, > which may lead to confusion. Maybe we should name this "Assigned Location"? Maybe, should I ask Matt about it? In mocks it is called "Location"...
https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.grdp File components/policy_strings.grdp (right): https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.g... components/policy_strings.grdp:253: Location: On 2015/04/02 12:17:26, Polina Bondarenko wrote: > On 2015/04/02 12:11:29, Mattias Nissler wrote: > > Hm, people may think this is the actual geolocation as determined by the > device, > > which may lead to confusion. Maybe we should name this "Assigned Location"? > > Maybe, should I ask Matt about it? In mocks it is called "Location"... Can't hurt to shoot him a quick email.
On 2015/04/02 09:06:19, Polina Bondarenko wrote: > mailto:nkostylev@chromium.org: Please review changes in Not sure what do you want me to review here.
Patchset #2 (id:20001) has been deleted
Fixed comments https://codereview.chromium.org/1052943002/diff/1/chrome/browser/resources/po... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/1052943002/diff/1/chrome/browser/resources/po... chrome/browser/resources/policy.js:51: // Populate the device naming information On 2015/04/02 12:11:29, Mattias Nissler wrote: > Nit (here and below): Sentences in comments should have a period in the end. Done. https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.grdp File components/policy_strings.grdp (right): https://codereview.chromium.org/1052943002/diff/1/components/policy_strings.g... components/policy_strings.grdp:253: Location: On 2015/04/02 12:18:22, Mattias Nissler wrote: > On 2015/04/02 12:17:26, Polina Bondarenko wrote: > > On 2015/04/02 12:11:29, Mattias Nissler wrote: > > > Hm, people may think this is the actual geolocation as determined by the > > device, > > > which may lead to confusion. Maybe we should name this "Assigned Location"? > > > > Maybe, should I ask Matt about it? In mocks it is called "Location"... > > Can't hurt to shoot him a quick email. Done.
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1052943002/#ps40001 (title: "Fixed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1052943002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/04/02 13:22:58, Nikita Kostylev wrote: > On 2015/04/02 09:06:19, Polina Bondarenko wrote: > > mailto:nkostylev@chromium.org: Please review changes in > > Not sure what do you want me to review here. Hi Nikita, The changes in the next files have to be reviewed: - chrome/browser/resources/policy.html - chrome/browser/resources/policy.js - chrome/browser/ui/webui/policy_ui.cc As I can see, you are an owner of them. Thanks.
lgtm with nits sorry for delay https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/resource... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/resource... chrome/browser/resources/policy.js:51: // Populate the device naming information. nit: insert empty line before comment. (here and below) https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/policy_ui.cc:176: policy->annotated_asset_id() : std::string("Not Specified"); This should be a localized string (here and below).
Patchset #3 (id:60001) has been deleted
Hi Nikita, Thank you for reviewing my code. I fixed your comments, but I'm not sure that I clearly understood the one about localizing strings. Could you please take a look again? Thanks https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/resource... File chrome/browser/resources/policy.js (right): https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/resource... chrome/browser/resources/policy.js:51: // Populate the device naming information. On 2015/04/14 13:21:39, Nikita Kostylev wrote: > nit: insert empty line before comment. > (here and below) Done. https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/policy_ui.cc:176: policy->annotated_asset_id() : std::string("Not Specified"); On 2015/04/14 13:21:39, Nikita Kostylev wrote: > This should be a localized string (here and below). Done, but I'm not sure that I fixed it as expected
lgtm
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nkostylev@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1052943002/#ps80001 (title: "Fixed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1052943002/80001
Thanks!
Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/policy_ui.cc (right): https://codereview.chromium.org/1052943002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/policy_ui.cc:176: policy->annotated_asset_id() : std::string("Not Specified"); On 2015/04/14 16:41:34, Polina Bondarenko wrote: > On 2015/04/14 13:21:39, Nikita Kostylev wrote: > > This should be a localized string (here and below). > > Done, but I'm not sure that I fixed it as expected Right, this string should be up for translation.
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nkostylev@chromium.org, atwilson@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1052943002/#ps120001 (title: "Fixed IOS whitelist")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1052943002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/36d002df861fc6da74af21a1c2e3974f33b1a552 Cr-Commit-Position: refs/heads/master@{#325225} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
