Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2924873007:
Don't activate aura::Window on creation from DesktopNativeWidgetAura if NativeWidgetDelegate prohib… (Closed)
DescriptionDon't activate aura::Window on creation from DesktopNativeWidgetAura if NativeWidgetDelegate prohibits it.
The bug was found during PasswordGenerationInteractiveTest.PopupShownAndPasswordSelected flakiness investigation. The autofill drop-down was unexpectedly closed due to focus change event. It happened when StatusBubbleViews disappeared. It turned out that StatusBubbleViews actually was opened as an active window despite it shouldn't.
The bug is in DesktopNativeWidgetAura. Lines #503 and #540 are duplicates. Thus, #503 is presumably redundant. Lines #542 and 540 are in the wrong order. The window is focused before the ActivationDelegate is set. Therefore, wm::FocusController had no chance to know that the window can't be activated.
BUG=407998
Review-Url: https://codereview.chromium.org/2924873007
Cr-Commit-Position: refs/heads/master@{#478316}
Committed: https://chromium.googlesource.com/chromium/src/+/1dd3ea2f36d5ad5da756bb3e296357f67232bbef
Patch Set 1 #
Total comments: 2
Patch Set 2 : Comment #
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't activate aura::Window on creation from DesktopNativeWidgetAura if NativeWidgetDelegate prohibits it. The bug was found during PasswordGenerationInteractiveTest.PopupShownAndPasswordSelected flakiness investigation. The autofill drop-down was unexpectedly closed due to focus change event. It happened when StatusBubbleViews disappeared. It turned out that StatusBubbleViews actually was opened as active window despite it shouldn't. BUG=407998 ========== to ========== Don't activate aura::Window on creation from DesktopNativeWidgetAura if NativeWidgetDelegate prohibits it. The bug was found during PasswordGenerationInteractiveTest.PopupShownAndPasswordSelected flakiness investigation. The autofill drop-down was unexpectedly closed due to focus change event. It happened when StatusBubbleViews disappeared. It turned out that StatusBubbleViews actually was opened as an active window despite it shouldn't. The bug is in DesktopNativeWidgetAura. Lines #503 and #540 are duplicates. Thus, #503 is presumably redundant. Lines #542 and 540 are in the wrong order. The window is focused before the ActivationDelegate is set. Therefore, wm::FocusController had no chance to know that the window can't be activated. BUG=407998 ==========
vasilii@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Scott, please review.
https://codereview.chromium.org/2924873007/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (left): https://codereview.chromium.org/2924873007/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:503: static_cast<aura::client::FocusClient*>(focus_client_.get())-> Shouldn't this code still focus the window for the CanActivate case?
The CQ bit was checked by vasilii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2924873007/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (left): https://codereview.chromium.org/2924873007/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:503: static_cast<aura::client::FocusClient*>(focus_client_.get())-> On 2017/06/08 19:45:32, sky wrote: > Shouldn't this code still focus the window for the CanActivate case? Done. Moved the call despite I don't see the place between #503 and #540 that cares about focus.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497028741463950,
"parent_rev": "324a3047c434fdb0ab247cedcc106de9178ea065", "commit_rev":
"1dd3ea2f36d5ad5da756bb3e296357f67232bbef"}
Message was sent while issue was closed.
Description was changed from ========== Don't activate aura::Window on creation from DesktopNativeWidgetAura if NativeWidgetDelegate prohibits it. The bug was found during PasswordGenerationInteractiveTest.PopupShownAndPasswordSelected flakiness investigation. The autofill drop-down was unexpectedly closed due to focus change event. It happened when StatusBubbleViews disappeared. It turned out that StatusBubbleViews actually was opened as an active window despite it shouldn't. The bug is in DesktopNativeWidgetAura. Lines #503 and #540 are duplicates. Thus, #503 is presumably redundant. Lines #542 and 540 are in the wrong order. The window is focused before the ActivationDelegate is set. Therefore, wm::FocusController had no chance to know that the window can't be activated. BUG=407998 ========== to ========== Don't activate aura::Window on creation from DesktopNativeWidgetAura if NativeWidgetDelegate prohibits it. The bug was found during PasswordGenerationInteractiveTest.PopupShownAndPasswordSelected flakiness investigation. The autofill drop-down was unexpectedly closed due to focus change event. It happened when StatusBubbleViews disappeared. It turned out that StatusBubbleViews actually was opened as an active window despite it shouldn't. The bug is in DesktopNativeWidgetAura. Lines #503 and #540 are duplicates. Thus, #503 is presumably redundant. Lines #542 and 540 are in the wrong order. The window is focused before the ActivationDelegate is set. Therefore, wm::FocusController had no chance to know that the window can't be activated. BUG=407998 Review-Url: https://codereview.chromium.org/2924873007 Cr-Commit-Position: refs/heads/master@{#478316} Committed: https://chromium.googlesource.com/chromium/src/+/1dd3ea2f36d5ad5da756bb3e2963... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1dd3ea2f36d5ad5da756bb3e2963...
Message was sent while issue was closed.
Vasilii, I'm moderately worried there was a reason for adding the later call. Could you check history to make sure there isn't some subtle case that removing the earlier call breaks? Thanks, -Scott On Fri, Jun 9, 2017 at 10:23 AM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Committed patchset #2 (id:20001) as > https://chromium.googlesource.com/chromium/src/+/ > 1dd3ea2f36d5ad5da756bb3e296357f67232bbef > > https://codereview.chromium.org/2924873007/ > -- 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.
Message was sent while issue was closed.
The second call was historically first. See https://chromiumcodereview.appspot.com/12342028. Even though I don't know what desktop_root_window_host_->InitFocus(window_) did, it seems that the bug originated from that CL. Later it was transformed into the current form (https://codereview.chromium.org/29983002) but it looks like a mechanical change. The first line comes from https://codereview.chromium.org/31043006. It was moving a chunk of code from the platform specifics to DesktopNativeWidgetAura. However, that particular line isn't a copy/paste. Nothing in the comments/description explains why it's needed. Maybe you overlooked the second call below. Given that line 540 was introduced as the only Focus call, I think we are safe to remove it now as redundant.
Message was sent while issue was closed.
Thanks for investigating. On Fri, Jun 9, 2017 at 12:40 PM, <vasilii@chromium.org> wrote: > The second call was historically first. See > https://chromiumcodereview.appspot.com/12342028. Even though I don't know > what > desktop_root_window_host_->InitFocus(window_) did, it seems that the bug > originated from that CL. Later it was transformed into the current form > (https://codereview.chromium.org/29983002) but it looks like a mechanical > change. > > The first line comes from https://codereview.chromium.org/31043006. It was > moving a chunk of code from the platform specifics to > DesktopNativeWidgetAura. > However, that particular line isn't a copy/paste. Nothing in the > comments/description explains why it's needed. Maybe you overlooked the > second > call below. > > Given that line 540 was introduced as the only Focus call, I think we are > safe > to remove it now as redundant. > > https://codereview.chromium.org/2924873007/ > -- 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
