Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1753473003:
A window should not get activated or get input focus if it's behind the lock screen. (Closed)
DescriptionA window should not get activated or get input focus if it's behind the lock screen.
BUG=515594
Committed: https://crrev.com/8692d6358f0fab7c1b85054fc6edc37371d9b1aa
Cr-Commit-Position: refs/heads/master@{#380724}
Patch Set 1 #Patch Set 2 : Add test. #
Total comments: 5
Patch Set 3 : Address sky@'s comments #
Total comments: 9
Patch Set 4 : Address sky@'s comments #Patch Set 5 : Address sky@'s comment. #
Total comments: 2
Patch Set 6 : Use IsActive() instead. #
Total comments: 2
Patch Set 7 : Don't use scoped ptr in the test. #Patch Set 8 : Fix failed try jobs #Patch Set 9 : Fix the failed try jobs #
Total comments: 7
Patch Set 10 : sorry, focus_manager_ is a scoped_ptr. #Patch Set 11 : Modify the test to use NativeWidgetAura when initializing widget. #
Messages
Total messages: 54 (18 generated)
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753473003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753473003/1
xdai@chromium.org changed reviewers: + sky@chromium.org
sky@, could you help review this CL please? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
How about some test coverage.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
On 2016/03/01 03:53:14, sky wrote: > How about some test coverage. sky@, I've added the test, please take another look. Thanks for your review!
https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... File ash/wm/ash_focus_rules_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... ash/wm/ash_focus_rules_unittest.cc:192: TEST_F(LockScreenAshFocusRulesTest, PreventFocusChangeWithLockScreenPresent) { Please add a test close to the code you are changing. https://codereview.chromium.org/1753473003/diff/60001/ui/views/test/widget_te... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/1753473003/diff/60001/ui/views/test/widget_te... ui/views/test/widget_test.h:156: class TestInitialFocusWidgetDelegate : public TestDesktopWidgetDelegate { Why is this code in widget_tesT? AFAICT you don't use it in widget_test.
sky@, please take another look, thanks for your help. https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... File ash/wm/ash_focus_rules_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... ash/wm/ash_focus_rules_unittest.cc:192: TEST_F(LockScreenAshFocusRulesTest, PreventFocusChangeWithLockScreenPresent) { On 2016/03/02 03:41:02, sky wrote: > Please add a test close to the code you are changing. I think it needs to be an ash test since it requires that the lock screen blocks the user session (so that EventClientImpl::CanProcessEventsWithinSubtree() returns false for a window behind the lock screen). Could you give suggestions on where I should add the test? Thanks a lot. https://codereview.chromium.org/1753473003/diff/60001/ui/views/test/widget_te... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/1753473003/diff/60001/ui/views/test/widget_te... ui/views/test/widget_test.h:156: class TestInitialFocusWidgetDelegate : public TestDesktopWidgetDelegate { On 2016/03/02 03:41:02, sky wrote: > Why is this code in widget_tesT? AFAICT you don't use it in widget_test. This class is also used by widget_interactive_uitest.cc. Since it can be also used by our test case, I extracted it out and put it here.
https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... File ash/wm/ash_focus_rules_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... ash/wm/ash_focus_rules_unittest.cc:192: TEST_F(LockScreenAshFocusRulesTest, PreventFocusChangeWithLockScreenPresent) { On 2016/03/02 21:15:55, xdai1 wrote: > On 2016/03/02 03:41:02, sky wrote: > > Please add a test close to the code you are changing. > > I think it needs to be an ash test since it requires that the lock screen blocks > the user session (so that EventClientImpl::CanProcessEventsWithinSubtree() > returns false for a window behind the lock screen). > > Could you give suggestions on where I should add the test? Thanks a lot. The code you are changing is in NativeWidgetAura. You should be able to construct a similar configuration using aura windows. I would expect the test in NativeWidgetAuraUnittest.
Patchset #3 (id:80001) has been deleted
On 2016/03/02 22:09:09, sky wrote: > https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... > File ash/wm/ash_focus_rules_unittest.cc (right): > > https://codereview.chromium.org/1753473003/diff/60001/ash/wm/ash_focus_rules_... > ash/wm/ash_focus_rules_unittest.cc:192: TEST_F(LockScreenAshFocusRulesTest, > PreventFocusChangeWithLockScreenPresent) { > On 2016/03/02 21:15:55, xdai1 wrote: > > On 2016/03/02 03:41:02, sky wrote: > > > Please add a test close to the code you are changing. > > > > I think it needs to be an ash test since it requires that the lock screen > blocks > > the user session (so that EventClientImpl::CanProcessEventsWithinSubtree() > > returns false for a window behind the lock screen). > > > > Could you give suggestions on where I should add the test? Thanks a lot. > > The code you are changing is in NativeWidgetAura. You should be able to > construct a similar configuration using aura windows. I would expect the test in > NativeWidgetAuraUnittest. Thanks! I added a test in native_widget_aura_unittest.cc. Please take another look. I think it might be good to also keep the ash unit test?
https://codereview.chromium.org/1753473003/diff/100001/ui/views/test/widget_t... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/1753473003/diff/100001/ui/views/test/widget_t... ui/views/test/widget_test.h:155: // should be initially focused. 'should be' ->is the initially focused view. https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:492: if (delegate_->CanActivate() && wm::CanActivateWindow(window_)) { I think this is the wrong place for the fix. If you call Activate() and the window isn't active, then the FocusController should ignore the activate request. https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:40: class TestFocusRules : public wm::BaseFocusRules { Add description. https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:45: void SetActivatable(bool can_activate) { can_activate_ = can_activate; } set_can_activate
sky@, I've addressed your comments. Please take another look, thanks for your review. https://codereview.chromium.org/1753473003/diff/100001/ui/views/test/widget_t... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/1753473003/diff/100001/ui/views/test/widget_t... ui/views/test/widget_test.h:155: // should be initially focused. On 2016/03/03 20:22:47, sky wrote: > 'should be' ->is the initially focused view. Done. https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:492: if (delegate_->CanActivate() && wm::CanActivateWindow(window_)) { On 2016/03/03 20:22:47, sky wrote: > I think this is the wrong place for the fix. If you call Activate() and the > window isn't active, then the FocusController should ignore the activate > request. You're right. Since the goal is to prevent SetInitalFocus() from being called, so maybe I should move this one line fix below? https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:40: class TestFocusRules : public wm::BaseFocusRules { On 2016/03/03 20:22:47, sky wrote: > Add description. Done. https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:45: void SetActivatable(bool can_activate) { can_activate_ = can_activate; } On 2016/03/03 20:22:47, sky wrote: > set_can_activate Done.
https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:492: if (delegate_->CanActivate() && wm::CanActivateWindow(window_)) { On 2016/03/03 22:01:31, xdai1 wrote: > On 2016/03/03 20:22:47, sky wrote: > > I think this is the wrong place for the fix. If you call Activate() and the > > window isn't active, then the FocusController should ignore the activate > > request. > > You're right. Since the goal is to prevent SetInitalFocus() from being called, > so maybe I should move this one line fix below? What is SetInitialFocus doing that is wrong? I think what should happen is if Activate fails then you supply ui::SHOW_STATE_INACTIVE to SetInitialFocus. But I could be misunderstanding the bug.
On 2016/03/03 23:39:33, sky wrote: > https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... > File ui/views/widget/native_widget_aura.cc (right): > > https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... > ui/views/widget/native_widget_aura.cc:492: if (delegate_->CanActivate() && > wm::CanActivateWindow(window_)) { > On 2016/03/03 22:01:31, xdai1 wrote: > > On 2016/03/03 20:22:47, sky wrote: > > > I think this is the wrong place for the fix. If you call Activate() and the > > > window isn't active, then the FocusController should ignore the activate > > > request. > > > > You're right. Since the goal is to prevent SetInitalFocus() from being called, > > so maybe I should move this one line fix below? > > What is SetInitialFocus doing that is wrong? I think what should happen is if > Activate fails then you supply ui::SHOW_STATE_INACTIVE to SetInitialFocus. But I > could be misunderstanding the bug. The reason is that if an app modal dialog (which has a initial focused view) shows up beneath the lock screen, the focused text input client is changed when SetInitialFocus() is called, and that's why the text was typed into the HTTP authentication field, not the password text field. Below is the call stack: #0 ui::InputMethod::SetFocusedTextInputClient() #1 views::Textfield::OnFocus() #2 views::View::Focus() #3 views::FocusManager::SetFocusedViewWithReason() #4 views::FocusManager::SetFocusedView() #5 views::View::RequestFocus() #6 views::Widget::SetInitialFocus() #7 views::NativeWidgetAura::SetInitialFocus() #8 views::NativeWidgetAura::ShowWithWindowState() #9 views::Widget::Show() #10 app_modal::JavaScriptAppModalDialogViews::ShowAppModalDialog()
I think it's valid to fix this like I outlined, but I also think IME should ignore focus changes in inactive windows. -Scott On Thu, Mar 3, 2016 at 4:00 PM, <xdai@chromium.org> wrote: > On 2016/03/03 23:39:33, sky wrote: >> > https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... >> File ui/views/widget/native_widget_aura.cc (right): >> >> > https://codereview.chromium.org/1753473003/diff/100001/ui/views/widget/native... >> ui/views/widget/native_widget_aura.cc:492: if (delegate_->CanActivate() && >> wm::CanActivateWindow(window_)) { >> On 2016/03/03 22:01:31, xdai1 wrote: >> > On 2016/03/03 20:22:47, sky wrote: >> > > I think this is the wrong place for the fix. If you call Activate() >> > > and > the >> > > window isn't active, then the FocusController should ignore the >> > > activate >> > > request. >> > >> > You're right. Since the goal is to prevent SetInitalFocus() from being > called, >> > so maybe I should move this one line fix below? >> >> What is SetInitialFocus doing that is wrong? I think what should happen is >> if >> Activate fails then you supply ui::SHOW_STATE_INACTIVE to SetInitialFocus. >> But > I >> could be misunderstanding the bug. > > The reason is that if an app modal dialog (which has a initial focused view) > shows up beneath the lock screen, the focused text input client is changed > when > SetInitialFocus() is called, and that's why the text was typed into the HTTP > authentication field, not the password text field. Below is the call stack: > #0 ui::InputMethod::SetFocusedTextInputClient() > #1 views::Textfield::OnFocus() > #2 views::View::Focus() > #3 views::FocusManager::SetFocusedViewWithReason() > #4 views::FocusManager::SetFocusedView() > #5 views::View::RequestFocus() > #6 views::Widget::SetInitialFocus() > #7 views::NativeWidgetAura::SetInitialFocus() > #8 views::NativeWidgetAura::ShowWithWindowState() > #9 views::Widget::Show() > #10 app_modal::JavaScriptAppModalDialogViews::ShowAppModalDialog() > > > https://codereview.chromium.org/1753473003/ -- 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.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Please take another look at the new patch. I guess that's what you suggest? Thanks for your review. Widget::SetInitialFocus() will prevent focus change if the window's state is ui::SHOW_STATE_INACTIVE, thus IME won't get the onFocus() event. On 2016/03/04 00:13:58, sky wrote: > I think it's valid to fix this like I outlined, but I also think IME > should ignore focus changes in inactive windows. > > -Scott
https://codereview.chromium.org/1753473003/diff/180001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/1753473003/diff/180001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:494: Activate(); I was thinking that in this branch you would see if Activate() failed you would change the state here. And add a comment as to why that happens.
sky@, please take another look, thanks for your review. https://codereview.chromium.org/1753473003/diff/180001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/1753473003/diff/180001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:494: Activate(); On 2016/03/04 16:32:55, sky wrote: > I was thinking that in this branch you would see if Activate() failed you would > change the state here. And add a comment as to why that happens. I see your point now. So in order to make this change (NativeWidgetAura::Activate() returns bool type), below is the list of functions we also need to change. I just want to confirm with you before making this change. If it looks good to you, I'll go ahead. Thanks for your help. NativeWidgetPrivate::Activate() NativeWidgetAura::Activate() ActivationClient::ActivateWindow() DefaultActivationClient::ActivateWindow() DefaultActivationClient::ActivateWindowImpl() FocusClient::FocusWindow() FocusController::ActivateWindow() FocusController::FocusWindow() FocusController::FocusAndActivateWindow() DesktopNativeWidgetAura::Activate() DesktopWindowTreeHost::Activate() DesktopWindowTreeHostX11::Activate() NativeWidgetMus::Activate()
How about calling IsActive() right after the activate to determine if it really got activated? On Fri, Mar 4, 2016 at 2:27 PM, <xdai@chromium.org> wrote: > sky@, please take another look, thanks for your review. > > > https://codereview.chromium.org/1753473003/diff/180001/ui/views/widget/native... > File ui/views/widget/native_widget_aura.cc (right): > > https://codereview.chromium.org/1753473003/diff/180001/ui/views/widget/native... > ui/views/widget/native_widget_aura.cc:494: Activate(); > On 2016/03/04 16:32:55, sky wrote: >> I was thinking that in this branch you would see if Activate() failed > you would >> change the state here. And add a comment as to why that happens. > > I see your point now. So in order to make this change > (NativeWidgetAura::Activate() returns bool type), below is the list of > functions we also need to change. I just want to confirm with you before > making this change. If it looks good to you, I'll go ahead. Thanks for > your help. > > NativeWidgetPrivate::Activate() > NativeWidgetAura::Activate() > ActivationClient::ActivateWindow() > DefaultActivationClient::ActivateWindow() > DefaultActivationClient::ActivateWindowImpl() > FocusClient::FocusWindow() > FocusController::ActivateWindow() > FocusController::FocusWindow() > FocusController::FocusAndActivateWindow() > DesktopNativeWidgetAura::Activate() > DesktopWindowTreeHost::Activate() > DesktopWindowTreeHostX11::Activate() > NativeWidgetMus::Activate() > > https://codereview.chromium.org/1753473003/ -- 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.
On 2016/03/04 23:01:53, sky wrote: > How about calling IsActive() right after the activate to determine if > it really got activated? > Addressed. Please take another look, thanks for your review.
LGTM https://codereview.chromium.org/1753473003/diff/200001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:527: scoped_ptr<views::Widget> w1(new views::Widget); You don't need a scoped_ptr here. Declare w1 on the stack. https://codereview.chromium.org/1753473003/diff/200001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:539: scoped_ptr<views::Widget> w2(new views::Widget); Same comment about w2.
On 2016/03/07 16:05:15, sky wrote: > LGTM > > https://codereview.chromium.org/1753473003/diff/200001/ui/views/widget/native... > File ui/views/widget/native_widget_aura_unittest.cc (right): > > https://codereview.chromium.org/1753473003/diff/200001/ui/views/widget/native... > ui/views/widget/native_widget_aura_unittest.cc:527: scoped_ptr<views::Widget> > w1(new views::Widget); > You don't need a scoped_ptr here. Declare w1 on the stack. > > https://codereview.chromium.org/1753473003/diff/200001/ui/views/widget/native... > ui/views/widget/native_widget_aura_unittest.cc:539: scoped_ptr<views::Widget> > w2(new views::Widget); > Same comment about w2. Addressed. Thanks for your review.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1753473003/#ps220001 (title: "Don't use scoped ptr in the test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753473003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753473003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753473003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753473003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #8 (id:240001) has been deleted
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753473003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753473003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
sky@, could you take a look at this CL to see if it's still looks good to you? Some builders failed so I tried to fix the failure. Thanks. https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:522: #if defined(OS_CHROMEOS) I added this because NativeWidgetAura is only used on Chrome OS. DesktopNativeWidgetAura is used on other OS and that's why this test failed on linux and win try jobs. https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/widget... ui/views/widget/widget.cc:1320: if (v && focus_manager_) focus_manager_ is null for non top level windows.
https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:522: #if defined(OS_CHROMEOS) On 2016/03/08 18:30:17, xdai1 wrote: > I added this because NativeWidgetAura is only used on Chrome OS. > DesktopNativeWidgetAura is used on other OS and that's why this test failed on > linux and win try jobs. That's not true. NWA is used on linux and windows as well. Only place it isn't is mac, but then this file isn't compbiled there. https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/widget... ui/views/widget/widget.cc:1320: if (v && focus_manager_) On 2016/03/08 18:30:17, xdai1 wrote: > focus_manager_ is null for non top level windows. A null focus_manager_ wasn't possible before, so it shouldn't be possible now. I suspect your test needs to be updated to ensure a focus_manager_ is created correctly.
sky@, please take another look. Thanks for your help. https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:522: #if defined(OS_CHROMEOS) On 2016/03/08 22:42:04, sky wrote: > On 2016/03/08 18:30:17, xdai1 wrote: > > I added this because NativeWidgetAura is only used on Chrome OS. > > DesktopNativeWidgetAura is used on other OS and that's why this test failed on > > linux and win try jobs. > > That's not true. NWA is used on linux and windows as well. Only place it isn't > is mac, but then this file isn't compbiled there. By looking at these failed builder, I noticed that DesktopNativeWidgetAura was used rather than NativeWidgetAura. For example, in win8_chromium_ng builder, the crash call stack in (https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng...) shows that views::DesktopNativeWidgetAura::InitNativeWidget() was called, which then caused a crash. In the linux_chromium_rel_ng build (https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...), I tested it locally, and it also suggested that DesktopNativeWidgetAura was used in the test NativeWidgetAuraTest.PreventFocusOnNonActivableWindow. So I guess some of the linux and windows builders used NativeWidgetAura but some still used DesktopNativeWidgetAura? https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/widget... ui/views/widget/widget.cc:1320: if (v && focus_manager_) On 2016/03/08 22:42:04, sky wrote: > On 2016/03/08 18:30:17, xdai1 wrote: > > focus_manager_ is null for non top level windows. > > A null focus_manager_ wasn't possible before, so it shouldn't be possible now. I > suspect your test needs to be updated to ensure a focus_manager_ is created > correctly. According to the comment in https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi..., a non top level widget doesn't have a focus manager. So I guess before this change, it's impossible for a non top level widget to show with SHOW_STATE_INACTIVE, but after this change it's possible now? Actually a null focus_manager_ is the main reason of the builder failure in Patch 7 (for example, more than 100 tests failed in linux_chromium_chromeos_rel_ng because of a null focus_manager_: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...).
https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1753473003/diff/280001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:522: #if defined(OS_CHROMEOS) On 2016/03/10 01:12:57, xdai1 wrote: > On 2016/03/08 22:42:04, sky wrote: > > On 2016/03/08 18:30:17, xdai1 wrote: > > > I added this because NativeWidgetAura is only used on Chrome OS. > > > DesktopNativeWidgetAura is used on other OS and that's why this test failed > on > > > linux and win try jobs. > > > > That's not true. NWA is used on linux and windows as well. Only place it isn't > > is mac, but then this file isn't compbiled there. > > By looking at these failed builder, I noticed that DesktopNativeWidgetAura was > used rather than NativeWidgetAura. For example, in win8_chromium_ng builder, the > crash call stack in > (https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng...) > shows that views::DesktopNativeWidgetAura::InitNativeWidget() was called, which > then caused a crash. In the linux_chromium_rel_ng build > (https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...), > I tested it locally, and it also suggested that DesktopNativeWidgetAura was used > in the test NativeWidgetAuraTest.PreventFocusOnNonActivableWindow. So I guess > some of the linux and windows builders used NativeWidgetAura but some still used > DesktopNativeWidgetAura? The NativeWidgetType is based on many factors. On chromeos you are guaranteed to get NativeWidgetAura, not so on desktop linux and windows. I believe if you call Init() as the other tests are doing here you will get a NWA.
On 2016/03/10 17:09:39, sky wrote: > > The NativeWidgetType is based on many factors. On chromeos you are guaranteed to > get NativeWidgetAura, not so on desktop linux and windows. I believe if you call > Init() as the other tests are doing here you will get a NWA. sky@, I've addressed your comment. Please take another look, thanks for your review.
LGTM
The CQ bit was checked by xdai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753473003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753473003/320001
Message was sent while issue was closed.
Committed patchset #11 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== A window should not get activated or get input focus if it's behind the lock screen. BUG=515594 ========== to ========== A window should not get activated or get input focus if it's behind the lock screen. BUG=515594 Committed: https://crrev.com/8692d6358f0fab7c1b85054fc6edc37371d9b1aa Cr-Commit-Position: refs/heads/master@{#380724} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8692d6358f0fab7c1b85054fc6edc37371d9b1aa Cr-Commit-Position: refs/heads/master@{#380724} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
