New issue
Advanced search Search tips

Issue 846736 link

Starred by 2 users
Status: Available
Owner: ----
Cc:
est...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Clean up WindowSizer, especially for Chrome OS and for tests

Project Member Reported by jamescook@chromium.org, May 25 2018 
* The code is a tangled mess split between window_sizer.cc, window_sizer_ash.cc, window_sizer_common_unittest.cc, window_sizer_ash_unittest.cc, etc.
* WindowSizer has ifdef'd member functions that are implemented in other files.
* Tests sometimes inject a Screen and sometimes don't, which adds complexity around target display lookup
* Platform-specific code lives in //c/b/ui

The core problem is that there is no platform-specific factory for these objects. If we had one we could subclass it for Chrome OS (and for Mac) and keep the impls in the platform directories.
Project Member

Comment 1 by bugdroid1@chromium.org, May 25 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7088679c129b24dbaa704e162bf4992246d5032f

commit 7088679c129b24dbaa704e162bf4992246d5032f
Author: James Cook <jamescook@chromium.org>
Date: Fri May 25 23:30:07 2018

Clean up WindowSizer tests

Simplify the TestScreen to use display::ScreenBase.
Always use the global Screen::GetScreen().

The eliminates one WindowSizer constructor and the |screen_| member,
such that the code in test is similar to the code in production.

Bug: 846736
Test: unit_tests
Change-Id: I39121a9b8ae1283c4bbea6194c528450243af24e
Reviewed-on: https://chromium-review.googlesource.com/1073961
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562060}
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/ash/shell_state_client.cc
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/window_sizer/window_sizer.cc
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/window_sizer/window_sizer.h
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/window_sizer/window_sizer_ash.cc
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/window_sizer/window_sizer_common_unittest.h
[modify] https://crrev.com/7088679c129b24dbaa704e162bf4992246d5032f/chrome/browser/ui/window_sizer/window_sizer_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, May 29 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d705a122ffcd22527bca5003d5c685ed52398ead

commit d705a122ffcd22527bca5003d5c685ed52398ead
Author: James Cook <jamescook@chromium.org>
Date: Tue May 29 17:59:43 2018

Clean up WindowSizer test code for ash

* Move ash-specific test helper functions into the ash unittest file
* Make all ash tests use the test harness display via UpdateDisplays()
  rather than adding an additional TestScreen
* Moved shared TestingProfile setup into test base class
* Remove unused enum Edge

Bug: 846736
Test: unit_tests
Change-Id: I062e6ec1632db5a99339c8d662d5343c963fd3f1
Reviewed-on: https://chromium-review.googlesource.com/1075723
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562501}
[modify] https://crrev.com/d705a122ffcd22527bca5003d5c685ed52398ead/chrome/browser/ui/window_sizer/window_sizer.h
[modify] https://crrev.com/d705a122ffcd22527bca5003d5c685ed52398ead/chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
[modify] https://crrev.com/d705a122ffcd22527bca5003d5c685ed52398ead/chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc
[modify] https://crrev.com/d705a122ffcd22527bca5003d5c685ed52398ead/chrome/browser/ui/window_sizer/window_sizer_common_unittest.h

Comment 3 by ovanieva@chromium.org, Jun 1 2018

Status: Available (was: Untriaged)
►

Sign in to add a comment