Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
331993009:
MacViews: Run native Cocoa context menus to support Services. (Closed)
|
Created:
6 years, 6 months ago by Andre Modified:
6 years, 5 months ago CC:
chromium-reviews, tfarina, mac-views-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMacViews: Run native Cocoa context menus to support Services.
The way AppKit populate context menus with Services items is not
exposed, so it is not currently possible to get Mac Services integration
without using NSMenu.
With this change we now use NSMenu when running context menus, while
still running the non-native views one everywhere else like Combobox.
This is achieved by splitting the interface of MenuRunnerImpl into an
abstract MenuRunnerImplInterface, and making a concrete implementation
MenuRunnerImplCocoa that runs a native context menu.
BUG=388455
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283999
Patch Set 1 : #
Total comments: 19
Patch Set 2 : Try moving run types to constructor #Patch Set 3 : #Patch Set 4 : Add unit test. #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : Rebase, don't run native menu when nested #Patch Set 8 : Add VIEWS_EXPORT for unit test access #
Total comments: 11
Patch Set 9 : Rebase. Fixes for sky. #
Total comments: 2
Patch Set 10 : Rebase off 393943006 #Patch Set 11 : Rebase to master #Messages
Total messages: 38 (0 generated)
Trent, can I get your opinion on this approach?
lg at a high level - I think this is a pretty nice approach. I'll have a deeper look today. https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner.h:137: internal::MenuRunnerImplInterface* CreateNativeImpl(); Id probably try to have MenuRunnerImplInterface::CreateForType(ui::MenuModel* model, int32 type); which can return NULL and have (in menu_runner_impl.cc) #if !defined(OS_MACOSX) MenuRunnerImplInterface* MenuRunnerImplInterface::CreateForType(ui::MenuModel* model, int32 type) { return NULL; } #endif then (somewhere) .. (..) { if (menu_model_) holder_ = MenuRunnerImplInterface::CreateForType(menu_model_, type); if (!holder_) { /* create adapter */ holder_ = new MenuRunnerImplInterface(things); } } but I'll have a deeper look to see if this will work. (ideally holder_ will always be non-null)
We should add some tests for this too (maybe after getting a goahead from sky), but I don't see any fundamental issues https://codereview.chromium.org/331993009/diff/40001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:186: BOOL valid = textInputClient_ && ((canWrite && (canRead || !returnType)) || This condition does my head in a bit ;) Maybe (assuming I understood it) if (returnType && !canRead) canWrite = false; if (sendType && !canWrite) canRead = false; if (textInputClient_ && (canRead || canWrite)) return self; return [super validRequestorForSendType:sendType returnType:returnType]; https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner.cc:16: : menu_model_(menu_model), menu_view_(NULL), holder_(NULL) { What are the prospects of refactoring call sites so that `types` is moved to a MenuRunner constructor argument, or into the menu model itself? Then I think you can still initialize holder_ here Something like holder_(MenuRunnerImplInterface::Create(menu_model, NULL)) and in the other constructor holder_(MenuRunnerImplInterface::Create(NULL, menu_view)) then with that stuff from my earlier comment - something like MenuRunnerImplInterface* MenuRunnerImplInterface::Create(.., ..) { MenuRunnerImplInterface* holder = NULL; if (menu_model) holder = MenuRunnerImplInterface::CreateForType(menu_model_, type); if (holder) return holder; /* create adapter */ return new MenuRunnerImplInterface(things); } but then if there's a caller that uses the same MenuRunner for both context menus and something else we might be breaking a valid use case. But I think the lazy-holder_-init approach would be breaking that anyway, so better to make it explicit. https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl.cc (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl.cc:33: return running_; can this just do holder_->running()? https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl.h (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl.h:24: class MenuRunnerImpl : public MenuRunnerImplInterface, nit: needs a class comment https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl_cocoa.h (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_cocoa.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. This separate header file might not be needed if you move MenuRunnerImplInterface* MenuRunnerImplInterface::CreateForType(ui::MenuModel* model, int32 type) {..} into menu_runner_impl_cocoa.mm (but we might still want a separate header if it helps with clarity/testing) https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_cocoa.h:10: #include "base/mac/scoped_nsobject.h" nit: import https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_cocoa.h:18: class MenuRunnerImplCocoa : public MenuRunnerImplInterface { nit: should proabbly have a class comment https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_cocoa.mm:5: #include "ui/views/controls/menu/menu_runner_impl_cocoa.h" nit: import https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_cocoa.mm:7: #include "ui/base/cocoa/menu_controller.h" nit: import https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl_interface.h (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_interface.h:19: virtual bool running() const = 0; nit: comment https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_interface.h:30: virtual void Cancel() = 0; nit: comment
Trent, I'm still not done with this but I've made more changes so you might want to take a look in the mean time. https://codereview.chromium.org/331993009/diff/40001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:186: BOOL valid = textInputClient_ && ((canWrite && (canRead || !returnType)) || On 2014/06/25 08:30:28, tapted wrote: > This condition does my head in a bit ;) > > Maybe (assuming I understood it) > > if (returnType && !canRead) > canWrite = false; > > if (sendType && !canWrite) > canRead = false; > > if (textInputClient_ && (canRead || canWrite)) > return self; > > return [super validRequestorForSendType:sendType returnType:returnType]; Yeah I didn't really like it either, but I'm also not so sure about the alternative. I'll play around with it some more. https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner.cc:16: : menu_model_(menu_model), menu_view_(NULL), holder_(NULL) { On 2014/06/25 08:30:28, tapted wrote: > What are the prospects of refactoring call sites so that `types` is moved to a > MenuRunner constructor argument, or into the menu model itself? > > > Then I think you can still initialize holder_ here > > Something like > > holder_(MenuRunnerImplInterface::Create(menu_model, NULL)) > > and in the other constructor > > holder_(MenuRunnerImplInterface::Create(NULL, menu_view)) > > > then with that stuff from my earlier comment - something like > > MenuRunnerImplInterface* MenuRunnerImplInterface::Create(.., ..) { > MenuRunnerImplInterface* holder = NULL; > if (menu_model) > holder = MenuRunnerImplInterface::CreateForType(menu_model_, type); > > if (holder) > return holder; > > /* create adapter */ > return new MenuRunnerImplInterface(things); > } > > > > but then if there's a caller that uses the same MenuRunner for both context > menus and something else we might be breaking a valid use case. But I think the > lazy-holder_-init approach would be breaking that anyway, so better to make it > explicit. It's a lot of changes, but looks doable. I've uploaded a patch and sending it to the trybot to check if I got them all. I also made MenuRunnerImplAdapter to keep the adaptation nicely separated. https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl.cc (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl.cc:33: return running_; On 2014/06/25 08:30:28, tapted wrote: > can this just do holder_->running()? I'm not sure what you mean, we are the holder_ here. https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_cocoa.mm:5: #include "ui/views/controls/menu/menu_runner_impl_cocoa.h" On 2014/06/25 08:30:28, tapted wrote: > nit: import Done. https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl_cocoa.mm:7: #include "ui/base/cocoa/menu_controller.h" On 2014/06/25 08:30:28, tapted wrote: > nit: import Done.
I think it's looking good - the approach looks promising. We just need to get sky on board :). Maybe worth looping him in for advice before spending time on tests. https://codereview.chromium.org/331993009/diff/40001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:186: BOOL valid = textInputClient_ && ((canWrite && (canRead || !returnType)) || On 2014/06/27 01:18:08, Andre wrote: > On 2014/06/25 08:30:28, tapted wrote: > > This condition does my head in a bit ;) > > > > Maybe (assuming I understood it) > > > > if (returnType && !canRead) > > canWrite = false; > > > > if (sendType && !canWrite) > > canRead = false; > > > > if (textInputClient_ && (canRead || canWrite)) > > return self; > > > > return [super validRequestorForSendType:sendType returnType:returnType]; > > Yeah I didn't really like it either, but I'm also not so sure about the > alternative. > I'll play around with it some more. hm I realised the alternative isn't quite the same too since canWrite could change for the second condition. Maybe just a comment describing the logic at a higher level. https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... File ui/views/controls/menu/menu_runner_impl.cc (right): https://codereview.chromium.org/331993009/diff/40001/ui/views/controls/menu/m... ui/views/controls/menu/menu_runner_impl.cc:33: return running_; On 2014/06/27 01:18:08, Andre wrote: > On 2014/06/25 08:30:28, tapted wrote: > > can this just do holder_->running()? > > I'm not sure what you mean, we are the holder_ here. oops - the diff probably threw me https://codereview.chromium.org/331993009/diff/220001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/331993009/diff/220001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.h:96: explicit MenuRunner(ui::MenuModel* menu_model, int32 types); nit: explicit not needed https://codereview.chromium.org/331993009/diff/220001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.h:108: // Runs the menu. |types| is a bitmask of RunTypes. If this returns comment wrt |types| needs to move up https://codereview.chromium.org/331993009/diff/220001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.h:135: int32 types_; maybe const? perhaps also run_types_ is clearer
sky, can we get your input on this? It's not quite complete but we'd like to get your opinion before continuing.
If we go this route, how will the nested message loop be impacted when the system menus get shown? Is there any weird interaction there? This also means that context menus on macs couldn't be customized. How often is the services menu used by context menus on the mac?
It may not be used often, but for those who do use it, this type of OS integration is not something we can bear to lose. It's likely only going to get more useful with Extensions in Yosemite as well. On Wed, Jul 9, 2014 at 1:20 PM, <sky@chromium.org> wrote: > If we go this route, how will the nested message loop be impacted when the > system menus get shown? Is there any weird interaction there? > > This also means that context menus on macs couldn't be customized. How > often is > the services menu used by context menus on the mac? > > https://codereview.chromium.org/331993009/ > -- Mike Pinkerton Mac Weenie pinkerton@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/09 17:20:39, sky wrote: > If we go this route, how will the nested message loop be impacted when the > system menus get shown? Is there any weird interaction there? > > This also means that context menus on macs couldn't be customized. How often is > the services menu used by context menus on the mac? AFAICT we currently don't have a case where a nested menu is run while running a context menu. Running the system context menu nested inside a views menu should work (such as right clicking a bookmark folder item). The Services menu is shown when there is a selection and something in the responder chain says that it can handle that selection type. So I think it's pretty important in the case of selecting text in a textfield, but it won't be there when showing a context menu for a bookmark folder item, we can choose to show the views context menu in this case. One option is to show the system menu only when CONTEXT_MENU is set and IS_NESTED is not set, that way we can customize the menu in case of bookmark folder menu and the extension overflow menu. Later on, we can also choose to show the views menu in cases where we know Services is not needed, such as the context menu for extension action buttons, but we may need to pass additional flags to the menu runner for this.
Do we show services in all context menus, or just certain ones? On Wed, Jul 9, 2014 at 10:46 AM, Mike Pinkerton <pinkerton@chromium.org> wrote: > It may not be used often, but for those who do use it, this type of OS > integration is not something we can bear to lose. It's likely only going to > get more useful with Extensions in Yosemite as well. > > > On Wed, Jul 9, 2014 at 1:20 PM, <sky@chromium.org> wrote: > >> If we go this route, how will the nested message loop be impacted when the >> system menus get shown? Is there any weird interaction there? >> >> This also means that context menus on macs couldn't be customized. How >> often is >> the services menu used by context menus on the mac? >> >> https://codereview.chromium.org/331993009/ >> > > > > -- > Mike Pinkerton > Mac Weenie > pinkerton@google.com > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Jul 9, 2014 at 11:26 AM, <andresantoso@chromium.org> wrote: > On 2014/07/09 17:20:39, sky wrote: > >> If we go this route, how will the nested message loop be impacted when the >> system menus get shown? Is there any weird interaction there? >> > > This also means that context menus on macs couldn't be customized. How >> often >> > is > >> the services menu used by context menus on the mac? >> > > AFAICT we currently don't have a case where a nested menu is run while > running > a context menu. I meant showing a context menu while one of our other menus is shown. For example, show the wrench menu than right click on a bookmark to get the context menu for the bookmark. If we made context menus use system menus this would mean we have both a non-native menu and a native menu showing. I wasn't sure if this would cause problems. Not allowing this to happen, meaning if a menu is already showing we never show a native menu is one way to deal with it. Hence my questions about the importance of this. > Running the system context menu nested inside a views menu > should > work (such as right clicking a bookmark folder item). > The Services menu is shown when there is a selection and something in the > responder > chain says that it can handle that selection type. So I think it's pretty > important in the > case of selecting text in a textfield, but it won't be there when showing a > context menu > for a bookmark folder item, we can choose to show the views context menu > in this > case. > Why wouldn't services show when right clicking on a bookmark menu item? Can't responders handle urls on the pasteboard? -Scott > One option is to show the system menu only when CONTEXT_MENU is set and > IS_NESTED > is not set, that way we can customize the menu in case of bookmark folder > menu > and > the extension overflow menu. Later on, we can also choose to show the > views menu > in > cases where we know Services is not needed, such as the context menu for > extension > action buttons, but we may need to pass additional flags to the menu > runner for > this. > > > https://codereview.chromium.org/331993009/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/09 21:47:33, sky wrote: > On Wed, Jul 9, 2014 at 11:26 AM, <mailto:andresantoso@chromium.org> wrote: > > > On 2014/07/09 17:20:39, sky wrote: > > > >> If we go this route, how will the nested message loop be impacted when the > >> system menus get shown? Is there any weird interaction there? > >> > > > > This also means that context menus on macs couldn't be customized. How > >> often > >> > > is > > > >> the services menu used by context menus on the mac? > >> > > > > AFAICT we currently don't have a case where a nested menu is run while > > running > > a context menu. > > > I meant showing a context menu while one of our other menus is shown. For > example, show the wrench menu than right click on a bookmark to get the > context menu for the bookmark. If we made context menus use system menus > this would mean we have both a non-native menu and a native menu showing. I > wasn't sure if this would cause problems. Not allowing this to happen, > meaning if a menu is already showing we never show a native menu is one way > to deal with it. Hence my questions about the importance of this. Please take a look at patch 7 where native menu is now shown only when IS_NESTED is not set. > > > > Running the system context menu nested inside a views menu > > should > > work (such as right clicking a bookmark folder item). > > The Services menu is shown when there is a selection and something in the > > responder > > chain says that it can handle that selection type. So I think it's pretty > > important in the > > case of selecting text in a textfield, but it won't be there when showing a > > context menu > > for a bookmark folder item, we can choose to show the views context menu > > in this > > case. > > > > Why wouldn't services show when right clicking on a bookmark menu item? > Can't responders handle urls on the pasteboard? We can make the bookmark view declare that it can write URLs to the pasteboard to make Service show in this case, but Mac Chrome does not currently do this so it should be ok to show the views nested menu. > > -Scott > > > > One option is to show the system menu only when CONTEXT_MENU is set and > > IS_NESTED > > is not set, that way we can customize the menu in case of bookmark folder > > menu > > and > > the extension overflow menu. Later on, we can also choose to show the > > views menu > > in > > cases where we know Services is not needed, such as the context menu for > > extension > > action buttons, but we may need to pass additional flags to the menu > > runner for > > this. > > > > > > https://codereview.chromium.org/331993009/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/07/09 23:23:00, Andre wrote: > On 2014/07/09 21:47:33, sky wrote: > > On Wed, Jul 9, 2014 at 11:26 AM, <mailto:andresantoso@chromium.org> wrote: > > > > > On 2014/07/09 17:20:39, sky wrote: > > > > > >> If we go this route, how will the nested message loop be impacted when the > > >> system menus get shown? Is there any weird interaction there? > > >> > > > > > > This also means that context menus on macs couldn't be customized. How > > >> often > > >> > > > is > > > > > >> the services menu used by context menus on the mac? > > >> > > > > > > AFAICT we currently don't have a case where a nested menu is run while > > > running > > > a context menu. > > > > > > I meant showing a context menu while one of our other menus is shown. For > > example, show the wrench menu than right click on a bookmark to get the > > context menu for the bookmark. If we made context menus use system menus > > this would mean we have both a non-native menu and a native menu showing. I > > wasn't sure if this would cause problems. Not allowing this to happen, > > meaning if a menu is already showing we never show a native menu is one way > > to deal with it. Hence my questions about the importance of this. > > Please take a look at patch 7 where native menu is now shown only when > IS_NESTED is not set. > > > > > > > > Running the system context menu nested inside a views menu > > > should > > > work (such as right clicking a bookmark folder item). > > > The Services menu is shown when there is a selection and something in the > > > responder > > > chain says that it can handle that selection type. So I think it's pretty > > > important in the > > > case of selecting text in a textfield, but it won't be there when showing a > > > context menu > > > for a bookmark folder item, we can choose to show the views context menu > > > in this > > > case. > > > > > > > Why wouldn't services show when right clicking on a bookmark menu item? > > Can't responders handle urls on the pasteboard? > > We can make the bookmark view declare that it can write URLs to the > pasteboard to make Service show in this case, but Mac Chrome does not > currently do this so it should be ok to show the views nested menu. > > > > > -Scott > > > > > > > One option is to show the system menu only when CONTEXT_MENU is set and > > > IS_NESTED > > > is not set, that way we can customize the menu in case of bookmark folder > > > menu > > > and > > > the extension overflow menu. Later on, we can also choose to show the > > > views menu > > > in > > > cases where we know Services is not needed, such as the context menu for > > > extension > > > action buttons, but we may need to pass additional flags to the menu > > > runner for > > > this. > > > > > > > > > https://codereview.chromium.org/331993009/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. Scott, WDYT?
On Sun, Jul 13, 2014 at 11:51 PM, <andresantoso@chromium.org> wrote: > On 2014/07/09 23:23:00, Andre wrote: > >> On 2014/07/09 21:47:33, sky wrote: >> >> > On Wed, Jul 9, 2014 at 11:26 AM, <mailto:andresantoso@chromium.org> >> wrote: >> > >> > > On 2014/07/09 17:20:39, sky wrote: >> > > >> > >> If we go this route, how will the nested message loop be impacted >> when >> > the > >> > >> system menus get shown? Is there any weird interaction there? >> > >> >> > > >> > > This also means that context menus on macs couldn't be customized. >> How >> > >> often >> > >> >> > > is >> > > >> > >> the services menu used by context menus on the mac? >> > >> >> > > >> > > AFAICT we currently don't have a case where a nested menu is run while >> > > running >> > > a context menu. >> > >> > >> > I meant showing a context menu while one of our other menus is shown. >> For >> > example, show the wrench menu than right click on a bookmark to get the >> > context menu for the bookmark. If we made context menus use system menus >> > this would mean we have both a non-native menu and a native menu >> showing. I >> > wasn't sure if this would cause problems. Not allowing this to happen, >> > meaning if a menu is already showing we never show a native menu is one >> way >> > to deal with it. Hence my questions about the importance of this. >> > > Please take a look at patch 7 where native menu is now shown only when >> IS_NESTED is not set. >> > > > >> > >> > > Running the system context menu nested inside a views menu >> > > should >> > > work (such as right clicking a bookmark folder item). >> > > The Services menu is shown when there is a selection and something in >> the >> > > responder >> > > chain says that it can handle that selection type. So I think it's >> pretty >> > > important in the >> > > case of selecting text in a textfield, but it won't be there when >> showing >> > a > >> > > context menu >> > > for a bookmark folder item, we can choose to show the views context >> menu >> > > in this >> > > case. >> > > >> > >> > Why wouldn't services show when right clicking on a bookmark menu item? >> > Can't responders handle urls on the pasteboard? >> > > We can make the bookmark view declare that it can write URLs to the >> pasteboard to make Service show in this case, but Mac Chrome does not >> currently do this so it should be ok to show the views nested menu. >> > > > >> > -Scott >> > >> > >> > > One option is to show the system menu only when CONTEXT_MENU is set >> and >> > > IS_NESTED >> > > is not set, that way we can customize the menu in case of bookmark >> folder >> > > menu >> > > and >> > > the extension overflow menu. Later on, we can also choose to show the >> > > views menu >> > > in >> > > cases where we know Services is not needed, such as the context menu >> for >> > > extension >> > > action buttons, but we may need to pass additional flags to the menu >> > > runner for >> > > this. >> > > >> > > >> > > https://codereview.chromium.org/331993009/ >> > > >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Scott, WDYT? > Sorry, I some how didn't see your earlier response. For clarity, what menus end up showing the services menu? Is it only the context menu shown when right clicking on content? -Scott > https://codereview.chromium.org/331993009/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Also the omnibox (since it's a text field). On Mon, Jul 14, 2014 at 1:09 PM, Scott Violet <sky@chromium.org> wrote: > > > > On Sun, Jul 13, 2014 at 11:51 PM, <andresantoso@chromium.org> wrote: > >> On 2014/07/09 23:23:00, Andre wrote: >> >>> On 2014/07/09 21:47:33, sky wrote: >>> >>> > On Wed, Jul 9, 2014 at 11:26 AM, <mailto:andresantoso@chromium.org> >>> wrote: >>> > >>> > > On 2014/07/09 17:20:39, sky wrote: >>> > > >>> > >> If we go this route, how will the nested message loop be impacted >>> when >>> >> the >> >>> > >> system menus get shown? Is there any weird interaction there? >>> > >> >>> > > >>> > > This also means that context menus on macs couldn't be customized. >>> How >>> > >> often >>> > >> >>> > > is >>> > > >>> > >> the services menu used by context menus on the mac? >>> > >> >>> > > >>> > > AFAICT we currently don't have a case where a nested menu is run >>> while >>> > > running >>> > > a context menu. >>> > >>> > >>> > I meant showing a context menu while one of our other menus is shown. >>> For >>> > example, show the wrench menu than right click on a bookmark to get the >>> > context menu for the bookmark. If we made context menus use system >>> menus >>> > this would mean we have both a non-native menu and a native menu >>> showing. I >>> > wasn't sure if this would cause problems. Not allowing this to happen, >>> > meaning if a menu is already showing we never show a native menu is >>> one way >>> > to deal with it. Hence my questions about the importance of this. >>> >> >> Please take a look at patch 7 where native menu is now shown only when >>> IS_NESTED is not set. >>> >> >> > >>> > >>> > > Running the system context menu nested inside a views menu >>> > > should >>> > > work (such as right clicking a bookmark folder item). >>> > > The Services menu is shown when there is a selection and something >>> in the >>> > > responder >>> > > chain says that it can handle that selection type. So I think it's >>> pretty >>> > > important in the >>> > > case of selecting text in a textfield, but it won't be there when >>> showing >>> >> a >> >>> > > context menu >>> > > for a bookmark folder item, we can choose to show the views context >>> menu >>> > > in this >>> > > case. >>> > > >>> > >>> > Why wouldn't services show when right clicking on a bookmark menu item? >>> > Can't responders handle urls on the pasteboard? >>> >> >> We can make the bookmark view declare that it can write URLs to the >>> pasteboard to make Service show in this case, but Mac Chrome does not >>> currently do this so it should be ok to show the views nested menu. >>> >> >> > >>> > -Scott >>> > >>> > >>> > > One option is to show the system menu only when CONTEXT_MENU is set >>> and >>> > > IS_NESTED >>> > > is not set, that way we can customize the menu in case of bookmark >>> folder >>> > > menu >>> > > and >>> > > the extension overflow menu. Later on, we can also choose to show the >>> > > views menu >>> > > in >>> > > cases where we know Services is not needed, such as the context menu >>> for >>> > > extension >>> > > action buttons, but we may need to pass additional flags to the menu >>> > > runner for >>> > > this. >>> > > >>> > > >>> > > https://codereview.chromium.org/331993009/ >>> > > >>> > >>> > To unsubscribe from this group and stop receiving emails from it, send >>> an >>> email >>> > to mailto:chromium-reviews+unsubscribe@chromium.org. >>> >> >> Scott, WDYT? >> > > Sorry, I some how didn't see your earlier response. For clarity, what > menus end up showing the services menu? Is it only the context menu shown > when right clicking on content? > > -Scott > > >> https://codereview.chromium.org/331993009/ >> > > -- Mike Pinkerton Mac Weenie pinkerton@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Right, content and omnibox are the main ones. It's also shown whenever we show a textfield. I found textfields in the bookmark bubble, and the "Edit", "Add Page" and "Add Folder" dialogs when right clicking the bookmark bar.
Ok, sounds like we have to go this route. Could you break this up into two patches? One that moves run_types to the constructor and then the rest? I'm fine with a TBR for the run_types move. https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.h:139: internal::MenuRunnerImplInterface* holder_; holder_ was always a poor name. Maybe impl_? https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_impl_adapter.h (right): https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_adapter.h:16: class MenuRunnerImplAdapter : public MenuRunnerImplInterface { This class confused me a bit. Is there a reason not to combine with MenuRunnerImpl? https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_adapter.h:20: virtual MenuItemView* GetMenu() const OVERRIDE; Prefix wit // MenuRunnerImplyInterface: (or something similar). https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_adapter.h:36: }; DISALLOW_... https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_impl_interface.h (right): https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_interface.h:50: }; Add protected destructor.
On 2014/07/14 21:02:23, sky wrote: > Ok, sounds like we have to go this route. Could you break this up into two > patches? One that moves run_types to the constructor and then the rest? I'm fine > with a TBR for the run_types move. > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner.h (right): > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner.h:139: internal::MenuRunnerImplInterface* > holder_; > holder_ was always a poor name. Maybe impl_? > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner_impl_adapter.h (right): > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_impl_adapter.h:16: class > MenuRunnerImplAdapter : public MenuRunnerImplInterface { > This class confused me a bit. Is there a reason not to combine with > MenuRunnerImpl? > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_impl_adapter.h:20: virtual MenuItemView* > GetMenu() const OVERRIDE; > Prefix wit // MenuRunnerImplyInterface: > (or something similar). > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_impl_adapter.h:36: }; > DISALLOW_... > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner_impl_interface.h (right): > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_impl_interface.h:50: }; > Add protected destructor. Done. Split off the run_types move to https://codereview.chromium.org/390183002/.
https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.h:139: internal::MenuRunnerImplInterface* holder_; On 2014/07/14 21:02:22, sky wrote: > holder_ was always a poor name. Maybe impl_? Done. https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_impl_adapter.h (right): https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_adapter.h:16: class MenuRunnerImplAdapter : public MenuRunnerImplInterface { On 2014/07/14 21:02:22, sky wrote: > This class confused me a bit. Is there a reason not to combine with > MenuRunnerImpl? I created this mainly so that I don't have create a second MenuRunnerImpl constructor that takes a MenuModel, and then having to factor out the initializations of its 8 members to a separate private init. I can't use delegating constructor because menu_model_adapter_ would need to be initialized first. But I can do that if you prefer. https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_adapter.h:20: virtual MenuItemView* GetMenu() const OVERRIDE; On 2014/07/14 21:02:22, sky wrote: > Prefix wit // MenuRunnerImplyInterface: > (or something similar). Done. https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_adapter.h:36: }; On 2014/07/14 21:02:22, sky wrote: > DISALLOW_... Done. https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_impl_interface.h (right): https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_interface.h:50: }; On 2014/07/14 21:02:22, sky wrote: > Add protected destructor. Done.
https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_impl_adapter.h (right): https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_adapter.h:16: class MenuRunnerImplAdapter : public MenuRunnerImplInterface { On 2014/07/14 22:59:42, Andre wrote: > On 2014/07/14 21:02:22, sky wrote: > > This class confused me a bit. Is there a reason not to combine with > > MenuRunnerImpl? > > I created this mainly so that I don't have create a second MenuRunnerImpl > constructor > that takes a MenuModel, and then having to factor out the initializations of its > 8 members > to a separate private init. I can't use delegating constructor because > menu_model_adapter_ > would need to be initialized first. But I can do that if you prefer. How about a helper class that both use then?
On 2014/07/15 04:33:44, sky wrote: > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner_impl_adapter.h (right): > > https://codereview.chromium.org/331993009/diff/390001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_impl_adapter.h:16: class > MenuRunnerImplAdapter : public MenuRunnerImplInterface { > On 2014/07/14 22:59:42, Andre wrote: > > On 2014/07/14 21:02:22, sky wrote: > > > This class confused me a bit. Is there a reason not to combine with > > > MenuRunnerImpl? > > > > I created this mainly so that I don't have create a second MenuRunnerImpl > > constructor > > that takes a MenuModel, and then having to factor out the initializations of > its > > 8 members > > to a separate private init. I can't use delegating constructor because > > menu_model_adapter_ > > would need to be initialized first. But I can do that if you prefer. > > How about a helper class that both use then? I'm not sure what you mean, can you explain?
On Mon, Jul 14, 2014 at 9:44 PM, <andresantoso@chromium.org> wrote: > On 2014/07/15 04:33:44, sky wrote: > > https://codereview.chromium.org/331993009/diff/390001/ui/ > views/controls/menu/menu_runner_impl_adapter.h > >> File ui/views/controls/menu/menu_runner_impl_adapter.h (right): >> > > > https://codereview.chromium.org/331993009/diff/390001/ui/ > views/controls/menu/menu_runner_impl_adapter.h#newcode16 > >> ui/views/controls/menu/menu_runner_impl_adapter.h:16: class >> MenuRunnerImplAdapter : public MenuRunnerImplInterface { >> On 2014/07/14 22:59:42, Andre wrote: >> > On 2014/07/14 21:02:22, sky wrote: >> > > This class confused me a bit. Is there a reason not to combine with >> > > MenuRunnerImpl? >> > >> > I created this mainly so that I don't have create a second >> MenuRunnerImpl >> > constructor >> > that takes a MenuModel, and then having to factor out the >> initializations of >> its >> > 8 members >> > to a separate private init. I can't use delegating constructor because >> > menu_model_adapter_ >> > would need to be initialized first. But I can do that if you prefer. >> > > How about a helper class that both use then? >> > > I'm not sure what you mean, can you explain? > You said: "I created this mainly so that I don't have create a second MenuRunnerImpl constructor that takes a MenuModel, and then having to factor out the initializations of its 8 members to a separate private init. I can't use delegating constructor because menu_model_adapter_ would need to be initialized first. But I can do that if you prefer. " I was suggesting maybe moving what you have in menu_runner_impl_adapter into a helper class that is used by the two MenuRunnerImpls. -Scott > > https://codereview.chromium.org/331993009/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/15 16:13:26, sky wrote: > > You said: "I created this mainly so that I don't have create a second > MenuRunnerImpl > constructor that takes a MenuModel, and then having to factor out the > initializations > of its 8 members to a separate private init. I can't use delegating > constructor because menu_model_adapter_ would need to be initialized first. > But I can do that if you prefer. " > > I was suggesting maybe moving what you have in menu_runner_impl_adapter > into a helper class that is used by the two MenuRunnerImpls. > > -Scott Thanks Scott. Does this helper class still inherit from MenuRunnerImplInterface? If not, then wouldn't it be reduced to the existing ui::MenuModelAdapter class? How does MenuRunnerImplCocoa use this helper class? The helper class converts a MenuModel to a MenuView, which is only useful for running the Views menu runner. But MenuRunnerImplCocoa does not need a MenuView because it doesn't need to run the Views menu. I may be misunderstanding your suggestion, let me know if that's the case.
Ok, I think I get why you did it the way you did. I think we need it this way to keep the current ownership model too:( https://codereview.chromium.org/331993009/diff/450001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_impl_interface.h (right): https://codereview.chromium.org/331993009/diff/450001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_interface.h:29: virtual MenuItemView* GetMenu() const = 0; It's unfortunate the MenuItemView has to be exposed as it makes no sense for coca. Did you verify all call sites deal with NULL correctly?
On 2014/07/15 20:36:39, sky wrote: > Ok, I think I get why you did it the way you did. I think we need it this way to > keep the current ownership model too:( > > https://codereview.chromium.org/331993009/diff/450001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner_impl_interface.h (right): > > https://codereview.chromium.org/331993009/diff/450001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_impl_interface.h:29: virtual MenuItemView* > GetMenu() const = 0; > It's unfortunate the MenuItemView has to be exposed as it makes no sense for > coca. Did you verify all call sites deal with NULL correctly? Unfortunately, it's not clear how to update call sites to handle NULL correctly. I made an attempt to update call sites to no longer call GetMenu(), PTAL at https://codereview.chromium.org/393943006/ and let me know what you think.
Scott PTAL. I've rebased this patch off https://codereview.chromium.org/393943006. https://codereview.chromium.org/331993009/diff/450001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_impl_interface.h (right): https://codereview.chromium.org/331993009/diff/450001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_impl_interface.h:29: virtual MenuItemView* GetMenu() const = 0; On 2014/07/15 20:36:38, sky wrote: > It's unfortunate the MenuItemView has to be exposed as it makes no sense for > coca. Did you verify all call sites deal with NULL correctly? Done. I've removed GetMenu, see https://codereview.chromium.org/393943006.
Nice, LGTM
On 2014/07/16 19:36:49, sky wrote: > Nice, LGTM Thanks! Can you LGTM https://codereview.chromium.org/393943006/ as well? It needs to land before this one.
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/331993009/47...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29494) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/33902)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29515)
The CQ bit was unchecked by andresantoso@chromium.org
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/331993009/49...
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 283999 |
