Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1009403002:
Refactor the avatar button/icon class (Closed)
DescriptionRefactor the avatar button/icon class.
Such that they are based on the same base class.
BUG=452524
Committed: https://crrev.com/56078ce434955a67159b59303f6691adb55d7ca1
Cr-Commit-Position: refs/heads/master@{#329249}
Patch Set 1 #Patch Set 2 : Just make it compile. #Patch Set 3 : roughly working. #Patch Set 4 : Cleanup code. #Patch Set 5 : Fix unit tests. #Patch Set 6 : Adding a testing profile is unnecessary. #Patch Set 7 : Nits #
Total comments: 15
Patch Set 8 : Address comments. #
Total comments: 34
Patch Set 9 : Rebase. #Patch Set 10 : Address comments. #
Total comments: 6
Patch Set 11 : Rebase. #Patch Set 12 : Address comments. #
Total comments: 5
Patch Set 13 : Address comments. #Patch Set 14 : Rebase #Messages
Total messages: 34 (10 generated)
Patchset #2 (id:20001) has been deleted
yiyaoliu@chromium.org changed reviewers: + noms@chromium.org
PTAL, thanks!
https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_controller.h (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:6: #define CHROME_BROWSER_UI_VIEWS_PROFILES_AVATAR_BASE_CONTROLLER_H_ nit: I think Mac UI files are "controllers" but not on windows. So, maybe, avatar_base_button, avatar_icon_button, avatar_button? What do you think? https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:12: // This view controller manages the button that sits in the top of the window nit: This view (no controller) https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:12: // This view controller manages the button that sits in the top of the window nit: . at the end of the comment https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:150: // Check the validity of browser() so that this function can be bypassed in nit: maybe just "The browser can be null in tests"? Also, just to double check, is this still true, after the TestingManager fix? https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.cc:175: // If there is an error, show an warning icon. nit: a warning*
Thanks for the review. https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_controller.h (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:6: #define CHROME_BROWSER_UI_VIEWS_PROFILES_AVATAR_BASE_CONTROLLER_H_ On 2015/03/26 15:26:56, Monica Dinculescu wrote: > nit: I think Mac UI files are "controllers" but not on windows. So, maybe, > avatar_base_button, avatar_icon_button, avatar_button? What do you think? Changed to avatar_base_button. https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:12: // This view controller manages the button that sits in the top of the window On 2015/03/26 15:26:56, Monica Dinculescu wrote: > nit: . at the end of the comment Done. https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:12: // This view controller manages the button that sits in the top of the window On 2015/03/26 15:26:56, Monica Dinculescu wrote: > nit: This view (no controller) Done. https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:150: // Check the validity of browser() so that this function can be bypassed in On 2015/03/26 15:26:56, Monica Dinculescu wrote: > nit: maybe just "The browser can be null in tests"? Also, just to double check, > is this still true, after the TestingManager fix? Yes, because nullptr is passsed when the button is created. See: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.cc:175: // If there is an error, show an warning icon. On 2015/03/26 15:26:56, Monica Dinculescu wrote: > nit: a warning* Done.
https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_controller.h (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:6: #define CHROME_BROWSER_UI_VIEWS_PROFILES_AVATAR_BASE_CONTROLLER_H_ On 2015/03/26 18:45:01, yao wrote: > On 2015/03/26 15:26:56, Monica Dinculescu wrote: > > nit: I think Mac UI files are "controllers" but not on windows. So, maybe, > > avatar_base_button, avatar_icon_button, avatar_button? What do you think? > > Changed to avatar_base_button. Will you do the other renames in a separate CL?
https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_controller.h (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:6: #define CHROME_BROWSER_UI_VIEWS_PROFILES_AVATAR_BASE_CONTROLLER_H_ On 2015/03/30 14:54:54, Monica Dinculescu wrote: > On 2015/03/26 18:45:01, yao wrote: > > On 2015/03/26 15:26:56, Monica Dinculescu wrote: > > > nit: I think Mac UI files are "controllers" but not on windows. So, maybe, > > > avatar_base_button, avatar_icon_button, avatar_button? What do you think? > > > > Changed to avatar_base_button. > > Will you do the other renames in a separate CL? This file has been reverted/deleted. I though I renamed everything. Is there anything I'm missing?
https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_controller.h (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:6: #define CHROME_BROWSER_UI_VIEWS_PROFILES_AVATAR_BASE_CONTROLLER_H_ On 2015/03/30 15:02:16, yao wrote: > On 2015/03/30 14:54:54, Monica Dinculescu wrote: > > On 2015/03/26 18:45:01, yao wrote: > > > On 2015/03/26 15:26:56, Monica Dinculescu wrote: > > > > nit: I think Mac UI files are "controllers" but not on windows. So, maybe, > > > > avatar_base_button, avatar_icon_button, avatar_button? What do you think? > > > > > > Changed to avatar_base_button. > > > > Will you do the other renames in a separate CL? > > This file has been reverted/deleted. > > I though I renamed everything. Is there anything I'm missing? I had suggested renaming new_avatar_button to avatar_button, and avatar_menu_button to avatar_icon to match their intent and the Mac hierarchy (also because at this point the new_avatar_button is no longer really new)
https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_controller.h (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:6: #define CHROME_BROWSER_UI_VIEWS_PROFILES_AVATAR_BASE_CONTROLLER_H_ On 2015/03/30 15:04:00, Monica Dinculescu wrote: > On 2015/03/30 15:02:16, yao wrote: > > On 2015/03/30 14:54:54, Monica Dinculescu wrote: > > > On 2015/03/26 18:45:01, yao wrote: > > > > On 2015/03/26 15:26:56, Monica Dinculescu wrote: > > > > > nit: I think Mac UI files are "controllers" but not on windows. So, > maybe, > > > > > avatar_base_button, avatar_icon_button, avatar_button? What do you > think? > > > > > > > > Changed to avatar_base_button. > > > > > > Will you do the other renames in a separate CL? > > > > This file has been reverted/deleted. > > > > I though I renamed everything. Is there anything I'm missing? > > I had suggested renaming new_avatar_button to avatar_button, and > avatar_menu_button to avatar_icon to match their intent and the Mac hierarchy > (also because at this point the new_avatar_button is no longer really new) Ic, I misunderstood your initial comments, but now I got it. I took a look at the code, the renaming will touch quite some files and I think it will be better if I rename them in a separate CL, unless you have a different preference.
lgtm https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_controller.h (right): https://codereview.chromium.org/1009403002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_controller.h:6: #define CHROME_BROWSER_UI_VIEWS_PROFILES_AVATAR_BASE_CONTROLLER_H_ On 2015/03/30 23:12:12, yao wrote: > On 2015/03/30 15:04:00, Monica Dinculescu wrote: > > On 2015/03/30 15:02:16, yao wrote: > > > On 2015/03/30 14:54:54, Monica Dinculescu wrote: > > > > On 2015/03/26 18:45:01, yao wrote: > > > > > On 2015/03/26 15:26:56, Monica Dinculescu wrote: > > > > > > nit: I think Mac UI files are "controllers" but not on windows. So, > > maybe, > > > > > > avatar_base_button, avatar_icon_button, avatar_button? What do you > > think? > > > > > > > > > > Changed to avatar_base_button. > > > > > > > > Will you do the other renames in a separate CL? > > > > > > This file has been reverted/deleted. > > > > > > I though I renamed everything. Is there anything I'm missing? > > > > I had suggested renaming new_avatar_button to avatar_button, and > > avatar_menu_button to avatar_icon to match their intent and the Mac hierarchy > > (also because at this point the new_avatar_button is no longer really new) > > Ic, I misunderstood your initial comments, but now I got it. > > I took a look at the code, the renaming will touch quite some files and I think > it will be better if I rename them in a separate CL, unless you have a different > preference. Sounds good!
yiyaoliu@chromium.org changed reviewers: + msw@chromium.org
+Mike for owner's review, thanks!
https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (left): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:219: gfx::Image avatar; Should this call avatar_button_->Update() instead now? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:140: : profile_manager_(TestingBrowserProcess::GetGlobal()) {} nit: can you inline this in SetUp and remove the class member? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:145: ASSERT_TRUE(profile_manager_.SetUp()); Why is this necessary? Can you add a comment? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_button.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.cc:58: NOTREACHED(); Why not make this pure-virtual? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_button.h (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.h:14: class AvatarBaseButton : public ProfileInfoCacheObserver { Two overarching questions here: 1) Don't we plan to deprecate/remove the old button soon anyway? (if so, why bother with this? if not, why not?) 2) Will you consolidate more code here? as-is this refactoring doesn't offer much compelling cleanup. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.h:20: Browser* browser() const {return browser_;} nit: spaces inside curly braces. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.h:35: virtual void Update(); Shouldn't this be protected for subclasses overrides? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:50: Update(); Is this necessary if it wasn't being performed on construction previously? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:159: bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu(); nit: const https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:159: bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu(); The old value was true if the button existed (browser_non_client_frame_view.cc had "bool should_show_avatar_menu = avatar_button_ || AvatarMenu::ShouldShowAvatarMenu();"), do you know why that was the case? It seems like the button would always be enabled if it existed, but now it might exist but not be enabled, is this intentional? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:164: return; Why does this early return? Shouldn't the button still be enabled or disabled regardless of the ability to get the avatar images? https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:168: if (!AvatarMenu::ShouldShowAvatarMenu()) nit: use the cached |should_show_avatar_menu| value here. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:169: this->SetEnabled(false); nit: remove "this->" https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:169: this->SetEnabled(false); Should this button ever be enabled, and if so, what calls SetEnabled(true)? Should this be unconditionally called as SetEnabled(should_show_avatar_menu); https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:170: this->SetAvatarIcon(avatar, is_rectangle); ditto nit: remove "this->" https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.cc:108: SchedulePaint(); nit (aside): I wonder if this is really necessary. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.cc:175: // If there is an error, show an warning. nit: "show a warning."
Patchset #9 (id:180001) has been deleted
Hi Mike, Sorry for the delay. I'm not quite sure about some of your questions, I think noms@ knows better. But she's moving recently, so I'm not sure how available she is, or if she will have time to reply. I can try to ask @rogerta tmr to see if he knows about it. Sorry for this. Yiyao https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (left): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:219: gfx::Image avatar; On 2015/04/01 01:30:16, msw wrote: > Should this call avatar_button_->Update() instead now? No, I don't think so, it was called here before because the avatar_button itself does not listen to profile info cache events. Now itself listens to it, and it will update by itself. (like how the new_avatar_button handles this.) https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:140: : profile_manager_(TestingBrowserProcess::GetGlobal()) {} On 2015/04/01 01:30:16, msw wrote: > nit: can you inline this in SetUp and remove the class member? Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:145: ASSERT_TRUE(profile_manager_.SetUp()); On 2015/04/01 01:30:16, msw wrote: > Why is this necessary? Can you add a comment? Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_button.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.cc:58: NOTREACHED(); On 2015/04/01 01:30:16, msw wrote: > Why not make this pure-virtual? Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_button.h (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.h:14: class AvatarBaseButton : public ProfileInfoCacheObserver { On 2015/04/01 01:30:16, msw wrote: > Two overarching questions here: > 1) Don't we plan to deprecate/remove the old button soon anyway? (if so, why > bother with this? if not, why not?) > 2) Will you consolidate more code here? as-is this refactoring doesn't offer > much compelling cleanup. I'm not sure, I did not know much of the history of this menu/bubble. I only know about this bug: crbug/452524. Maybe @noms can provide more details here, but she's moving recently, not sure if she'll see this. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.h:20: Browser* browser() const {return browser_;} On 2015/04/01 01:30:16, msw wrote: > nit: spaces inside curly braces. Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.h:35: virtual void Update(); On 2015/04/01 01:30:16, msw wrote: > Shouldn't this be protected for subclasses overrides? Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:50: Update(); On 2015/04/01 01:30:17, msw wrote: > Is this necessary if it wasn't being performed on construction previously? Yes, Update just redraws the button. when this is being constructed, the button needs to be drew for the first time. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:159: bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu(); On 2015/04/01 01:30:16, msw wrote: > nit: const Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:164: return; On 2015/04/01 01:30:16, msw wrote: > Why does this early return? Shouldn't the button still be enabled or disabled > regardless of the ability to get the avatar images? I don't quite know exactly why here either. but this function was previously called in BrowserNonClientFrameView::UpdateAvatarInfo, which is triggered whenever ProfileInfoCacheObserver observes something. Now AvatarMenuButton observes ProfileInfoCacheObserver itself, I just moved the same code here. This way the button will behave exactly the same as before. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:168: if (!AvatarMenu::ShouldShowAvatarMenu()) On 2015/04/01 01:30:16, msw wrote: > nit: use the cached |should_show_avatar_menu| value here. Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:169: this->SetEnabled(false); On 2015/04/01 01:30:16, msw wrote: > nit: remove "this->" Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:170: this->SetAvatarIcon(avatar, is_rectangle); On 2015/04/01 01:30:17, msw wrote: > ditto nit: remove "this->" Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.cc:108: SchedulePaint(); On 2015/04/01 01:30:17, msw wrote: > nit (aside): I wonder if this is really necessary. Done. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.cc:175: // If there is an error, show an warning. On 2015/04/01 01:30:17, msw wrote: > nit: "show a warning." Done.
https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_button.h (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.h:14: class AvatarBaseButton : public ProfileInfoCacheObserver { On 2015/04/01 01:30:16, msw wrote: > Two overarching questions here: > 1) Don't we plan to deprecate/remove the old button soon anyway? (if so, why > bother with this? if not, why not?) > 2) Will you consolidate more code here? as-is this refactoring doesn't offer > much compelling cleanup. 1) Yes we do, sort of. The button will be gone, but it will still be used to display the incognito badge (so the click listener code will go away, but unfortunately the code around it won't). 2) I really hate the "avatar button" and "new avatar button" names of classes, so i was hoping this could be a stepping stone towards what we have on the mac: avatar base/avatar icon (the incognito badge) and avatar button (the actual button). If they both inherit from the same base, then we would only need to hold one avatar_button_ member in all the browser views, and it would just do the right thing. What do you think?
I'm okay with the general premise here if Monica thinks it's worthwhile. https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:164: return; On 2015/04/10 00:43:32, yao wrote: > On 2015/04/01 01:30:16, msw wrote: > > Why does this early return? Shouldn't the button still be enabled or disabled > > regardless of the ability to get the avatar images? > > I don't quite know exactly why here either. but this function was previously > called in BrowserNonClientFrameView::UpdateAvatarInfo, which is triggered > whenever ProfileInfoCacheObserver observes something. > > Now AvatarMenuButton observes ProfileInfoCacheObserver itself, I just moved the > same code here. > > This way the button will behave exactly the same as before. That's mostly okay for a refactoring, but if code looks wrong it's worth investigating/fixing. https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc (right): https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:145: // ProfileManager::GetProfileInfoCache won't crash when constructing Can you make AvatarBaseButton work when g_browser_process->profile_manager() is null and comment that it may be null in tests? I think that is better design than forcing the frame layout tests to construct a test profile manager. https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:164: return; I still think this shouldn't early return... If we can't load the image, then we shouldn't call SetAvatarIcon, but regardless of that, we should call SetEnabled(should_show_avatar_menu). https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:167: // Disable the menu when we should not show the menu. Ping, I think this code should unconditionally call SetEnabled(should_show_avatar_menu);
Patchset #11 (id:240001) has been deleted
https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc (right): https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:145: // ProfileManager::GetProfileInfoCache won't crash when constructing On 2015/04/16 22:53:34, msw wrote: > Can you make AvatarBaseButton work when g_browser_process->profile_manager() is > null and comment that it may be null in tests? I think that is better design > than forcing the frame layout tests to construct a test profile manager. Done. https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:164: return; On 2015/04/16 22:53:34, msw wrote: > I still think this shouldn't early return... If we can't load the image, then we > shouldn't call SetAvatarIcon, but regardless of that, we should call > SetEnabled(should_show_avatar_menu). Done. https://codereview.chromium.org/1009403002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:167: // Disable the menu when we should not show the menu. On 2015/04/16 22:53:34, msw wrote: > Ping, I think this code should unconditionally call > SetEnabled(should_show_avatar_menu); Done.
lgtm with nits https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_button.cc (right): https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.cc:16: // profile_manager might be null in tests. nit: vertical brackets around identifier names: |profile_manager|. ditto below https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:154: const bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu(); nit: inline this in the SetEnabled call below.
https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_base_button.cc (right): https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_base_button.cc:16: // profile_manager might be null in tests. On 2015/05/05 17:36:39, msw wrote: > nit: vertical brackets around identifier names: |profile_manager|. ditto below Done. https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:154: const bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu(); On 2015/05/05 17:36:39, msw wrote: > nit: inline this in the SetEnabled call below. You mean like: SetEnabled(AvatarMenu::ShouldShowAvatarMenu())? But should_show_avatar_menu is used again in the function below: AvatarMenuButton::GetAvatarImages.
https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_menu_button.cc (right): https://codereview.chromium.org/1009403002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_menu_button.cc:154: const bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu(); On 2015/05/05 17:45:00, yao wrote: > On 2015/05/05 17:36:39, msw wrote: > > nit: inline this in the SetEnabled call below. > > You mean like: SetEnabled(AvatarMenu::ShouldShowAvatarMenu())? > > But should_show_avatar_menu is used again in the function below: > AvatarMenuButton::GetAvatarImages. Ah, ok. Ignore that comment.
The CQ bit was checked by yiyaoliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from noms@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1009403002/#ps300001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009403002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyaoliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, noms@chromium.org Link to the patchset: https://codereview.chromium.org/1009403002/#ps320001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009403002/320001
Message was sent while issue was closed.
Committed patchset #14 (id:320001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/56078ce434955a67159b59303f6691adb55d7ca1 Cr-Commit-Position: refs/heads/master@{#329249}
Message was sent while issue was closed.
On 2015/05/11 22:14:57, I haz the power (commit-bot) wrote: > Patchset 14 (id:??) landed as > https://crrev.com/56078ce434955a67159b59303f6691adb55d7ca1 > Cr-Commit-Position: refs/heads/master@{#329249} I think this change caused some flakiness in the ChromiumOS Tests bot causing browser_tests to sometimes fail. An attempt to fix it is here: https://codereview.chromium.org/1139773003/
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:320001) has been created in https://codereview.chromium.org/1139943002/ by pneubeck@chromium.org. The reason for reverting is: Very likely has caused the latest flakiness of BrowserNonClientFrameViewAshTest.AvatarDisplayOnTeleportedWindow and BrowserNonClientFrameViewAshTest.AvatarMenuButtonHitTestOnChromeOS on ChromeOS dbg: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... |
