Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
303463010:
[Mac] Show a warning in the new avatar button/menu if there's an auth error (Closed)
|
Created:
6 years, 6 months ago by noms (inactive) Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] Show a warning in the new avatar button/menu if there's an auth error.
This error is shown both in the avatar button, and in the avatar menu. When the
account for which we're displaying the auth error is clicked in the avatar menu,
we show the Gaia reauth view.
Screenshot:
https://drive.google.com/file/d/0B1B1Up4p2NRMQW90VmcwcGZQVEU/edit?usp=sharing
I've also fixed a bug where the available text for the account name was
incorrectly calculated, and eliding overlapped the delete button. Screenshot:
https://drive.google.com/file/d/0B1B1Up4p2NRMaU9DcWdqTVZNMVk/edit?usp=sharing
BUG=311235
TEST= Sign into Chrome with the --new-profile-management flag on. Go to
accounts.google.com and revoke Chrome's access to that account, from the
Security tab. Try to bookmark the current page. The avatar button should be
badged like in the attached screenshot.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277908
Patch Set 1 : #
Total comments: 36
Patch Set 2 : msw comments #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : fix warning icon location and account eliding #
Total comments: 29
Patch Set 6 : rachel comments and a sneaked in rebase (the IsSupervised() stuff) #
Total comments: 11
Patch Set 7 : rachel nits #Patch Set 8 : rachel nits #
Messages
Total messages: 22 (0 generated)
Hi Mike, Here is the Cocoa implementation of the auth errors, since you reviewed them on Windows as well. I've gone with the resizing-the-larger image implementation from the start. Please take a look. Thanks!
I'm not a Cocoa pro; for changes more complex than this, I'd appreciated first pass review by someone that works in Cocoa more often than myself. I can figure it all out, but I might miss some nuances. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:36: - (void)updateErrorStatus:(BOOL)hasError; nit: consider a more descriptive name or add a comment. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:52: if (error) nit: Can this generally ever be NIL? (remove if statements everywhere otherwise) https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:60: if(error) nit: space after if https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:90: [avatarButton_ updateErrorStatus:error->HasError()]; When won't error exist? Should the button be updated then? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (left): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:195: // TODO(noms): Figure out something similar for themed windows, if possible. Why is this no more applicable? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:83: CGFloat errorImageWidth = hasError_ ? kAuthErrorIconWidth : 0; nit: errorWidth or imageWidth for a one-liner below. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:182: [cell setHasError:error->HasError()]; ditto nit: should the button state be set if |error| is NIL? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:233: bool HasAuthError(Profile* profile) { nit: this is used twice, is a helper necessary? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:239: std::string GetAuthErrorAccountId(Profile* profile) { nit: this is used once, please inline it. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:243: if (!error) nit: return error ? error->error_account_id() : std::string() if you keep this. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:249: std::string GetAuthErrorUsername(Profile* profile) { nit: this is used once, please inline it. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:252: if (!error) nit: return error ? error->error_username() : std::string() if you keep this. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1524: // Get state of authentication error, if any. nit: this comment should be made more helpful or removed. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1535: reauthRequired:errorAccountId == accounts[i]]; Couldn't multiple accounts have auth errors at the same time? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1558: int messageId; nit: initialize this value https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1572: case profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH: { nit: you shouldn't need these curly braces. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1803: reauthImageWidth = [reauthImage size].width; Do you need to add some padding, like kHorizontalSpacing, here or below?
Mike: Ah, sorry, I had seen some reviews go by on my team where you did cocoa reviews, and I (incorrectly) assumed you were an owner. My bad! Rachel: Would you mind doing a cocoa review, please? Thanks! https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:36: - (void)updateErrorStatus:(BOOL)hasError; On 2014/06/03 04:15:31, msw wrote: > nit: consider a more descriptive name or add a comment. Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:52: if (error) Yes. The SigninErrorController can be null for Guest or Incognito profiles. On 2014/06/03 04:15:31, msw wrote: > nit: Can this generally ever be NIL? (remove if statements everywhere otherwise) https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:60: if(error) On 2014/06/03 04:15:31, msw wrote: > nit: space after if Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:90: [avatarButton_ updateErrorStatus:error->HasError()]; |error| can be null for guest or incognito profiles. Since the profile can't change types, |error| will always be null or not-null for the same profile. Now, |error->HasError()| can change as you encounter/clear auth errors, in which case we need to remove the notification. OnErrorChanged() will be called for both error->no error and no error->error transitions, and the icon needs to change around that. On 2014/06/03 04:15:31, msw wrote: > When won't error exist? Should the button be updated then? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (left): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:195: // TODO(noms): Figure out something similar for themed windows, if possible. Left over todo that never got updated :) The code below deals with themed windows (see the else branch) On 2014/06/03 04:15:31, msw wrote: > Why is this no more applicable? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:83: CGFloat errorImageWidth = hasError_ ? kAuthErrorIconWidth : 0; On 2014/06/03 04:15:31, msw wrote: > nit: errorWidth or imageWidth for a one-liner below. Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:182: [cell setHasError:error->HasError()]; As mentioned before, |error| can't be nil once, and non-nil afterwards, so there won't be a chance that you've displayed a notification and never get to clear it. On 2014/06/03 04:15:31, msw wrote: > ditto nit: should the button state be set if |error| is NIL? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:233: bool HasAuthError(Profile* profile) { I thought it looked a little nicer, because the three lines of code look a little annoying, but I can inline it if you want to. On 2014/06/03 04:15:31, msw wrote: > nit: this is used twice, is a helper necessary? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:239: std::string GetAuthErrorAccountId(Profile* profile) { On 2014/06/03 04:15:31, msw wrote: > nit: this is used once, please inline it. Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:243: if (!error) On 2014/06/03 04:15:31, msw wrote: > nit: return error ? error->error_account_id() : std::string() if you keep this. Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:249: std::string GetAuthErrorUsername(Profile* profile) { On 2014/06/03 04:15:31, msw wrote: > nit: this is used once, please inline it. Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:252: if (!error) On 2014/06/03 04:15:31, msw wrote: > nit: return error ? error->error_username() : std::string() if you keep this. Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1524: // Get state of authentication error, if any. On 2014/06/03 04:15:31, msw wrote: > nit: this comment should be made more helpful or removed. Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1535: reauthRequired:errorAccountId == accounts[i]]; They could, but the way it works right now is that the SigninErrorController will only return one error at a time. Once you resolve that, you might get another error, for a different account. On 2014/06/03 04:15:31, msw wrote: > Couldn't multiple accounts have auth errors at the same time? https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1558: int messageId; On 2014/06/03 04:15:31, msw wrote: > nit: initialize this value Done. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1572: case profiles::BUBBLE_VIEW_MODE_GAIA_REAUTH: { Without the curly braces, the compiler is unhappy with defining the SigninErrorController in only one of the cases, since all the cases are in the same scope. I've moved the definition to outside the switch. https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1803: reauthImageWidth = [reauthImage size].width; On 2014/06/03 04:15:31, msw wrote: > Do you need to add some padding, like kHorizontalSpacing, here or below? Done.
https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1535: reauthRequired:errorAccountId == accounts[i]]; On 2014/06/06 20:33:44, Monica Dinculescu wrote: > They could, but the way it works right now is that the SigninErrorController > will only return one error at a time. Once you resolve that, you might get > another error, for a different account. > On 2014/06/03 04:15:31, msw wrote: > > Couldn't multiple accounts have auth errors at the same time? > It's also unclear to me how this retains the authorization error for account/button A if another error appears for account/button B. Does SigninErrorController only express an authorization error for one account at a time, and then express subsequent account errors once the first error has been resolved? (if so, why?). Can you add a comment explaining the pattern here, it's definitely not obvious.
Rachel: ping! I've also updated the position of the warning icon since I last pinged you, but i don't think you had a chance to look at it :) Mike: tried to answer the error badging question. Let me know if it makes more sense... https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/303463010/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1535: reauthRequired:errorAccountId == accounts[i]]; A little background: this error controller used to be used in the hot dog menu, where we could only badge the menu for one error at a time. So the controller sort of queues up errors. If you're badging account A with an error, and account B also has an error, the controller will only care about the account A error until it gets resolved. Once it gets resolved, it goes and looks for any other errors. This works the same in the Windows menu too. In theory, now that we *could* display multiple errors in parallel, the controller could change, but that's a bit of a giant yak shave that I don't thing belongs in this CL. :) On 2014/06/09 19:02:38, msw wrote: > On 2014/06/06 20:33:44, Monica Dinculescu wrote: > > They could, but the way it works right now is that the SigninErrorController > > will only return one error at a time. Once you resolve that, you might get > > another error, for a different account. > > On 2014/06/03 04:15:31, msw wrote: > > > Couldn't multiple accounts have auth errors at the same time? > > > > It's also unclear to me how this retains the authorization error for > account/button A if another error appears for account/button B. Does > SigninErrorController only express an authorization error for one account at a > time, and then express subsequent account errors once the first error has been > resolved? (if so, why?). Can you add a comment explaining the pattern here, it's > definitely not obvious.
lgtm with a nit. Review from a Cocoa expert would still be nice. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1811: CGFloat availableTextWidth = rect.size.width - 2 * kHorizontalSpacing - nit: should the available text width only include one kHorizontalSpacing if warningImage will not be shown?
Incoming. Was OOO Fri/Mon
https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:58: base::scoped_nsobject<NSImage> authErrorImage_; authenticationErrorImage_, please. (This is Cocoa. Verbosity is a must :) https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:71: gfx::ImageSkia icon = gfx::ImageSkiaOperations::CreateResizedImage( Why are we resizing a static resource? Can we instead just check in a properly sized one? https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:106: frame.origin.x - imageSize.width - kButtonTitleImageSpacing, Am I misreading this, or are you planning to draw outside the frame given to you in drawImage? (In which case, eeew! ;) It'd probably cleaner to draw the error icon in -drawInteriorWithFrame:, and then call super's -drawInterior with a reduced frame. It also will consolidate the handling of the spacing between errorimage and title. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:144: - (void)setHasError:(BOOL)hasError { Assuming that having an error on an account is rare, I'd suggest getting rid of hasError_ and just resetting authErrorImage_ appropriately. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:181: [cell setHasError:error->HasError()]; I'd actually remove this - it gets rid of the dependency on the SigninErrorController. Either have callers call -updateErrorStatus: immediately after -initWithBrowser:, or if you don't trust the caller, pass in as a param. (I'd trust the caller :) https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h:111: - (IBAction)showAccountReauthView:(id)sender; showAccountReauthenticationView, please https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:232: const SigninErrorController* error = error_controller, please - here and elsewhere. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1530: // badged with a warning icon. Question: Is there ever a chance of multiple accounts with an auth error? https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1591: return NULL; use break instead of NULL, or kill NOTREACHED. (Style guide says either handle errors or DCHECK/NOTREACHED, but not both. In fact, if this handles all viewModes, you can kill the default - that way, clang will warn if a new enum is added that would lead to an unhandled case. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1809: CGFloat warningImageWidth = reauthRequired ? [warningImage size].width : 0; Technically, no need to check - just [warningImage size].width. [nil size] returns an NSRect initialized to 0 (See https://developer.apple.com/library/mac/documentation/cocoa/conceptual/Progra... "If you expect a return value from a message sent to nil, the return value will be nil for object return types, 0 for numeric types, and NO for BOOL types. Returned structures have all members initialized to zero.") Objective-C is awesome :) https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1832: rect.origin = NSMakePoint( Easier: NSRect buttonRect; NSDivideRect(rect, &buttonRect, &rect, deleteImageWidth, NSMaxXEdge); This slices off a rect of |deleteImageWidth| at the MaxXEdge, i.e. the right-hand side, stores it in buttonRect, and stuffs the remainder in rect. Less math is good math :)
https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:71: gfx::ImageSkia icon = gfx::ImageSkiaOperations::CreateResizedImage( Mike didn't want me to check in duplicate, just resized, images, so he asked me to do the resizing. You guys can fight it out. I'd prefer the duplicate resource, personally, because a) it's already there and b) the resizing code is scary :) On 2014/06/10 19:28:14, groby wrote: > Why are we resizing a static resource? Can we instead just check in a properly > sized one?
https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:71: gfx::ImageSkia icon = gfx::ImageSkiaOperations::CreateResizedImage( On 2014/06/11 15:12:46, Monica Dinculescu wrote: > Mike didn't want me to check in duplicate, just resized, images, so he asked me > to do the resizing. You guys can fight it out. I'd prefer the duplicate > resource, personally, because a) it's already there and b) the resizing code is > scary :) > > On 2014/06/10 19:28:14, groby wrote: > > Why are we resizing a static resource? Can we instead just check in a properly > > sized one? > Don't block this CL on that, you can use the duplicate resource, I'm just sad that we have a dozen nearly identical icons.
https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:71: gfx::ImageSkia icon = gfx::ImageSkiaOperations::CreateResizedImage( On 2014/06/11 16:42:36, msw wrote: > On 2014/06/11 15:12:46, Monica Dinculescu wrote: > > Mike didn't want me to check in duplicate, just resized, images, so he asked > me > > to do the resizing. You guys can fight it out. I'd prefer the duplicate > > resource, personally, because a) it's already there and b) the resizing code > is > > scary :) > > > > On 2014/06/10 19:28:14, groby wrote: > > > Why are we resizing a static resource? Can we instead just check in a > properly > > > sized one? > > > > Don't block this CL on that, you can use the duplicate resource, I'm just sad > that we have a dozen nearly identical icons. Not a blocking issue for me, either - I'm just sad we have code to resize static image resources :) (Mike: Just to make your day, we have them all in multiple densities, too ;)
Feel free to proceed either way for the icon here and in you corresponding Views CL: https://codereview.chromium.org/297143008 and sorry for the trouble. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:71: gfx::ImageSkia icon = gfx::ImageSkiaOperations::CreateResizedImage( On 2014/06/11 17:19:09, groby wrote: > On 2014/06/11 16:42:36, msw wrote: > > On 2014/06/11 15:12:46, Monica Dinculescu wrote: > > > Mike didn't want me to check in duplicate, just resized, images, so he asked > > me > > > to do the resizing. You guys can fight it out. I'd prefer the duplicate > > > resource, personally, because a) it's already there and b) the resizing code > > is > > > scary :) > > > > > > On 2014/06/10 19:28:14, groby wrote: > > > > Why are we resizing a static resource? Can we instead just check in a > > properly > > > > sized one? > > > > > > > Don't block this CL on that, you can use the duplicate resource, I'm just sad > > that we have a dozen nearly identical icons. > > Not a blocking issue for me, either - I'm just sad we have code to resize static > image resources :) > > (Mike: Just to make your day, we have them all in multiple densities, too ;) > My concern wasn't entirely about binary size, we have inconsistent iconography. See http://crbug.com/378467: Chrome has too many distinct warning icons.
Woohoo! Now that the other CL of death is done with, I've addressed all-but-one of the comments here (one for which I left a question in), hopefully in an actually correct way! Rachel, please take a look :) https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:58: base::scoped_nsobject<NSImage> authErrorImage_; On 2014/06/10 19:28:14, groby wrote: > authenticationErrorImage_, please. (This is Cocoa. Verbosity is a must :) Done. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:71: gfx::ImageSkia icon = gfx::ImageSkiaOperations::CreateResizedImage( On 2014/06/11 17:33:59, msw wrote: > On 2014/06/11 17:19:09, groby wrote: > > On 2014/06/11 16:42:36, msw wrote: > > > On 2014/06/11 15:12:46, Monica Dinculescu wrote: > > > > Mike didn't want me to check in duplicate, just resized, images, so he > asked > > > me > > > > to do the resizing. You guys can fight it out. I'd prefer the duplicate > > > > resource, personally, because a) it's already there and b) the resizing > code > > > is > > > > scary :) > > > > > > > > On 2014/06/10 19:28:14, groby wrote: > > > > > Why are we resizing a static resource? Can we instead just check in a > > > properly > > > > > sized one? > > > > > > > > > > Don't block this CL on that, you can use the duplicate resource, I'm just > sad > > > that we have a dozen nearly identical icons. > > > > Not a blocking issue for me, either - I'm just sad we have code to resize > static > > image resources :) > > > > (Mike: Just to make your day, we have them all in multiple densities, too ;) > > > > My concern wasn't entirely about binary size, we have inconsistent iconography. > See http://crbug.com/378467: Chrome has too many distinct warning icons. Done. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:106: frame.origin.x - imageSize.width - kButtonTitleImageSpacing, Hmm, so the layout of the button is: [padding][title][spacing][optional error icon][optional spacing][drop down arrow]. I was planning (and succeeding) on committing the "draw outside a frame sin" , so I moved the drawing of the icon in -drawTitle though, so that it's being drawn inside a given frame. Does it look ok? On 2014/06/10 19:28:14, groby wrote: > Am I misreading this, or are you planning to draw outside the frame given to you > in drawImage? (In which case, eeew! ;) > > It'd probably cleaner to draw the error icon in -drawInteriorWithFrame:, and > then call super's -drawInterior with a reduced frame. It also will consolidate > the handling of the spacing between errorimage and title. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:144: - (void)setHasError:(BOOL)hasError { On 2014/06/10 19:28:14, groby wrote: > Assuming that having an error on an account is rare, I'd suggest getting rid of > hasError_ and just resetting authErrorImage_ appropriately. Done. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:181: [cell setHasError:error->HasError()]; Hmm, but then it adds that dependency to the BrowserWindowController. Isn't that worse? On 2014/06/10 19:28:14, groby wrote: > I'd actually remove this - it gets rid of the dependency on the > SigninErrorController. > > Either have callers call -updateErrorStatus: immediately after > -initWithBrowser:, or if you don't trust the caller, pass in as a param. (I'd > trust the caller :) https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h:111: - (IBAction)showAccountReauthView:(id)sender; On 2014/06/10 19:28:14, groby wrote: > showAccountReauthenticationView, please Done. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:232: const SigninErrorController* error = On 2014/06/10 19:28:14, groby wrote: > error_controller, please - here and elsewhere. Done. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1530: // badged with a warning icon. Nope. Mike asked the same question on a previous patch. The tl; dr is: this is the same ErrorController that was used on the hot dog menu, so it can only badge the menu for one error at a time. If there's multiple accounts that have errors, only the first one is returned. Once that's fixed, you get another error and so on. On 2014/06/10 19:28:15, groby wrote: > Question: Is there ever a chance of multiple accounts with an auth error? https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1591: return NULL; On 2014/06/10 19:28:14, groby wrote: > use break instead of NULL, or kill NOTREACHED. (Style guide says either handle > errors or DCHECK/NOTREACHED, but not both. > > In fact, if this handles all viewModes, you can kill the default - that way, > clang will warn if a new enum is added that would lead to an unhandled case. Done. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1809: CGFloat warningImageWidth = reauthRequired ? [warningImage size].width : 0; Oh god. Ok. I see. Done. On 2014/06/10 19:28:14, groby wrote: > Technically, no need to check - just [warningImage size].width. [nil size] > returns an NSRect initialized to 0 > > (See > https://developer.apple.com/library/mac/documentation/cocoa/conceptual/Progra... > > > "If you expect a return value from a message sent to nil, the return value will > be nil for object return types, 0 for numeric types, and NO for BOOL types. > Returned structures have all members initialized to zero.") > > Objective-C is awesome :) https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1811: CGFloat availableTextWidth = rect.size.width - 2 * kHorizontalSpacing - On 2014/06/10 17:13:03, msw wrote: > nit: should the available text width only include one kHorizontalSpacing if > warningImage will not be shown? Done. https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1832: rect.origin = NSMakePoint( On 2014/06/10 19:28:15, groby wrote: > Easier: > NSRect buttonRect; > NSDivideRect(rect, &buttonRect, &rect, deleteImageWidth, NSMaxXEdge); > > This slices off a rect of |deleteImageWidth| at the MaxXEdge, i.e. the > right-hand side, stores it in buttonRect, and stuffs the remainder in rect. > > Less math is good math :) Done.
LGTM w/ nits https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right): https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:36: - (void)updateAvatarButtonAndLayoutParent:(BOOL)layoutParent; nit: Please add a space between declaration and the comment for the next one. https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:46: AvatarBaseController* avatarButton) nit: avatarController https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:54: SigninErrorController* error = profiles::GetSigninErrorController(profile_); nit:errorController https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:144: authenticationErrorImage_.reset(nil); nit: can skip the nil
https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/220001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:106: frame.origin.x - imageSize.width - kButtonTitleImageSpacing, On 2014/06/17 17:07:32, Monica Dinculescu wrote: > Hmm, so the layout of the button is: > [padding][title][spacing][optional error icon][optional spacing][drop down > arrow]. > > I was planning (and succeeding) on committing the "draw outside a frame sin" , > so I moved the drawing of the icon in -drawTitle though, so that it's being > drawn inside > a given frame. Does it look ok? > It looked at first glance... let me recheck > On 2014/06/10 19:28:14, groby wrote: > > Am I misreading this, or are you planning to draw outside the frame given to > you > > in drawImage? (In which case, eeew! ;) > > > > It'd probably cleaner to draw the error icon in -drawInteriorWithFrame:, and > > then call super's -drawInterior with a reduced frame. It also will consolidate > > the handling of the spacing between errorimage and title. > https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:103: // button's drop down image. It does indeed, provided you never size smaller than cellSize. But then, all bets are off anyways :)
https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right): https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:36: - (void)updateAvatarButtonAndLayoutParent:(BOOL)layoutParent; On 2014/06/17 17:13:27, groby wrote: > nit: Please add a space between declaration and the comment for the next one. Done. https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:46: AvatarBaseController* avatarButton) On 2014/06/17 17:13:27, groby wrote: > nit: avatarController Done. https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:54: SigninErrorController* error = profiles::GetSigninErrorController(profile_); On 2014/06/17 17:13:26, groby wrote: > nit:errorController Done. https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:144: authenticationErrorImage_.reset(nil); Hmm, if we skip the nil then we can't clear the error (i.e. you have an auth error, you resolve you, you need to un-display it) On 2014/06/17 17:13:27, groby wrote: > nit: can skip the nil
https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:144: authenticationErrorImage_.reset(nil); On 2014/06/17 18:12:44, Monica Dinculescu wrote: > Hmm, if we skip the nil then we can't clear the error (i.e. you have an auth > error, you resolve you, you need to un-display it) > On 2014/06/17 17:13:27, groby wrote: > > nit: can skip the nil > Maybe I was too terse :) What I meant is that just authenticationErrorImage_.reset() is fine.
https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right): https://codereview.chromium.org/303463010/diff/260001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:144: authenticationErrorImage_.reset(nil); OH, herp derp. I think I'm losing my mind. Done. On 2014/06/17 18:20:26, groby wrote: > On 2014/06/17 18:12:44, Monica Dinculescu wrote: > > Hmm, if we skip the nil then we can't clear the error (i.e. you have an auth > > error, you resolve you, you need to un-display it) > > On 2014/06/17 17:13:27, groby wrote: > > > nit: can skip the nil > > > > Maybe I was too terse :) What I meant is that just > authenticationErrorImage_.reset() is fine.
> OH, herp derp. I think I'm losing my mind. Common side effect of too much ObjectiveC++ :) SLGTM.
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/303463010/300001
Message was sent while issue was closed.
Change committed as 277908 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
