Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22562005:
linux_aura: Implement the dynamic History menu in the dbusmenu. (Closed)
|
Created:
7 years, 4 months ago by Elliot Glaysher Modified:
7 years, 4 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionlinux_aura: Implement the dynamic History menu in the dbusmenu.
This merges and reimplements what was the GlobalHistoryMenu class from
the GTK+ port into linux_aura's GlobalMenuBarX11 class.
BUG=265560
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216868
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 5
Patch Set 3 : fixes for jamescook #Patch Set 4 : Reupload #Patch Set 5 : Reupload part 2 #
Total comments: 14
Patch Set 6 : Redo how HistoryItems are stored. #
Total comments: 3
Messages
Total messages: 15 (0 generated)
sadrul: more of the same review jamescook: owners stamp
Is this code being ported/converted from somewhere else, or is it all new?
On 2013/08/07 21:25:36, James Cook wrote: > Is this code being ported/converted from somewhere else, or is it all new? This code was translated from the GTK implementation in c/b/u/gtk/global_history_menu.cc. Most of what's different is differences between the GtkMenu interface and the DbusmenuMenuitem interface.
On 2013/08/07 21:28:08, Elliot Glaysher wrote: > On 2013/08/07 21:25:36, James Cook wrote: > > Is this code being ported/converted from somewhere else, or is it all new? > > This code was translated from the GTK implementation in > c/b/u/gtk/global_history_menu.cc. Most of what's different is differences > between the GtkMenu interface and the DbusmenuMenuitem interface. ping!
A few small comments/questions https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right): https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:483: if (command_id == MENU_DISABLED_LABEL) { optional nit: It feels a little weird that MENU_DISABLED_LABEL isn't called MENU_DISABLED_ID to imply that it is a command id. I thought at first that it was a special label string. https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:636: menuitem_foreach( Do you know if this is smart enough to handle having items deleted out from under it in the middle of the iteration? I presume there's some reason why you can't do this by iterating over the list from menuitem_get_children -- the docs I find for that function suggest the list may be invalidated. It might be worth a comment here explaining why you're doing it this way. https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... File chrome/browser/ui/views/frame/global_menu_bar_x11.h (right): https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... chrome/browser/ui/views/frame/global_menu_bar_x11.h:138: // For TabRestoreServiceObserver nit: either: // TabRestoreServiceObserver: or // Overridden from TabRestoreServiceObserver: https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... chrome/browser/ui/views/frame/global_menu_bar_x11.h:172: base::WeakPtrFactory<GlobalMenuBarX11> weak_ptr_factory_; I think this should be the last item in the member list, so it is destroyed first, see comments in base/memory/weak_ptr.h
ptal https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right): https://codereview.chromium.org/22562005/diff/3001/chrome/browser/ui/views/fr... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:636: menuitem_foreach( On 2013/08/08 18:42:16, James Cook wrote: > Do you know if this is smart enough to handle having items deleted out from > under it in the middle of the iteration? > > I presume there's some reason why you can't do this by iterating over the list > from menuitem_get_children -- the docs I find for that function suggest the list > may be invalidated. It might be worth a comment here explaining why you're > doing it this way. After reading the implementation, I'm pretty sure it isn't smart enough, even if it appears to work. I've rewritten this entire method to not use their foreach (and deleted three looked-up function pointers in the process.)
LGTM but please let Sadrul make a pass
https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right): https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:276: class GlobalMenuBarX11::HistoryItem { This should probably be struct? https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:515: const TabRestoreService::Tab& entry) { This could be a separate function in the anonymous namespace. https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:541: menu_item_history_map_.insert(std::make_pair(menu_item, item)); Instead of using the map, can you use g_object_set_data_full()? That would be consistent with the use of g_object_set_data for type-tag, command-id. (and this too could be a separate function in the anonymous namespace). https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:591: GList* childs = menuitem_get_children(menu); childs https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:777: tab_restore_service_->AddObserver(this); Should AddObserver() happen before LoadTabsFromLastSession()? https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/global_menu_bar_x11.h (right): https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.h:58: static const int TAG_RECENTLY_CLOSED_HEADER = 4; Can they be in the .cc instead? https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.h:72: DbusmenuMenuitem* BuildMenuItem(const std::string& label, int tag_id); Do these need to be public? https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.h:118: int GetIndexOfMenuItemWithTag(DbusmenuMenuitem* menu, int tag_id); Please add a note that the invisible entries are also taken into account (so the index returned from this function is not the 'visible index').
So I've redone a lot of stuff about memory. - HistoryItems are now stored on the GObjects. I forgot that this was something glib could do. - I'm manually unrefing Dbusmenuitems now. Unlike GTK, it looks like this library doesn't use sink references and objects are given a real full reference by default when created. (I figured this out when trying to debug why the new DeleteHistoryItem() function wasn't getting called.) https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right): https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:515: const TabRestoreService::Tab& entry) { On 2013/08/09 06:38:54, sadrul wrote: > This could be a separate function in the anonymous namespace. It actually can't without making HistoryItem public instead of a private class implementation detail. https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:541: menu_item_history_map_.insert(std::make_pair(menu_item, item)); On 2013/08/09 06:38:54, sadrul wrote: > Instead of using the map, can you use g_object_set_data_full()? That would be > consistent with the use of g_object_set_data for type-tag, command-id. (and this > too could be a separate function in the anonymous namespace). This is a really good point. I forgot that you could do that with gobjects. Done. https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:777: tab_restore_service_->AddObserver(this); On 2013/08/09 06:38:54, sadrul wrote: > Should AddObserver() happen before LoadTabsFromLastSession()? I think so. All other cases I could find ordered things this way (however the only other one that I could find where we both loaded tabs and added an observer was in RecnetlyClosedTabsHandler in the webui code). https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/global_menu_bar_x11.h (right): https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.h:72: DbusmenuMenuitem* BuildMenuItem(const std::string& label, int tag_id); On 2013/08/09 06:38:54, sadrul wrote: > Do these need to be public? no. done. https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.h:118: int GetIndexOfMenuItemWithTag(DbusmenuMenuitem* menu, int tag_id); On 2013/08/09 06:38:54, sadrul wrote: > Please add a note that the invisible entries are also taken into account (so the > index returned from this function is not the 'visible index'). I'm not sure what you're saying here. All entries in the menu are visible. All times that I set the visible property, I'm setting it to true and I'm doing it immediately after construction.
LGTM https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/global_menu_bar_x11.h (right): https://codereview.chromium.org/22562005/diff/63002/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.h:118: int GetIndexOfMenuItemWithTag(DbusmenuMenuitem* menu, int tag_id); On 2013/08/09 20:19:25, Elliot Glaysher wrote: > On 2013/08/09 06:38:54, sadrul wrote: > > Please add a note that the invisible entries are also taken into account (so > the > > index returned from this function is not the 'visible index'). > > I'm not sure what you're saying here. All entries in the menu are visible. All > times that I set the visible property, I'm setting it to true and I'm doing it > immediately after construction. Ah, I noticed that the visibility property was being set, and assumed there could be invisible items in the list. https://codereview.chromium.org/22562005/diff/28003/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right): https://codereview.chromium.org/22562005/diff/28003/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:466: g_object_unref(menu_item); Ah, this is interesting. I wonder if BuildSeperator/BuildMenuItem etc. should return a ScopedMenuItem that does the unref()ing in the dtor. https://codereview.chromium.org/22562005/diff/28003/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:532: DeleteHistoryItem); Is it possible to use base::DeletePointer? (although I don't know if template functions can be used here)
https://codereview.chromium.org/22562005/diff/28003/chrome/browser/ui/views/f... File chrome/browser/ui/views/frame/global_menu_bar_x11.cc (right): https://codereview.chromium.org/22562005/diff/28003/chrome/browser/ui/views/f... chrome/browser/ui/views/frame/global_menu_bar_x11.cc:532: DeleteHistoryItem); On 2013/08/09 20:31:34, sadrul wrote: > Is it possible to use base::DeletePointer? (although I don't know if template > functions can be used here) At some point I have to put the reinterpret cast. I chose to put it in the deleter function (so that the deleter's function signature matched g_object_set_data_full()'s).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erg@chromium.org/22562005/28003
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erg@chromium.org/22562005/28003
Message was sent while issue was closed.
Change committed as 216868 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
