Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(143)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 425363002: Moving coordinate conversion methods from ash/wm to ui/wm. (Closed)

Created:
6 years, 4 months ago by mfomitchev
Modified:
6 years, 4 months ago
CC:
chromium-reviews, sadrul, tfarina, ben+aura_chromium.org, jam, dcheng, darin-cc_chromium.org, kalyank, ben+ash_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Moving coordinate conversion methods from ash/wm to ui/wm. Moving ConvertPointToScreen and ConvertPointFromScreen from ash::wm into aura::Window. The suggestion to do this was made here: https://codereview.chromium.org/420603011/diff/1/athena/wm/coordinate_conversion.h BUG=NONE Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287609

Patch Set 1 #

Patch Set 2 : Moving methods to ui/wm instead of aura::Window #

Patch Set 3 : git cl format #

Total comments: 6

Patch Set 4 : Fixing nits #

Total comments: 4

Patch Set 5 : Replacing CHECK with DCHECKs. #

Total comments: 2

Patch Set 6 : Adding DCHECKs to ConvertPointFromScreen for symmetry. #

Patch Set 7 : Fixing the Ozone build #

Patch Set 8 : More fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+124 lines, -80 lines) Patch
M ash/autoclick/autoclick_controller.cc View 1 2 3 3 chunks +4 lines, -4 lines 0 comments Download
M ash/display/event_transformation_handler.cc View 1 3 chunks +2 lines, -2 lines 0 comments Download
M ash/display/mouse_cursor_event_filter.cc View 1 6 chunks +5 lines, -5 lines 0 comments Download
M ash/display/mouse_cursor_event_filter_ozone.cc View 1 2 3 4 5 6 7 4 chunks +4 lines, -3 lines 0 comments Download
M ash/drag_drop/drag_drop_controller.cc View 1 2 4 chunks +4 lines, -4 lines 0 comments Download
M ash/drag_drop/drag_drop_tracker.cc View 1 2 3 chunks +4 lines, -4 lines 0 comments Download
M ash/shelf/shelf_bezel_event_filter.cc View 1 2 chunks +2 lines, -2 lines 0 comments Download
M ash/shelf/shelf_view.cc View 1 2 4 chunks +7 lines, -8 lines 0 comments Download
M ash/shelf/shelf_view_unittest.cc View 1 3 chunks +2 lines, -2 lines 0 comments Download
M ash/test/ash_test_base.cc View 1 3 chunks +2 lines, -2 lines 0 comments Download
M ash/wm/coordinate_conversion.h View 1 chunk +0 lines, -10 lines 0 comments Download
M ash/wm/coordinate_conversion.cc View 1 chunk +0 lines, -12 lines 0 comments Download
M ash/wm/dock/docked_window_layout_manager_unittest.cc View 1 2 chunks +2 lines, -1 line 0 comments Download
M ash/wm/dock/docked_window_resizer.cc View 1 3 chunks +2 lines, -2 lines 0 comments Download
M ash/wm/dock/docked_window_resizer_unittest.cc View 1 4 chunks +3 lines, -3 lines 0 comments Download
M ash/wm/drag_window_resizer.cc View 1 2 3 chunks +4 lines, -3 lines 0 comments Download
M ash/wm/gestures/overview_gesture_handler.cc View 1 2 chunks +2 lines, -2 lines 0 comments Download
M ash/wm/panels/panel_window_resizer.cc View 1 3 chunks +2 lines, -2 lines 0 comments Download
M ash/wm/window_resizer.cc View 1 2 chunks +2 lines, -1 line 0 comments Download
M ash/wm/workspace/multi_window_resize_controller.cc View 1 3 chunks +2 lines, -2 lines 0 comments Download
M ash/wm/workspace/workspace_window_resizer.cc View 1 2 4 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab_drag_controller.cc View 1 3 chunks +2 lines, -2 lines 0 comments Download
A ui/wm/core/coordinate_conversion.h View 1 2 3 1 chunk +32 lines, -0 lines 0 comments Download
A ui/wm/core/coordinate_conversion.cc View 1 2 3 4 5 1 chunk +29 lines, -0 lines 0 comments Download
M ui/wm/wm.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
mfomitchev
6 years, 4 months ago (2014-07-30 18:55:12 UTC) #1
oshima
I think these method should live in ui/wm. +sky for owner's opinion
6 years, 4 months ago (2014-07-30 20:32:03 UTC) #2
sky
I like ui/wm better too.
6 years, 4 months ago (2014-07-30 21:54:38 UTC) #3
mfomitchev
ui/wm/core/coordinate_conversion.* ? Or is there an existing file where we can put these two methods?
6 years, 4 months ago (2014-07-30 22:03:32 UTC) #4
sky
SGTM On Wed, Jul 30, 2014 at 3:03 PM, <mfomitchev@chromium.org> wrote: > ui/wm/core/coordinate_conversion.* ? Or ...
6 years, 4 months ago (2014-07-30 22:16:34 UTC) #5
mfomitchev
Please review the updated CL. Thanks!
6 years, 4 months ago (2014-08-01 18:52:32 UTC) #6
oshima
ash/ lgtm with nits https://codereview.chromium.org/425363002/diff/40001/ash/autoclick/autoclick_controller.cc File ash/autoclick/autoclick_controller.cc (right): https://codereview.chromium.org/425363002/diff/40001/ash/autoclick/autoclick_controller.cc#newcode132 ash/autoclick/autoclick_controller.cc:132: ::wm::ConvertPointToScreen(ash::wm::GetRootWindowAt(mouse_location), remove ash:: (as this ...
6 years, 4 months ago (2014-08-02 00:34:14 UTC) #7
mfomitchev
https://codereview.chromium.org/425363002/diff/40001/ash/autoclick/autoclick_controller.cc File ash/autoclick/autoclick_controller.cc (right): https://codereview.chromium.org/425363002/diff/40001/ash/autoclick/autoclick_controller.cc#newcode132 ash/autoclick/autoclick_controller.cc:132: ::wm::ConvertPointToScreen(ash::wm::GetRootWindowAt(mouse_location), On 2014/08/02 00:34:13, oshima wrote: > remove ash:: ...
6 years, 4 months ago (2014-08-02 14:50:17 UTC) #8
sky
Also, be sure and update the .gn file. https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc File ui/wm/core/coordinate_conversion.cc (right): https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc#newcode13 ui/wm/core/coordinate_conversion.cc:13: CHECK(aura::client::GetScreenPositionClient(window->GetRootWindow())); ...
6 years, 4 months ago (2014-08-04 17:17:09 UTC) #9
mfomitchev
https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc File ui/wm/core/coordinate_conversion.cc (right): https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc#newcode13 ui/wm/core/coordinate_conversion.cc:13: CHECK(aura::client::GetScreenPositionClient(window->GetRootWindow())); This was copied from the original implementation where ...
6 years, 4 months ago (2014-08-04 21:16:19 UTC) #10
sky
https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc File ui/wm/core/coordinate_conversion.cc (right): https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc#newcode13 ui/wm/core/coordinate_conversion.cc:13: CHECK(aura::client::GetScreenPositionClient(window->GetRootWindow())); On 2014/08/04 21:16:19, mfomitchev wrote: > This was ...
6 years, 4 months ago (2014-08-04 21:49:57 UTC) #11
sadrul
Removing myself from the reviewers list, since reviews from sky@/oshima@ are sufficient. Please be sure ...
6 years, 4 months ago (2014-08-05 01:40:15 UTC) #12
mfomitchev
https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc File ui/wm/core/coordinate_conversion.cc (right): https://codereview.chromium.org/425363002/diff/60001/ui/wm/core/coordinate_conversion.cc#newcode13 ui/wm/core/coordinate_conversion.cc:13: CHECK(aura::client::GetScreenPositionClient(window->GetRootWindow())); On 2014/08/04 21:49:56, sky wrote: > On 2014/08/04 ...
6 years, 4 months ago (2014-08-05 15:09:42 UTC) #13
sky
LGTM https://codereview.chromium.org/425363002/diff/80001/ui/wm/core/coordinate_conversion.cc File ui/wm/core/coordinate_conversion.cc (right): https://codereview.chromium.org/425363002/diff/80001/ui/wm/core/coordinate_conversion.cc#newcode13 ui/wm/core/coordinate_conversion.cc:13: DCHECK(window); For consistency, add these DCHECKs to ConvertPointFromScreen ...
6 years, 4 months ago (2014-08-05 17:24:53 UTC) #14
mfomitchev
https://codereview.chromium.org/425363002/diff/80001/ui/wm/core/coordinate_conversion.cc File ui/wm/core/coordinate_conversion.cc (right): https://codereview.chromium.org/425363002/diff/80001/ui/wm/core/coordinate_conversion.cc#newcode13 ui/wm/core/coordinate_conversion.cc:13: DCHECK(window); On 2014/08/05 17:24:53, sky wrote: > For consistency, ...
6 years, 4 months ago (2014-08-05 18:08:52 UTC) #15
mfomitchev
The CQ bit was checked by mfomitchev@chromium.org
6 years, 4 months ago (2014-08-05 18:15:17 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/425363002/100001
6 years, 4 months ago (2014-08-05 18:16:06 UTC) #17
commit-bot: I haz the power
Change committed as 287609
6 years, 4 months ago (2014-08-05 22:06:35 UTC) #18
robliao
6 years, 4 months ago (2014-08-05 22:44:18 UTC) #19
Message was sent while issue was closed. 
A revert of this CL has been created in
https://codereview.chromium.org/445703005/ by robliao@chromium.org.

The reason for reverting is: Build break.

../../ash/display/mouse_cursor_event_filter_ozone.cc:21:7:error: no member named
'ConvertPointToScreen' in namespace 'ash::wm'
  wm::ConvertPointToScreen(target, &point_in_screen);
  ~~~~^
../../ash/display/mouse_cursor_event_filter_ozone.cc:45:7:error: no member named
'ConvertPointFromScreen' in namespace 'ash::wm'
  wm::ConvertPointFromScreen(root_at_point, &point_in_root);
  ~~~~^
../../ash/display/mouse_cursor_event_filter_ozone.cc:86:7:error: no member named
'ConvertPointFromScreen' in namespace 'ash::wm'
  wm::ConvertPointFromScreen(dst_root, &point_in_dst_screen);
  ~~~~^
.

Powered by Google App Engine
This is Rietveld 408576698