Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748983004:
cros: Fix shelf overlaps bottom of maximized window in guest session (Closed)
|
Created:
3 years, 9 months ago by Qiang(Joe) Xu Modified:
3 years, 9 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Fix shelf overlaps bottom of maximized window in guest session
Changes:
CL in https://codereview.chromium.org/2688353005 makes change that leave the shelf bottom locked and hidden until user prefs are loaded. When ChromeLauncherController::SetShelfBehaviorsFromPrefs(), it will be blocked on https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_controller.cc?l=96, which leaves shelf alignment bottom locked in guest session.
We don't set display work area change for bottom locked, that is why bug happens.
Solution is to delete the CanChangeShelfAlignment() check as it should be enforced on shelf context menu already.
BUG=699661
TEST=emulator test saw reporter's bug fixed
Review-Url: https://codereview.chromium.org/2748983004
Cr-Commit-Position: refs/heads/master@{#457246}
Committed: https://chromium.googlesource.com/chromium/src/+/5859fcdeb7fa7508f902d80d401958c956cb964f
Patch Set 1 #
Messages
Total messages: 21 (12 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 ========== cros: Fix shelf overlaps bottom of maximized window in guest session BUG=699661 ========== to ========== cros: Fix shelf overlaps bottom of maximized window in guest session BUG=699661 ==========
warx@chromium.org changed reviewers: + msw@chromium.org
Description was changed from ========== cros: Fix shelf overlaps bottom of maximized window in guest session BUG=699661 ========== to ========== cros: Fix shelf overlaps bottom of maximized window in guest session Changes: CL in https://codereview.chromium.org/2688353005 makes change that leave the shelf bottom locked and hidden until user prefs are loaded. When ChromeLauncherController::SetShelfBehaviorsFromPrefs(), it will be blocked on https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_controller.cc?l=96, which leaves shelf alignment bottom locked in guest session. We don't set display work area change for bottom locked, that is why bug happens. Solution is to delete the CanChangeShelfAlignment() check as it should be enforced on shelf context menu already. BUG=699661 ==========
warx@chromium.org changed reviewers: + jamescook@chromium.org
Description was changed from ========== cros: Fix shelf overlaps bottom of maximized window in guest session Changes: CL in https://codereview.chromium.org/2688353005 makes change that leave the shelf bottom locked and hidden until user prefs are loaded. When ChromeLauncherController::SetShelfBehaviorsFromPrefs(), it will be blocked on https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_controller.cc?l=96, which leaves shelf alignment bottom locked in guest session. We don't set display work area change for bottom locked, that is why bug happens. Solution is to delete the CanChangeShelfAlignment() check as it should be enforced on shelf context menu already. BUG=699661 ========== to ========== cros: Fix shelf overlaps bottom of maximized window in guest session Changes: CL in https://codereview.chromium.org/2688353005 makes change that leave the shelf bottom locked and hidden until user prefs are loaded. When ChromeLauncherController::SetShelfBehaviorsFromPrefs(), it will be blocked on https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_controller.cc?l=96, which leaves shelf alignment bottom locked in guest session. We don't set display work area change for bottom locked, that is why bug happens. Solution is to delete the CanChangeShelfAlignment() check as it should be enforced on shelf context menu already. BUG=699661 TEST=emulator test saw reporter's bug fixed ==========
Hi all, ptal whether this is a right fix for you, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hmm, this is probably okay; the check is really only needed to prevent changes triggered by user interaction (via the context menu, and it is also checked there). We shouldn't actually block SetAlignment calls when loading the default alignment for these various session types. lgtm, but a test would be nice.
LGTM too. I agree a test would be nice. I suggest backporting this fix to M58.
On 2017/03/15 20:47:32, James Cook wrote: > LGTM too. I agree a test would be nice. > > I suggest backporting this fix to M58. I have a question about what the test case could be here. We actually need to test if shelf alignment is properly set when guest session starts. However, in tests now it is done on-shelf-created by ShelfInitializer. So before this change, in test even in guest session it is already set to bottom aligned, not bottom locked. So, I am a little unclear about "test if shelf alignment is properly set when guest session starts", can we do this? Another is, to test whether we can set alignment to other alignments in guest session. This is a bit of trivial. Do we really need to test this? What do you think?
On 2017/03/15 22:19:13, Qiang(Joe) Xu wrote: > I have a question about what the test case could be here. > > We actually need to test if shelf alignment is properly set when guest session > starts. However, in tests now it is done on-shelf-created by ShelfInitializer. > So before this change, in test even in guest session it is already set to bottom > aligned, not bottom locked. So, I am a little unclear about "test if shelf > alignment is properly set when guest session starts", can we do this? > > Another is, to test whether we can set alignment to other alignments in guest > session. This is a bit of trivial. Do we really need to test this? > > What do you think? Yeah, ShelfInitializer makes unit-testing fairly pointless... Perhaps browser_tests or interactive_ui_tests would offer a better place to start up a real guest session and check the shelf alignment (or maybe check the work area and/or maximized window bounds versus shelf bounds). If this is a lot of work, feel free to land this as-is to fix the issue and merge something smaller back to M58, then pursue a test in a followup?
On 2017/03/15 22:23:07, msw wrote: > On 2017/03/15 22:19:13, Qiang(Joe) Xu wrote: > > I have a question about what the test case could be here. > > > > We actually need to test if shelf alignment is properly set when guest session > > starts. However, in tests now it is done on-shelf-created by ShelfInitializer. > > So before this change, in test even in guest session it is already set to > bottom > > aligned, not bottom locked. So, I am a little unclear about "test if shelf > > alignment is properly set when guest session starts", can we do this? > > > > Another is, to test whether we can set alignment to other alignments in guest > > session. This is a bit of trivial. Do we really need to test this? > > > > What do you think? > > Yeah, ShelfInitializer makes unit-testing fairly pointless... Perhaps > browser_tests or interactive_ui_tests would offer a better place to start up a > real guest session and check the shelf alignment (or maybe check the work area > and/or maximized window bounds versus shelf bounds). If this is a lot of work, > feel free to land this as-is to fix the issue and merge something smaller back > to M58, then pursue a test in a followup? +1 to Mike's suggestion.
On 2017/03/15 22:23:07, msw wrote: > On 2017/03/15 22:19:13, Qiang(Joe) Xu wrote: > > I have a question about what the test case could be here. > > > > We actually need to test if shelf alignment is properly set when guest session > > starts. However, in tests now it is done on-shelf-created by ShelfInitializer. > > So before this change, in test even in guest session it is already set to > bottom > > aligned, not bottom locked. So, I am a little unclear about "test if shelf > > alignment is properly set when guest session starts", can we do this? > > > > Another is, to test whether we can set alignment to other alignments in guest > > session. This is a bit of trivial. Do we really need to test this? > > > > What do you think? > > Yeah, ShelfInitializer makes unit-testing fairly pointless... Perhaps > browser_tests or interactive_ui_tests would offer a better place to start up a > real guest session and check the shelf alignment (or maybe check the work area > and/or maximized window bounds versus shelf bounds). If this is a lot of work, > feel free to land this as-is to fix the issue and merge something smaller back > to M58, then pursue a test in a followup? Agree, that should probably be a test involves ChromeLauncherController. OK, I will land this first and file-bug/work on the test coverage later when I get time, since test coverage doesn't need to do merge. Thanks for review!
The CQ bit was checked by warx@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": 1, "attempt_start_ts": 1489617848682920, "parent_rev":
"56024c61ffdabb977c7645098cb4654c6a1fdcc1", "commit_rev":
"5859fcdeb7fa7508f902d80d401958c956cb964f"}
Message was sent while issue was closed.
Description was changed from ========== cros: Fix shelf overlaps bottom of maximized window in guest session Changes: CL in https://codereview.chromium.org/2688353005 makes change that leave the shelf bottom locked and hidden until user prefs are loaded. When ChromeLauncherController::SetShelfBehaviorsFromPrefs(), it will be blocked on https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_controller.cc?l=96, which leaves shelf alignment bottom locked in guest session. We don't set display work area change for bottom locked, that is why bug happens. Solution is to delete the CanChangeShelfAlignment() check as it should be enforced on shelf context menu already. BUG=699661 TEST=emulator test saw reporter's bug fixed ========== to ========== cros: Fix shelf overlaps bottom of maximized window in guest session Changes: CL in https://codereview.chromium.org/2688353005 makes change that leave the shelf bottom locked and hidden until user prefs are loaded. When ChromeLauncherController::SetShelfBehaviorsFromPrefs(), it will be blocked on https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_controller.cc?l=96, which leaves shelf alignment bottom locked in guest session. We don't set display work area change for bottom locked, that is why bug happens. Solution is to delete the CanChangeShelfAlignment() check as it should be enforced on shelf context menu already. BUG=699661 TEST=emulator test saw reporter's bug fixed Review-Url: https://codereview.chromium.org/2748983004 Cr-Commit-Position: refs/heads/master@{#457246} Committed: https://chromium.googlesource.com/chromium/src/+/5859fcdeb7fa7508f902d80d4019... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/5859fcdeb7fa7508f902d80d4019... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
