Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259943003:
Settings People CrOS: Display supervised users' names correctly. (Closed)
|
Created:
4 years, 4 months ago by tommycli Modified:
4 years, 4 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, michaelpg+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSettings People CrOS: Display supervised users' names correctly.
This fixes usernames displayed as: 0@locally-managed.localhost in
the ChromeOS User List section of Settings.
BUG=614583
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/978d57269a979053a57da3c1fa799eb4ad541cf0
Cr-Commit-Position: refs/heads/master@{#413567}
Patch Set 1 #Patch Set 2 : fix closure compile #
Total comments: 4
Patch Set 3 : fix test variables #
Total comments: 2
Messages
Total messages: 35 (18 generated)
Description was changed from ========== Settings People CrOS: Display supervised users' names correctly. This fixes usernames displayed as: 0@locally-managed.localhost in the ChromeOS User List section of Settings. BUG=614583 ========== to ========== Settings People CrOS: Display supervised users' names correctly. This fixes usernames displayed as: 0@locally-managed.localhost in the ChromeOS User List section of Settings. BUG=614583 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
tommycli@chromium.org changed reviewers: + orenb@chromium.org
orenb: PTAL. thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm (with nits) https://codereview.chromium.org/2259943003/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/users_private/test.js (right): https://codereview.chromium.org/2259943003/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/users_private/test.js:29: if (user.email == kEmail1 && user.name == kEmail1) { I think you're reusing the kEmail1 variable for user.name, but this makes the test a bit confusing (I thought it was a mistake until I thought about it a bit more). Please make a new "kName1" variable and just use that here and in the test below instead. https://codereview.chromium.org/2259943003/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/users_private/test.js:58: chrome.test.assertEq(kEmail2, users[0].name); So this would be kName2
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
orenb: thanks! https://codereview.chromium.org/2259943003/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/users_private/test.js (right): https://codereview.chromium.org/2259943003/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/users_private/test.js:29: if (user.email == kEmail1 && user.name == kEmail1) { On 2016/08/19 18:47:48, Oren Blasberg wrote: > I think you're reusing the kEmail1 variable for user.name, but this makes the > test a bit confusing (I thought it was a mistake until I thought about it a bit > more). Please make a new "kName1" variable and just use that here and in the > test below instead. Done. https://codereview.chromium.org/2259943003/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/users_private/test.js:58: chrome.test.assertEq(kEmail2, users[0].name); On 2016/08/19 18:47:48, Oren Blasberg wrote: > So this would be kName2 Done.
The CQ bit was unchecked by tommycli@chromium.org
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from orenb@chromium.org Link to the patchset: https://codereview.chromium.org/2259943003/#ps40001 (title: "fix test variables")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tommycli@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin: PTAL, thanks!
lgtm https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/users_private/test.js (right): https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/users_private/test.js:18: var kName1 = kEmail1; it'd probably be good to make this something different so that we'd catch a bug if we *were* returning the same value for the name and email.
https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/users_private/test.js (right): https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/users_private/test.js:18: var kName1 = kEmail1; On 2016/08/20 01:23:04, Devlin wrote: > it'd probably be good to make this something different so that we'd catch a bug > if we *were* returning the same value for the name and email. So... they would only be different for locally managed users with an email that looks like: 0@locally-managed.localhost There's no provision for adding that in here. I'd have to figure out how to do that on the .cc file side. Is that worth?
On 2016/08/20 01:35:57, tommycli wrote: > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > File chrome/test/data/extensions/api_test/users_private/test.js (right): > > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > chrome/test/data/extensions/api_test/users_private/test.js:18: var kName1 = > kEmail1; > On 2016/08/20 01:23:04, Devlin wrote: > > it'd probably be good to make this something different so that we'd catch a > bug > > if we *were* returning the same value for the name and email. > > So... they would only be different for locally managed users with an email that > looks like: mailto:0@locally-managed.localhost > > There's no provision for adding that in here. I'd have to figure out how to do > that on the .cc file side. > > Is that worth? I don't know how difficult it would be, so I'll defer to your judgment. :) It's fine as is if making the change is complicated.
On 2016/08/20 01:43:06, Devlin wrote: > On 2016/08/20 01:35:57, tommycli wrote: > > > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > > File chrome/test/data/extensions/api_test/users_private/test.js (right): > > > > > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > > chrome/test/data/extensions/api_test/users_private/test.js:18: var kName1 = > > kEmail1; > > On 2016/08/20 01:23:04, Devlin wrote: > > > it'd probably be good to make this something different so that we'd catch a > > bug > > > if we *were* returning the same value for the name and email. > > > > So... they would only be different for locally managed users with an email > that > > looks like: mailto:0@locally-managed.localhost > > > > There's no provision for adding that in here. I'd have to figure out how to do > > that on the .cc file side. > > > > Is that worth? > > I don't know how difficult it would be, so I'll defer to your judgment. :) It's > fine as is if making the change is complicated. Okay I will investigate on Monday. It would be nice to have a test. Have a nice weekend.
On 2016/08/20 01:49:55, tommycli wrote: > On 2016/08/20 01:43:06, Devlin wrote: > > On 2016/08/20 01:35:57, tommycli wrote: > > > > > > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > > > File chrome/test/data/extensions/api_test/users_private/test.js (right): > > > > > > > > > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > > > chrome/test/data/extensions/api_test/users_private/test.js:18: var kName1 = > > > kEmail1; > > > On 2016/08/20 01:23:04, Devlin wrote: > > > > it'd probably be good to make this something different so that we'd catch > a > > > bug > > > > if we *were* returning the same value for the name and email. > > > > > > So... they would only be different for locally managed users with an email > > that > > > looks like: mailto:0@locally-managed.localhost > > > > > > There's no provision for adding that in here. I'd have to figure out how to > do > > > that on the .cc file side. > > > > > > Is that worth? > > > > I don't know how difficult it would be, so I'll defer to your judgment. :) > It's > > fine as is if making the change is complicated. > > Okay I will investigate on Monday. It would be nice to have a test. > > Have a nice weekend. To followup: I spent some time investigating, but it's not obvious how to test that local-only supervised-user test case within apitest.cc. I'm going to send it in and manually verify on my Chromebook once it hits Canary. Thank you!
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/22 18:12:35, tommycli wrote: > On 2016/08/20 01:49:55, tommycli wrote: > > On 2016/08/20 01:43:06, Devlin wrote: > > > On 2016/08/20 01:35:57, tommycli wrote: > > > > > > > > > > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > > > > File chrome/test/data/extensions/api_test/users_private/test.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2259943003/diff/40001/chrome/test/data/extens... > > > > chrome/test/data/extensions/api_test/users_private/test.js:18: var kName1 > = > > > > kEmail1; > > > > On 2016/08/20 01:23:04, Devlin wrote: > > > > > it'd probably be good to make this something different so that we'd > catch > > a > > > > bug > > > > > if we *were* returning the same value for the name and email. > > > > > > > > So... they would only be different for locally managed users with an email > > > that > > > > looks like: mailto:0@locally-managed.localhost > > > > > > > > There's no provision for adding that in here. I'd have to figure out how > to > > do > > > > that on the .cc file side. > > > > > > > > Is that worth? > > > > > > I don't know how difficult it would be, so I'll defer to your judgment. :) > > It's > > > fine as is if making the change is complicated. > > > > Okay I will investigate on Monday. It would be nice to have a test. > > > > Have a nice weekend. > > To followup: I spent some time investigating, but it's not obvious how to test > that local-only supervised-user test case within apitest.cc. I'm going to send > it in and manually verify on my Chromebook once it hits Canary. > > Thank you! Thanks for looking into it. :)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Settings People CrOS: Display supervised users' names correctly. This fixes usernames displayed as: 0@locally-managed.localhost in the ChromeOS User List section of Settings. BUG=614583 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings People CrOS: Display supervised users' names correctly. This fixes usernames displayed as: 0@locally-managed.localhost in the ChromeOS User List section of Settings. BUG=614583 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/978d57269a979053a57da3c1fa799eb4ad541cf0 Cr-Commit-Position: refs/heads/master@{#413567} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/978d57269a979053a57da3c1fa799eb4ad541cf0 Cr-Commit-Position: refs/heads/master@{#413567} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
