Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517663002:
Fix unable to delete extensions continuously through keyboard (Closed)
|
Created:
4 years, 1 month ago by Qiang(Joe) Xu Modified:
4 years, 1 month ago Reviewers:
sky CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix unable to delete extensions continuously through keyboard
Changes:
(1) revert the cl in Issue 2451323002.
(2) using a different approach, which is to avoid restorefocusedview call if desktopnativewidget is not active (since that should be handled in HandleActivationChanged call).
BUG=665380
TEST=manual test see bug fixed.
Also, this reverts the cl in Issue 2451323002, checked that the bug in that issue is not regressed.
Committed: https://crrev.com/34ce9a4718f571b8f8ee0f95b6d153aff38223db
Cr-Commit-Position: refs/heads/master@{#433706}
Patch Set 1 #
Total comments: 2
Patch Set 2 : based on comment #
Total comments: 3
Patch Set 3 : nit #
Messages
Total messages: 25 (16 generated)
The CQ bit was checked by warx@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 ========== Fix unable to delete extensions continuously through keyboard BUG = 665380 ========== to ========== Fix unable to delete extensions continuously through keyboard BUG=665380 TEST=manual test see bug fixed. Also, this reverts the cl in Issue 2451323002, checking that the bug in that issue is not regressed. ==========
warx@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== Fix unable to delete extensions continuously through keyboard BUG=665380 TEST=manual test see bug fixed. Also, this reverts the cl in Issue 2451323002, checking that the bug in that issue is not regressed. ========== to ========== Fix unable to delete extensions continuously through keyboard Changes: (1) revert the cl in Issue 2451323002. (2) using a different approach, which is to avoid restorefocusedview call if desktopnativewidget is not active (since that should be handled in HandleActivationChanged call). BUG=665380 TEST=manual test see bug fixed. Also, this reverts the cl in Issue 2451323002, checked that the bug in that issue is not regressed. ==========
Hi Scott, ptal, thanks! Changes are described in issue description and code comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2517663002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2517663002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1101: GetWidget()->GetFocusManager()->RestoreFocusedView(); For the non-linux case don't we end up in HandleActivationChanged and RestoreFocusedView there?
The CQ bit was checked by warx@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/2517663002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2517663002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1101: GetWidget()->GetFocusManager()->RestoreFocusedView(); On 2016/11/19 15:33:23, sky wrote: > For the non-linux case don't we end up in HandleActivationChanged and > RestoreFocusedView there? Good catch. I think make restore_focus_on_activate_ = false there can avoid dup RestoreFocusedView call. Then both OS_LINUX and non-OS_LINUX should be properly processed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2517663002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2517663002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1098: restore_focus_on_activate_ = false; I'm still confused as to why we need logic here? Isn't HandleActivationChanged() always called, so that the code to restore isn't needed?
Hi Scott, here is the reply, ptal, thanks! https://codereview.chromium.org/2517663002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2517663002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:1098: restore_focus_on_activate_ = false; On 2016/11/21 16:19:08, sky wrote: > I'm still confused as to why we need logic here? Isn't HandleActivationChanged() > always called, so that the code to restore isn't needed? HandleActivationChanged is not always called. It is called only when there is a desktop native widget activation or deactivation change. If there is a child aura::Window activation/deactivation, but it doesn't trigger desktop native level activation change, like the case in the bug link (verified by log), then the restorefocusedview call here is needed. So there are two cases: (1) desktop widget is not active, child aura window gets activated. Under this case, because of the new logic in FocusManager, it will trigger |widget_|->Activate(). So skip the OnWindowActivated's RestoreFocusedView here and rely on HandleActivationChange's RestoreFocusedView. (2) desktop widget is active, child aura window get activated. In this case, HandleActivationChange is not called. We then should rely on OnWindowActivated's RestoreFocuseView. For case one, set restore_focus_on_activate_ = false in HandleActivationChanged, which grantees that if RestoreFocusedView becomes relying on desktop_native widget activation, it will not depend on OnWindowActivated anymore.
Ok, thanks, LGTM https://codereview.chromium.org/2517663002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right): https://codereview.chromium.org/2517663002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:365: else if (view_for_activation == focus_manager->GetStoredFocusView()) { When you add parens to one branch of a conditional, add them too all (meaning line 363 above).
The CQ bit was checked by warx@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/2517663002/#ps40001 (title: "nit")
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": 40001, "attempt_start_ts": 1479768355695640,
"parent_rev": "97691f7ce4a08bd6cf9d64c9211d71f478d98f01", "commit_rev":
"c0838faab4b0063d5ccedc19d0a0887a5655181f"}
Message was sent while issue was closed.
Description was changed from ========== Fix unable to delete extensions continuously through keyboard Changes: (1) revert the cl in Issue 2451323002. (2) using a different approach, which is to avoid restorefocusedview call if desktopnativewidget is not active (since that should be handled in HandleActivationChanged call). BUG=665380 TEST=manual test see bug fixed. Also, this reverts the cl in Issue 2451323002, checked that the bug in that issue is not regressed. ========== to ========== Fix unable to delete extensions continuously through keyboard Changes: (1) revert the cl in Issue 2451323002. (2) using a different approach, which is to avoid restorefocusedview call if desktopnativewidget is not active (since that should be handled in HandleActivationChanged call). BUG=665380 TEST=manual test see bug fixed. Also, this reverts the cl in Issue 2451323002, checked that the bug in that issue is not regressed. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix unable to delete extensions continuously through keyboard Changes: (1) revert the cl in Issue 2451323002. (2) using a different approach, which is to avoid restorefocusedview call if desktopnativewidget is not active (since that should be handled in HandleActivationChanged call). BUG=665380 TEST=manual test see bug fixed. Also, this reverts the cl in Issue 2451323002, checked that the bug in that issue is not regressed. ========== to ========== Fix unable to delete extensions continuously through keyboard Changes: (1) revert the cl in Issue 2451323002. (2) using a different approach, which is to avoid restorefocusedview call if desktopnativewidget is not active (since that should be handled in HandleActivationChanged call). BUG=665380 TEST=manual test see bug fixed. Also, this reverts the cl in Issue 2451323002, checked that the bug in that issue is not regressed. Committed: https://crrev.com/34ce9a4718f571b8f8ee0f95b6d153aff38223db Cr-Commit-Position: refs/heads/master@{#433706} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/34ce9a4718f571b8f8ee0f95b6d153aff38223db Cr-Commit-Position: refs/heads/master@{#433706} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
