Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(32)

Issue 2534953006: Fix shelf auto-hide calculation for app-list visibility. (Closed)

Created:
3 years, 8 months ago by msw
Modified:
3 years, 8 months ago
Reviewers:
James Cook
CC:
chromium-reviews, kalyank, sadrul, Matt Giuca, tfarina, stevenjb
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix shelf auto-hide calculation for app-list visibility. AppListButton tracks per-display [target] visibility. (sets a new flag on Shown/Dismissed callbacks) Check widget/button state, not global shell state. Ignore dismissed app-list windows that are animating closed. Fix a ShelfLayoutManager test to actually catch this. BUG=654661 TEST=Shelf stays visible when the app-list is hidden. R=jamescook@chromium.org Committed: https://crrev.com/f05a6730ca4ecdfe336276f0f96d33c2aeb028be Cr-Commit-Position: refs/heads/master@{#435554}

Patch Set 1 #

Patch Set 2 : Cleanup. #

Total comments: 8

Patch Set 3 : try another approach. #

Total comments: 4

Patch Set 4 : Address comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -30 lines) Patch
M ash/app_list/app_list_presenter_delegate.cc View 1 2 1 chunk +1 line, -5 lines 0 comments Download
M ash/common/shelf/app_list_button.h View 1 2 3 2 chunks +7 lines, -1 line 0 comments Download
M ash/common/shelf/app_list_button.cc View 1 2 3 3 chunks +5 lines, -0 lines 0 comments Download
M ash/common/shelf/shelf_layout_manager.cc View 1 2 3 chunks +15 lines, -14 lines 0 comments Download
M ash/common/shelf/shelf_widget.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M ash/common/shelf/shelf_widget.cc View 1 2 3 chunks +6 lines, -1 line 0 comments Download
M ash/shelf/shelf_layout_manager_unittest.cc View 3 chunks +3 lines, -8 lines 0 comments Download

Messages

Total messages: 28 (20 generated)
msw
Hey James, please take a look at ash/*, thanks! Hey Steven, please take a look ...
3 years, 8 months ago (2016-11-30 22:58:59 UTC) #7
James Cook
nit: Typo "visibility" in CL description Thanks for jumping on this! https://codereview.chromium.org/2534953006/diff/40001/ash/common/wm_shell.h File ash/common/wm_shell.h (right): ...
3 years, 8 months ago (2016-11-30 23:27:20 UTC) #10
msw
Hey James, please take a fresh look at this new approach; thanks! It's not ideal, ...
3 years, 8 months ago (2016-12-01 01:43:47 UTC) #14
James Cook
LGTM with nits https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_list_button.h File ash/common/shelf/app_list_button.h (right): https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_list_button.h#newcode68 ash/common/shelf/app_list_button.h:68: // True if the app list ...
3 years, 8 months ago (2016-12-01 02:02:08 UTC) #17
msw
https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_list_button.h File ash/common/shelf/app_list_button.h (right): https://codereview.chromium.org/2534953006/diff/60001/ash/common/shelf/app_list_button.h#newcode68 ash/common/shelf/app_list_button.h:68: // True if the app list is currently showing ...
3 years, 8 months ago (2016-12-01 04:18:13 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2534953006/80001
3 years, 8 months ago (2016-12-01 04:18:53 UTC) #23
commit-bot: I haz the power
Committed patchset #4 (id:80001)
3 years, 8 months ago (2016-12-01 04:52:21 UTC) #26
commit-bot: I haz the power
3 years, 8 months ago (2016-12-01 04:55:40 UTC) #28
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/f05a6730ca4ecdfe336276f0f96d33c2aeb028be
Cr-Commit-Position: refs/heads/master@{#435554}

Powered by Google App Engine
This is Rietveld 408576698