Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1833383002:
Correctly layout the profile name button, and disable the button listener for supervised user. (Closed)
|
Created:
4 years, 9 months ago by Shu Chen Modified:
4 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCorrectly layout the profile name button, and disable the button listener for supervised user.
BUG=596423
Committed: https://crrev.com/73b0c4641caccf9783eb2d5b09b4019b92204343
Cr-Commit-Position: refs/heads/master@{#383674}
Patch Set 1 #
Total comments: 8
Patch Set 2 : uses views::Label for supervised user. #
Total comments: 2
Patch Set 3 : nit. #Messages
Total messages: 16 (6 generated)
shuchen@chromium.org changed reviewers: + pkasting@chromium.org
Peter, can you please review this? Thanks. Sorry I didn't successfully verify the previous cl on my local build, because I cannot setup supervised user successfully on my local build. Now I've locally revised the code to run through the condition |is_editing_allowed|==false, and verified this cl can correctly make the profile name button correctly shown.
The CQ bit was checked by shuchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833383002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833383002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:396: new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0)); Nit: Add blank line below https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:403: if (!is_editing_allowed) { Nit: Use member, not param, name https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:435: Nit: Remove this line https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:477: bool is_editing_allowed_; Hmm. Maybe instead of adding this, we should use a label rather than a button for the case where we don't want the "button" to look or act like a button.
https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:396: new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0)); On 2016/03/28 04:17:13, Peter Kasting wrote: > Nit: Add blank line below Done. https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:403: if (!is_editing_allowed) { On 2016/03/28 04:17:13, Peter Kasting wrote: > Nit: Use member, not param, name Done. https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:435: On 2016/03/28 04:17:14, Peter Kasting wrote: > Nit: Remove this line Done. https://codereview.chromium.org/1833383002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:477: bool is_editing_allowed_; On 2016/03/28 04:17:13, Peter Kasting wrote: > Hmm. Maybe instead of adding this, we should use a label rather than a button > for the case where we don't want the "button" to look or act like a button. Done.
LGTM https://codereview.chromium.org/1833383002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1833383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:474: views::Label* label_; Nit: I suggest giving these members comments noting what they are for/when they're visible.
https://codereview.chromium.org/1833383002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1833383002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:474: views::Label* label_; On 2016/03/29 02:30:41, Peter Kasting wrote: > Nit: I suggest giving these members comments noting what they are for/when > they're visible. Done.
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1833383002/#ps40001 (title: "nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833383002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833383002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Correctly layout the profile name button, and disable the button listener for supervised user. BUG=596423 ========== to ========== Correctly layout the profile name button, and disable the button listener for supervised user. BUG=596423 Committed: https://crrev.com/73b0c4641caccf9783eb2d5b09b4019b92204343 Cr-Commit-Position: refs/heads/master@{#383674} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/73b0c4641caccf9783eb2d5b09b4019b92204343 Cr-Commit-Position: refs/heads/master@{#383674} |
