Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
267593005:
Refactor menu controller to isolate aura dependency. (Closed)
|
Created:
6 years, 7 months ago by Andre Modified:
6 years, 7 months ago CC:
chromium-reviews, sadrul, ben+aura_chromium.org, tfarina, kalyank, mac-views-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor menu controller to isolate aura dependency.
We want to run menus without aura for the mac port of views.
This change allows menus to run with or without aura.
This is accomplished by introducing a MenuMessageLoop interface to abstract
the platform-dependent parts of the menu controller. It is responsible for
running and quitting the nested message loop, and reposting events.
BUG=366007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270881
Patch Set 1 #
Total comments: 5
Patch Set 2 : Removing ifdefs, work in progress #
Total comments: 26
Patch Set 3 : Cleanup #
Total comments: 19
Patch Set 4 : Rebase to master. Fixes for tapted. #
Total comments: 11
Patch Set 5 : Fixes for tapted and trybots, plus rebase #
Total comments: 2
Patch Set 6 : Fix missing initializer #
Total comments: 1
Patch Set 7 : Formatting and mac compile fixes. #Patch Set 8 : Rebase and merge with http://crrev.com/269819 #
Total comments: 25
Patch Set 9 : Fixes for sky #Patch Set 10 : More fixes for sky #Messages
Total messages: 34 (0 generated)
Trent, can you take a first look? https://codereview.chromium.org/267593005/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:2452: return root ? gfx::Screen::GetScreenFor(root->GetNativeView()) I'm not sure about this change (limited experience with views code). I will run on linux and test if this works. Trent, let me know if this looks wrong.
The subject line should be shorter (72-80 in the max). Could you also add a more detailed description after the subject? I don't see any changes to combobox, scrollview, etc in this change, why do you mention them in the subject?
On 2014/05/01 22:42:02, tfarina wrote: > The subject line should be shorter (72-80 in the max). > > Could you also add a more detailed description after the subject? > > I don't see any changes to combobox, scrollview, etc in this change, why do you > mention them in the subject? Done. The views.gyp change causes combobox etc to no longer be excluded when use_aura=0.
https://codereview.chromium.org/267593005/diff/1/ui/aura/env.h File ui/aura/env.h (left): https://codereview.chromium.org/267593005/diff/1/ui/aura/env.h#oldcode42 ui/aura/env.h:42: const int mouse_button_flags() const { return mouse_button_flags_; } yeah - clang warns me about this too... but we shouldn't expose this header to anything in a non-aura build. All the ui/aura and ui/wm headers should be avoided completely if !aura. guarding the #includes with #if defined(..) is an option. But it's ugly. https://codereview.chromium.org/267593005/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:110: #if defined(USE_AURA) I think there will be resistance to this many #ifdefs :(. But I could be wrong (we could ask :). Ideally helper functions - maybe on menu_config_foo, drag_utils_foo or native_widget_private/native_widget_foo (and menu_host..) mean the #ifdefs and aura dependencies disappear from this file. However, I explored this a bit yesterday and a neat solution didn't start standing out. You can see my early attempts here: https://codereview.chromium.org/264953005 . But I didn't get very far (currently swamped in code review for m36 things..). One option (just came to me..) might be a new class with a virtual interface, like class MenuActivationHelper { pubilc: virtual void RunMessageLoop(); virtual void RepostMouseEventToRootWindow(const ui::MouseEvent& event) = 0; static bool IsDragDropInProgress(); static scoped_ptr<MenuActivationHelper> Create(MenuController* controller, gfx::NativeWindow* window); }; Then do classes/files forMenuActivationHelperAura MenuActivationHelperMac etc. I *think* this will be nicer than #ifdefs, but I haven't tried it out yet. https://codereview.chromium.org/267593005/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:2452: return root ? gfx::Screen::GetScreenFor(root->GetNativeView()) On 2014/05/01 22:38:43, Andre wrote: > I'm not sure about this change (limited experience with views code). I will run > on linux and test if this works. > Trent, let me know if this looks wrong. This looks good! (I think :) https://codereview.chromium.org/267593005/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/267593005/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_scroll_view_container.cc:285: if (menu_config.native_theme == ui::NativeThemeAura::instance()) { This could maybe be something like virtual int NativeTheme::MenuBorderPadding(bool* use_border) = 0; but tweaking this #ifdef is probably fine for a first pass.
https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_host.cc (left): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_host.cc:58: SetShadowType(GetNativeView(), wm::SHADOW_TYPE_NONE); Moved this to InitNativeWidget(). https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_host.cc:74: ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); The first argument is unused when the second argument is NULL.
https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_config_mac.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_config_mac.cc:35: static MenuConfig* views_instance = NULL; This should probably be CR_DEFINE_STATIC_LOCAL(MenuConfig, mac_instance, (theme ? theme : ui::NativeTheme::instance())); return mac_instance; https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_message_loop.h (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: No "(c)" in headers any more https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:16: class MenuMessageLoop { nit: should have a class comment https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:22: virtual void Run(MenuController*, bool nested_menu); It's more typical to declare a pure-virtual interface, and just expose a static `Create()` method that needs to be implemented per-platform. See e.g. what's done for ui/views/widget/native_widget_private.h It means re-declaring a lot of stuff, but allows the platform classes to have their own data members if they need it. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:27: }; nit (unless it's just pure virtual methods): private: DISALLOW_COPY_AND_ASSIGN(..) https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_message_loop_aura.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_aura.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_aura.cc:10: #include "ui/views/controls/menu/menu_message_loop.h" nit: should be the first include, followed by a new line https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_message_loop_mac.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no c https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:6: #include "ui/views/controls/menu/menu_message_loop.h" nit: first include https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:11: } should have a NOTIMPLEMENTED()? https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:14: base::MessageLoopForUI* loop = base::MessageLoopForUI::current(); nit: should #include base/message_loop/message_loop.h (IWUY). run_loop.h should really just have a forward declare of MessageLoop... https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w... ui/views/widget/native_widget_aura.cc:181: if (!params.has_dropshadow) This might be a different kind of shadow. I think the wm::ShadowType is the thing CrOS (and Mac!) do when a window gains focus -- it's a soft shadow around all sides that gets darker when it's the main window. Whereas the drop shadow is a hard/thin/projected shadow that's just on the right and bottom of the window (and menus actually have them :). .. or at least that's the conclusion I made when I was poking at this dependency.
https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_config_mac.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_config_mac.cc:35: static MenuConfig* views_instance = NULL; On 2014/05/04 23:43:39, tapted wrote: > This should probably be > > CR_DEFINE_STATIC_LOCAL(MenuConfig, > mac_instance, > (theme ? theme : ui::NativeTheme::instance())); > return mac_instance; Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_message_loop.h (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/04 23:43:39, tapted wrote: > nit: No "(c)" in headers any more Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:16: class MenuMessageLoop { On 2014/05/04 23:43:39, tapted wrote: > nit: should have a class comment Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:22: virtual void Run(MenuController*, bool nested_menu); On 2014/05/04 23:43:39, tapted wrote: > It's more typical to declare a pure-virtual interface, and just expose a static > `Create()` method that needs to be implemented per-platform. See e.g. what's > done for ui/views/widget/native_widget_private.h > > It means re-declaring a lot of stuff, but allows the platform classes to have > their own data members if they need it. Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop.h:27: }; On 2014/05/04 23:43:39, tapted wrote: > nit (unless it's just pure virtual methods): > > private: > DISALLOW_COPY_AND_ASSIGN(..) Changed to pure virtual. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_message_loop_aura.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_aura.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/04 23:43:39, tapted wrote: > nit: no (c) Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_aura.cc:10: #include "ui/views/controls/menu/menu_message_loop.h" On 2014/05/04 23:43:39, tapted wrote: > nit: should be the first include, followed by a new line Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_message_loop_mac.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/04 23:43:39, tapted wrote: > nit: no c Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:6: #include "ui/views/controls/menu/menu_message_loop.h" On 2014/05/04 23:43:39, tapted wrote: > nit: first include Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:11: } On 2014/05/04 23:43:39, tapted wrote: > should have a NOTIMPLEMENTED()? Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/controls/menu/m... ui/views/controls/menu/menu_message_loop_mac.cc:14: base::MessageLoopForUI* loop = base::MessageLoopForUI::current(); On 2014/05/04 23:43:39, tapted wrote: > nit: should #include base/message_loop/message_loop.h (IWUY). run_loop.h should > really just have a forward declare of MessageLoop... Done. https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/267593005/diff/20001/ui/views/widget/native_w... ui/views/widget/native_widget_aura.cc:181: if (!params.has_dropshadow) On 2014/05/04 23:43:39, tapted wrote: > This might be a different kind of shadow. I think the wm::ShadowType is the > thing CrOS (and Mac!) do when a window gains focus -- it's a soft shadow around > all sides that gets darker when it's the main window. Whereas the drop shadow is > a hard/thin/projected shadow that's just on the right and bottom of the window > (and menus actually have them :). > > .. or at least that's the conclusion I made when I was poking at this > dependency. OK, I added Widget::SetHasShadow to replace this.
nice! Looks really good. I just have some nits, but we should rebase off master and run some try jobs before sending to ben https://codereview.chromium.org/267593005/diff/120001/ui/aura/env.h File ui/aura/env.h (right): https://codereview.chromium.org/267593005/diff/120001/ui/aura/env.h#newcode42 ui/aura/env.h:42: int mouse_button_flags() const { return mouse_button_flags_; } nit: the change here shouldn't be required any more - it would be a good way to check we're not including it by accident :) (but also it should still be done at some point - just looks suspicious in this CL) https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_config_mac.cc (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_config_mac.cc:9: #include "ui/views/controls/menu/menu_config.h" nit: i'd probably put this as first include with a gap https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_config_mac.cc:15: submenu_horizontal_inset = 1; Maybe add a TODO - "tune these parameters for Mac"? https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:364: message_loop_.reset(MenuMessageLoop::Create()); I think maybe do this unconditionally in the constructor, since it needs no special args and is pretty lightweight. (doing it like this suggests to me that it might go away at some point as well) https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.cc (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:145: } owner_ = NULL; here? https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:173: } owner_ = NULL; here? https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.h (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.h:31: DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopAura); nit: this comes right at the end https://codereview.chromium.org/267593005/diff/120001/ui/views/views.gyp File ui/views/views.gyp (left): https://codereview.chromium.org/267593005/diff/120001/ui/views/views.gyp#oldc... ui/views/views.gyp:170: 'controls/menu/menu_runner_mac.mm', I think you can rebase on master and land this principally as a refactoring change for non-Mac https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/native_... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/native_... ui/views/widget/native_widget_mac.mm:174: [window_ setHasShadow:has_shadow]; You'll have to leave this bit off for now after rebasing on master https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/widget.... ui/views/widget/widget.h:500: void SetHasShadow(bool has_shadow); Maybe call this `SetHasActivationShadow`? And borrow some words from the comment in shadow_controller.h
On 2014/05/06 00:02:45, tapted wrote: > nice! Looks really good. I just have some nits, but we should rebase off master > and run some try jobs before sending to ben > > https://codereview.chromium.org/267593005/diff/120001/ui/aura/env.h > File ui/aura/env.h (right): > > https://codereview.chromium.org/267593005/diff/120001/ui/aura/env.h#newcode42 > ui/aura/env.h:42: int mouse_button_flags() const { return mouse_button_flags_; } > nit: the change here shouldn't be required any more - it would be a good way to > check we're not including it by accident :) (but also it should still be done at > some point - just looks suspicious in this CL) > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_config_mac.cc (right): > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > ui/views/controls/menu/menu_config_mac.cc:9: #include > "ui/views/controls/menu/menu_config.h" > nit: i'd probably put this as first include with a gap > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > ui/views/controls/menu/menu_config_mac.cc:15: submenu_horizontal_inset = 1; > Maybe add a TODO - "tune these parameters for Mac"? > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_controller.cc (right): > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > ui/views/controls/menu/menu_controller.cc:364: > message_loop_.reset(MenuMessageLoop::Create()); > I think maybe do this unconditionally in the constructor, since it needs no > special args and is pretty lightweight. (doing it like this suggests to me that > it might go away at some point as well) > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_aura.cc (right): > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_aura.cc:145: } > owner_ = NULL; here? > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_aura.cc:173: } > owner_ = NULL; here? > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_aura.h (right): > > https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_aura.h:31: > DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopAura); > nit: this comes right at the end > > https://codereview.chromium.org/267593005/diff/120001/ui/views/views.gyp > File ui/views/views.gyp (left): > > https://codereview.chromium.org/267593005/diff/120001/ui/views/views.gyp#oldc... > ui/views/views.gyp:170: 'controls/menu/menu_runner_mac.mm', > I think you can rebase on master and land this principally as a refactoring > change for non-Mac > > https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/native_... > File ui/views/widget/native_widget_mac.mm (right): > > https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/native_... > ui/views/widget/native_widget_mac.mm:174: [window_ setHasShadow:has_shadow]; > You'll have to leave this bit off for now after rebasing on master > > https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/widget.h > File ui/views/widget/widget.h (right): > > https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/widget.... > ui/views/widget/widget.h:500: void SetHasShadow(bool has_shadow); > Maybe call this `SetHasActivationShadow`? And borrow some words from the > comment in shadow_controller.h Thanks for the comments! I will be sheriffing the next few days, so might have to get back to this afterwards.
Rebased off master. https://codereview.chromium.org/267593005/diff/120001/ui/aura/env.h File ui/aura/env.h (right): https://codereview.chromium.org/267593005/diff/120001/ui/aura/env.h#newcode42 ui/aura/env.h:42: int mouse_button_flags() const { return mouse_button_flags_; } On 2014/05/06 00:02:46, tapted wrote: > nit: the change here shouldn't be required any more - it would be a good way to > check we're not including it by accident :) (but also it should still be done at > some point - just looks suspicious in this CL) Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_config_mac.cc (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_config_mac.cc:9: #include "ui/views/controls/menu/menu_config.h" On 2014/05/06 00:02:46, tapted wrote: > nit: i'd probably put this as first include with a gap Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_config_mac.cc:15: submenu_horizontal_inset = 1; On 2014/05/06 00:02:46, tapted wrote: > Maybe add a TODO - "tune these parameters for Mac"? Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:364: message_loop_.reset(MenuMessageLoop::Create()); On 2014/05/06 00:02:46, tapted wrote: > I think maybe do this unconditionally in the constructor, since it needs no > special args and is pretty lightweight. (doing it like this suggests to me that > it might go away at some point as well) Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.cc (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:145: } On 2014/05/06 00:02:46, tapted wrote: > owner_ = NULL; here? Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:173: } On 2014/05/06 00:02:46, tapted wrote: > owner_ = NULL; here? Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.h (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.h:31: DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopAura); On 2014/05/06 00:02:46, tapted wrote: > nit: this comes right at the end Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/views.gyp File ui/views/views.gyp (left): https://codereview.chromium.org/267593005/diff/120001/ui/views/views.gyp#oldc... ui/views/views.gyp:170: 'controls/menu/menu_runner_mac.mm', On 2014/05/06 00:02:46, tapted wrote: > I think you can rebase on master and land this principally as a refactoring > change for non-Mac Done. https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/267593005/diff/120001/ui/views/widget/widget.... ui/views/widget/widget.h:500: void SetHasShadow(bool has_shadow); On 2014/05/06 00:02:46, tapted wrote: > Maybe call this `SetHasActivationShadow`? And borrow some words from the > comment in shadow_controller.h Done.
lgtm Maybe add a brief "how" summary in the CL description - a good CL description usually has a why, what and a how. You have the first two, but I'd add something like ~"This is accomplished by introducing a MenuMessageLoop interface to abstract the platform-dependent parts of the menu controller. It is responsible for running and quitting the nested message loop, and reposting events." ben might also know a nicer place for SetHasActivationShadow, but where it is seems OK to me. https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/display_change_listener_mac.cc (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/display_change_listener_mac.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/menu_config_mac.cc (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/menu_config_mac.cc:16: // TODO: Tune these parameters for Mac. nit: TODOs should have a username in brackets - usually the person who added it :). To minimise churn, It may even be better just setting a couple of these (i.e. just so it's usable), and emitting NOTIMPLEMENTED() https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/menu_host.cc:77: ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); maybe re-attach the earlier comment about this when you send the patch to ben https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.h (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.h:20: MenuMessageLoopAura(); clang might warn about having a non-trivial compiler generated inline destructor - maybe good just to add one anyway with an empty body in the .cc https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_mac.h (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_mac.h:25: DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopAura); MenuMessageLoopAura -> MenuMessageLoopMac I.. think this hides the default constructor too. I'd just add empty definitions of the constructor and destructor - it's a good habit for any class that isn't just an interface (compiler-generated destructors are really bloaty when inlined) https://codereview.chromium.org/267593005/diff/130001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/widget/widget.... ui/views/widget/widget.h:499: // Sets whether a shadow should be drawn around the window. nit: I think this needs a more verbose comment. Something like // Informs the window manager whether to create a rectangular shadow around // the window, and keep it updated in response to activation changes.
On 2014/05/08 00:25:28, tapted wrote: > lgtm > > Maybe add a brief "how" summary in the CL description - a good CL description > usually has a why, what and a how. You have the first two, but I'd add something > like ~"This is accomplished by introducing a MenuMessageLoop interface to > abstract the platform-dependent parts of the menu controller. It is responsible > for running and quitting the nested message loop, and reposting events." > > ben might also know a nicer place for SetHasActivationShadow, but where it is > seems OK to me. > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > File ui/views/controls/menu/display_change_listener_mac.cc (right): > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > ui/views/controls/menu/display_change_listener_mac.cc:1: // Copyright (c) 2014 > The Chromium Authors. All rights reserved. > nit: no (c) > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_config_mac.cc (right): > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > ui/views/controls/menu/menu_config_mac.cc:16: // TODO: Tune these parameters for > Mac. > nit: TODOs should have a username in brackets - usually the person who added it > :). > > To minimise churn, It may even be better just setting a couple of these (i.e. > just so it's usable), and emitting NOTIMPLEMENTED() > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_host.cc (right): > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > ui/views/controls/menu/menu_host.cc:77: > ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); > maybe re-attach the earlier comment about this when you send the patch to ben > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_aura.h (right): > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_aura.h:20: MenuMessageLoopAura(); > clang might warn about having a non-trivial compiler generated inline destructor > - maybe good just to add one anyway with an empty body in the .cc > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_mac.h (right): > > https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_mac.h:25: > DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopAura); > MenuMessageLoopAura -> MenuMessageLoopMac > > I.. think this hides the default constructor too. I'd just add empty definitions > of the constructor and destructor - it's a good habit for any class that isn't > just an interface (compiler-generated destructors are really bloaty when > inlined) > > https://codereview.chromium.org/267593005/diff/130001/ui/views/widget/widget.h > File ui/views/widget/widget.h (right): > > https://codereview.chromium.org/267593005/diff/130001/ui/views/widget/widget.... > ui/views/widget/widget.h:499: // Sets whether a shadow should be drawn around > the window. > nit: I think this needs a more verbose comment. Something like > > // Informs the window manager whether to create a rectangular shadow around > // the window, and keep it updated in response to activation changes. Done. Fixed compile issue on Windows and unit test failure on ChromeOS. owner_ should not be cleared until we exit all nested message loops. Starting a new round of try jobs.
https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/display_change_listener_mac.cc (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/display_change_listener_mac.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/08 00:25:28, tapted wrote: > nit: no (c) Done. https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/menu_config_mac.cc (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/menu_config_mac.cc:16: // TODO: Tune these parameters for Mac. On 2014/05/08 00:25:28, tapted wrote: > nit: TODOs should have a username in brackets - usually the person who added it > :). > > To minimise churn, It may even be better just setting a couple of these (i.e. > just so it's usable), and emitting NOTIMPLEMENTED() Ok, deleted them and replaced with NOTIMPLEMENTED(). It's not clear what needs to be set to make it usable since I can't run it on the mac yet. I will add them as needed when I bring up the mac menu. Done. https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.h (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.h:20: MenuMessageLoopAura(); On 2014/05/08 00:25:28, tapted wrote: > clang might warn about having a non-trivial compiler generated inline destructor > - maybe good just to add one anyway with an empty body in the .cc Done. https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_mac.h (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_mac.h:25: DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopAura); On 2014/05/08 00:25:28, tapted wrote: > MenuMessageLoopAura -> MenuMessageLoopMac > > I.. think this hides the default constructor too. I'd just add empty definitions > of the constructor and destructor - it's a good habit for any class that isn't > just an interface (compiler-generated destructors are really bloaty when > inlined) Done. Thanks for the tip! https://codereview.chromium.org/267593005/diff/130001/ui/views/widget/widget.h File ui/views/widget/widget.h (right): https://codereview.chromium.org/267593005/diff/130001/ui/views/widget/widget.... ui/views/widget/widget.h:499: // Sets whether a shadow should be drawn around the window. On 2014/05/08 00:25:28, tapted wrote: > nit: I think this needs a more verbose comment. Something like > > // Informs the window manager whether to create a rectangular shadow around > // the window, and keep it updated in response to activation changes. Done.
slgtm with the following https://codereview.chromium.org/267593005/diff/200001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_mac.cc (right): https://codereview.chromium.org/267593005/diff/200001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_mac.cc:17: MenuMessageLoopMac::MenuMessageLoopMac() {} message_loop_depth_ needs an initializer
On 2014/05/09 03:07:57, tapted wrote: > slgtm with the following > > https://codereview.chromium.org/267593005/diff/200001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_mac.cc (right): > > https://codereview.chromium.org/267593005/diff/200001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_mac.cc:17: > MenuMessageLoopMac::MenuMessageLoopMac() {} > message_loop_depth_ needs an initializer Oops! It has been awhile since I worked with C++ :-)
https://codereview.chromium.org/267593005/diff/200001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_mac.cc (right): https://codereview.chromium.org/267593005/diff/200001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_mac.cc:17: MenuMessageLoopMac::MenuMessageLoopMac() {} On 2014/05/09 03:07:58, tapted wrote: > message_loop_depth_ needs an initializer Done.
Ben, I think this is ready for you to review. Please take a look.
https://codereview.chromium.org/267593005/diff/240001/ui/views/controls/menu/... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/267593005/diff/240001/ui/views/controls/menu/... ui/views/controls/menu/menu_host.cc:77: ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); The first argument is ignored if the second argument is NULL.
note there's a recent change in http://crrev.com/269819 that will need to be incorporated.
menu: -me, +sky
On 2014/05/14 19:09:44, Ben Goodger (Google) wrote: > menu: -me, +sky +sadrul, I see you are working on https://codereview.chromium.org/280483003 which is touching the same code.
How about pulling SetHasActivationShadow into its own cl since it's not really related to the other menu changes. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:791: owner_ = NULL; I think you should NULL out owner_ in MenuMessageLoopAura here too. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1091: message_loop_->Run(this, owner_, nested_menu); Inline this and remove the RunMessageLoop function (it's only called from one place). https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:37: virtual bool ShouldQuitNow() const = 0; Add description for functions. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:41: gfx::Point screen_loc) = 0; const gfx::Point& https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:45: virtual int message_loop_depth() const = 0; Don't use unix_hacker_style for virtual methods (see style guide). Also, is there really a reason to move this to MenuMessageLoop? It appears to be the same between the two implementations and can just as easily be tracked in MenuController. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:105: : owner_(NULL), message_loop_depth_(0) { when you wrap, each param on its own line. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:171: base::MessageLoopForUI* loop = base::MessageLoopForUI::current(); How about a common function that has these four lines in it? Both 151-154 and 171-174 use it. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.h:23: virtual void Run(MenuController* controller, Prefix section with something indicating these are overrides, eg // MenuMessageLoop overrides: or just // MenuMessageLoop: https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_mac.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_mac.h:18: virtual void Run(MenuController* controller, Same comment about prefixing section. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_mac.h:30: DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopMac); nit: newline between 29/30.
> +sadrul, I see you are working on https://codereview.chromium.org/280483003 > which is touching the same code. That CL still needs to be merged with ToT (and I notice some related try failures on windows, so might need additional work). From a quick look at your CL, it doesn't look like it will conflict a lot. So please feel free to land your CL with sky@s approval, and I will update my CL accordingly. Somewhat related: I remember some work being done to have aura going on mac too. Is there a reason we are doing non-aura views on mac? (I realize this is not really related to this CL, but wondering if/where that discussion already happened) https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_host.cc:77: ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); This looks suspicious. Can you explain this change?
On Thu, May 15, 2014 at 9:53 AM, <sadrul@chromium.org> wrote: >> +sadrul, I see you are working on >> https://codereview.chromium.org/280483003 >> which is touching the same code. > > > That CL still needs to be merged with ToT (and I notice some related try > failures on windows, so might need additional work). From a quick look at > your > CL, it doesn't look like it will conflict a lot. So please feel free to land > your CL with sky@s approval, and I will update my CL accordingly. > > Somewhat related: I remember some work being done to have aura going on mac > too. > Is there a reason we are doing non-aura views on mac? (I realize this is not > really related to this CL, but wondering if/where that discussion already > happened) I'll reinterpret what Sadrul is requesting here. Please send an email to chromium-dev documenting what you guys are doing in the short term, long term and what that means for the ifdefs we have. -Scott > > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_host.cc (right): > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > > ui/views/controls/menu/menu_host.cc:77: > ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); > This looks suspicious. Can you explain this change? > > https://codereview.chromium.org/267593005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/15 16:53:08, sadrul wrote: > > +sadrul, I see you are working on https://codereview.chromium.org/280483003 > > which is touching the same code. > > That CL still needs to be merged with ToT (and I notice some related try > failures on windows, so might need additional work). From a quick look at your > CL, it doesn't look like it will conflict a lot. So please feel free to land > your CL with sky@s approval, and I will update my CL accordingly. > > Somewhat related: I remember some work being done to have aura going on mac too. > Is there a reason we are doing non-aura views on mac? (I realize this is not > really related to this CL, but wondering if/where that discussion already > happened) > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_host.cc (right): > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_host.cc:77: > ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); > This looks suspicious. Can you explain this change? Thanks Sadrul. That discussion is documented here: https://docs.google.com/a/google.com/document/d/1pPTQ0ggIa5nEr0SNHtaJftN1CigN... Of specific interest might be the section in page 4 titled "Meeting with beng@ and sky@" Please also take a look at the design doc: https://docs.google.com/a/google.com/document/d/1VYUeqaM0oFRweeMmS75vCcobhXQB... I will discuss with the team how to communicate this to a wider audience.
On 2014/05/15 16:07:31, sky wrote: > How about pulling SetHasActivationShadow into its own cl since it's not really > related to the other menu changes. > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_controller.cc (right): > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_controller.cc:791: owner_ = NULL; > I think you should NULL out owner_ in MenuMessageLoopAura here too. > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_controller.cc:1091: message_loop_->Run(this, owner_, > nested_menu); > Inline this and remove the RunMessageLoop function (it's only called from one > place). > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop.h (right): > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop.h:37: virtual bool ShouldQuitNow() > const = 0; > Add description for functions. > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop.h:41: gfx::Point screen_loc) = 0; > const gfx::Point& > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop.h:45: virtual int message_loop_depth() > const = 0; > Don't use unix_hacker_style for virtual methods (see style guide). Also, is > there really a reason to move this to MenuMessageLoop? It appears to be the same > between the two implementations and can just as easily be tracked in > MenuController. > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_aura.cc (right): > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_aura.cc:105: : owner_(NULL), > message_loop_depth_(0) { > when you wrap, each param on its own line. > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_aura.cc:171: base::MessageLoopForUI* > loop = base::MessageLoopForUI::current(); > How about a common function that has these four lines in it? Both 151-154 and > 171-174 use it. > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_aura.h (right): > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_aura.h:23: virtual void > Run(MenuController* controller, > Prefix section with something indicating these are overrides, eg > // MenuMessageLoop overrides: > or just > // MenuMessageLoop: > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_message_loop_mac.h (right): > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_mac.h:18: virtual void > Run(MenuController* controller, > Same comment about prefixing section. > > https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... > ui/views/controls/menu/menu_message_loop_mac.h:30: > DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopMac); > nit: newline between 29/30. Uploaded updated patch, sky please take a look. The SetHasActivationShadow changes is moved to a separate CL: https://codereview.chromium.org/290553002/
https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:791: owner_ = NULL; On 2014/05/15 16:07:32, sky wrote: > I think you should NULL out owner_ in MenuMessageLoopAura here too. MenuMessageLoop clears owner_ when exiting the loop. Are you thinking about a case where the widget destroys while the menu loop is still running? Should I add SetOwner() to MenuMessageLoop and call it with NULL here? Or should we tell the loop to exit here if the widget is destroying? https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:1091: message_loop_->Run(this, owner_, nested_menu); On 2014/05/15 16:07:32, sky wrote: > Inline this and remove the RunMessageLoop function (it's only called from one > place). The RunMessageLoop public method is also called by MenuControllerTest. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_host.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_host.cc:77: ui::GestureRecognizer::Get()->TransferEventsTo(NULL, NULL); On 2014/05/15 16:53:09, sadrul wrote: > This looks suspicious. Can you explain this change? Ah yes, there was an attached comment on an earlier patch, but I've uploaded new patches since then so that comment is not showing. The first argument is ignored if the second argument is NULL, it means that all event targets are canceled. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:37: virtual bool ShouldQuitNow() const = 0; On 2014/05/15 16:07:32, sky wrote: > Add description for functions. Done. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:41: gfx::Point screen_loc) = 0; On 2014/05/15 16:07:32, sky wrote: > const gfx::Point& Done. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:45: virtual int message_loop_depth() const = 0; On 2014/05/15 16:07:32, sky wrote: > Don't use unix_hacker_style for virtual methods (see style guide). Also, is > there really a reason to move this to MenuMessageLoop? It appears to be the same > between the two implementations and can just as easily be tracked in > MenuController. Done. Renamed the method to GetNestedDepth(). This was moved here because we can only clear owner_ after we exited all nested loops, in MenuMessageLoopAura:Run, message_loop_depth_--; if (message_loop_depth_ == 0) owner_ = NULL; https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:105: : owner_(NULL), message_loop_depth_(0) { On 2014/05/15 16:07:32, sky wrote: > when you wrap, each param on its own line. Done. Now fits in one line after the name change to nested_depth_. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.cc:171: base::MessageLoopForUI* loop = base::MessageLoopForUI::current(); On 2014/05/15 16:07:32, sky wrote: > How about a common function that has these four lines in it? Both 151-154 and > 171-174 use it. Lines 153 and 173 are different, and nested_dispatcher is a MessagePumpDispatcher that is a window only type. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_aura.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_aura.h:23: virtual void Run(MenuController* controller, On 2014/05/15 16:07:32, sky wrote: > Prefix section with something indicating these are overrides, eg > // MenuMessageLoop overrides: > or just > // MenuMessageLoop: Done. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop_mac.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop_mac.h:30: DISALLOW_COPY_AND_ASSIGN(MenuMessageLoopMac); On 2014/05/15 16:07:32, sky wrote: > nit: newline between 29/30. Done.
https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:791: owner_ = NULL; On 2014/05/15 18:45:59, Andre wrote: > On 2014/05/15 16:07:32, sky wrote: > > I think you should NULL out owner_ in MenuMessageLoopAura here too. > > MenuMessageLoop clears owner_ when exiting the loop. > Are you thinking about a case where the widget destroys while the menu loop is > still running? Exactly. I don't like it that MenuMessageLoop may be holding a Widget that has been deleted. > Should I add SetOwner() to MenuMessageLoop and call it with NULL here? > Or should we tell the loop to exit here if the widget is destroying? Go with the SetOwner for now. I believe quitting is already handled. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:45: virtual int message_loop_depth() const = 0; On 2014/05/15 18:45:59, Andre wrote: > On 2014/05/15 16:07:32, sky wrote: > > Don't use unix_hacker_style for virtual methods (see style guide). Also, is > > there really a reason to move this to MenuMessageLoop? It appears to be the > same > > between the two implementations and can just as easily be tracked in > > MenuController. > > Done. > Renamed the method to GetNestedDepth(). > This was moved here because we can only clear owner_ after we exited all nested > loops, in MenuMessageLoopAura:Run, > message_loop_depth_--; > if (message_loop_depth_ == 0) > owner_ = NULL; Do you still need it once you get rid of SetOwner?
https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:791: owner_ = NULL; On 2014/05/15 19:33:01, sky wrote: > On 2014/05/15 18:45:59, Andre wrote: > > On 2014/05/15 16:07:32, sky wrote: > > > I think you should NULL out owner_ in MenuMessageLoopAura here too. > > > > MenuMessageLoop clears owner_ when exiting the loop. > > Are you thinking about a case where the widget destroys while the menu loop is > > still running? > > Exactly. I don't like it that MenuMessageLoop may be holding a Widget that has > been deleted. > > > Should I add SetOwner() to MenuMessageLoop and call it with NULL here? > > Or should we tell the loop to exit here if the widget is destroying? > > Go with the SetOwner for now. I believe quitting is already handled. Done. I went with ClearOwner because owner was passed in as part of the call to Run() and it doesn't make sense to change while running. https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... File ui/views/controls/menu/menu_message_loop.h (right): https://codereview.chromium.org/267593005/diff/330001/ui/views/controls/menu/... ui/views/controls/menu/menu_message_loop.h:45: virtual int message_loop_depth() const = 0; On 2014/05/15 19:33:01, sky wrote: > On 2014/05/15 18:45:59, Andre wrote: > > On 2014/05/15 16:07:32, sky wrote: > > > Don't use unix_hacker_style for virtual methods (see style guide). Also, is > > > there really a reason to move this to MenuMessageLoop? It appears to be the > > same > > > between the two implementations and can just as easily be tracked in > > > MenuController. > > > > Done. > > Renamed the method to GetNestedDepth(). > > This was moved here because we can only clear owner_ after we exited all > nested > > loops, in MenuMessageLoopAura:Run, > > message_loop_depth_--; > > if (message_loop_depth_ == 0) > > owner_ = NULL; > > Do you still need it once you get rid of SetOwner? No longer needed with the addition of ClearOwner. Moved it back to MenuController.
LGTM
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/267593005/37...
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
Message was sent while issue was closed.
Change committed as 270881 |
