Chromium Code Reviews
Help | Chromium Project | Sign in
(80)

Issue 2449103004: Refactor WindowFinder definition (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by tonikitoo
Modified:
4 months, 3 weeks ago
CC:
chromium-reviews, tfarina, dcheng, mac-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Refactor WindowFinder definition CL reworks the WindowFinder implementation as follows: (Before): - There was a WindowFinder base class, with empty ctor and dtor definitions, and one method GetLocalProcessWindowAtPoint, with platform specific implementations living in window_finder_{x11,mac,chromeos,win}. - There used to be a derived class WindowFinderMus, overriding GetLocalProcessWindowAtPoint. - TabDragController instantiated either WindowFinder or WindowFinderMus depending whether the "Mus" code path is being executed. (After) - The Mac platform does not support Mus. So only window_finder_mac.mm and window_finder.h are built. - Other platforms (linux_desktop, chromeos, win) build window_finder.cc, window_finder_mus.cc as well a platform specific file; e.g. window_finder_chromeos.cc in case of ChromeOS. - CL also moves the logic that checks whether chrome is running with the Mus path to within GetLocalProcessWindowAtPointMus. This CL is a preparation to CL [1], where chrome --mash capabilities are extended to run through Ozone. [1] https://codereview.chromium.org/2408803002/ BUG=295089 Committed: https://crrev.com/dad9ff3ca51c167a4473909bb68588b7ede0da52 Cr-Commit-Position: refs/heads/master@{#429010}

Patch Set 1 #

Total comments: 4

Patch Set 2 : addressed fwang's remarks #

Patch Set 3 : fixed the unittests build #

Total comments: 1

Patch Set 4 : less ifdef's #

Patch Set 5 : Refactor WindowFinder definition #

Patch Set 6 : addressed sky's preference #

Patch Set 7 : fixed gn gen <out> --chec #

Patch Set 8 : fixed gn gen <out> --check #

Patch Set 9 : no ifdefs #

Total comments: 2

Patch Set 10 : class-less #

Patch Set 11 : Refactor WindowFinder definition #

Total comments: 2

Patch Set 12 : addressed more of sky's feedback #

Total comments: 4

Patch Set 13 : Refactor WindowFinder definition #

Total comments: 10

Patch Set 14 : addressed more remarks from sky #

Total comments: 2

Patch Set 15 : setting mus_window to null, for safety #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -37 lines) Patch
M chrome/browser/ui/views/tabs/tab_drag_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +1 line, -12 lines 0 comments Download
M chrome/browser/ui/views/tabs/window_finder.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/views/tabs/window_finder_chromeos.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +5 lines, -0 lines 0 comments Download
D chrome/browser/ui/views/tabs/window_finder_mus.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +8 lines, -13 lines 0 comments Download
M chrome/browser/ui/views/tabs/window_finder_mus.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +14 lines, -8 lines 0 comments Download
M chrome/browser/ui/views/tabs/window_finder_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/tabs/window_finder_x11.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +6 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 87 (53 generated)
tonikitoo
PTAL
5 months ago (2016-10-26 05:35:58 UTC) #2
tonikitoo
5 months ago (2016-10-26 05:36:35 UTC) #4
fwang
l g t m https://codereview.chromium.org/2449103004/diff/1/chrome/browser/ui/views/tabs/window_finder.cc File chrome/browser/ui/views/tabs/window_finder.cc (right): https://codereview.chromium.org/2449103004/diff/1/chrome/browser/ui/views/tabs/window_finder.cc#newcode8 chrome/browser/ui/views/tabs/window_finder.cc:8: #include "services/service_manager/runner/common/client_util.h" It looks like ...
5 months ago (2016-10-26 07:42:58 UTC) #6
tonikitoo
https://codereview.chromium.org/2449103004/diff/1/chrome/browser/ui/views/tabs/window_finder.cc File chrome/browser/ui/views/tabs/window_finder.cc (right): https://codereview.chromium.org/2449103004/diff/1/chrome/browser/ui/views/tabs/window_finder.cc#newcode8 chrome/browser/ui/views/tabs/window_finder.cc:8: #include "services/service_manager/runner/common/client_util.h" On 2016/10/26 07:42:58, fwang wrote: > It ...
5 months ago (2016-10-26 13:41:46 UTC) #7
fwang
informal l g t m
5 months ago (2016-10-26 13:57:14 UTC) #13
sky
Can you clarify why this is better? What exactly are you trying to make clearer ...
5 months ago (2016-10-26 17:42:01 UTC) #20
rjkroege
On 2016/10/26 17:42:01, sky wrote: > Can you clarify why this is better? What exactly ...
5 months ago (2016-10-26 18:34:04 UTC) #21
sky
Ok, thanks for the clarification. As to this fix, I would rather have window_finder_x11 check ...
5 months ago (2016-10-26 19:26:00 UTC) #22
tonikitoo
On 2016/10/26 17:42:01, sky wrote: Thanks for looking @sky / @rjkroege. > Can you clarify ...
5 months ago (2016-10-26 19:30:46 UTC) #23
tonikitoo
On 2016/10/26 19:26:00, sky wrote: > Ok, thanks for the clarification. > > As to ...
5 months ago (2016-10-26 19:35:59 UTC) #24
sky
https://codereview.chromium.org/2449103004/diff/40001/chrome/browser/ui/views/tabs/window_finder.cc File chrome/browser/ui/views/tabs/window_finder.cc (right): https://codereview.chromium.org/2449103004/diff/40001/chrome/browser/ui/views/tabs/window_finder.cc#newcode23 chrome/browser/ui/views/tabs/window_finder.cc:23: #if defined(USE_AURA) && !defined(OS_MACOSX) I dislike the amount of ...
5 months ago (2016-10-26 22:01:28 UTC) #25
tonikitoo
On 2016/10/26 22:01:28, sky wrote: > https://codereview.chromium.org/2449103004/diff/40001/chrome/browser/ui/views/tabs/window_finder.cc > File chrome/browser/ui/views/tabs/window_finder.cc (right): > > https://codereview.chromium.org/2449103004/diff/40001/chrome/browser/ui/views/tabs/window_finder.cc#newcode23 > ...
5 months ago (2016-10-27 17:40:21 UTC) #32
sky
As I stated earlier, I prefer the platforms that are aura to have the check, ...
4 months, 4 weeks ago (2016-10-27 19:35:29 UTC) #33
tonikitoo
On 2016/10/27 19:35:29, sky wrote: > As I stated earlier, I prefer the platforms that ...
4 months, 4 weeks ago (2016-10-28 13:57:41 UTC) #45
sky
https://codereview.chromium.org/2449103004/diff/160001/chrome/browser/ui/views/tabs/window_finder.h File chrome/browser/ui/views/tabs/window_finder.h (right): https://codereview.chromium.org/2449103004/diff/160001/chrome/browser/ui/views/tabs/window_finder.h#newcode22 chrome/browser/ui/views/tabs/window_finder.h:22: class WindowFinder { There is no point in making ...
4 months, 4 weeks ago (2016-10-28 16:54:48 UTC) #48
tonikitoo
https://codereview.chromium.org/2449103004/diff/160001/chrome/browser/ui/views/tabs/window_finder.h File chrome/browser/ui/views/tabs/window_finder.h (right): https://codereview.chromium.org/2449103004/diff/160001/chrome/browser/ui/views/tabs/window_finder.h#newcode22 chrome/browser/ui/views/tabs/window_finder.h:22: class WindowFinder { On 2016/10/28 16:54:48, sky wrote: > ...
4 months, 4 weeks ago (2016-10-28 20:05:48 UTC) #54
sky
On 2016/10/28 20:05:48, tonikitoo wrote: > https://codereview.chromium.org/2449103004/diff/160001/chrome/browser/ui/views/tabs/window_finder.h > File chrome/browser/ui/views/tabs/window_finder.h (right): > > https://codereview.chromium.org/2449103004/diff/160001/chrome/browser/ui/views/tabs/window_finder.h#newcode22 > ...
4 months, 4 weeks ago (2016-10-28 21:47:38 UTC) #55
tonikitoo
On 2016/10/28 21:47:38, sky wrote: > On 2016/10/28 20:05:48, tonikitoo wrote: > > > https://codereview.chromium.org/2449103004/diff/160001/chrome/browser/ui/views/tabs/window_finder.h ...
4 months, 4 weeks ago (2016-10-28 21:52:19 UTC) #56
fwang
OK, it's looking good :-) https://codereview.chromium.org/2449103004/diff/200001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2449103004/diff/200001/chrome/browser/ui/BUILD.gn#newcode3041 chrome/browser/ui/BUILD.gn:3041: # - Mus is ...
4 months, 3 weeks ago (2016-10-31 11:46:06 UTC) #62
tonikitoo
https://codereview.chromium.org/2449103004/diff/200001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2449103004/diff/200001/chrome/browser/ui/BUILD.gn#newcode3041 chrome/browser/ui/BUILD.gn:3041: # - Mus is not support on Mac, so ...
4 months, 3 weeks ago (2016-10-31 14:20:29 UTC) #63
sky
In case I wasn't clear, when I said I want the implementation to look like: ...
4 months, 3 weeks ago (2016-10-31 15:34:35 UTC) #64
tonikitoo
On 2016/10/31 15:34:35, sky wrote: > In case I wasn't clear, when I said I ...
4 months, 3 weeks ago (2016-10-31 16:39:58 UTC) #65
sky
Your diff makes it look like window_finder_mus.h is being deleted. Please make sure that isn't ...
4 months, 3 weeks ago (2016-10-31 20:32:46 UTC) #66
sky
https://codereview.chromium.org/2449103004/diff/220001/chrome/browser/ui/views/tabs/window_finder_chromeos.cc File chrome/browser/ui/views/tabs/window_finder_chromeos.cc (right): https://codereview.chromium.org/2449103004/diff/220001/chrome/browser/ui/views/tabs/window_finder_chromeos.cc#newcode17 chrome/browser/ui/views/tabs/window_finder_chromeos.cc:17: gfx::NativeWindow out; out is descriptive, maybe mus_result? (same comment ...
4 months, 3 weeks ago (2016-10-31 20:33:20 UTC) #67
tonikitoo
On 2016/10/31 20:32:46, sky wrote: > Your diff makes it look like window_finder_mus.h is being ...
4 months, 3 weeks ago (2016-10-31 21:09:02 UTC) #68
tonikitoo
https://codereview.chromium.org/2449103004/diff/220001/chrome/browser/ui/views/tabs/window_finder_chromeos.cc File chrome/browser/ui/views/tabs/window_finder_chromeos.cc (right): https://codereview.chromium.org/2449103004/diff/220001/chrome/browser/ui/views/tabs/window_finder_chromeos.cc#newcode17 chrome/browser/ui/views/tabs/window_finder_chromeos.cc:17: gfx::NativeWindow out; On 2016/10/31 20:33:20, sky wrote: > out ...
4 months, 3 weeks ago (2016-10-31 21:09:14 UTC) #69
sky
https://codereview.chromium.org/2449103004/diff/240001/chrome/browser/ui/views/tabs/tab_drag_controller.cc File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2449103004/diff/240001/chrome/browser/ui/views/tabs/tab_drag_controller.cc#newcode225 chrome/browser/ui/views/tabs/tab_drag_controller.cc:225: window_finder_(new WindowFinder), Use MakeUnique (see threads on chromium-dev). https://codereview.chromium.org/2449103004/diff/240001/chrome/browser/ui/views/tabs/window_finder_mus.cc ...
4 months, 3 weeks ago (2016-10-31 22:43:46 UTC) #70
tonikitoo
https://codereview.chromium.org/2449103004/diff/240001/chrome/browser/ui/views/tabs/tab_drag_controller.cc File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2449103004/diff/240001/chrome/browser/ui/views/tabs/tab_drag_controller.cc#newcode225 chrome/browser/ui/views/tabs/tab_drag_controller.cc:225: window_finder_(new WindowFinder), On 2016/10/31 22:43:45, sky wrote: > Use ...
4 months, 3 weeks ago (2016-10-31 23:39:37 UTC) #71
sky
https://codereview.chromium.org/2449103004/diff/260001/chrome/browser/ui/views/tabs/window_finder_mus.cc File chrome/browser/ui/views/tabs/window_finder_mus.cc (right): https://codereview.chromium.org/2449103004/diff/260001/chrome/browser/ui/views/tabs/window_finder_mus.cc#newcode17 chrome/browser/ui/views/tabs/window_finder_mus.cc:17: content::ServiceManagerConnection* service_manager_connection = For safety set mus_result to null ...
4 months, 3 weeks ago (2016-11-01 15:07:38 UTC) #76
tonikitoo
https://codereview.chromium.org/2449103004/diff/260001/chrome/browser/ui/views/tabs/window_finder_mus.cc File chrome/browser/ui/views/tabs/window_finder_mus.cc (right): https://codereview.chromium.org/2449103004/diff/260001/chrome/browser/ui/views/tabs/window_finder_mus.cc#newcode17 chrome/browser/ui/views/tabs/window_finder_mus.cc:17: content::ServiceManagerConnection* service_manager_connection = On 2016/11/01 15:07:38, sky wrote: > ...
4 months, 3 weeks ago (2016-11-01 15:16:39 UTC) #79
sky
LGTM
4 months, 3 weeks ago (2016-11-01 15:26:30 UTC) #80
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2449103004/280001
4 months, 3 weeks ago (2016-11-01 15:47:44 UTC) #83
commit-bot: I haz the power
Committed patchset #15 (id:280001)
4 months, 3 weeks ago (2016-11-01 15:54:57 UTC) #85
commit-bot: I haz the power
4 months, 3 weeks ago (2016-11-01 16:07:40 UTC) #87
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/dad9ff3ca51c167a4473909bb68588b7ede0da52
Cr-Commit-Position: refs/heads/master@{#429010}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62