Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2859333003:
cros: Fix bubble dialog shows on desktop when parent window is invisible (Closed)
|
Created:
3 years, 7 months ago by Qiang(Joe) Xu Modified:
3 years, 7 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Fix bubble dialog shows on desktop when parent window is invisible
changes:
CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=native_widget_aura&dr=C&l=180. We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility.
BUG=704350
TEST=added test coverage
After this change, the reporter's bug will become: when browser is minimized, the dialog will activate/unminimize the browser.
Review-Url: https://codereview.chromium.org/2859333003
Cr-Commit-Position: refs/heads/master@{#470231}
Committed: https://chromium.googlesource.com/chromium/src/+/ca0239ad985f47fde15a73d64e3cdac1d8d3660f
Patch Set 1 #Patch Set 2 : fix test failures #Patch Set 3 : BubbleDialogDelegate #Patch Set 4 : tests failure #Patch Set 5 : fix bubble showing on desktop when minimized only #
Total comments: 2
Patch Set 6 : add comment #
Total comments: 2
Patch Set 7 : comment and added test coverage #
Total comments: 4
Patch Set 8 : feedback #
Total comments: 4
Patch Set 9 : just create widget on stack #
Messages
Total messages: 54 (41 generated)
Description was changed from ========== cros: TYPE_BUBBLE widget as transient child should have visibility controlled by parent BUG=704350 ========== to ========== TYPE_BUBBLE widget as transient child should have visibility controlled by parent BUG=704350 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 ========== TYPE_BUBBLE widget as transient child should have visibility controlled by parent BUG=704350 ========== to ========== TYPE_BUBBLE widget as transient child should have visibility controlled by parent changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ==========
Description was changed from ========== TYPE_BUBBLE widget as transient child should have visibility controlled by parent changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ========== to ========== TYPE_BUBBLE widget as transient child should have visibility controlled by parent changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ==========
Description was changed from ========== TYPE_BUBBLE widget as transient child should have visibility controlled by parent changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ========== to ========== cros: Fix permission bubble wrong behavior on Chrome OS changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ==========
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Description was changed from ========== cros: Fix permission bubble wrong behavior on Chrome OS changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ========== to ========== cros: Fix permission bubble wrong behavior on Chrome OS changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
warx@chromium.org changed reviewers: + oshima@chromium.org, sky@chromium.org
Description was changed from ========== cros: Fix permission bubble wrong behavior on Chrome OS changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ========== to ========== crcros: Fix bubble dialog wrong behavior on Chrome OS changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ==========
Description was changed from ========== crcros: Fix bubble dialog wrong behavior on Chrome OS changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ========== to ========== cros: Fix bubble dialog wrong behavior changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ==========
Description was changed from ========== cros: Fix bubble dialog wrong behavior changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ========== to ========== cros: Fix bubble dialog behavior and make it follow other platforms changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== cros: Fix bubble dialog behavior and make it follow other platforms changes: (1) CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. (2) Make TYPE_BUBBLE widget default non-activatable and make BubbleDialogDelegate's InitParams.activatable default. After these two changes, the permission bubble will follow browser window's visibility, so that it will not show when browser is not visible. And it will not make browser window activated, which also follows the behavior on other platforms. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc ========== to ========== cros: Fix bubble dialog shows on desktop when parent window is invisible changes: CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc. After this change, the reporter's bug will become: when browser is minimized, the dialog will activate/unminimize the browser. ==========
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...
Hi all, PTAL, thanks! About bubble dialog stealing focus (activating browser window), let us make it a separate issue. The mac test failure drives me to recheck the behavior. I found chromeos/windows/mac permission bubble will steal the focus, only linux platform will not. I feel it is better to deal with it separately. I will also update the bugtracker.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2859333003/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2859333003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_aura.cc:186: if (params.type == Widget::InitParams::TYPE_BUBBLE) { This is very subtle and worth a comment.
A comment is added. PTAL, thanks https://codereview.chromium.org/2859333003/diff/80001/ui/views/widget/native_... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2859333003/diff/80001/ui/views/widget/native_... ui/views/widget/native_widget_aura.cc:186: if (params.type == Widget::InitParams::TYPE_BUBBLE) { On 2017/05/05 23:23:19, sky wrote: > This is very subtle and worth a comment. Done.
can you add a unit test?
SLGTM - as Oshima says though, a test would be good here. https://codereview.chromium.org/2859333003/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2859333003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:186: // For the TYPE_BUBBLE transient child window, make its visibility Your comment just documents the code, document *why*. I think you want something like: Generally transient bubbles are showing state associated to the parent window. Make sure the transient bubble is only visible if the parent is visible, otherwise the bubble may not make sense by itself.
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 ========== cros: Fix bubble dialog shows on desktop when parent window is invisible changes: CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. BUG=704350 TEST=set_parent_controls_visibility API is covered in transient_window_manager_unittest.cc. After this change, the reporter's bug will become: when browser is minimized, the dialog will activate/unminimize the browser. ========== to ========== cros: Fix bubble dialog shows on desktop when parent window is invisible changes: CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. BUG=704350 TEST=added test coverage After this change, the reporter's bug will become: when browser is minimized, the dialog will activate/unminimize the browser. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hi all, comment updated & test coverage added, PTAL https://codereview.chromium.org/2859333003/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_aura.cc (right): https://codereview.chromium.org/2859333003/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_aura.cc:186: // For the TYPE_BUBBLE transient child window, make its visibility On 2017/05/07 22:29:38, sky wrote: > Your comment just documents the code, document *why*. I think you want something > like: > > Generally transient bubbles are showing state associated to the parent window. > Make sure the transient bubble is only visible if the parent is visible, > otherwise the bubble may not make sense by itself. done, thanks
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2859333003/diff/120001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2859333003/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:659: Widget* parent = new Widget; Is this leaked? Was you're using WIDGET_OWNS_NATIVE_WIDGET define the value on the stack. https://codereview.chromium.org/2859333003/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:665: parent->Show(); Isn't the bug scenario in the bug where the parent is initially hidden?
Updated PTAL thanks https://codereview.chromium.org/2859333003/diff/120001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2859333003/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:659: Widget* parent = new Widget; On 2017/05/08 21:24:29, sky wrote: > Is this leaked? Was you're using WIDGET_OWNS_NATIVE_WIDGET define the value on > the stack. changed to std::unique_ptr https://codereview.chromium.org/2859333003/diff/120001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:665: parent->Show(); On 2017/05/08 21:24:29, sky wrote: > Isn't the bug scenario in the bug where the parent is initially hidden? In the last, I tested when parent is hidden. But testing with initial hidden state would use less code lines. I updated it. Thanks!
LGTM https://codereview.chromium.org/2859333003/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2859333003/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:659: std::unique_ptr<Widget> parent(new Widget); No need for the unique_ptr here, declare parent on the stack. https://codereview.chromium.org/2859333003/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:667: std::unique_ptr<Widget> child(new Widget); Same comment here.
Thanks, updated https://codereview.chromium.org/2859333003/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/2859333003/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:659: std::unique_ptr<Widget> parent(new Widget); On 2017/05/08 22:42:11, sky wrote: > No need for the unique_ptr here, declare parent on the stack. Done. https://codereview.chromium.org/2859333003/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_aura_unittest.cc:667: std::unique_ptr<Widget> child(new Widget); On 2017/05/08 22:42:11, sky wrote: > Same comment here. Done.
lgtm thanks!
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/2859333003/#ps160001 (title: "just create widget on stack")
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": 160001, "attempt_start_ts": 1494299108487210,
"parent_rev": "c55169a7c2279f0f30e15bbca66678f48e7df106", "commit_rev":
"ca0239ad985f47fde15a73d64e3cdac1d8d3660f"}
Message was sent while issue was closed.
Description was changed from ========== cros: Fix bubble dialog shows on desktop when parent window is invisible changes: CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. BUG=704350 TEST=added test coverage After this change, the reporter's bug will become: when browser is minimized, the dialog will activate/unminimize the browser. ========== to ========== cros: Fix bubble dialog shows on desktop when parent window is invisible changes: CreateBubbleWidget in bubble_dialog_delegate.cc does not set params.child, so it is added as transient child on chrome os: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?q=.... We should set_parent_controls_visibility(true) for it, which makes the window's visibility following the transient parent's visibility. BUG=704350 TEST=added test coverage After this change, the reporter's bug will become: when browser is minimized, the dialog will activate/unminimize the browser. Review-Url: https://codereview.chromium.org/2859333003 Cr-Commit-Position: refs/heads/master@{#470231} Committed: https://chromium.googlesource.com/chromium/src/+/ca0239ad985f47fde15a73d64e3c... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/ca0239ad985f47fde15a73d64e3c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
