Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
148143004:
Add notification mechanism when BrowserActionButton's icon has been updated. (Closed)
|
Created:
6 years, 10 months ago by leiyi.jp Modified:
6 years, 10 months ago CC:
chromium-reviews, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd notification mechanism when BrowserActionButton's icon has been updated.
In the browser actions container's chevron menu, a MenuItemView's icon comes from BrowserActionView::GetIconWithBadge() (which comes from BrowserActionButton's icon) when the MenuItemView is created. But, BrowserActionButton's icon may be not yet loaded in that timing because it is read from filesystem in another thread.
To solve this issue, we design an icon state observer in BrowserActionButton, a lifecycle observer in MenuItemView. A helper class whose instance binds to MenuItemView's lifecycle and listens to button's icon state, refresh the host menu item's icon when the button's icon was updated.
R=finnur@chromium.org
BUG=319619
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253406
Patch Set 1 #
Total comments: 16
Patch Set 2 #
Total comments: 5
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Messages
Total messages: 37 (0 generated)
The CQ bit was checked by leiyi.jp@gmail.com
The CQ bit was unchecked by leiyi.jp@gmail.com
Thank you for uploading. I have a few comments, mostly just nits, as I like the overall approach. Let's see what the try bots think about this change... https://codereview.chromium.org/148143004/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/148143004/diff/1/AUTHORS#newcode235 AUTHORS:235: Peng Jiang <peng.jiang@shenma-inc.com> This does not match the email you provided in the legal document and looks like a company address, which there is a separate legal document for. https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc (right): https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:27: // BrowserActionButton's icon may be not yet loaded in that timing because it s/in that timing/in time/ https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:29: // The IconStateSensitiveMenuItemViewHelper will change the MenuItemView's s/change/update/ https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:31: class IconStateSensitiveMenuItemViewHelper Nit: This name is a bit long and unwieldy. How about just IconUpdater? (don't forget to update line 29 also) https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:44: virtual void OnIconUpdated(const gfx::ImageSkia& icon) OVERRIDE { Add a comment: // BrowserActionButton::IconObserver methods: https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:47: virtual void OnDeleted(views::MenuItemView* view) OVERRIDE { Add a comment: // views::MenuItemView::LifecycleObserver methods: https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:52: views::MenuItemView* host_; Not sure I'd use the word host_ here. Why not just menu_item_view_? https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:53: BrowserActionButton* button_; Document these two members. https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/exte... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:54: }; Add DISABLE_COPY_AND_ASSIGN(IconStateSensitiveMenuItemViewHelper) ... or IconUpdater, as appropriate, to the end of this class. https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:320: icon = gfx::ImageSkiaOperations::CreateTransparentImage(icon, .25); This is copied almost verbatim from GetIconWithBadge, but needs to be kept in sync. Can you instead extract this into a helper function that both call? Have it take for example a |tab_id| and an |icon_factory| as params. https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:245: IconObserver* icon_observer_; nit: Document. https://codereview.chromium.org/148143004/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/148143004/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_item_view.cc:592: } nit: When the body of an if clause is only 1 line (and there is 0-1 lines in the else clause) then we omit the braces. https://codereview.chromium.org/148143004/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_item_view.cc:647: lifecycle_observer_ = NULL; Looks like the convention in this class is to null in the initializer list for the constructor (line 583). https://codereview.chromium.org/148143004/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_item_view.h (right): https://codereview.chromium.org/148143004/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_item_view.h:541: LifecycleObserver* lifecycle_observer_; nit: Document.
https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/148143004/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:118: icon_observer_(NULL), Move this line down one to avoid compile error seen on Linux try bot.
All done. Please check again.
https://codereview.chromium.org/148143004/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/148143004/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_item_view.cc:647: lifecycle_observer_ = NULL; Hmmm... Now that I look at this again I realize this class has two constructors, and we are now only updating one. Both need to make sure the lifecycle_observer_ start out as NULL so maybe I was wrong to ask you to move this to the ctor. https://codereview.chromium.org/148143004/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/148143004/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_action_view.cc:438: } This function is declared above GetIconForTest() in the .h file and should therefore be moved above it in this file (line 390). https://codereview.chromium.org/148143004/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/148143004/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_action_view.h:192: // Gets icon with the badge. nit: s/icon with the badge/the icon of this button and its badge/ https://codereview.chromium.org/148143004/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_action_view.h:193: gfx::ImageSkia BrowserActionButton::GetIconWithBadge(); Copy-paste error: Remove "BrowserActionButton::" https://codereview.chromium.org/148143004/diff/160001/ui/views/controls/menu/... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/148143004/diff/160001/ui/views/controls/menu/... ui/views/controls/menu/menu_item_view.cc:563: MenuItemView::MenuItemView(MenuItemView* parent, Can you add a comment saying: // Note: This class also has a protected constructor and a common Init function // to initialize both. https://codereview.chromium.org/148143004/diff/160001/ui/views/controls/menu/... ui/views/controls/menu/menu_item_view.cc:631: void MenuItemView::Init(MenuItemView* parent, Can you add a comment saying: // Note: This class also has a public constructor and a common Init function // to initialize both.
Sorry to have so many problems after I adjusted the codes. Please take a look at it again.
> Sorry to have so many problems after I adjusted the codes No need to apologize. It is normal for code to evolve during code reviews, and that does not automatically reflect poorly on the person who wrote the code. Especially not for this changelist, as the code only needed minor augmentations (as far as I could tell) due to what you could call nitpicking. So I'd say this has gone fairly well, so far. Of course, I realize now that I'm not an OWNER for Views, so you'll need another appropriate person to review this before submitting. I'm CC-ing Scott for that. But the code LGTM, with two minor nits. https://codereview.chromium.org/148143004/diff/280001/ui/views/controls/menu/... File ui/views/controls/menu/menu_item_view.cc (right): https://codereview.chromium.org/148143004/diff/280001/ui/views/controls/menu/... ui/views/controls/menu/menu_item_view.cc:114: // function to initialize both. I was hoping to see this comment on line 91 (above the constructor) because other people are going to make the same mistake we did. Find the first constructor in the file, add their variable to it and call it a day (totally missing the other constructor). https://codereview.chromium.org/148143004/diff/280001/ui/views/controls/menu/... ui/views/controls/menu/menu_item_view.cc:587: // to initialize both. Same here, move to line 564.
On 2014/02/13 10:19:43, Finnur wrote: > > Sorry to have so many problems after I adjusted the codes > > No need to apologize. It is normal for code to evolve during code reviews, and > that does not automatically reflect poorly on the person who wrote the code. > Especially not for this changelist, as the code only needed minor augmentations > (as far as I could tell) due to what you could call nitpicking. So I'd say this > has gone fairly well, so far. > > Of course, I realize now that I'm not an OWNER for Views, so you'll need another > appropriate person to review this before submitting. I'm CC-ing Scott for that. > > But the code LGTM, with two minor nits. > > https://codereview.chromium.org/148143004/diff/280001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_item_view.cc (right): > > https://codereview.chromium.org/148143004/diff/280001/ui/views/controls/menu/... > ui/views/controls/menu/menu_item_view.cc:114: // function to initialize both. > I was hoping to see this comment on line 91 (above the constructor) because > other people are going to make the same mistake we did. Find the first > constructor in the file, add their variable to it and call it a day (totally > missing the other constructor). > > https://codereview.chromium.org/148143004/diff/280001/ui/views/controls/menu/... > ui/views/controls/menu/menu_item_view.cc:587: // to initialize both. > Same here, move to line 564. Done. Thanks for help, Finnur!
https://codereview.chromium.org/148143004/diff/350001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc (right): https://codereview.chromium.org/148143004/diff/350001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:27: // BrowserActionButton's icon may be not yet loaded in time because it is read may not be loaded https://codereview.chromium.org/148143004/diff/350001/ui/views/controls/menu/... File ui/views/controls/menu/menu_item_view.h (right): https://codereview.chromium.org/148143004/diff/350001/ui/views/controls/menu/... ui/views/controls/menu/menu_item_view.h:66: class LifecycleObserver { I don't want to add this to MenuItemView. Can you instead use an id to locate the MenuItemView? You can check if the menu is still open by way of MenuRunner::IsRunner.
On 2014/02/18 16:03:50, sky wrote: > https://codereview.chromium.org/148143004/diff/350001/ui/views/controls/menu/... > ui/views/controls/menu/menu_item_view.h:66: class LifecycleObserver { > I don't want to add this to MenuItemView. Can you instead use an id to locate > the MenuItemView? You can check if the menu is still open by way of > MenuRunner::IsRunner. I found all menu item views would be deleted only when the browser action overflow menu controller was being destructed. So it is safe that deleting all IconUpdater instances in the BrowserActionOverflowMenuController's destructor. Please check again, thanks.
Looks like we are heading in the right direction. I have just a few nits. https://chromiumcodereview.appspot.com/148143004/diff/460001/chrome/browser/u... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc (right): https://chromiumcodereview.appspot.com/148143004/diff/460001/chrome/browser/u... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:106: } There are few utility functions that do this in a simpler way. Search for STLDelete* to see samples of more. But in this case I think you want just: STLDeleteElements(&icon_updaters_); https://chromiumcodereview.appspot.com/148143004/diff/460001/chrome/browser/u... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h (right): https://chromiumcodereview.appspot.com/148143004/diff/460001/chrome/browser/u... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h:121: // is being destructed s/destructed/destroyed./ Note the period in the end. However, this comment doesn't say much more than what you would gather from just looking at line 122. Maybe instead talk about why we keep a vector of those pointers and what the purpose is? https://chromiumcodereview.appspot.com/148143004/diff/460001/chrome/browser/u... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://chromiumcodereview.appspot.com/148143004/diff/460001/chrome/browser/u... chrome/browser/ui/views/toolbar/browser_action_view.h:124: } nit: constructor should appear ahead of this function (move this function to line 136).
https://chromiumcodereview.appspot.com/148143004/diff/460001/ui/views/control... File ui/views/controls/menu/menu_item_view.cc (right): https://chromiumcodereview.appspot.com/148143004/diff/460001/ui/views/control... ui/views/controls/menu/menu_item_view.cc:564: // Note: This class also has a public constructor and a common Init function These comments aren't helpful, nuke them.
Done. Thanks.
> These comments aren't helpful, nuke them. Well, if you look at the history of this CL you'll see that we made exactly this error (initializing a member in only one ctor). If these comments had been there we would not have made that mistake. This class definitely needs some comment explaining how members need to be initialized because we have two ctors that initialize 19 members and one Init function that initializes only 11. Why only 11? Why initialize _any_ members in either ctor? Why not *just* use Init for that purpose?
Ugh! Multiple constructors can be a pain, eh? Seems like we should try and make it so there is only one.
On 2014/02/20 15:59:52, sky wrote:
> Ugh! Multiple constructors can be a pain, eh? Seems like we should try and
make
> it so there is only one.
I think there are 2 easy ways to solve this problem:
1. Change MenuItemView(MenuItemView* parent, int command, Type type) to
MenuItemView(MenuItemView* parent, int command, Type type, MenuDelegate*
delegate) (or simply change Init() to a new constructor). Modify calling codes,
one in wrench_menu.cc and the other in menu_item_view.cc. Make the
MenuItemView(MenuDelegate* delegate) calling the new constructor:
MenuItemView::MenuItemView(MenuDelegate* delegate) {
MenuItemView(NULL, 0, SUBMENU, delegate);
}
The benefit is that we can use an initializer list.
2. Move as much as possible member initialization to Init().
I prefer the first way.
So, should I do the refactor of MenuItemView's constructors, or just simply add
comments in menu_item_view.cc, or do not touch menu_item_view.cc/.h in this
issue?
Lets change MenuItemView separately, so don't both touching it in this patch. On Thu, Feb 20, 2014 at 6:41 PM, <leiyi.jp@gmail.com> wrote: > On 2014/02/20 15:59:52, sky wrote: >> >> Ugh! Multiple constructors can be a pain, eh? Seems like we should try and > > make >> >> it so there is only one. > > > I think there are 2 easy ways to solve this problem: > 1. Change MenuItemView(MenuItemView* parent, int command, Type type) to > MenuItemView(MenuItemView* parent, int command, Type type, MenuDelegate* > delegate) (or simply change Init() to a new constructor). Modify calling > codes, > one in wrench_menu.cc and the other in menu_item_view.cc. Make the > MenuItemView(MenuDelegate* delegate) calling the new constructor: > MenuItemView::MenuItemView(MenuDelegate* delegate) { > MenuItemView(NULL, 0, SUBMENU, delegate); > } > The benefit is that we can use an initializer list. > 2. Move as much as possible member initialization to Init(). > I prefer the first way. > > So, should I do the refactor of MenuItemView's constructors, or just simply > add > comments in menu_item_view.cc, or do not touch menu_item_view.cc/.h in this > issue? > > https://chromiumcodereview.appspot.com/148143004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/21 16:28:41, sky wrote: > Lets change MenuItemView separately, so don't both touching it in this patch. OK. Is there anything I can do for this patch?
Nope. Looks like you just need an OWNERS approval from sky@.
https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc (right): https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:49: private: nit: newline 48/49. https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:53: // The button to be observed. When its icon was updated, update the 'was updated'-> changes https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h (right): https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h:123: std::vector<IconUpdater*> icon_updaters_; How about a scopedvector? https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... chrome/browser/ui/views/toolbar/browser_action_view.h:118: virtual ~IconObserver() {} Make the destructor protected so that it's clear BrowserActionButton doesn't own the observer.
On 2014/02/24 16:34:17, sky wrote: > https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... > chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:49: > private: > nit: newline 48/49. Done. > https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... > chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:53: > // The button to be observed. When its icon was updated, update the > 'was updated'-> changes Done. > https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... > chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h:123: > std::vector<IconUpdater*> icon_updaters_; > How about a scopedvector? Of course! Thank you for reminding me. > https://chromiumcodereview.appspot.com/148143004/diff/560001/chrome/browser/u... > chrome/browser/ui/views/toolbar/browser_action_view.h:118: virtual > ~IconObserver() {} > Make the destructor protected so that it's clear BrowserActionButton doesn't own > the observer. Done.
LGTM
The CQ bit was checked by leiyi.jp@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leiyi.jp@gmail.com/148143004/690001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by leiyi.jp@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leiyi.jp@gmail.com/148143004/690001
The CQ bit was unchecked by phajdan.jr@chromium.org
The CQ bit was checked by leiyi.jp@gmail.com
The CQ bit was checked by phajdan.jr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leiyi.jp@gmail.com/148143004/690001
The CQ bit was unchecked by leiyi.jp@gmail.com
The CQ bit was checked by leiyi.jp@gmail.com
Message was sent while issue was closed.
Change committed as 253406 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
