Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639623003:
ScreenWin Testability and Restructuring (Closed)
DescriptionScreenWin Testability and Restructuring
To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow
override of system OS calls through virtual methods as well as override the
available displays available in the system.
This CL also adds support to hold system pixel display rects that will be
used by multi-DPI aware scaling coming in a later CL.
BUG=426656
Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f
Cr-Commit-Position: refs/heads/master@{#373582}
Committed: https://crrev.com/eb17ae163b2ba4a2a67cd88a25b349249ed63f71
Cr-Commit-Position: refs/heads/master@{#374208}
Patch Set 1 #
Total comments: 49
Patch Set 2 : CR Feedback #Patch Set 3 : Rebase to bdd95f1 #
Total comments: 15
Patch Set 4 : CR Feedback #
Total comments: 10
Patch Set 5 : CR Feedback #Patch Set 6 : CR Feedback #
Total comments: 2
Patch Set 7 : #
Total comments: 12
Patch Set 8 : CR Feedback #Patch Set 9 : Fix Consts #Patch Set 10 : Sync + VS2015 Friendly - struct -> class to maintain immutability #
Messages
Total messages: 89 (38 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/1
Description was changed from ========== ScreenWin Testability Restructuring BUG=426656 ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL introduces a level of indirection called the DisplayManager. The DisplayManager manages all of the displays and contains virtual methods that call system APIs that can be overridden by tests. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ==========
robliao@chromium.org changed reviewers: + oshima@chromium.org
oshima: Please review this CL. It introduces testability to ScreenWin as well as prepares the necessary infrastructure for multi-dpi displays. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1639623003/diff/1/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/display.h#newcode164 ui/gfx/display.h:164: } One concern I have is that this differ from identifying / looking up display which uses id, and this may be misused in the non test code in the future. how about moving to ui/gfx/test/display_util.h ? https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h#newcode34 ui/gfx/screen_win.h:34: virtual ~ScreenWin() override; nuke virtual https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h#newcode56 ui/gfx/screen_win.h:56: // Overridden from gfx::win::DisplayManagerObserver: // gfx::win::DisplayManagerObserver: https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h#newcode66 ui/gfx/screen_win.h:66: gfx::win::DisplayManager* display_manager_; document ownership https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.cc File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:75: } nuke {} https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:105: } nuke {} https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:161: }; Do you need this interface? Are you planning to add different initializer? https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:165: TestScreenWinInitializerImpl() : hwndLast_(0) {} you can initialize in the definition below. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:241: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:259: HWND fake_hwnd_; = 0/nullptr ? https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:260: }; ditto https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:325: }; ditto https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:479: }; ditto https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:579: ditto https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_info.cc File ui/gfx/win/display_info.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_info.cc#... ui/gfx/win/display_info.cc:77: } any reason not to inline these? https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.h File ui/gfx/win/display_manager.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.... ui/gfx/win/display_manager.h:43: const std::vector<ScreenWinDisplay>& GetScreenWinDisplays(); maybe you should have new line here and https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.... ui/gfx/win/display_manager.h:49: ScreenWinDisplay GetPrimaryScreenWinDisplay() const; document this https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.... ui/gfx/win/display_manager.h:51: // For Unit Tests. methods for test should either be named XxxForTest/XxxForTesting, or in a separate test api class with test sub namespace. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager_... File ui/gfx/win/display_manager_observer.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager_... ui/gfx/win/display_manager_observer.h:25: virtual ~DisplayManagerObserver(); you may inline this https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/screen_win_displ... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/screen_win_displ... ui/gfx/win/screen_win_display.h:26: const Rect& physical_bounds() const; you may inline thes
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Thanks for the review! https://codereview.chromium.org/1639623003/diff/1/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/display.h#newcode164 ui/gfx/display.h:164: } On 2016/01/28 18:32:04, oshima wrote: > One concern I have is that this differ from identifying / looking up display > which uses id, and this may be misused in the non test code in the future. > > how about moving to ui/gfx/test/display_util.h ? sgtm. Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h#newcode34 ui/gfx/screen_win.h:34: virtual ~ScreenWin() override; On 2016/01/28 18:32:04, oshima wrote: > nuke virtual Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h#newcode56 ui/gfx/screen_win.h:56: // Overridden from gfx::win::DisplayManagerObserver: On 2016/01/28 18:32:04, oshima wrote: > // gfx::win::DisplayManagerObserver: Updated the gfx::Screen: comment for consistency too then. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win.h#newcode66 ui/gfx/screen_win.h:66: gfx::win::DisplayManager* display_manager_; On 2016/01/28 18:32:04, oshima wrote: > document ownership Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.cc File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:75: } On 2016/01/28 18:32:04, oshima wrote: > nuke {} Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:105: } On 2016/01/28 18:32:04, oshima wrote: > nuke {} Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:161: }; On 2016/01/28 18:32:04, oshima wrote: > Do you need this interface? Are you planning to add different initializer? This limits what methods the tests can use and was cleaner than using a friend class. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:165: TestScreenWinInitializerImpl() : hwndLast_(0) {} On 2016/01/28 18:32:04, oshima wrote: > you can initialize in the definition below. Huh, fun new C++ feature! https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:241: }; On 2016/01/28 18:32:04, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:259: HWND fake_hwnd_; On 2016/01/28 18:32:04, oshima wrote: > = 0/nullptr ? Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:260: }; On 2016/01/28 18:32:04, oshima wrote: > ditto Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:325: }; On 2016/01/28 18:32:04, oshima wrote: > ditto Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:479: }; On 2016/01/28 18:32:04, oshima wrote: > ditto Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:579: On 2016/01/28 18:32:04, oshima wrote: > ditto Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_info.cc File ui/gfx/win/display_info.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_info.cc#... ui/gfx/win/display_info.cc:77: } On 2016/01/28 18:32:04, oshima wrote: > any reason not to inline these? See https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... "Be careful about your accessors" https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.h File ui/gfx/win/display_manager.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.... ui/gfx/win/display_manager.h:43: const std::vector<ScreenWinDisplay>& GetScreenWinDisplays(); On 2016/01/28 18:32:05, oshima wrote: > maybe you should have new line here and Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.... ui/gfx/win/display_manager.h:49: ScreenWinDisplay GetPrimaryScreenWinDisplay() const; On 2016/01/28 18:32:04, oshima wrote: > document this Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager.... ui/gfx/win/display_manager.h:51: // For Unit Tests. On 2016/01/28 18:32:05, oshima wrote: > methods for test should either be named XxxForTest/XxxForTesting, or in a > separate test api class with test sub namespace. These are called by production code, but they are virtual for the benefit of unit testing. Clarified the docs. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager_... File ui/gfx/win/display_manager_observer.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/display_manager_... ui/gfx/win/display_manager_observer.h:25: virtual ~DisplayManagerObserver(); On 2016/01/28 18:32:05, oshima wrote: > you may inline this Indeed. Done. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/screen_win_displ... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/win/screen_win_displ... ui/gfx/win/screen_win_display.h:26: const Rect& physical_bounds() const; On 2016/01/28 18:32:05, oshima wrote: > you may inline thes See previous comment.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/screen_win.h#new... ui/gfx/screen_win.h:67: gfx::win::DisplayManager* display_manager_; Please use GetInstance(), as it may refer to stale object. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/test/display_util.h File ui/gfx/test/display_util.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/test/display_uti... ui/gfx/test/display_util.h:10: namespace gfx { namespace test namespace is meaningless for operator but it'll be useful if we add new function in the future. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... File ui/gfx/win/display_manager.cc (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager.cc:142: default_options)); fit to single line? https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... File ui/gfx/win/display_manager_observer.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager_observer.h:20: const std::vector<gfx::win::ScreenWinDisplay> old_screen_win_displays, & https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager_observer.h:21: const std::vector<gfx::win::ScreenWinDisplay> new_screen_win_displays) = & and indent https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/screen_win_d... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/screen_win_d... ui/gfx/win/screen_win_display.h:25: const Display& display() const; inline them. (or GetDisplay()/GetPhysicalBounds()) if you really want not to inline them)
https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/screen_win.h#new... ui/gfx/screen_win.h:67: gfx::win::DisplayManager* display_manager_; On 2016/01/30 00:10:33, oshima wrote: > Please use GetInstance(), as it may refer to stale object. Done. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/test/display_util.h File ui/gfx/test/display_util.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/test/display_uti... ui/gfx/test/display_util.h:10: namespace gfx { On 2016/01/30 00:10:33, oshima wrote: > namespace test > > > namespace is meaningless for operator but it'll be useful if we add new function > in the future. Oddly enough, it does matter. http://stackoverflow.com/questions/5195512/namespaces-and-operator-resolution The compiler can't find the operator otherwise. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... File ui/gfx/win/display_manager.cc (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager.cc:142: default_options)); On 2016/01/30 00:10:33, oshima wrote: > fit to single line? Done. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... File ui/gfx/win/display_manager_observer.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager_observer.h:20: const std::vector<gfx::win::ScreenWinDisplay> old_screen_win_displays, On 2016/01/30 00:10:33, oshima wrote: > & Done. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager_observer.h:21: const std::vector<gfx::win::ScreenWinDisplay> new_screen_win_displays) = On 2016/01/30 00:10:33, oshima wrote: > & > > and indent An indent shouldn't be necessary here as it's just the next item in the argument list. The 0 below is part of the assignment expression, which means a 4 space indent from the start of the expression. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/screen_win_d... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/screen_win_d... ui/gfx/win/screen_win_display.h:25: const Display& display() const; On 2016/01/30 00:10:33, oshima wrote: > inline them. (or GetDisplay()/GetPhysicalBounds()) if you really want not to > inline them) These are simple accessors and have to be named display() and physical_bounds() per https://www.chromium.org/developers/coding-style However, they shouldn't be inlined since they aren't light weight per https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... Having said that, it appears the inlining guidance is advisory instead of required, so if you're okay inlining these, so be it. I've updated the display_info as well for consistency. Done.
https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.cc File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:161: }; On 2016/01/29 01:44:40, robliao wrote: > On 2016/01/28 18:32:04, oshima wrote: > > Do you need this interface? Are you planning to add different initializer? > > This limits what methods the tests can use and was cleaner than using a friend > class. Sorry I missed your comment here. What's the advantage to do this way than using public/private? I can understand if you want to hide impl from client, but that advantage is limited as both are in the same file. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/test/display_util.h File ui/gfx/test/display_util.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/test/display_uti... ui/gfx/test/display_util.h:10: namespace gfx { On 2016/01/30 01:23:52, robliao wrote: > On 2016/01/30 00:10:33, oshima wrote: > > namespace test > > > > > > namespace is meaningless for operator but it'll be useful if we add new > function > > in the future. > > Oddly enough, it does matter. > http://stackoverflow.com/questions/5195512/namespaces-and-operator-resolution > > The compiler can't find the operator otherwise. interesting, thanks for the pointer. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... File ui/gfx/win/display_manager_observer.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager_observer.h:21: const std::vector<gfx::win::ScreenWinDisplay> new_screen_win_displays) = On 2016/01/30 01:23:52, robliao wrote: > On 2016/01/30 00:10:33, oshima wrote: > > & > > > > and indent > > An indent shouldn't be necessary here as it's just the next item in the argument > list. > > The 0 below is part of the assignment expression, which means a 4 space indent > from the start of the expression. git cl indent moves the argument to the next line. If you don't like it, you may keep it. https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... ui/gfx/screen_win_unittest.cc:265: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... ui/gfx/screen_win_unittest.cc:295: EXPECT_EQ(display, screen->GetDisplayNearestPoint(gfx::Point(1919, 1199))); what happens for the out of bounds point? https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... ui/gfx/screen_win_unittest.cc:303: screen->GetDisplayMatching(gfx::Rect(1819, 1099, 100, 100))); same here https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_info.h File ui/gfx/win/display_info.h (right): https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_inf... ui/gfx/win/display_info.h:18: class GFX_EXPORT DisplayInfo { final? https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_man... File ui/gfx/win/display_manager.h (right): https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_man... ui/gfx/win/display_manager.h:59: // Virtual for the benefit of Unit Tests. These are virtual to allow unit tests to mock the display manager behavior.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.cc File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:161: }; On 2016/02/01 19:07:37, oshima wrote: > On 2016/01/29 01:44:40, robliao wrote: > > On 2016/01/28 18:32:04, oshima wrote: > > > Do you need this interface? Are you planning to add different initializer? > > > > This limits what methods the tests can use and was cleaner than using a > friend > > class. > > Sorry I missed your comment here. What's the advantage to do this way than using > public/private? > > I can understand if you want to hide impl from client, but that advantage is > limited as both are in the same file. It prevents tests from accidentally initializing the ScreenWin twice for example. This actually happened during the development of the code. The alternative is to use a allow TestScreenWinInitializerImpl to friend class ScreenWinTest, which didn't seem as clean. This at least defines what tests can and cannot do. https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... File ui/gfx/win/display_manager_observer.h (right): https://codereview.chromium.org/1639623003/diff/80001/ui/gfx/win/display_mana... ui/gfx/win/display_manager_observer.h:21: const std::vector<gfx::win::ScreenWinDisplay> new_screen_win_displays) = On 2016/02/01 19:07:37, oshima wrote: > On 2016/01/30 01:23:52, robliao wrote: > > On 2016/01/30 00:10:33, oshima wrote: > > > & > > > > > > and indent > > > > An indent shouldn't be necessary here as it's just the next item in the > argument > > list. > > > > The 0 below is part of the assignment expression, which means a 4 space indent > > from the start of the expression. > > > git cl indent moves the argument to the next line. If you don't like it, you may > keep it. Sounds good. I think this is clearer this way as the = 0 is less important than the args. This sounds like a bug in git cl indentation is I would expect it to break at a higher syntactic level. https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... ui/gfx/screen_win_unittest.cc:265: On 2016/02/01 19:07:37, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... ui/gfx/screen_win_unittest.cc:295: EXPECT_EQ(display, screen->GetDisplayNearestPoint(gfx::Point(1919, 1199))); On 2016/02/01 19:07:37, oshima wrote: > what happens for the out of bounds point? The test will crash since the test harness doesn't support getting out of bounds points (See MonitorInfoFromScreenPoint's NOTREACHED). I figured this is reasonable as this will be handled by the Windows calls in the end and we're not looking to replicate all behaviors from those calls. https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/screen_win_unit... ui/gfx/screen_win_unittest.cc:303: screen->GetDisplayMatching(gfx::Rect(1819, 1099, 100, 100))); On 2016/02/01 19:07:37, oshima wrote: > same here See above. The test here is a bit more lenient in that it only requires the rect to at least be intersecting. https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_info.h File ui/gfx/win/display_info.h (right): https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_inf... ui/gfx/win/display_info.h:18: class GFX_EXPORT DisplayInfo { On 2016/02/01 19:07:37, oshima wrote: > final? Done. https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_man... File ui/gfx/win/display_manager.h (right): https://codereview.chromium.org/1639623003/diff/100001/ui/gfx/win/display_man... ui/gfx/win/display_manager.h:59: // Virtual for the benefit of Unit Tests. On 2016/02/01 19:07:37, oshima wrote: > These are virtual to allow unit tests to mock the display manager behavior. Changed to // Virtual to support mocking by unit tests.
lgtm with nits and minor q https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.cc File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:161: }; On 2016/02/01 21:35:04, robliao wrote: > On 2016/02/01 19:07:37, oshima wrote: > > On 2016/01/29 01:44:40, robliao wrote: > > > On 2016/01/28 18:32:04, oshima wrote: > > > > Do you need this interface? Are you planning to add different initializer? > > > > > > This limits what methods the tests can use and was cleaner than using a > > friend > > > class. > > > > Sorry I missed your comment here. What's the advantage to do this way than > using > > public/private? > > > > I can understand if you want to hide impl from client, but that advantage is > > limited as both are in the same file. > > It prevents tests from accidentally initializing the ScreenWin twice for > example. This actually happened during the development of the code. > > The alternative is to use a allow TestScreenWinInitializerImpl to friend class > ScreenWinTest, which didn't seem as clean. This at least defines what tests can > and cannot do. Other alternatives are to eliminate separate interface. I understand the motivation, so you may keep it. I just felt it's overkill. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:205: scoped_ptr<ScreenWin> screen_win_; it looks to me that this should belong to ScreenWinTest, rather than "initializer". You can set the screen in SetUp/TearDown (or dtor), and use gfx::Screen::GetInstance() to check if it's null. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:232: } It's better to use gfx::Screen::GetInstance() as it's more real. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:454: gfx::SetDefaultDeviceScaleFactor(2.0); just noticed this. why this is necessary?
https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.cc File ui/gfx/screen_win_unittest.cc (right): https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:205: scoped_ptr<ScreenWin> screen_win_; On 2016/02/01 22:15:44, oshima wrote: > it looks to me that this should belong to ScreenWinTest, rather than > "initializer". > > You can set the screen in SetUp/TearDown (or dtor), and > use gfx::Screen::GetInstance() to check if it's null. Similar to how Screen's are handled in production code, the intent of TestScreenWinInitializerImpl was to provide the same sort of environment to the tests where they wouldn't have to deal with the details of setting up the screen environment and tearing it down. These include things like setting up the display manager and setting the screen instance. I've renamed this to TestScreenWinManager to be more in line with what it actually does. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:232: } On 2016/02/01 22:15:44, oshima wrote: > It's better to use gfx::Screen::GetInstance() as it's more real. sgtm. Done and moved out of ScreenWinTest. https://codereview.chromium.org/1639623003/diff/1/ui/gfx/screen_win_unittest.... ui/gfx/screen_win_unittest.cc:454: gfx::SetDefaultDeviceScaleFactor(2.0); On 2016/02/01 22:15:44, oshima wrote: > just noticed this. why this is necessary? We still call into the gfx::win:: family of scaling APIs, which requires the scale factors to be set properly from the system.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/180001
robliao@chromium.org changed reviewers: + sky@chromium.org
sky: Please provide owner approval for ui/gfx/... . Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... File ui/gfx/win/display_manager.h (right): https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { Why do we need both DisplayManager and ScreenWin? AFAICT DisplayManager is only used by ScreenWin. Why not make ScreenWin have this functionality? For the mocking it would be clearer to have a pure virtual class.
https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... File ui/gfx/win/display_manager.h (right): https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { On 2016/02/02 16:18:20, sky wrote: > Why do we need both DisplayManager and ScreenWin? AFAICT DisplayManager is only > used by ScreenWin. Why not make ScreenWin have this functionality? For the > mocking it would be clearer to have a pure virtual class. Coming up in a next CL are some static scaling calls that can't depend on the existence of a ScreenWin. The scaling calls need access to physical display information that can't be provided by unit tests that substitute their own screen harness (gfx::TestScreen). The display manager separation allows these static calls to obtain physical information and fallback to 1.0x scaling for tests that use gfx::TestScreen
On Tue, Feb 2, 2016 at 10:08 AM, <robliao@chromium.org> wrote: > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > File ui/gfx/win/display_manager.h (right): > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { > On 2016/02/02 16:18:20, sky wrote: >> Why do we need both DisplayManager and ScreenWin? AFAICT > DisplayManager is only >> used by ScreenWin. Why not make ScreenWin have this functionality? For > the >> mocking it would be clearer to have a pure virtual class. > > Coming up in a next CL are some static scaling calls that can't depend > on the existence of a ScreenWin. Do you have a cl you can point me at to help understand the dependencies? > The scaling calls need access to physical display information that can't > be provided by unit tests that substitute their own screen harness > (gfx::TestScreen). I wasn't suggesting you use TestScreen for mocking, rather move the virtual functions you have in DisplayManager into its own class, call it ScreenLookupWin. That way you can still mock out the windows lookup part. But it's entirely possible I'm missing what you're after. -Scott > > The display manager separation allows these static calls to obtain > physical information and fallback to 1.0x scaling for tests that use > gfx::TestScreen > > https://codereview.chromium.org/1639623003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/02 18:22:07, sky wrote: > On Tue, Feb 2, 2016 at 10:08 AM, <mailto:robliao@chromium.org> wrote: > > > > > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > > File ui/gfx/win/display_manager.h (right): > > > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > > ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { > > On 2016/02/02 16:18:20, sky wrote: > >> Why do we need both DisplayManager and ScreenWin? AFAICT > > DisplayManager is only > >> used by ScreenWin. Why not make ScreenWin have this functionality? For > > the > >> mocking it would be clearer to have a pure virtual class. > > > > Coming up in a next CL are some static scaling calls that can't depend > > on the existence of a ScreenWin. > > Do you have a cl you can point me at to help understand the dependencies? Yup. You can check out https://codereview.chromium.org/1426933002/ This CL is a partitioning of that one to reduce code delta size. > > > The scaling calls need access to physical display information that can't > > be provided by unit tests that substitute their own screen harness > > (gfx::TestScreen). > > I wasn't suggesting you use TestScreen for mocking, rather move the > virtual functions you have in DisplayManager into its own class, call > it ScreenLookupWin. That way you can still mock out the windows lookup > part. But it's entirely possible I'm missing what you're after. Are you suggesting class DisplayManager : public ScreenLookupWin or class ScreenWin : public ScreenLookupWin? If it's the latter, static DPI scaling functions need access to a ScreenWinDisplay for the physical pixel rect. If that's provided by ScreenWin, the static calls have no guarantee that gfx::Screen::GetScreen() is actually a ScreenWin. DisplayManager provides the guarantee that a suitable source exists, regardless of the embedded Screen. Thanks! > > -Scott > > > > > The display manager separation allows these static calls to obtain > > physical information and fallback to 1.0x scaling for tests that use > > gfx::TestScreen > > > > https://codereview.chromium.org/1639623003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
On Tue, Feb 2, 2016 at 11:01 AM, <robliao@chromium.org> wrote: > On 2016/02/02 18:22:07, sky wrote: >> On Tue, Feb 2, 2016 at 10:08 AM, <mailto:robliao@chromium.org> wrote: >> > >> > >> > >> > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... >> > File ui/gfx/win/display_manager.h (right): >> > >> > >> > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... >> > ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { >> > On 2016/02/02 16:18:20, sky wrote: >> >> Why do we need both DisplayManager and ScreenWin? AFAICT >> > DisplayManager is only >> >> used by ScreenWin. Why not make ScreenWin have this functionality? For >> > the >> >> mocking it would be clearer to have a pure virtual class. >> > >> > Coming up in a next CL are some static scaling calls that can't depend >> > on the existence of a ScreenWin. >> >> Do you have a cl you can point me at to help understand the dependencies? > Yup. You can check out https://codereview.chromium.org/1426933002/ > This CL is a partitioning of that one to reduce code delta size. > >> >> > The scaling calls need access to physical display information that can't >> > be provided by unit tests that substitute their own screen harness >> > (gfx::TestScreen). >> >> I wasn't suggesting you use TestScreen for mocking, rather move the >> virtual functions you have in DisplayManager into its own class, call >> it ScreenLookupWin. That way you can still mock out the windows lookup >> part. But it's entirely possible I'm missing what you're after. > > Are you suggesting > class DisplayManager : public ScreenLookupWin or > class ScreenWin : public ScreenLookupWin? Neither. I'm suggesting ScreenWin has a ScreenLookupWin. ScreenLookupWin has two implementations, the real one, and tests replace it with the mocked one. > If it's the latter, static DPI scaling functions need access to a > ScreenWinDisplay for the physical pixel rect. Can you point me at the specific places that are problematic? -Scott > If that's provided by ScreenWin, the static calls have no guarantee that > gfx::Screen::GetScreen() is actually a ScreenWin. > > DisplayManager provides the guarantee that a suitable source exists, > regardless > of the embedded Screen. > > Thanks! > >> >> -Scott >> >> > >> > The display manager separation allows these static calls to obtain >> > physical information and fallback to 1.0x scaling for tests that use >> > gfx::TestScreen >> > >> > https://codereview.chromium.org/1639623003/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/1639623003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/02 20:29:48, sky wrote: > On Tue, Feb 2, 2016 at 11:01 AM, <mailto:robliao@chromium.org> wrote: > > On 2016/02/02 18:22:07, sky wrote: > >> On Tue, Feb 2, 2016 at 10:08 AM, <mailto:robliao@chromium.org> wrote: > >> > > >> > > >> > > >> > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > >> > File ui/gfx/win/display_manager.h (right): > >> > > >> > > >> > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > >> > ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { > >> > On 2016/02/02 16:18:20, sky wrote: > >> >> Why do we need both DisplayManager and ScreenWin? AFAICT > >> > DisplayManager is only > >> >> used by ScreenWin. Why not make ScreenWin have this functionality? For > >> > the > >> >> mocking it would be clearer to have a pure virtual class. > >> > > >> > Coming up in a next CL are some static scaling calls that can't depend > >> > on the existence of a ScreenWin. > >> > >> Do you have a cl you can point me at to help understand the dependencies? > > Yup. You can check out https://codereview.chromium.org/1426933002/ > > This CL is a partitioning of that one to reduce code delta size. > > > >> > >> > The scaling calls need access to physical display information that can't > >> > be provided by unit tests that substitute their own screen harness > >> > (gfx::TestScreen). > >> > >> I wasn't suggesting you use TestScreen for mocking, rather move the > >> virtual functions you have in DisplayManager into its own class, call > >> it ScreenLookupWin. That way you can still mock out the windows lookup > >> part. But it's entirely possible I'm missing what you're after. > > > > Are you suggesting > > class DisplayManager : public ScreenLookupWin or > > class ScreenWin : public ScreenLookupWin? > > Neither. I'm suggesting ScreenWin has a ScreenLookupWin. > ScreenLookupWin has two implementations, the real one, and tests > replace it with the mocked one. > > > If it's the latter, static DPI scaling functions need access to a > > ScreenWinDisplay for the physical pixel rect. > > Can you point me at the specific places that are problematic? Not seeing exactly what you want to do here, I can think of two possibilities: . Add a static getter to ScreenWin. This way code that cares about ScreenWin can get at it. . Perhaps the state you care about should be set on the Display. Seems fine to me to push pixel bounds to the Display. > > -Scott > > > If that's provided by ScreenWin, the static calls have no guarantee that > > gfx::Screen::GetScreen() is actually a ScreenWin. > > > > DisplayManager provides the guarantee that a suitable source exists, > > regardless > > of the embedded Screen. > > > > Thanks! > > > >> > >> -Scott > >> > >> > > >> > The display manager separation allows these static calls to obtain > >> > physical information and fallback to 1.0x scaling for tests that use > >> > gfx::TestScreen > >> > > >> > https://codereview.chromium.org/1639623003/ > >> > >> -- > >> You received this message because you are subscribed to the Google Groups > >> "Chromium-reviews" group. > >> To unsubscribe from this group and stop receiving emails from it, send an > > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > https://codereview.chromium.org/1639623003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
On 2016/02/02 20:48:52, sky wrote: > On 2016/02/02 20:29:48, sky wrote: > > On Tue, Feb 2, 2016 at 11:01 AM, <mailto:robliao@chromium.org> wrote: > > > On 2016/02/02 18:22:07, sky wrote: > > >> On Tue, Feb 2, 2016 at 10:08 AM, <mailto:robliao@chromium.org> wrote: > > >> > > > >> > > > >> > > > >> > > > > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > > >> > File ui/gfx/win/display_manager.h (right): > > >> > > > >> > > > >> > > > > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > > >> > ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { > > >> > On 2016/02/02 16:18:20, sky wrote: > > >> >> Why do we need both DisplayManager and ScreenWin? AFAICT > > >> > DisplayManager is only > > >> >> used by ScreenWin. Why not make ScreenWin have this functionality? For > > >> > the > > >> >> mocking it would be clearer to have a pure virtual class. > > >> > > > >> > Coming up in a next CL are some static scaling calls that can't depend > > >> > on the existence of a ScreenWin. > > >> > > >> Do you have a cl you can point me at to help understand the dependencies? > > > Yup. You can check out https://codereview.chromium.org/1426933002/ > > > This CL is a partitioning of that one to reduce code delta size. > > > > > >> > > >> > The scaling calls need access to physical display information that can't > > >> > be provided by unit tests that substitute their own screen harness > > >> > (gfx::TestScreen). > > >> > > >> I wasn't suggesting you use TestScreen for mocking, rather move the > > >> virtual functions you have in DisplayManager into its own class, call > > >> it ScreenLookupWin. That way you can still mock out the windows lookup > > >> part. But it's entirely possible I'm missing what you're after. > > > > > > Are you suggesting > > > class DisplayManager : public ScreenLookupWin or > > > class ScreenWin : public ScreenLookupWin? > > > > Neither. I'm suggesting ScreenWin has a ScreenLookupWin. > > ScreenLookupWin has two implementations, the real one, and tests > > replace it with the mocked one. > > > > > If it's the latter, static DPI scaling functions need access to a > > > ScreenWinDisplay for the physical pixel rect. > > > > Can you point me at the specific places that are problematic? > > Not seeing exactly what you want to do here, I can think of two possibilities: > . Add a static getter to ScreenWin. This way code that cares about ScreenWin can > get at it. > . Perhaps the state you care about should be set on the Display. Seems fine to > me to push pixel bounds to the Display. > > > > > -Scott > > > > > If that's provided by ScreenWin, the static calls have no guarantee that > > > gfx::Screen::GetScreen() is actually a ScreenWin. > > > > > > DisplayManager provides the guarantee that a suitable source exists, > > > regardless > > > of the embedded Screen. > > > > > > Thanks! > > > > > >> > > >> -Scott > > >> > > >> > > > >> > The display manager separation allows these static calls to obtain > > >> > physical information and fallback to 1.0x scaling for tests that use > > >> > gfx::TestScreen > > >> > > > >> > https://codereview.chromium.org/1639623003/ > > >> > > >> -- > > >> You received this message because you are subscribed to the Google Groups > > >> "Chromium-reviews" group. > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > email > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > >> > > > > > > > > > https://codereview.chromium.org/1639623003/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > The problematic calls are in this specific patchset in the large review: https://codereview.chromium.org/1426933002/diff/300001/ui/gfx/screen_win.cc Everything that calls GetNativeScreenWin will not work. I made an (incorrect) assumption that these would only be called in a ScreenWin environment. Turns out, these are also called with TestScreenWin. Since all these calls really want is access to the display information, I figured refactoring that data out is the way to go, and then these calls can always get the right object regardless of initialization environment.
On 2016/02/02 20:53:10, robliao wrote: > On 2016/02/02 20:48:52, sky wrote: > > On 2016/02/02 20:29:48, sky wrote: > > > On Tue, Feb 2, 2016 at 11:01 AM, <mailto:robliao@chromium.org> wrote: > > > > On 2016/02/02 18:22:07, sky wrote: > > > >> On Tue, Feb 2, 2016 at 10:08 AM, <mailto:robliao@chromium.org> wrote: > > > >> > > > > >> > > > > >> > > > > >> > > > > > > > > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > > > >> > File ui/gfx/win/display_manager.h (right): > > > >> > > > > >> > > > > >> > > > > > > > > > > https://codereview.chromium.org/1639623003/diff/180001/ui/gfx/win/display_man... > > > >> > ui/gfx/win/display_manager.h:34: class GFX_EXPORT DisplayManager { > > > >> > On 2016/02/02 16:18:20, sky wrote: > > > >> >> Why do we need both DisplayManager and ScreenWin? AFAICT > > > >> > DisplayManager is only > > > >> >> used by ScreenWin. Why not make ScreenWin have this functionality? For > > > >> > the > > > >> >> mocking it would be clearer to have a pure virtual class. > > > >> > > > > >> > Coming up in a next CL are some static scaling calls that can't depend > > > >> > on the existence of a ScreenWin. > > > >> > > > >> Do you have a cl you can point me at to help understand the dependencies? > > > > Yup. You can check out https://codereview.chromium.org/1426933002/ > > > > This CL is a partitioning of that one to reduce code delta size. > > > > > > > >> > > > >> > The scaling calls need access to physical display information that > can't > > > >> > be provided by unit tests that substitute their own screen harness > > > >> > (gfx::TestScreen). > > > >> > > > >> I wasn't suggesting you use TestScreen for mocking, rather move the > > > >> virtual functions you have in DisplayManager into its own class, call > > > >> it ScreenLookupWin. That way you can still mock out the windows lookup > > > >> part. But it's entirely possible I'm missing what you're after. > > > > > > > > Are you suggesting > > > > class DisplayManager : public ScreenLookupWin or > > > > class ScreenWin : public ScreenLookupWin? > > > > > > Neither. I'm suggesting ScreenWin has a ScreenLookupWin. > > > ScreenLookupWin has two implementations, the real one, and tests > > > replace it with the mocked one. > > > > > > > If it's the latter, static DPI scaling functions need access to a > > > > ScreenWinDisplay for the physical pixel rect. > > > > > > Can you point me at the specific places that are problematic? > > > > Not seeing exactly what you want to do here, I can think of two possibilities: > > . Add a static getter to ScreenWin. This way code that cares about ScreenWin > can > > get at it. > > . Perhaps the state you care about should be set on the Display. Seems fine to > > me to push pixel bounds to the Display. > > > > > > > > -Scott > > > > > > > If that's provided by ScreenWin, the static calls have no guarantee that > > > > gfx::Screen::GetScreen() is actually a ScreenWin. > > > > > > > > DisplayManager provides the guarantee that a suitable source exists, > > > > regardless > > > > of the embedded Screen. > > > > > > > > Thanks! > > > > > > > >> > > > >> -Scott > > > >> > > > >> > > > > >> > The display manager separation allows these static calls to obtain > > > >> > physical information and fallback to 1.0x scaling for tests that use > > > >> > gfx::TestScreen > > > >> > > > > >> > https://codereview.chromium.org/1639623003/ > > > >> > > > >> -- > > > >> You received this message because you are subscribed to the Google Groups > > > >> "Chromium-reviews" group. > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > >> > > > > > > > > > > > > https://codereview.chromium.org/1639623003/ > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > The problematic calls are in this specific patchset in the large review: > https://codereview.chromium.org/1426933002/diff/300001/ui/gfx/screen_win.cc > Everything that calls GetNativeScreenWin will not work. > > I made an (incorrect) assumption that these would only be called in a > ScreenWin environment. Turns out, these are also called with TestScreenWin. > > Since all these calls really want is access to the display information, I > figured > refactoring that data out is the way to go, and then these calls can always > get the right object regardless of initialization environment. Offline notes from a discussion with sky: 1) ScreenWin will set a static variable on construction and clear it on destruction. 2) Static calls will use ScreenWin's GetInstance to verify that a ScreenWin is available. 3) DisplayManager will be absorbed into ScreenWin. 4) We're going to keep DisplayInfo.
Patchset #7 (id:200001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/220001
(1) and (2) on the above list are small enough to go into the future High-DPI CL. The change for merging DisplayManager into ScreenWin (3) is in this one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/screen_win.cc#n... ui/gfx/screen_win.cc:217: MONITORINFOEX monitor_info = MonitorInfoFromWindow(NULL, nullptr https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/screen_win.cc#n... ui/gfx/screen_win.cc:235: // When the system isn't initialized, it means we're likely under a test. Can the tests properly initialize things? I ask as it would be nice if this could return a const ref rather than a value. https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/display_info.h File ui/gfx/win/display_info.h (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/display_inf... ui/gfx/win/display_info.h:18: class GFX_EXPORT DisplayInfo final { Similar comment about making this a struct. https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/display_inf... ui/gfx/win/display_info.h:20: static int64_t HashDeviceName(const wchar_t* device_name); constructor/destructor before other functions. Also, I want name this to make it clear the value from this is used as the id, eg DeviceIdFromDeviceName(), that it hashes is an implementation detail. https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... ui/gfx/win/screen_win_display.h:20: class ScreenWinDisplay final { Did you consider making this a struct and the two members public?
Patchset #8 (id:240001) has been deleted
https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/screen_win.cc#n... ui/gfx/screen_win.cc:217: MONITORINFOEX monitor_info = MonitorInfoFromWindow(NULL, On 2016/02/03 21:47:37, sky wrote: > nullptr Done. https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/screen_win.cc#n... ui/gfx/screen_win.cc:235: // When the system isn't initialized, it means we're likely under a test. On 2016/02/03 21:47:37, sky wrote: > Can the tests properly initialize things? I ask as it would be nice if this > could return a const ref rather than a value. Clarified the comment. The nature of search functions makes it tricky to hand out const refs since the search can fail. The alternative here is to have a base::LazyInstance<ScreenWinDisplay> for cases when this may happen and then we can avoid all these copies (though in theory, the compiler should be optimizing these copies out). If you'd prefer that, I could put that here. The DCHECK is here to ensure 1:1 correspondence between MONITORINFOEX and ScreenWinDisplay. However, if there are no displays, then we had out the default display as a fallback. The test comment doesn't apply anymore as we'll be checking for a ScreenWin before making these calls. https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/display_info.h File ui/gfx/win/display_info.h (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/display_inf... ui/gfx/win/display_info.h:18: class GFX_EXPORT DisplayInfo final { On 2016/02/03 21:47:38, sky wrote: > Similar comment about making this a struct. This can be a struct. Changed. https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/display_inf... ui/gfx/win/display_info.h:20: static int64_t HashDeviceName(const wchar_t* device_name); On 2016/02/03 21:47:38, sky wrote: > constructor/destructor before other functions. > Also, I want name this to make it clear the value from this is used as the id, > eg DeviceIdFromDeviceName(), that it hashes is an implementation detail. Done and done. https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... ui/gfx/win/screen_win_display.h:20: class ScreenWinDisplay final { On 2016/02/03 21:47:38, sky wrote: > Did you consider making this a struct and the two members public? I did and erred on the side of keeping this a class in case this picked up more responsibility. Looking at the future changelist, I think we can safely make this a struct. Done.
https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... ui/gfx/win/screen_win_display.h:20: class ScreenWinDisplay final { On 2016/02/03 22:47:20, robliao wrote: > On 2016/02/03 21:47:38, sky wrote: > > Did you consider making this a struct and the two members public? > > I did and erred on the side of keeping this a class in case this picked up more > responsibility. Looking at the future changelist, I think we can safely make > this a struct. > > Done. I also forgot that I got bitten by this: http://stackoverflow.com/questions/8467968/vector-of-structs-with-const-members In the later CL, I use a vector to track the remaining display infos that need to be placed. This requires that the data type be assignable. If there's no workaround, this might have to go back to being a class.
https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... File ui/gfx/win/screen_win_display.h (right): https://codereview.chromium.org/1639623003/diff/220001/ui/gfx/win/screen_win_... ui/gfx/win/screen_win_display.h:20: class ScreenWinDisplay final { On 2016/02/04 01:11:30, robliao wrote: > On 2016/02/03 22:47:20, robliao wrote: > > On 2016/02/03 21:47:38, sky wrote: > > > Did you consider making this a struct and the two members public? > > > > I did and erred on the side of keeping this a class in case this picked up > more > > responsibility. Looking at the future changelist, I think we can safely make > > this a struct. > > > > Done. > > I also forgot that I got bitten by this: > http://stackoverflow.com/questions/8467968/vector-of-structs-with-const-members > > In the later CL, I use a vector to track the remaining display infos that need > to be placed. This requires that the data type be assignable. > > If there's no workaround, this might have to go back to being a class. And fixed.
LGTM
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1639623003/#ps280001 (title: "Fix Consts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/280001
Description was changed from ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL introduces a level of indirection called the DisplayManager. The DisplayManager manages all of the displays and contains virtual methods that call system APIs that can be overridden by tests. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ==========
Description was changed from ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ==========
Message was sent while issue was closed.
Description was changed from ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:280001) has been created in https://codereview.chromium.org/1674433002/ by robliao@chromium.org. The reason for reverting is: std::vector<const T> wasn't intended to be possible by the C++ standard and does not work in VS2015..
Message was sent while issue was closed.
Description was changed from ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582} ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582} ==========
Patchset #10 (id:300001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/320001
Patchset #10 (id:320001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/340001
In light of the VS2015 find, I've switch DisplayInfo and ScreenWinDisplay back to being class so we can have some level of immutability after these objects are created. If there are no objections, I can send this change through. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Remind me why you need the extra level of immutability? On Thu, Feb 4, 2016 at 5:37 PM, <robliao@chromium.org> wrote: > In light of the VS2015 find, I've switch DisplayInfo and ScreenWinDisplay > back > to being class so we can have some level of immutability after these objects > are > created. > > If there are no objections, I can send this change through. > > Thanks! > > https://codereview.chromium.org/1639623003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/05 04:55:00, sky wrote: > Remind me why you need the extra level of immutability? > > On Thu, Feb 4, 2016 at 5:37 PM, <mailto:robliao@chromium.org> wrote: > > In light of the VS2015 find, I've switch DisplayInfo and ScreenWinDisplay > > back > > to being class so we can have some level of immutability after these objects > > are > > created. > > > > If there are no objections, I can send this change through. > > > > Thanks! > > > > https://codereview.chromium.org/1639623003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > The data structures represent characteristics of the environment. A DisplayInfo is strictly the metrics of a display. The pixel bounds of a ScreenWInDIsplay is determined by the environment. Code isn't allowed to change them after they are set. The only time change is allowed is if the environment changes (e.g. displays added or removed). This means we generate a new set of these and reason over them. Immutability guarantees the above behavior.
LGTM - I think what you have is overkill, but I'm fine with it.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1639623003/#ps340001 (title: "Sync + VS2015 Friendly - struct -> class to maintain immutability")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639623003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639623003/340001
Message was sent while issue was closed.
Description was changed from ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582} ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582} ========== to ========== ScreenWin Testability and Restructuring To allow easier testing of ScreenWin, this CL restructures ScreenWin to allow override of system OS calls through virtual methods as well as override the available displays available in the system. This CL also adds support to hold system pixel display rects that will be used by multi-DPI aware scaling coming in a later CL. BUG=426656 Committed: https://crrev.com/663b48643605198c21bfb8b8f81968223833021f Cr-Commit-Position: refs/heads/master@{#373582} Committed: https://crrev.com/eb17ae163b2ba4a2a67cd88a25b349249ed63f71 Cr-Commit-Position: refs/heads/master@{#374208} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/eb17ae163b2ba4a2a67cd88a25b349249ed63f71 Cr-Commit-Position: refs/heads/master@{#374208}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/6039/ste... ..\..\ui\gfx\win\display_info.cc(50,7) : error: field 'screen_work_rect_' will be initialized after field 'rotation_' [-Werror,-Wreorder] screen_work_rect_(monitor_info.rcWork), ^ I'll send you a CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
