Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
297143008:
[Win] UI fixes for the new avatar button/bubble auth errors. (Closed)
|
Created:
6 years, 7 months ago by noms (inactive) Modified:
6 years, 6 months ago Reviewers:
msw CC:
chromium-reviews, tfarina, Elliot Glaysher Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Win] UI fixes for the new avatar button/bubble auth errors.
Avatar bubble:
Screenshots:
https://drive.google.com/file/d/0B1B1Up4p2NRMT2xPMnp3dEJvUms/edit?usp=sharing
- the previous code was using a temporary warning icon (while we were waiting
on UI resources), so this CL updates both the avatar button and bubble to
use the correct one
- only the warning icon was clickable for accounts with an auth error in the
avatar bubble; changed this to have the entire account name be clickable, after
chatting with Travis/Alan
- moved the icon to in front of the account name, after
conversations with UI
- fixed eliding for the account button (which wasn't missing
one of the horizontal edge margins). Screenshot:
https://drive.google.com/file/d/0B1B1Up4p2NRMczBVQ29IRlREcG8/edit?usp=sharing
Avatar button:
Screenshots:
https://drive.google.com/file/d/0B1B1Up4p2NRMZnJsV3hlZTFjcGM/edit?usp=sharing
- fixes a bug where the avatar button text for glass windows was
displayed incorrectly when the window was maximized.
BUG=311235, 381713
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278814
Patch Set 1 #
Total comments: 7
Patch Set 2 : resize icon instead of duplicating. a rebase sneaked in here :( #
Total comments: 6
Patch Set 3 : rebase #Patch Set 4 : fix avatar buttons + redo the bubble warning icon #
Total comments: 6
Patch Set 5 : rebase #Patch Set 6 : undid auth image resizing + fixed glass button positioning #
Total comments: 12
Patch Set 7 : rebase #Patch Set 8 : msw nits #
Total comments: 2
Patch Set 9 : final nit weeeeee #Patch Set 10 : rebase #
Messages
Total messages: 38 (0 generated)
Hi Mike, I've fixed the UI for the auth warnings in the new avatar menu/bubble. Please take a look! Thanks. The 3px bottom inset was chosen by visual inspection. I hope that's ok.
https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:29: const int kBottomInset = 3; I'm not sure this change does the right thing. In the images you posted with the CL description, the 'j' in "ninja" looks too low, especially on the normal (restored) window's button (I sent you a pic in e-mail). Also, the glass shading of the button doesn't match the other caption buttons (minimize, restore, close). Also, the alignment of the content with that glass shading is inconsistent between normal (restored) and maximized. Can you address these issues (perhaps separately if they weren't changed at all by this CL?) https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:173: IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR).ToImageSkia(); nit: We have many similar warning icons: update_failed.png pageinfo_warning_major.png infobar_warning.png favicon_conflicts.png avatar_button_auth_error.png avatar_menu_auth_error.png alert_small.png about_conflicts.png Other related warning images: pageinfo_warning_minor.png omnibox_https_warning.png reset_warning.png And other related info images: pageinfo_info.png notification_alert.png input_alert_menu.png input_alert.png info_small.png Any chance you could try consolidating some? (perhaps remove avatar_button_auth_error.png and use a down-sized avatar_menu_auth_error.png).
https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:29: const int kBottomInset = 3; The misalignment of the drop-down arrow and the shading seem to have been around since the button's been around, and now i'm super sad I didn't catch them earlier. I might as well fix them in this CL. On 2014/05/28 19:34:58, msw wrote: > I'm not sure this change does the right thing. In the images you posted with the > CL description, the 'j' in "ninja" looks too low, especially on the normal > (restored) window's button (I sent you a pic in e-mail). Also, the glass shading > of the button doesn't match the other caption buttons (minimize, restore, > close). Also, the alignment of the content with that glass shading is > inconsistent between normal (restored) and maximized. Can you address these > issues (perhaps separately if they weren't changed at all by this CL?) https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:173: IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR).ToImageSkia(); Oh my. So I was just given the new avatar_button_auth resources. This makes me want to sit down with a designer and go through all of the resources in Chrome, but I think that's a different issue. However, I annotated your list with what the icons look like, to illustrate there's at most one I can reuse. I was originally going to resize the avatar_button_auth_error, but since I was given the smaller resource, I thought it would be easier to just use that (there's a Mac CL using it coming your way soon too). Would you prefer I just resized it? update_failed.png — yellow exclamation mark pageinfo_warning_major.png - yellow exclamation mark infobar_warning.png - grey warning favicon_conflicts.png - grey warning avatar_button_auth_error.png avatar_menu_auth_error.png alert_small.png - potentially good yellow warning sign, but looks less flat about_conflicts.png - blue warning Other related warning images: pageinfo_warning_minor.png - lock with a warning icon omnibox_https_warning.png - lock with a yellow warning reset_warning.png - exclamation mark And other related info images: pageinfo_info.png - blue i icon notification_alert.png - little exclamation mark in a giant yellow square input_alert_menu.png - white exclamation mark in a yellow square input_alert.png - same as above but a different size info_small.png - blue i icon in a blue square On 2014/05/28 19:34:58, msw wrote: > nit: We have many similar warning icons: > update_failed.png > pageinfo_warning_major.png > infobar_warning.png > favicon_conflicts.png > avatar_button_auth_error.png > avatar_menu_auth_error.png > alert_small.png > about_conflicts.png > > Other related warning images: > pageinfo_warning_minor.png > omnibox_https_warning.png > reset_warning.png > > And other related info images: > pageinfo_info.png > notification_alert.png > input_alert_menu.png > input_alert.png > info_small.png > > Any chance you could try consolidating some? (perhaps remove > avatar_button_auth_error.png and use a down-sized avatar_menu_auth_error.png).
https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:173: IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR).ToImageSkia(); On 2014/05/28 19:59:27, Monica Dinculescu wrote: > Oh my. So I was just given the new avatar_button_auth resources. This makes me > want to sit down with a designer and go through all of the resources in Chrome, > but I think that's a different issue. > However, I annotated your list with what the icons look like, to illustrate > there's at most one I can reuse. > > I was originally going to resize the avatar_button_auth_error, but since I was > given the smaller resource, I thought it would be easier to just use that > (there's a Mac CL using it coming your way soon too). Would you prefer I just > resized it? > > update_failed.png — yellow exclamation mark > pageinfo_warning_major.png - yellow exclamation mark > infobar_warning.png - grey warning > favicon_conflicts.png - grey warning > avatar_button_auth_error.png > avatar_menu_auth_error.png > alert_small.png - potentially good yellow warning sign, but looks less flat > about_conflicts.png - blue warning > > Other related warning images: > pageinfo_warning_minor.png - lock with a warning icon > omnibox_https_warning.png - lock with a yellow warning > reset_warning.png - exclamation mark > > And other related info images: > pageinfo_info.png - blue i icon > notification_alert.png - little exclamation mark in a giant yellow square > input_alert_menu.png - white exclamation mark in a yellow square > input_alert.png - same as above but a different size > info_small.png - blue i icon in a blue square > > > > On 2014/05/28 19:34:58, msw wrote: > > nit: We have many similar warning icons: > > update_failed.png > > pageinfo_warning_major.png > > infobar_warning.png > > favicon_conflicts.png > > avatar_button_auth_error.png > > avatar_menu_auth_error.png > > alert_small.png > > about_conflicts.png > > > > Other related warning images: > > pageinfo_warning_minor.png > > omnibox_https_warning.png > > reset_warning.png > > > > And other related info images: > > pageinfo_info.png > > notification_alert.png > > input_alert_menu.png > > input_alert.png > > info_small.png > > > > Any chance you could try consolidating some? (perhaps remove > > avatar_button_auth_error.png and use a down-sized avatar_menu_auth_error.png). > If scaling down the larger icon looks okay (and isn't inefficiently scaled on each call to Paint), then i'd do that. Yeah, the icons are a mess, I filed crbug.com/378467: Chrome has too many distinct warning icons.
I've changed the button to resize the normal-sized auth error. If that's ok, I'll remove the resource in a separate CL. Please take a look. https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:29: const int kBottomInset = 3; Actually, I'd rather fix them in a separate CL. That CL will be delayed by a bit, and the auth error looks pretty terrible right now. :( On 2014/05/28 19:59:27, Monica Dinculescu wrote: > The misalignment of the drop-down arrow and the shading seem to have been around > since the button's been around, and now i'm super sad I didn't catch them > earlier. I might as well fix them in this CL. > > On 2014/05/28 19:34:58, msw wrote: > > I'm not sure this change does the right thing. In the images you posted with > the > > CL description, the 'j' in "ninja" looks too low, especially on the normal > > (restored) window's button (I sent you a pic in e-mail). Also, the glass > shading > > of the button doesn't match the other caption buttons (minimize, restore, > > close). Also, the alignment of the content with that glass shading is > > inconsistent between normal (restored) and maximized. Can you address these > > issues (perhaps separately if they weren't changed at all by this CL?) > https://codereview.chromium.org/297143008/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:173: IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR).ToImageSkia(); On 2014/05/28 20:56:27, msw wrote: > On 2014/05/28 19:59:27, Monica Dinculescu wrote: > > Oh my. So I was just given the new avatar_button_auth resources. This makes me > > want to sit down with a designer and go through all of the resources in > Chrome, > > but I think that's a different issue. > > However, I annotated your list with what the icons look like, to illustrate > > there's at most one I can reuse. > > > > I was originally going to resize the avatar_button_auth_error, but since I was > > given the smaller resource, I thought it would be easier to just use that > > (there's a Mac CL using it coming your way soon too). Would you prefer I just > > resized it? > > > > update_failed.png — yellow exclamation mark > > pageinfo_warning_major.png - yellow exclamation mark > > infobar_warning.png - grey warning > > favicon_conflicts.png - grey warning > > avatar_button_auth_error.png > > avatar_menu_auth_error.png > > alert_small.png - potentially good yellow warning sign, but looks less flat > > about_conflicts.png - blue warning > > > > Other related warning images: > > pageinfo_warning_minor.png - lock with a warning icon > > omnibox_https_warning.png - lock with a yellow warning > > reset_warning.png - exclamation mark > > > > And other related info images: > > pageinfo_info.png - blue i icon > > notification_alert.png - little exclamation mark in a giant yellow square > > input_alert_menu.png - white exclamation mark in a yellow square > > input_alert.png - same as above but a different size > > info_small.png - blue i icon in a blue square > > > > > > > > On 2014/05/28 19:34:58, msw wrote: > > > nit: We have many similar warning icons: > > > update_failed.png > > > pageinfo_warning_major.png > > > infobar_warning.png > > > favicon_conflicts.png > > > avatar_button_auth_error.png > > > avatar_menu_auth_error.png > > > alert_small.png > > > about_conflicts.png > > > > > > Other related warning images: > > > pageinfo_warning_minor.png > > > omnibox_https_warning.png > > > reset_warning.png > > > > > > And other related info images: > > > pageinfo_info.png > > > notification_alert.png > > > input_alert_menu.png > > > input_alert.png > > > info_small.png > > > > > > Any chance you could try consolidating some? (perhaps remove > > > avatar_button_auth_error.png and use a down-sized > avatar_menu_auth_error.png). > > > > If scaling down the larger icon looks okay (and isn't inefficiently scaled on > each call to Paint), then i'd do that. Yeah, the icons are a mess, I filed > crbug.com/378467: Chrome has too many distinct warning icons. Done.
https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:30: const int kBottomInset = 3; My biggest concern for this CL is that it's regressing the vertical placement of the text (and icon?) in the button in one of the glass/themed restored/maximized cases. The 'j' in "ninja" still overlaps the inside edge of the button in the glass restored case, which appears to be a regression from my local ToT build @273000. Am I wrong, or is this intended? Are themed 'j's also too low? https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1267: rb->GetImageNamed(IDR_ICON_PROFILES_ACCOUNT_AUTH_ERROR).ToImageSkia() : nit: Are you doing this to have consistent iconography for the profile auth errors? This should be noted in your CL description, and you should probably have a before/after pic on the bug. https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1306: views::ImageView* reauth_icon = new views::ImageView(); So you're changing the email_button to handle clicks related to re-auth? Why is this changing? Can you add more information to your CL description?
Done! I think, most memorably, I fixed the avatar button for ALL the things :) https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:30: const int kBottomInset = 3; On 2014/05/30 18:24:13, msw wrote: > My biggest concern for this CL is that it's regressing the vertical placement of > the text (and icon?) in the button in one of the glass/themed restored/maximized > cases. The 'j' in "ninja" still overlaps the inside edge of the button in the > glass restored case, which appears to be a regression from my local ToT build > @273000. Am I wrong, or is this intended? Are themed 'j's also too low? Done. https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1267: rb->GetImageNamed(IDR_ICON_PROFILES_ACCOUNT_AUTH_ERROR).ToImageSkia() : I haven't added a before screenshot, but I've updated the explanation. We were basically using a temporary icon. On 2014/05/30 18:24:13, msw wrote: > nit: Are you doing this to have consistent iconography for the profile auth > errors? This should be noted in your CL description, and you should probably > have a before/after pic on the bug. https://codereview.chromium.org/297143008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1306: views::ImageView* reauth_icon = new views::ImageView(); Added, but the tl; dr was : the change was done as a temporary fix, without talking to UI. These changes reflect what the actual UI should look/behave as. Updated the description too. On 2014/05/30 18:24:13, msw wrote: > So you're changing the email_button to handle clicks related to re-auth? Why is > this changing? Can you add more information to your CL description?
Sorry for all the trouble here :-/ https://codereview.chromium.org/297143008/diff/60001/chrome/app/theme/theme_r... File chrome/app/theme/theme_resources.grd (left): https://codereview.chromium.org/297143008/diff/60001/chrome/app/theme/theme_r... chrome/app/theme/theme_resources.grd:355: <structure type="chrome_scaled_image" name="IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR" file="common/avatar_button_auth_error.png" /> Remove the actual image file in this CL too, please. https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:452: const int maximized_frame_offset = 7; This should probably be determined by some NonClientFrameView value like FrameBorderThickness/NonClientBorderThickness/NonClientTopBorderHeight, as it might vary by system DPI, Vista/7, etc. Can you try that approach again (ditto below), without expending too much effort? Maybe ping ben/sky for ideas? https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (left): https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:136: rect.Offset(0, -rect.y()); Sorry, but this will be changing with Elliot's <http://codereview.chromium.org/298813002>, can you rebase on that?
Sorry for taking so long -- I wanted on Elliot's CL to land so I can do a proper rebase (it's since been reverted, which is why the bots are red). Please take another look. Thanks! :) https://codereview.chromium.org/297143008/diff/60001/chrome/app/theme/theme_r... File chrome/app/theme/theme_resources.grd (left): https://codereview.chromium.org/297143008/diff/60001/chrome/app/theme/theme_r... chrome/app/theme/theme_resources.grd:355: <structure type="chrome_scaled_image" name="IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR" file="common/avatar_button_auth_error.png" /> As discussed in the Mac CL, going back to using this resource. On 2014/06/09 21:42:40, msw wrote: > Remove the actual image file in this CL too, please. https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:452: const int maximized_frame_offset = 7; Done! I've cleaned up the code a bit too. There's still 1px hard coded, but everything else around it is better. (I hope). On 2014/06/09 21:42:40, msw wrote: > This should probably be determined by some NonClientFrameView value like > FrameBorderThickness/NonClientBorderThickness/NonClientTopBorderHeight, as it > might vary by system DPI, Vista/7, etc. Can you try that approach again (ditto > below), without expending too much effort? Maybe ping ben/sky for ideas? https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (left): https://codereview.chromium.org/297143008/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:136: rect.Offset(0, -rect.y()); On 2014/06/09 21:42:40, msw wrote: > Sorry, but this will be changing with Elliot's > <http://codereview.chromium.org/298813002>, can you rebase on that? Done.
https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:461: NonClientTopBorderHeight() + kTabstripTopShadowThickness - 1; optional nit: inline this below (and negate the ternary conditional?). https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:36: // Target size of the warning icon. Remove these unused consts. https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:56: border->set_insets(gfx::Insets(top_inset, kLeftRightInset, Ah, I'm sorry to put you out, but can you see if this works well with my WIP CL. I have other pressing tasks, and your help here would be much appreciated. Please try this with https://codereview.chromium.org/343853002 https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:179: IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR).ToImageSkia(); Did you mean to leave this as IDR_WARNING? https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1302: *rb->GetImageNamed(IDR_ICON_PROFILES_ACCOUNT_BUTTON_ERROR).ToImageSkia() : Ditto: Did you mean to leave this as IDR_WARNING? https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1329: 0, optional nit: you can wrap some args here.
https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:461: NonClientTopBorderHeight() + kTabstripTopShadowThickness - 1; On 2014/06/19 04:10:17, msw wrote: > optional nit: inline this below (and negate the ternary conditional?). Done. https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:36: // Target size of the warning icon. On 2014/06/19 04:10:18, msw wrote: > Remove these unused consts. Done. https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:56: border->set_insets(gfx::Insets(top_inset, kLeftRightInset, Patched in your CL, and your insets look good. I've added a bit more top inset, and left the changes here, so that you have an easier merge. Also because this pixel math is way easier than my pixel math :) On 2014/06/19 04:10:17, msw wrote: > Ah, I'm sorry to put you out, but can you see if this works well with my WIP CL. > I have other pressing tasks, and your help here would be much appreciated. > Please try this with https://codereview.chromium.org/343853002 https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:179: IDR_ICON_PROFILES_AVATAR_BUTTON_ERROR).ToImageSkia(); Nope! IDR_WARNING was a placeholder while we were waiting on the real resources (which is the one I'm using, and was the original reason for this CL) On 2014/06/19 04:10:18, msw wrote: > Did you mean to leave this as IDR_WARNING? https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1302: *rb->GetImageNamed(IDR_ICON_PROFILES_ACCOUNT_BUTTON_ERROR).ToImageSkia() : Same answer as before :) On 2014/06/19 04:10:18, msw wrote: > Ditto: Did you mean to leave this as IDR_WARNING? https://codereview.chromium.org/297143008/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1329: 0, On 2014/06/19 04:10:18, msw wrote: > optional nit: you can wrap some args here. Done.
Lgtm with a nit. https://codereview.chromium.org/297143008/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:97: CreateBorder(kNormalImageSet, kHotImageSet, kPushedImageSet)); Nit: revert these spacing changes.
https://codereview.chromium.org/297143008/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/297143008/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:97: CreateBorder(kNormalImageSet, kHotImageSet, kPushedImageSet)); On 2014/06/19 16:04:19, msw wrote: > Nit: revert these spacing changes. Done.
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/297143008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
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/297143008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
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/297143008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
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/297143008/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
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/297143008/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
The CQ bit was unchecked by noms@chromium.org
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/297143008/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 278814 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
