Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2596763003:
On macOS, avoid forcible activation during Show.
DescriptionOn macOS, avoid forcible activation during Show.
When Chromium is already launched, and another application requests it
to open an URL in background, browser should not activate itself.
BUG=676313
Patch Set 1 #
Total comments: 6
Patch Set 2 : Reword comment. #
Total comments: 2
Messages
Total messages: 11 (4 generated)
Description was changed from ========== On macOS, avoid forcible activation during Show. When Chromium is already launched, and another application requests it to open an URL in background, browser should not activate itself. BUG=676313 ========== to ========== On macOS, avoid forcible activation during Show. When Chromium is already launched, and another application requests it to open an URL in background, browser should not activate itself. BUG=676313 ==========
anpol@yandex-team.ru changed reviewers: + pkasting@chromium.org
PTAL
Hmm. I don't feel I understand enough about the ways in which Show() is called to be sure this is the right place to fix this. Basically, I'm worried about other cases besides the one you're fixing trying to call this and expecting the window to get activated, but having that not happen. Do you have confidence that cannot occur? You may want to replace me with sky@, who might be more likely to know such a thing. Is this change testable? Can we write some kind of browsertest or interactive UI test that tries to launch the browser in this way and verifies the window doesn't get erroneously focused? This is the sort of thing that can easily regress, so it's worth testing it even if the test has to be an interactive UI test. https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:604: BrowserList::SetLastActive(browser()); Is it inconsistent that we SetLastActive() a window we're not going activate? Do we still want to do this in the "show in background" case? https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:609: #if !defined(OS_MACOSX) Nit: Remove "!" and reverse arms, so "else" doesn't read like a mental double-negative. https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:612: // When opening a URL from other application, macOS will activate browser Nit: other -> another, browser -> the browser https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:613: // window in case when "Open" action is used, but won't do it in case when Nit: in case -> in the case (2x) https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:615: // for us, just put the window above other browser windows. Nit: , -> ;
anpol@yandex-team.ru changed reviewers: + sky@chromium.org, tapted@chromium.org
On 2016/12/22 00:38:42, Peter Kasting wrote: > Hmm. I don't feel I understand enough about the ways in which Show() is called > to be sure this is the right place to fix this. Basically, I'm worried about > other cases besides the one you're fixing trying to call this and expecting the > window to get activated, but having that not happen. Do you have confidence > that cannot occur? > > You may want to replace me with sky@, who might be more likely to know such a > thing. +sky@, +tapted@ for macOS expertise. > Is this change testable? Can we write some kind of browsertest or interactive > UI test that tries to launch the browser in this way and verifies the window > doesn't get erroneously focused? This is the sort of thing that can easily > regress, so it's worth testing it even if the test has to be an interactive UI > test. I'll try to investigate. Does anyone know for sure if it's possible or not? > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:604: > BrowserList::SetLastActive(browser()); > Is it inconsistent that we SetLastActive() a window we're not going activate? > Do we still want to do this in the "show in background" case? As far as I understand, ShowInactive() actually activates the window _within_ the application, but it doesn't activate the application itself. > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:609: #if !defined(OS_MACOSX) > Nit: Remove "!" and reverse arms, so "else" doesn't read like a mental > double-negative. > > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:612: // When opening a URL from > other application, macOS will activate browser > Nit: other -> another, browser -> the browser > > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:613: // window in case when "Open" > action is used, but won't do it in case when > Nit: in case -> in the case (2x) > > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:615: // for us, just put the > window above other browser windows. > Nit: , -> ; Done.
https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:604: BrowserList::SetLastActive(browser()); Please respond to comments on the code itself using the `Reply` link - I'm copying your response below On 2016/12/22 00:38:42, Peter Kasting OOO Dec 25-Jan 3 wrote: > Is it inconsistent that we SetLastActive() a window we're not going activate? > Do we still want to do this in the "show in background" case? > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > > File chrome/browser/ui/views/frame/browser_view.cc (right): > > > > > https://codereview.chromium.org/2596763003/diff/1/chrome/browser/ui/views/fra... > > chrome/browser/ui/views/frame/browser_view.cc:604: > > BrowserList::SetLastActive(browser()); > > Is it inconsistent that we SetLastActive() a window we're not going activate? > > Do we still want to do this in the "show in background" case? > > As far as I understand, ShowInactive() actually activates the window _within_ > the application, but it doesn't activate the application itself. That's incorrect. ShowInactive "raises" the window but does not transfer focus ("key state" on Mac). SetLastActive should only be called when key status is being transferred. https://codereview.chromium.org/2596763003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2596763003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:639: frame_->Show(); This isn't quite right either. If Chrome has been hidden with [NSApp hide] (e.g. Cmd+h) then the window won't be visible. But the window still should not activate in that case (currently `open -a Chromium -g http://www.google.com` after Cmd+h will move focus away) Similarly, if there are no browser windows open, then `open -g` should not activate the newly created window. Unfortunately, this will not do the right thing for calls coming from WindowsCreateFunction::Run() using the tabs extension API. We will need a solution that fixes both. One possible approach could be to add an appropriate WidgetDelegate method. There is `CanActivate` but currently that means something more like "CanAcceptKeyboardInput" which isn't right. Perhaps bool WidgetDelegate::WidgetActivatesApplication() const; which defaults to `true` BrowserView overrides this on Mac so that it returns `false` when being called via -[AppController openUrls:] in app_controller_mac.mm To do that.. it's a bit ugly, but there is already something similar needed in app_controller_mac's `g_is_opening_new_window`, used by session_service.cc So, e.g., add app_controller_mac::IsExternallyTriggeredURLOpen(), and have -[AppController openUrls:] do something similar to CreateBrowser(..) in app_controller_mac.mm. The final piece is to have BridgedNativeWidget::SetVisibilityState() skip calling `[NSApp activateIgnoringOtherApps:YES];` if the widget delegate returns false from `WidgetActivatesApplication()` Then add a test. There's already a bunch of stuff in app_controller_mac_browsertest.mm for invoking openUrls:. You might need to use a swizzler to ensure nothing is invoking [NSApp activateIgnoringOtherApps:] rather than trying to observe the actual activation, since otherwise window activation is pretty flaky. Note you will still need sky@ to chime in about WidgetDelegate::WidgetActivatesApplication() -- I think that's the neatest option, but he might see an approach I've missed.
https://codereview.chromium.org/2596763003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2596763003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:639: frame_->Show(); On 2017/01/03 03:27:06, tapted wrote: > This isn't quite right either. If Chrome has been hidden with [NSApp hide] (e.g. > Cmd+h) then the window won't be visible. But the window still should not > activate in that case (currently `open -a Chromium -g http://www.google.com%60 > after Cmd+h will move focus away) > > Similarly, if there are no browser windows open, then `open -g` should not > activate the newly created window. > > Unfortunately, this will not do the right thing for calls coming from > WindowsCreateFunction::Run() using the tabs extension API. We will need a > solution that fixes both. > > One possible approach could be to add an appropriate WidgetDelegate method. > There is `CanActivate` but currently that means something more like > "CanAcceptKeyboardInput" which isn't right. > > Perhaps > > bool WidgetDelegate::WidgetActivatesApplication() const; > > which defaults to `true` > > BrowserView overrides this on Mac so that it returns `false` when being called > via -[AppController openUrls:] in app_controller_mac.mm > > To do that.. it's a bit ugly, but there is already something similar needed in > app_controller_mac's `g_is_opening_new_window`, used by session_service.cc > > So, e.g., add app_controller_mac::IsExternallyTriggeredURLOpen(), and have > -[AppController openUrls:] do something similar to CreateBrowser(..) in > app_controller_mac.mm. > > The final piece is to have BridgedNativeWidget::SetVisibilityState() skip > calling `[NSApp activateIgnoringOtherApps:YES];` if the widget delegate returns > false from `WidgetActivatesApplication()` > > > Then add a test. There's already a bunch of stuff in > app_controller_mac_browsertest.mm for invoking openUrls:. You might need to use > a swizzler to ensure nothing is invoking [NSApp activateIgnoringOtherApps:] > rather than trying to observe the actual activation, since otherwise window > activation is pretty flaky. > > > Note you will still need sky@ to chime in about > WidgetDelegate::WidgetActivatesApplication() -- I think that's the neatest > option, but he might see an approach I've missed. Is the problem that there are two many random places calling show and you need a way to alter what Show does without changing every place? And that you want to do something like: make_show_not_activate_app = true; Show(); make_show_not_activate_app = false; ?
On 2017/01/03 16:38:01, sky (OOO) wrote: > > Note you will still need sky@ to chime in about > > WidgetDelegate::WidgetActivatesApplication() -- I think that's the neatest > > option, but he might see an approach I've missed. > > Is the problem that there are two many random places calling show and you need a > way to alter what Show does without changing every place? And that you want to > do something like: > > make_show_not_activate_app = true; > Show(); > make_show_not_activate_app = false; > > ? That's a good summary. Mac effectively has a "key" window per-application, so Show/Activate/ShowInactive don't cover all the operations. Although, with the current understanding of the problem, the number of callsites may be manageable. E.g. adding Widget::ShowWithoutActivatingApplication() (or Widget::MakeKeyWindow()?), along with an equivalent ui::BaseWindow function could be enough. Note BrowserWindowCocoa::Show() effectively always calls Widget::ShowWithoutActivatingApplication(), and BrowserWindowCocoa::Activate() effectively always calls Show(). So something else to investigate would be to be more rigid about this. That is, *only* a call to ui::BaseWindow::Activate() or Widget::Activate() would activate the application - but that will require some epic updates to tests and things, and it doesn't fit the current contract of Show()/Activate(). I.e. (from BaseWindow) // Shows the window, or activates it if it's already visible. virtual void Show() = 0; // Show the window, but do not activate it. Does nothing if window // is already visible. virtual void ShowInactive() = 0; // Activates (brings to front) the window. Restores the window from minimized // state if necessary. virtual void Activate() = 0; These would need to be something like // Shows the window, raises it, and focuses it (possibly un-minimizing as well). // Does not activate the application on Mac. virtual void Show() = 0; // Show the window and raises it, but does not transfer focus or activate the // application. virtual void ShowInactive() = 0; // Calls Show(), and activates the application. virtual void Activate() = 0;
On Tue, Jan 3, 2017 at 3:18 PM, <tapted@chromium.org> wrote: > On 2017/01/03 16:38:01, sky (OOO) wrote: >> > Note you will still need sky@ to chime in about >> > WidgetDelegate::WidgetActivatesApplication() -- I think that's the >> > neatest >> > option, but he might see an approach I've missed. >> >> Is the problem that there are two many random places calling show and you >> need > a >> way to alter what Show does without changing every place? And that you >> want to >> do something like: >> >> make_show_not_activate_app = true; >> Show(); >> make_show_not_activate_app = false; >> >> ? > > That's a good summary. > > Mac effectively has a "key" window per-application, so > Show/Activate/ShowInactive don't cover all the operations. > > Although, with the current understanding of the problem, the number of > callsites > may be manageable. E.g. adding Widget::ShowWithoutActivatingApplication() > (or > Widget::MakeKeyWindow()?), along with an equivalent ui::BaseWindow function > could be enough. > > Note BrowserWindowCocoa::Show() effectively always calls > Widget::ShowWithoutActivatingApplication(), and > BrowserWindowCocoa::Activate() > effectively always calls Show(). > > So something else to investigate would be to be more rigid about this. That > is, > *only* a call to ui::BaseWindow::Activate() or Widget::Activate() would > activate > the application - but that will require some epic updates to tests and > things, > and it doesn't fit the current contract of Show()/Activate(). I.e. (from > BaseWindow) > > // Shows the window, or activates it if it's already visible. > virtual void Show() = 0; > > // Show the window, but do not activate it. Does nothing if window > // is already visible. > virtual void ShowInactive() = 0; > > // Activates (brings to front) the window. Restores the window from > minimized > // state if necessary. > virtual void Activate() = 0; > > > These would need to be something like > > // Shows the window, raises it, and focuses it (possibly un-minimizing as > well). > // Does not activate the application on Mac. > virtual void Show() = 0; > > // Show the window and raises it, but does not transfer focus or activate > the > // application. > virtual void ShowInactive() = 0; > > // Calls Show(), and activates the application. > virtual void Activate() = 0; The disadvantage of Show() is that it's easy to miss it doesn't activate. I would rather have a single Show() that takes a parameter: INACTIVE, FOCUS, ACTIVATE. -Scott -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
pkasting@chromium.org changed reviewers: - pkasting@chromium.org |
