Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824233003:
Disallow toggling of shelf auto-hide with touch when there are no windows open (Closed)
DescriptionDisallow toggling of shelf auto-hide with touch when there are no windows open.
When there are no windows opened, do not change the auto_hide status when
swiping down (or swiping up and then releasing) on the shelf.
BUG=310322
Review-Url: https://codereview.chromium.org/2824233003
Cr-Commit-Position: refs/heads/master@{#468156}
Committed: https://chromium.googlesource.com/chromium/src/+/a88d3cb58a45213da672a9614c04c949c2e0f215
Patch Set 1 #Patch Set 2 : Add UT. #
Total comments: 5
Patch Set 3 : Addressed comments. #
Total comments: 12
Patch Set 4 : Addressed comments. #
Total comments: 1
Patch Set 5 : Fix comments. #
Messages
Total messages: 52 (32 generated)
The CQ bit was checked by minch@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.
Description was changed from ========== Disallow toggling of shelf auto-hide with touch when there are no windows open BUG=310322 ========== to ========== Disallow toggling of shelf auto-hide with touch when there are no windows open. When there are no windows opened, do not change the auto_hide status when swiping down (or swiping up and then releasing) on the shelf. BUG=310322 ==========
minch@chromium.org changed reviewers: + oshima@chromium.org, tdanderson@chromium.org, xiaoyinh@chromium.org
minch@chromium.org changed reviewers: - xiaoyinh@chromium.org
oshima@, tdanderson@, can you help review? Thanks.
can you add a unit test?
The CQ bit was checked by minch@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...
minch@chromium.org changed reviewers: + xiaoyinh@chromium.org - oshima@chromium.org, tdanderson@chromium.org
Hi, xiaoyinh@, can you help take a look? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:885: visible_window = true; nit: Just return true here, and return false at the end of the function. visible_window won't be needed. https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:1142: // Since the shelf will always be shown when there are no windows opend. There nit: s/opend/opened https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager_unittest.cc (right): https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager_unittest.cc:622: // windows. nit: EXPECT_FALSE(layout_manager->HasVisibleWindow()); here?
https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:885: visible_window = true; On 2017/04/21 04:13:12, xiaoyinh wrote: > nit: Just return true here, and return false at the end of the function. > visible_window won't be needed. good point. Thanks. https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager_unittest.cc (right): https://codereview.chromium.org/2824233003/diff/20001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager_unittest.cc:622: // windows. On 2017/04/21 04:13:12, xiaoyinh wrote: > nit: EXPECT_FALSE(layout_manager->HasVisibleWindow()); here? No window opened after line 608. I think maybe there is no need to call it again here.
The CQ bit was checked by minch@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.
minch@chromium.org changed reviewers: + oshima@chromium.org, tdanderson@chromium.org
Hi, oshima@, tdanderson@, xiaoyinh@, can you help review? Thanks.
Hi, oshima@, tdanderson@, xiaoyinh@, can you help review? Thanks.
I've left a couple of comments but will defer to oshima@ for a more detailed review since I am not an owner of the Ash shelf. https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:1136: // on the shelf. Based on this comment, shouldn't line 1139 instead be !HasVisibleWindow()? nit: comma instead of period at the end of line 1134. nit: start the comment at the end of line 1133 instead of on its own line. https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:1139: HasVisibleWindow()) nit: use {} after the if and else since the if condition is now more than a single line. https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager_unittest.cc (right): https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager_unittest.cc:605: // effect. Consider just "Close the window" or omitting comments here entirely since you say the same thing on line 610.
https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:1136: // on the shelf. On 2017/04/24 16:57:23, tdanderson wrote: > Based on this comment, shouldn't line 1139 instead be !HasVisibleWindow()? > > nit: comma instead of period at the end of line 1134. > > nit: start the comment at the end of line 1133 instead of on its own line. Line 1140 will call SetAutoHideBehavior() which will change the shelf auto hide state. But we don't want this happens when there are no windows open. Keep shelf state as 'auto hide' if it is 'auto hide' regardless swiping actions. The same as 'non auto hide'.
https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:1136: // on the shelf. On 2017/04/24 17:42:40, minch1 wrote: > On 2017/04/24 16:57:23, tdanderson wrote: > > Based on this comment, shouldn't line 1139 instead be !HasVisibleWindow()? > > > > nit: comma instead of period at the end of line 1134. > > > > nit: start the comment at the end of line 1133 instead of on its own line. > > Line 1140 will call SetAutoHideBehavior() which will change the shelf auto hide > state. But we don't want this happens when there are no windows open. Keep shelf > state as 'auto hide' if it is 'auto hide' regardless swiping actions. The same > as 'non auto hide'. Ah yes, I was mis-reading the code, sorry. As an optional suggestion, consider a re-wording of the comment on line 1134 to be "Only change the auto-hide behavior if there is at least one window visible."
https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:876: for (size_t i = 0; i < windows.size(); ++i) { auto* window : windows https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:877: if (windows[i] && windows[i]->IsVisible() && !IsAppListWindow(windows[i]) && the list shouldn't contain null. Was this null check necessary? https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:878: !windows[i]->GetWindowState()->IsMinimized() && IIRC, minimized window isn't visible, no?
oshima@, can you help check the replies? https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:877: if (windows[i] && windows[i]->IsVisible() && !IsAppListWindow(windows[i]) && On 2017/04/25 18:02:16, oshima wrote: > the list shouldn't contain null. Was this null check necessary? Checked line 873 seems it will not contains null windows. Not sure when it will has null windows. If it will has no null windows here, we don't need to check it here. https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:878: !windows[i]->GetWindowState()->IsMinimized() && On 2017/04/25 18:02:16, oshima wrote: > IIRC, minimized window isn't visible, no? Checked BuildWindowListsIgnoredModal() in line 873. It focuses on the windows that considered as activated. Seems that minimized windows can always be activated? What we need to consider is not the visibility of the window but is it included in the |windows| in line 872 or not?
https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:877: if (windows[i] && windows[i]->IsVisible() && !IsAppListWindow(windows[i]) && On 2017/04/25 23:43:08, minch1 wrote: > On 2017/04/25 18:02:16, oshima wrote: > > the list shouldn't contain null. Was this null check necessary? > > Checked line 873 seems it will not contains null windows. Not sure when it will > has null windows. If it will has no null windows here, we don't need to check it > here. It shouldn't have, as it only returns activatable ones. https://codereview.chromium.org/2824233003/diff/40001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:878: !windows[i]->GetWindowState()->IsMinimized() && On 2017/04/25 23:43:08, minch1 wrote: > On 2017/04/25 18:02:16, oshima wrote: > > IIRC, minimized window isn't visible, no? > > Checked BuildWindowListsIgnoredModal() in line 873. It focuses on the windows > that considered as activated. Seems that minimized windows can always be > activated? Yes, a minimized window can be activated. > What we need to consider is not the visibility of the window but is > it included in the |windows| in line 872 or not? A minimized window is such case, but there is no guarantee (from this code) that that's only case. I think you should be able to just check visibility.
The CQ bit was checked by minch@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.
oshima@, can you take a look again? Thanks.
https://codereview.chromium.org/2824233003/diff/60001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2824233003/diff/60001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.cc:871: .id(); Sorry I missed this suggestion. It's simpler and faster to use the root window like this. root = shelf_widget_->GetNativeWindow()->GetRootWindow(); then root->Contains(window) below, instead of checking display id.
The CQ bit was checked by minch@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_chromeos_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 minch@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.
oshima@, can you help take a look? Thanks.
lgtm thanks!
The CQ bit was checked by minch@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": 80001, "attempt_start_ts": 1493416038333320,
"parent_rev": "1465b8cd7572282cebd6f05d226cc358cef80aec", "commit_rev":
"a88d3cb58a45213da672a9614c04c949c2e0f215"}
Message was sent while issue was closed.
Description was changed from ========== Disallow toggling of shelf auto-hide with touch when there are no windows open. When there are no windows opened, do not change the auto_hide status when swiping down (or swiping up and then releasing) on the shelf. BUG=310322 ========== to ========== Disallow toggling of shelf auto-hide with touch when there are no windows open. When there are no windows opened, do not change the auto_hide status when swiping down (or swiping up and then releasing) on the shelf. BUG=310322 Review-Url: https://codereview.chromium.org/2824233003 Cr-Commit-Position: refs/heads/master@{#468156} Committed: https://chromium.googlesource.com/chromium/src/+/a88d3cb58a45213da672a9614c04... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a88d3cb58a45213da672a9614c04... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
