Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2012083002:
Multiple DPI Tracking for ScreenWin (Closed)
DescriptionMultiple DPI Tracking for ScreenWin
This CL completes ScreenWin's migration to be multiple DPI capable. Scaling
functions now attempt to resolve the DPI context based off of the HWND or
argument location if the HWND is unavailable.
An example of this is in ScreenWin's unittests. All calls that could change the
global default device scale factor have been removed.
The last remaining step is to hook up the monitor DPI specific info to ScreenWin
once multiple DPI mode is enabled.
The visual behavior of Chrome should not change with this CL.
BUG=426656, 501259
Committed: https://crrev.com/bacee65d8140989fdb60dc59b5c3ddfaf1a332c9
Cr-Commit-Position: refs/heads/master@{#396934}
Patch Set 1 #
Total comments: 23
Patch Set 2 : CR Feedback #
Total comments: 3
Patch Set 3 : CR Feedback #
Messages
Total messages: 37 (10 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
robliao@chromium.org changed reviewers: + oshima@chromium.org
oshima: Please review this CL. Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012083002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012083002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/scaling_util.cc File ui/display/win/scaling_util.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/scaling_util... ui/display/win/scaling_util.cc:236: gfx::Rect rect_work(CanonicalizeRelativePosition(rect, relative_position)); Just naming suggestion. how about: Rotation degree = ComputeRotationToLayoutPortrait(rect_to_be_upper, rect_top_be_lower); // LayoutVertical? upper_rect = RotateRect(rect_to_be_upper, degree); lower_rect = RotateRect(rect_to_be_lower, degree); ... ? https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; nit: screen_win_instance_ https://www.chromium.org/developers/coding-style/cpp-dos-and-donts I prefer to just use the utility function though: gfx::ScreenWin* GetScreenWin() { return static_cast<ScreenWin*>(Screen::GetScreen()); } Will this work? https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:183: DCHECK(g_screen_win_instance == this); DCHECK_EQ https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) when/how can this happen?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/scaling_util.cc File ui/display/win/scaling_util.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/scaling_util... ui/display/win/scaling_util.cc:236: gfx::Rect rect_work(CanonicalizeRelativePosition(rect, relative_position)); On 2016/05/26 21:52:57, oshima wrote: > Just naming suggestion. how about: > > Rotation degree = ComputeRotationToLayoutPortrait(rect_to_be_upper, > rect_top_be_lower); // LayoutVertical? > > upper_rect = RotateRect(rect_to_be_upper, degree); > lower_rect = RotateRect(rect_to_be_lower, degree); > > ... > > > ? I want to avoid using display::Rotation as that semantically has a different meaning than the rotation used here. I'll use CoordinateRotation instead. display::Rotation is a rotation with respect to the display. The rotation here is with respect to a coordinate plane. The layout considered here is a specific layout where |ref| is always on the top. Adjusted the code to reflect the intent behind the comment. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; On 2016/05/26 21:52:58, oshima wrote: > nit: screen_win_instance_ > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > I prefer to just use the utility function though: > > gfx::ScreenWin* GetScreenWin() { > return static_cast<ScreenWin*>(Screen::GetScreen()); > } > > Will this work? https://google.github.io/styleguide/cppguide.html#Variable_Names Global variables follow this style (g_hacker_case_var). This is well established in Chromium https://code.google.com/p/chromium/codesearch#search/&q=%5C%20g_%20-file:thir... The utility function is unsafe and will crash with tests that use TestScreen provided by Aura: https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/test/test_... This is the safest way to make sure we're actually using a ScreenWin. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:183: DCHECK(g_screen_win_instance == this); On 2016/05/26 21:52:58, oshima wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/26 21:52:58, oshima wrote: > when/how can this happen? This happens in tests that don't use ScreenWin and yet call scaling APIs. These tests rely on the default device scale factor for the scale instead.
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/scaling_util.cc File ui/display/win/scaling_util.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/scaling_util... ui/display/win/scaling_util.cc:236: gfx::Rect rect_work(CanonicalizeRelativePosition(rect, relative_position)); On 2016/05/26 22:28:47, robliao wrote: > On 2016/05/26 21:52:57, oshima wrote: > > Just naming suggestion. how about: > > > > Rotation degree = ComputeRotationToLayoutPortrait(rect_to_be_upper, > > rect_top_be_lower); // LayoutVertical? > > > > upper_rect = RotateRect(rect_to_be_upper, degree); > > lower_rect = RotateRect(rect_to_be_lower, degree); > > > > ... > > > > > > ? > > I want to avoid using display::Rotation as that semantically has a different > meaning than the rotation used here. I'll use CoordinateRotation instead. I didn't mean to suggest display::Rotation. Sorry it wasn't clear. What I wanted to say is that, the code rotates the rect so naming should reflect that. The new one looks good. thanks. > > display::Rotation is a rotation with respect to the display. The rotation here > is with respect to a coordinate plane. > > The layout considered here is a specific layout where |ref| is always on the > top. > > Adjusted the code to reflect the intent behind the comment. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; On 2016/05/26 22:28:47, robliao wrote: > On 2016/05/26 21:52:58, oshima wrote: > > nit: screen_win_instance_ > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > > > I prefer to just use the utility function though: > > > > gfx::ScreenWin* GetScreenWin() { > > return static_cast<ScreenWin*>(Screen::GetScreen()); > > } > > > > Will this work? > > https://google.github.io/styleguide/cppguide.html#Variable_Names > Global variables follow this style (g_hacker_case_var). > > This is well established in Chromium > https://code.google.com/p/chromium/codesearch#search/&q=%5C%20g_%20-file:thir... which I'm trying to fix when I see it to be more compliant to the style guide. g_ should mean that it's global that can be accessed in multiple files, and discouraged, while this one is not. > > > > The utility function is unsafe and will crash with tests that use TestScreen > provided by Aura: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/test/test_... > > This is the safest way to make sure we're actually using a ScreenWin. Sounds like it should not happen, and such test should be updated to use platform's screen instead, otherwise, there are two screen instances/impl, and you may get incorrect results. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/26 22:28:47, robliao wrote: > On 2016/05/26 21:52:58, oshima wrote: > > when/how can this happen? > > This happens in tests that don't use ScreenWin and yet call scaling APIs. These > tests rely on the default device scale factor for the scale instead. > Shouldn't such tests should create screen object? Again, I want to avoid having mixing two unless it makes sense. https://codereview.chromium.org/2012083002/diff/40001/ui/display/win/scaling_... File ui/display/win/scaling_util.cc (right): https://codereview.chromium.org/2012083002/diff/40001/ui/display/win/scaling_... ui/display/win/scaling_util.cc:223: // This function rotates the rectangles so that ref is always on top of rect, Since the commnd says ref is on top of rect, why don't we just call them rect_top, rect_bottom? What way, the code is more descriptive.
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; On 2016/05/26 22:54:47, oshima wrote: > On 2016/05/26 22:28:47, robliao wrote: > > On 2016/05/26 21:52:58, oshima wrote: > > > nit: screen_win_instance_ > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > > > > > > I prefer to just use the utility function though: > > > > > > gfx::ScreenWin* GetScreenWin() { > > > return static_cast<ScreenWin*>(Screen::GetScreen()); > > > } > > > > > > Will this work? > > > > https://google.github.io/styleguide/cppguide.html#Variable_Names > > Global variables follow this style (g_hacker_case_var). > > > > This is well established in Chromium > > > https://code.google.com/p/chromium/codesearch#search/&q=%5C%20g_%20-file:thir... > > which I'm trying to fix when I see it to be more compliant to the style guide. > > g_ should mean that it's global that can be accessed in multiple files, and > discouraged, while this one is not. > > > > > > > > > The utility function is unsafe and will crash with tests that use TestScreen > > provided by Aura: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/test/test_... > > > > This is the safest way to make sure we're actually using a ScreenWin. > > Sounds like it should not happen, and such test should be updated to use > platform's screen instead, otherwise, there are two screen instances/impl, and > you may get incorrect results. > g_ should mean that it's global that can be accessed in multiple files, and > discouraged, while this one is not. As commonly used, a global variable doesn't guarantee external linkage and it doesn't guarantee that it can be accessed by other files. From the style guide: There are no special requirements for global variables, which should be rare in any case, but if you use one, consider prefixing it with g_ or some other marker to easily distinguish it from local variables. This anonymous namespace makes this variable global to this file. The linked search results show also other consistent uses like g_screen_win_instance. Many global LazyInstances contained in anonymouse namespaces also follow the same g_* naming pattern. Trailing underscores are reserved for data members of classes: https://google.github.io/styleguide/cppguide.html#Variable_Names Which style reference do you refer to with regards to the naming of this variable? > Sounds like it should not happen, and such test should be updated to use > platform's screen instead, otherwise, there are two screen instances/impl, and > you may get incorrect results. I suspect those tests are attempting to be platform agnostic. In either case, we aren't guaranteed that the platform screen is always returned by Screen::GetScreen. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/26 22:54:47, oshima wrote: > On 2016/05/26 22:28:47, robliao wrote: > > On 2016/05/26 21:52:58, oshima wrote: > > > when/how can this happen? > > > > This happens in tests that don't use ScreenWin and yet call scaling APIs. > These > > tests rely on the default device scale factor for the scale instead. > > > > Shouldn't such tests should create screen object? Again, I want to avoid having > mixing two unless it makes sense. I don't think we want to force these tests to do this if a screen object is not important to what they're checking. They can't easily mock a platform screen. These tests aren't generally checking scaling computations. https://codereview.chromium.org/2012083002/diff/40001/ui/display/win/scaling_... File ui/display/win/scaling_util.cc (right): https://codereview.chromium.org/2012083002/diff/40001/ui/display/win/scaling_... ui/display/win/scaling_util.cc:223: // This function rotates the rectangles so that ref is always on top of rect, On 2016/05/26 22:54:48, oshima wrote: > Since the commnd says ref is on top of rect, why don't we just call them > > rect_top, rect_bottom? > > What way, the code is more descriptive. At this point ref is not necessarily on top of rect. It would be misleading to rename ref to rect_top.
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; On 2016/05/26 23:19:24, robliao wrote: > On 2016/05/26 22:54:47, oshima wrote: > > On 2016/05/26 22:28:47, robliao wrote: > > > On 2016/05/26 21:52:58, oshima wrote: > > > > nit: screen_win_instance_ > > > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > > > > > > > > > I prefer to just use the utility function though: > > > > > > > > gfx::ScreenWin* GetScreenWin() { > > > > return static_cast<ScreenWin*>(Screen::GetScreen()); > > > > } > > > > > > > > Will this work? > > > > > > https://google.github.io/styleguide/cppguide.html#Variable_Names > > > Global variables follow this style (g_hacker_case_var). > > > > > > This is well established in Chromium > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%5C%20g_%20-file:thir... > > > > which I'm trying to fix when I see it to be more compliant to the style guide. > > > > g_ should mean that it's global that can be accessed in multiple files, and > > discouraged, while this one is not. > > > > > > > > > > > > > > The utility function is unsafe and will crash with tests that use TestScreen > > > provided by Aura: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/test/test_... > > > > > > This is the safest way to make sure we're actually using a ScreenWin. > > > > Sounds like it should not happen, and such test should be updated to use > > platform's screen instead, otherwise, there are two screen instances/impl, and > > you may get incorrect results. > > > g_ should mean that it's global that can be accessed in multiple files, and > > discouraged, while this one is not. > > As commonly used, a global variable doesn't guarantee external linkage and it > doesn't guarantee that it can be accessed by other files. extenral linkage has nothing to do with g_ naming. g_ simply means it's global variable and may be, and will be accessed by other files (otherwise, you can put it in anonymous namespace). It's discouraged and should be avoided if possible. One in anonymous namespace is encouraged, and that's big difference IMO. > > From the style guide: > There are no special requirements for global variables, which should be rare in > any case, but if you use one, consider prefixing it with g_ or some other marker > to easily distinguish it from local variables. > > This anonymous namespace makes this variable global to this file. > > The linked search results show also other consistent uses like > g_screen_win_instance. > > Many global LazyInstances contained in anonymouse namespaces also follow the > same g_* naming pattern. > > > Trailing underscores are reserved for data members of classes: > https://google.github.io/styleguide/cppguide.html#Variable_Names It's talking about global variable, not anonymous namespace. Also, do and don't, and this https://www.chromium.org/developers/coding-style are addendum to google style guide. > Which style reference do you refer to with regards to the naming of this > variable? > > > Sounds like it should not happen, and such test should be updated to use > > platform's screen instead, otherwise, there are two screen instances/impl, and > > you may get incorrect results. > I suspect those tests are attempting to be platform agnostic. In either case, we > aren't guaranteed that the platform screen is always returned by > Screen::GetScreen. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/26 23:19:24, robliao wrote: > On 2016/05/26 22:54:47, oshima wrote: > > On 2016/05/26 22:28:47, robliao wrote: > > > On 2016/05/26 21:52:58, oshima wrote: > > > > when/how can this happen? > > > > > > This happens in tests that don't use ScreenWin and yet call scaling APIs. > > These > > > tests rely on the default device scale factor for the scale instead. > > > > > > > Shouldn't such tests should create screen object? Again, I want to avoid > having > > mixing two unless it makes sense. > > I don't think we want to force these tests to do this if a screen object is not > important to what they're checking. They can't easily mock a platform screen. > These tests aren't generally checking scaling computations. Which test is it? I want to be convinced because we should avoid this type of code in general. Say, if someone add a production code that calls this before creating screen object, it will use the wrong value without us knowing it. At least, we should make it test specific (fails in production). https://codereview.chromium.org/2012083002/diff/40001/ui/display/win/scaling_... File ui/display/win/scaling_util.cc (right): https://codereview.chromium.org/2012083002/diff/40001/ui/display/win/scaling_... ui/display/win/scaling_util.cc:223: // This function rotates the rectangles so that ref is always on top of rect, On 2016/05/26 23:19:24, robliao wrote: > On 2016/05/26 22:54:48, oshima wrote: > > Since the commnd says ref is on top of rect, why don't we just call them > > > > rect_top, rect_bottom? > > > > What way, the code is more descriptive. > > At this point ref is not necessarily on top of rect. It would be misleading to > rename ref to rect_top. Ah ok. nvm.
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; On 2016/05/26 23:43:13, oshima wrote: > On 2016/05/26 23:19:24, robliao wrote: > > On 2016/05/26 22:54:47, oshima wrote: > > > On 2016/05/26 22:28:47, robliao wrote: > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > nit: screen_win_instance_ > > > > > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > > > > > > > > > > > > I prefer to just use the utility function though: > > > > > > > > > > gfx::ScreenWin* GetScreenWin() { > > > > > return static_cast<ScreenWin*>(Screen::GetScreen()); > > > > > } > > > > > > > > > > Will this work? > > > > > > > > https://google.github.io/styleguide/cppguide.html#Variable_Names > > > > Global variables follow this style (g_hacker_case_var). > > > > > > > > This is well established in Chromium > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%5C%20g_%20-file:thir... > > > > > > which I'm trying to fix when I see it to be more compliant to the style > guide. > > > > > > g_ should mean that it's global that can be accessed in multiple files, and > > > discouraged, while this one is not. > > > > > > > > > > > > > > > > > > > The utility function is unsafe and will crash with tests that use > TestScreen > > > > provided by Aura: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/test/test_... > > > > > > > > This is the safest way to make sure we're actually using a ScreenWin. > > > > > > Sounds like it should not happen, and such test should be updated to use > > > platform's screen instead, otherwise, there are two screen instances/impl, > and > > > you may get incorrect results. > > > > > g_ should mean that it's global that can be accessed in multiple files, and > > > discouraged, while this one is not. > > > > As commonly used, a global variable doesn't guarantee external linkage and it > > doesn't guarantee that it can be accessed by other files. > > extenral linkage has nothing to do with g_ naming. g_ simply means it's global > variable > and may be, and will be accessed by other files (otherwise, you can put it in > anonymous namespace). > > It's discouraged and should be avoided if possible. One in anonymous namespace > is encouraged, > and that's big difference IMO. > > > > > From the style guide: > > There are no special requirements for global variables, which should be rare > in > > any case, but if you use one, consider prefixing it with g_ or some other > marker > > to easily distinguish it from local variables. > > > > This anonymous namespace makes this variable global to this file. > > > > The linked search results show also other consistent uses like > > g_screen_win_instance. > > > > Many global LazyInstances contained in anonymouse namespaces also follow the > > same g_* naming pattern. > > > > > > Trailing underscores are reserved for data members of classes: > > https://google.github.io/styleguide/cppguide.html#Variable_Names > > It's talking about global variable, not anonymous namespace. > > Also, do and don't, and this https://www.chromium.org/developers/coding-style > are addendum to google style guide. > > > Which style reference do you refer to with regards to the naming of this > > variable? > > > > > Sounds like it should not happen, and such test should be updated to use > > > platform's screen instead, otherwise, there are two screen instances/impl, > and > > > you may get incorrect results. > > I suspect those tests are attempting to be platform agnostic. In either case, > we > > aren't guaranteed that the platform screen is always returned by > > Screen::GetScreen. > Can you cite the rule you're discussing in those two docs? I can't seem to find it. https://www.chromium.org/developers/coding-style Doesn't seem to discuss global variables or anonymous namespaces https://www.chromium.org/developers/coding-style/cpp-dos-and-donts Discusses moving static implementation details to anonymous namespaces, which is what's going on here. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/26 23:43:13, oshima wrote: > On 2016/05/26 23:19:24, robliao wrote: > > On 2016/05/26 22:54:47, oshima wrote: > > > On 2016/05/26 22:28:47, robliao wrote: > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > when/how can this happen? > > > > > > > > This happens in tests that don't use ScreenWin and yet call scaling APIs. > > > These > > > > tests rely on the default device scale factor for the scale instead. > > > > > > > > > > Shouldn't such tests should create screen object? Again, I want to avoid > > having > > > mixing two unless it makes sense. > > > > I don't think we want to force these tests to do this if a screen object is > not > > important to what they're checking. They can't easily mock a platform screen. > > These tests aren't generally checking scaling computations. > > Which test is it? I want to be convinced because we should avoid this type of > code in general. > > Say, if someone add a production code that calls this before creating screen > object, it will use the wrong value without us knowing it. > > At least, we should make it test specific (fails in production). I'll get back to you on this at the patchset that had the global test failure is gone by now (happened many months back).
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/27 00:47:13, robliao wrote: > On 2016/05/26 23:43:13, oshima wrote: > > On 2016/05/26 23:19:24, robliao wrote: > > > On 2016/05/26 22:54:47, oshima wrote: > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > when/how can this happen? > > > > > > > > > > This happens in tests that don't use ScreenWin and yet call scaling > APIs. > > > > These > > > > > tests rely on the default device scale factor for the scale instead. > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I want to avoid > > > having > > > > mixing two unless it makes sense. > > > > > > I don't think we want to force these tests to do this if a screen object is > > not > > > important to what they're checking. They can't easily mock a platform > screen. > > > These tests aren't generally checking scaling computations. > > > > Which test is it? I want to be convinced because we should avoid this type of > > code in general. > > > > Say, if someone add a production code that calls this before creating screen > > object, it will use the wrong value without us knowing it. > > > > At least, we should make it test specific (fails in production). > > I'll get back to you on this at the patchset that had the global test failure is > gone by now (happened many months back). The ash_unittests fail: https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... As we expect since they're using their own test screen.
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; First of all, google style guide has *no recommendation* for a variable in anonymous namespace. The following section is for global variable, not for anonymous namespace. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables -------------- Global Variables There are no special requirements for global variables, which should be rare in any case, but if you use one, consider prefixing it with g_ or some other marker to easily distinguish it from local variables. -------------- g_ is a good choice (but not required), but again it's for global variable, not for a variable in anonymous namespace. Chromium has a recommendation: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts and it has a clear example how to define such a variable. https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/27 01:39:58, robliao wrote: > On 2016/05/27 00:47:13, robliao wrote: > > On 2016/05/26 23:43:13, oshima wrote: > > > On 2016/05/26 23:19:24, robliao wrote: > > > > On 2016/05/26 22:54:47, oshima wrote: > > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > > when/how can this happen? > > > > > > > > > > > > This happens in tests that don't use ScreenWin and yet call scaling > > APIs. > > > > > These > > > > > > tests rely on the default device scale factor for the scale instead. > > > > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I want to avoid > > > > having > > > > > mixing two unless it makes sense. > > > > > > > > I don't think we want to force these tests to do this if a screen object > is > > > not > > > > important to what they're checking. They can't easily mock a platform > > screen. > > > > These tests aren't generally checking scaling computations. > > > > > > Which test is it? I want to be convinced because we should avoid this type > of > > > code in general. > > > > > > Say, if someone add a production code that calls this before creating screen > > > object, it will use the wrong value without us knowing it. > > > > > > At least, we should make it test specific (fails in production). > > > > I'll get back to you on this at the patchset that had the global test failure > is > > gone by now (happened many months back). > > The ash_unittests fail: > https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... The link seems to be obsolete. > > As we expect since they're using their own test screen. Ash does not use test screen. It use real ash screen, which means that we're mixing two screen logic, which may imply real problem in ash. Can you run bots again?
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:29: ScreenWin* g_screen_win_instance = nullptr; On 2016/05/27 02:02:04, oshima wrote: > First of all, google style guide has *no recommendation* for a variable in > anonymous namespace. The following section is for global variable, not for > anonymous namespace. > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > -------------- > Global Variables > > There are no special requirements for global variables, which should be rare in > any case, but if you use one, consider prefixing it with g_ or some other marker > to easily distinguish it from local variables. > -------------- > > g_ is a good choice (but not required), but again it's for global variable, not > for a variable in anonymous namespace. > > Chromium has a recommendation: > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > and it has a clear example how to define such a variable. Please note that the reference you cited from the style guide is the exact reference I gave in Message 4 for this thread. The request here is to cite from https://www.chromium.org/developers/coding-style/cpp-dos-and-donts I think I see the reference now in the cpp-dos-and-donts. That's in fact a typo namespace { BigImplementationDetail detail_; } // namespace And in direct violation of https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/27 02:02:04, oshima wrote: > On 2016/05/27 01:39:58, robliao wrote: > > On 2016/05/27 00:47:13, robliao wrote: > > > On 2016/05/26 23:43:13, oshima wrote: > > > > On 2016/05/26 23:19:24, robliao wrote: > > > > > On 2016/05/26 22:54:47, oshima wrote: > > > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > > > when/how can this happen? > > > > > > > > > > > > > > This happens in tests that don't use ScreenWin and yet call scaling > > > APIs. > > > > > > These > > > > > > > tests rely on the default device scale factor for the scale instead. > > > > > > > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I want to > avoid > > > > > having > > > > > > mixing two unless it makes sense. > > > > > > > > > > I don't think we want to force these tests to do this if a screen object > > is > > > > not > > > > > important to what they're checking. They can't easily mock a platform > > > screen. > > > > > These tests aren't generally checking scaling computations. > > > > > > > > Which test is it? I want to be convinced because we should avoid this type > > of > > > > code in general. > > > > > > > > Say, if someone add a production code that calls this before creating > screen > > > > object, it will use the wrong value without us knowing it. > > > > > > > > At least, we should make it test specific (fails in production). > > > > > > I'll get back to you on this at the patchset that had the global test > failure > > is > > > gone by now (happened many months back). > > > > The ash_unittests fail: > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... > > The link seems to be obsolete. > > > > > As we expect since they're using their own test screen. > > Ash does not use test screen. It use real ash screen, which means that we're > mixing two screen logic, which may imply real problem in ash. Can you run bots > again? > This is the full list of failures due to either the use of aura::TestScreen or with an uninitialized Screen (ignore the display_unittests as those are expected to fail since they test that the functions still work in the absence of a ScreenWin). https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...
https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) On 2016/05/27 17:15:30, robliao wrote: > On 2016/05/27 02:02:04, oshima wrote: > > On 2016/05/27 01:39:58, robliao wrote: > > > On 2016/05/27 00:47:13, robliao wrote: > > > > On 2016/05/26 23:43:13, oshima wrote: > > > > > On 2016/05/26 23:19:24, robliao wrote: > > > > > > On 2016/05/26 22:54:47, oshima wrote: > > > > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > > > > when/how can this happen? > > > > > > > > > > > > > > > > This happens in tests that don't use ScreenWin and yet call > scaling > > > > APIs. > > > > > > > These > > > > > > > > tests rely on the default device scale factor for the scale > instead. > > > > > > > > > > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I want to > > avoid > > > > > > having > > > > > > > mixing two unless it makes sense. > > > > > > > > > > > > I don't think we want to force these tests to do this if a screen > object > > > is > > > > > not > > > > > > important to what they're checking. They can't easily mock a platform > > > > screen. > > > > > > These tests aren't generally checking scaling computations. > > > > > > > > > > Which test is it? I want to be convinced because we should avoid this > type > > > of > > > > > code in general. > > > > > > > > > > Say, if someone add a production code that calls this before creating > > screen > > > > > object, it will use the wrong value without us knowing it. > > > > > > > > > > At least, we should make it test specific (fails in production). > > > > > > > > I'll get back to you on this at the patchset that had the global test > > failure > > > is > > > > gone by now (happened many months back). > > > > > > The ash_unittests fail: > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... > > > > The link seems to be obsolete. > > > > > > > > As we expect since they're using their own test screen. > > > > Ash does not use test screen. It use real ash screen, which means that we're > > mixing two screen logic, which may imply real problem in ash. Can you run bots > > again? > > > > This is the full list of failures due to either the use of aura::TestScreen or > with an uninitialized Screen (ignore the display_unittests as those are expected > to fail since they test that the functions still work in the absence of a > ScreenWin). > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... Since stacks aren't working, you can use this: https://gist.github.com/anonymous/031563dc457d70e7c1181dee02053bbc
On 2016/05/27 17:51:35, robliao wrote: > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc > File ui/display/win/screen_win.cc (right): > > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... > ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) > On 2016/05/27 17:15:30, robliao wrote: > > On 2016/05/27 02:02:04, oshima wrote: > > > On 2016/05/27 01:39:58, robliao wrote: > > > > On 2016/05/27 00:47:13, robliao wrote: > > > > > On 2016/05/26 23:43:13, oshima wrote: > > > > > > On 2016/05/26 23:19:24, robliao wrote: > > > > > > > On 2016/05/26 22:54:47, oshima wrote: > > > > > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > > > > > when/how can this happen? > > > > > > > > > > > > > > > > > > This happens in tests that don't use ScreenWin and yet call > > scaling > > > > > APIs. > > > > > > > > These > > > > > > > > > tests rely on the default device scale factor for the scale > > instead. > > > > > > > > > > > > > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I want to > > > avoid > > > > > > > having > > > > > > > > mixing two unless it makes sense. > > > > > > > > > > > > > > I don't think we want to force these tests to do this if a screen > > object > > > > is > > > > > > not > > > > > > > important to what they're checking. They can't easily mock a > platform > > > > > screen. > > > > > > > These tests aren't generally checking scaling computations. > > > > > > > > > > > > Which test is it? I want to be convinced because we should avoid this > > type > > > > of > > > > > > code in general. > > > > > > > > > > > > Say, if someone add a production code that calls this before creating > > > screen > > > > > > object, it will use the wrong value without us knowing it. > > > > > > > > > > > > At least, we should make it test specific (fails in production). > > > > > > > > > > I'll get back to you on this at the patchset that had the global test > > > failure > > > > is > > > > > gone by now (happened many months back). > > > > > > > > The ash_unittests fail: > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... > > > > > > The link seems to be obsolete. > > > > > > > > > > > As we expect since they're using their own test screen. > > > > > > Ash does not use test screen. It use real ash screen, which means that we're > > > mixing two screen logic, which may imply real problem in ash. Can you run > bots > > > again? > > > > > > > This is the full list of failures due to either the use of aura::TestScreen or > > with an uninitialized Screen (ignore the display_unittests as those are > expected > > to fail since they test that the functions still work in the absence of a > > ScreenWin). > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > Since stacks aren't working, you can use this: > https://gist.github.com/anonymous/031563dc457d70e7c1181dee02053bbc This one is for views_unittests right? Can you send me the one for ash_unittests?
On 2016/05/27 18:11:17, oshima wrote: > On 2016/05/27 17:51:35, robliao wrote: > > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc > > File ui/display/win/screen_win.cc (right): > > > > > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... > > ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) > > On 2016/05/27 17:15:30, robliao wrote: > > > On 2016/05/27 02:02:04, oshima wrote: > > > > On 2016/05/27 01:39:58, robliao wrote: > > > > > On 2016/05/27 00:47:13, robliao wrote: > > > > > > On 2016/05/26 23:43:13, oshima wrote: > > > > > > > On 2016/05/26 23:19:24, robliao wrote: > > > > > > > > On 2016/05/26 22:54:47, oshima wrote: > > > > > > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > > > > > > when/how can this happen? > > > > > > > > > > > > > > > > > > > > This happens in tests that don't use ScreenWin and yet call > > > scaling > > > > > > APIs. > > > > > > > > > These > > > > > > > > > > tests rely on the default device scale factor for the scale > > > instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I want > to > > > > avoid > > > > > > > > having > > > > > > > > > mixing two unless it makes sense. > > > > > > > > > > > > > > > > I don't think we want to force these tests to do this if a screen > > > object > > > > > is > > > > > > > not > > > > > > > > important to what they're checking. They can't easily mock a > > platform > > > > > > screen. > > > > > > > > These tests aren't generally checking scaling computations. > > > > > > > > > > > > > > Which test is it? I want to be convinced because we should avoid > this > > > type > > > > > of > > > > > > > code in general. > > > > > > > > > > > > > > Say, if someone add a production code that calls this before > creating > > > > screen > > > > > > > object, it will use the wrong value without us knowing it. > > > > > > > > > > > > > > At least, we should make it test specific (fails in production). > > > > > > > > > > > > I'll get back to you on this at the patchset that had the global test > > > > failure > > > > > is > > > > > > gone by now (happened many months back). > > > > > > > > > > The ash_unittests fail: > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... > > > > > > > > The link seems to be obsolete. > > > > > > > > > > > > > > As we expect since they're using their own test screen. > > > > > > > > Ash does not use test screen. It use real ash screen, which means that > we're > > > > mixing two screen logic, which may imply real problem in ash. Can you run > > bots > > > > again? > > > > > > > > > > This is the full list of failures due to either the use of aura::TestScreen > or > > > with an uninitialized Screen (ignore the display_unittests as those are > > expected > > > to fail since they test that the functions still work in the absence of a > > > ScreenWin). > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > > > Since stacks aren't working, you can use this: > > https://gist.github.com/anonymous/031563dc457d70e7c1181dee02053bbc > > This one is for views_unittests right? Can you send me the one for > ash_unittests? Here's the one for ash_unittests https://gist.github.com/anonymous/f9e47cc181609fb997c5b8546d086815 Most if not all stem from InputMethodWin.
On 2016/05/27 19:26:54, robliao wrote: > On 2016/05/27 18:11:17, oshima wrote: > > On 2016/05/27 17:51:35, robliao wrote: > > > > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc > > > File ui/display/win/screen_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... > > > ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) > > > On 2016/05/27 17:15:30, robliao wrote: > > > > On 2016/05/27 02:02:04, oshima wrote: > > > > > On 2016/05/27 01:39:58, robliao wrote: > > > > > > On 2016/05/27 00:47:13, robliao wrote: > > > > > > > On 2016/05/26 23:43:13, oshima wrote: > > > > > > > > On 2016/05/26 23:19:24, robliao wrote: > > > > > > > > > On 2016/05/26 22:54:47, oshima wrote: > > > > > > > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > > > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > > > > > > > when/how can this happen? > > > > > > > > > > > > > > > > > > > > > > This happens in tests that don't use ScreenWin and yet call > > > > scaling > > > > > > > APIs. > > > > > > > > > > These > > > > > > > > > > > tests rely on the default device scale factor for the scale > > > > instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I > want > > to > > > > > avoid > > > > > > > > > having > > > > > > > > > > mixing two unless it makes sense. > > > > > > > > > > > > > > > > > > I don't think we want to force these tests to do this if a > screen > > > > object > > > > > > is > > > > > > > > not > > > > > > > > > important to what they're checking. They can't easily mock a > > > platform > > > > > > > screen. > > > > > > > > > These tests aren't generally checking scaling computations. > > > > > > > > > > > > > > > > Which test is it? I want to be convinced because we should avoid > > this > > > > type > > > > > > of > > > > > > > > code in general. > > > > > > > > > > > > > > > > Say, if someone add a production code that calls this before > > creating > > > > > screen > > > > > > > > object, it will use the wrong value without us knowing it. > > > > > > > > > > > > > > > > At least, we should make it test specific (fails in production). > > > > > > > > > > > > > > I'll get back to you on this at the patchset that had the global > test > > > > > failure > > > > > > is > > > > > > > gone by now (happened many months back). > > > > > > > > > > > > The ash_unittests fail: > > > > > > > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... > > > > > > > > > > The link seems to be obsolete. > > > > > > > > > > > > > > > > > As we expect since they're using their own test screen. > > > > > > > > > > Ash does not use test screen. It use real ash screen, which means that > > we're > > > > > mixing two screen logic, which may imply real problem in ash. Can you > run > > > bots > > > > > again? > > > > > > > > > > > > > This is the full list of failures due to either the use of > aura::TestScreen > > or > > > > with an uninitialized Screen (ignore the display_unittests as those are > > > expected > > > > to fail since they test that the functions still work in the absence of a > > > > ScreenWin). > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > > > > > Since stacks aren't working, you can use this: > > > https://gist.github.com/anonymous/031563dc457d70e7c1181dee02053bbc > > > > This one is for views_unittests right? Can you send me the one for > > ash_unittests? > > Here's the one for ash_unittests > https://gist.github.com/anonymous/f9e47cc181609fb997c5b8546d086815 > > Most if not all stem from InputMethodWin. Hmm, which probably means it's not doing the right thing. InputMethodWin in win_ash no longer makes sense in win ash as metro mode has been removed. We probably should mock input method for win ash tests instead. I think the "correct" fix would be 1) move screen win impl to ui/views/widget/desktop_aura and handle conversion there. That's where the screen impl for linux desktop lives. 2) Refactor InputMethodWin into InputMethodAsh and platform specific parts. 2nd part may not worth as win ash isn't really the target platform. Can you add TODO there that we need this because we run some tests in two screen implementations, that should be fixed? For naming, it's not a big deal, so you may keep if you feel strongly about it.
On 2016/05/27 20:38:09, oshima wrote: > On 2016/05/27 19:26:54, robliao wrote: > > On 2016/05/27 18:11:17, oshima wrote: > > > On 2016/05/27 17:51:35, robliao wrote: > > > > > > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.cc > > > > File ui/display/win/screen_win.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2012083002/diff/1/ui/display/win/screen_win.c... > > > > ui/display/win/screen_win.cc:483: if (!g_screen_win_instance) > > > > On 2016/05/27 17:15:30, robliao wrote: > > > > > On 2016/05/27 02:02:04, oshima wrote: > > > > > > On 2016/05/27 01:39:58, robliao wrote: > > > > > > > On 2016/05/27 00:47:13, robliao wrote: > > > > > > > > On 2016/05/26 23:43:13, oshima wrote: > > > > > > > > > On 2016/05/26 23:19:24, robliao wrote: > > > > > > > > > > On 2016/05/26 22:54:47, oshima wrote: > > > > > > > > > > > On 2016/05/26 22:28:47, robliao wrote: > > > > > > > > > > > > On 2016/05/26 21:52:58, oshima wrote: > > > > > > > > > > > > > when/how can this happen? > > > > > > > > > > > > > > > > > > > > > > > > This happens in tests that don't use ScreenWin and yet > call > > > > > scaling > > > > > > > > APIs. > > > > > > > > > > > These > > > > > > > > > > > > tests rely on the default device scale factor for the > scale > > > > > instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Shouldn't such tests should create screen object? Again, I > > want > > > to > > > > > > avoid > > > > > > > > > > having > > > > > > > > > > > mixing two unless it makes sense. > > > > > > > > > > > > > > > > > > > > I don't think we want to force these tests to do this if a > > screen > > > > > object > > > > > > > is > > > > > > > > > not > > > > > > > > > > important to what they're checking. They can't easily mock a > > > > platform > > > > > > > > screen. > > > > > > > > > > These tests aren't generally checking scaling computations. > > > > > > > > > > > > > > > > > > Which test is it? I want to be convinced because we should avoid > > > this > > > > > type > > > > > > > of > > > > > > > > > code in general. > > > > > > > > > > > > > > > > > > Say, if someone add a production code that calls this before > > > creating > > > > > > screen > > > > > > > > > object, it will use the wrong value without us knowing it. > > > > > > > > > > > > > > > > > > At least, we should make it test specific (fails in production). > > > > > > > > > > > > > > > > I'll get back to you on this at the patchset that had the global > > test > > > > > > failure > > > > > > > is > > > > > > > > gone by now (happened many months back). > > > > > > > > > > > > > > The ash_unittests fail: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng... > > > > > > > > > > > > The link seems to be obsolete. > > > > > > > > > > > > > > > > > > > > As we expect since they're using their own test screen. > > > > > > > > > > > > Ash does not use test screen. It use real ash screen, which means that > > > we're > > > > > > mixing two screen logic, which may imply real problem in ash. Can you > > run > > > > bots > > > > > > again? > > > > > > > > > > > > > > > > This is the full list of failures due to either the use of > > aura::TestScreen > > > or > > > > > with an uninitialized Screen (ignore the display_unittests as those are > > > > expected > > > > > to fail since they test that the functions still work in the absence of > a > > > > > ScreenWin). > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > > > > > > > Since stacks aren't working, you can use this: > > > > https://gist.github.com/anonymous/031563dc457d70e7c1181dee02053bbc > > > > > > This one is for views_unittests right? Can you send me the one for > > > ash_unittests? > > > > Here's the one for ash_unittests > > https://gist.github.com/anonymous/f9e47cc181609fb997c5b8546d086815 > > > > Most if not all stem from InputMethodWin. > > Hmm, which probably means it's not doing the right thing. InputMethodWin in > win_ash no longer makes sense in win ash as metro mode has been removed. > We probably should mock input method for win ash tests instead. > > I think the "correct" fix would be > > 1) move screen win impl to ui/views/widget/desktop_aura and handle conversion > there. That's where the screen impl for linux desktop lives. > 2) Refactor InputMethodWin into InputMethodAsh and platform specific parts. > > 2nd part may not worth as win ash isn't really the target platform. > > Can you add TODO there that we need this because we run some tests in two screen > implementations, that should be fixed? Sounds good. Added a TODO with a link to the bug. > For naming, it's not a big deal, so you may keep if you feel strongly about it. Sounds good.
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/2012083002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012083002/60001
lgtm
On 2016/05/27 21:56:32, oshima wrote: > lgtm Thanks for the review! I'll send this in after the holiday weekend in case something does indeed break.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2012083002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012083002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2012083002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2012083002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Multiple DPI Tracking for ScreenWin This CL completes ScreenWin's migration to be multiple DPI capable. Scaling functions now attempt to resolve the DPI context based off of the HWND or argument location if the HWND is unavailable. An example of this is in ScreenWin's unittests. All calls that could change the global default device scale factor have been removed. The last remaining step is to hook up the monitor DPI specific info to ScreenWin once multiple DPI mode is enabled. The visual behavior of Chrome should not change with this CL. BUG=426656,501259 ========== to ========== Multiple DPI Tracking for ScreenWin This CL completes ScreenWin's migration to be multiple DPI capable. Scaling functions now attempt to resolve the DPI context based off of the HWND or argument location if the HWND is unavailable. An example of this is in ScreenWin's unittests. All calls that could change the global default device scale factor have been removed. The last remaining step is to hook up the monitor DPI specific info to ScreenWin once multiple DPI mode is enabled. The visual behavior of Chrome should not change with this CL. BUG=426656,501259 Committed: https://crrev.com/bacee65d8140989fdb60dc59b5c3ddfaf1a332c9 Cr-Commit-Position: refs/heads/master@{#396934} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bacee65d8140989fdb60dc59b5c3ddfaf1a332c9 Cr-Commit-Position: refs/heads/master@{#396934} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
