Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12539002:
Hide the add new profile link for managed users. (Closed)
|
Created:
7 years, 9 months ago by Adrian Kuegel Modified:
7 years, 8 months ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHide the add new profile link for managed users.
Do not show the add new profile link if ShowAddNewProfileLink returns false.
Screenshot for a managed user:
https://docs.google.com/a/google.com/file/d/0BzMCIwDgeNezRG9ZLVVucW9MSDQ/edit?usp=sharing
Screenshot for a normal user:
https://docs.google.com/a/google.com/file/d/0BzMCIwDgeNezZDRWdlFxUzNJOTA/edit?usp=sharing
BUG=228736
TEST=Manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194872
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove some vertical space when add profile link is hidden. #
Total comments: 5
Patch Set 3 : Change space at the bottom of the menu to 7 pixels. #Patch Set 4 : Fix problem with unit test. #
Messages
Total messages: 20 (0 generated)
Bernhard, can you please take a look at my change and possibly try it out?
This needs screenshots and tests.
@sail: This was not intended for a real review yet. I have no mac machine, so I even didn't have the possibility to compile it myself, so I asked Bernhard if he could try it out.
On 2013/03/06 15:06:36, Adrian Kuegel wrote: > @sail: This was not intended for a real review yet. I have no mac machine, so I > even didn't have the possibility to compile it myself, so I asked Bernhard if he > could try it out. Ahh, sounds good!
Screenshots uploaded at http://www.dropmocks.com/mBpogq. https://codereview.chromium.org/12539002/diff/1/chrome/browser/ui/cocoa/brows... File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/12539002/diff/1/chrome/browser/ui/cocoa/brows... chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:130: CGFloat yOffset = kLinkSpacing; If we're in pixel-pushing territory yet, I'd decrease the space at the bottom so it matches the space at the top (compare the first picture to the second).
@Bernhard: Maybe my new patch fixed it, but I am not sure. Could you please try it again? https://codereview.chromium.org/12539002/diff/1/chrome/browser/ui/cocoa/brows... File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/12539002/diff/1/chrome/browser/ui/cocoa/brows... chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:130: CGFloat yOffset = kLinkSpacing; Yes, this is something I missed because I couldn't try it out. Looking closely at the code, I think the right thing to do is to start with a yOffset of kLinkSpacing - kVerticalSpacing in case the link is not displayed, because it looks like there is a spacing of kVerticalSpacing for each profile entry, anyway.
https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:144: } else If one branch has braces, the other one should too. https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:145: yOffset -= kVerticalSpacing; This results in an offset of 5 pixels, but if you look at the screenshots, the space at the top is 7 pixels.
https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:144: } else On 2013/03/07 09:34:52, Bernhard Bauer wrote: > If one branch has braces, the other one should too. Done. https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:145: yOffset -= kVerticalSpacing; I guess I should have used gimp to see this, or which tool would you recommend? Anyway, it would be interesting to see how it looks like for profiles of non-managed users, how much space there is at the bottom.
Sorry this got left behind. Feel free to ping me if I don't respond to code review. https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right): https://codereview.chromium.org/12539002/diff/5001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:145: yOffset -= kVerticalSpacing; On 2013/03/07 09:45:46, Adrian Kuegel wrote: > I guess I should have used gimp to see this, or which tool would you recommend? I used Preview.app :-D > Anyway, it would be interesting to see how it looks like for profiles of > non-managed users, how much space there is at the bottom. See the third screenshot at http://www.dropmocks.com/mBpogq. The new user button doesn't light up, therefore it's a bit harder to tell what is button and what is space.
On 2013/04/09 11:41:59, Bernhard Bauer wrote: > Sorry this got left behind. Feel free to ping me if I don't respond to code > review. Since it was not an urgent issue, I didn't want to bother you. > On 2013/03/07 09:45:46, Adrian Kuegel wrote: > > I guess I should have used gimp to see this, or which tool would you > recommend? > > I used Preview.app :-D Which only runs on mac, right? Anyway, I used gimp now to look at the screenshots myself, and saw that for non-managed profiles there is no extra space between the horizontal bar over the "New user" menu item and the bottom profile. So this means we either should not have any extra space, or use the same 7 pixels as on the top. I will keep the 7 pixels for now. Could you please create updated screenshots for managed user profiles?
On 2013/04/09 13:19:07, Adrian Kuegel wrote: > On 2013/04/09 11:41:59, Bernhard Bauer wrote: > > Sorry this got left behind. Feel free to ping me if I don't respond to code > > review. > > Since it was not an urgent issue, I didn't want to bother you. > > > On 2013/03/07 09:45:46, Adrian Kuegel wrote: > > > I guess I should have used gimp to see this, or which tool would you > > recommend? > > > > I used Preview.app :-D > > Which only runs on mac, right? Anyway, I used gimp now to look at the > screenshots myself, and saw that for non-managed profiles there is no extra > space between the horizontal bar over the "New user" menu item and the bottom > profile. So this means we either should not have any extra space, or use the > same 7 pixels as on the top. I will keep the 7 pixels for now. Could you please > create updated screenshots for managed user profiles? Done, number 4 on http://www.dropmocks.com/mBpogq. Also, LGTM now!
@Sail: can you please review this changelist? I currently use 7 pixels whitespace below the bottom profile if the "New User" link is not shown, which matches the amount of whitespace at the top.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/12539002/10001
Retried try job too often on mac_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
@Sail: could you please take another look? I had to change the ShouldShowAddNewProfile method because it was making some tests fail. Using the last used profile if |browser_| is NULL was probably a bad idea, anyway.
@Sail: is it ok for you if I land this with the small change in the ShouldShowAddNewProfile function, which is needed to make the tests pass?
On 2013/04/16 14:09:48, Adrian Kuegel wrote: > @Sail: is it ok for you if I land this with the small change in the > ShouldShowAddNewProfile function, which is needed to make the tests pass? LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akuegel@chromium.org/12539002/35001
Message was sent while issue was closed.
Change committed as 194872 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
