Introduces window_finder_android.cc

Introduces window_finder_android.cc

Also adds dragged_window parameters to function GetLocalProcessWindowAtPoint

BUG=507792
Committed: https://crrev.com/f6cef5f6e07fe5680de420ca0a1af9bb9bf3128f
Cr-Commit-Position: refs/heads/master@{#358632}

[bshe, 2015-10-23 17:16:55]

bshe@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[bshe, 2015-10-23 17:16:56]

Hi Mikhail. Do you mind take a look at this CL? It basically try to upstream
this file:
https://codereview.chromium.org/1391893003/diff/1/chrome/browser/ui/views/tab...

[mfomitchev, 2015-10-23 18:28:29]

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1805: gfx::NativeWindow
dragged_window =
Seems like the right thing to do would be to put this implementation into
GetLocalProcessWindowAtPoint() in window_finder_android.cc? As is we have two
unconditional returns one after another on Android, so it wouldn't even compile.

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_impl.cc (right):

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_impl.cc:16: gfx::NativeWindow window)
{
Can we give a more descriptive name to "window"? root_window?

[mfomitchev, 2015-10-29 21:35:58]

mfomitchev@chromium.org changed reviewers:
+ sadrul@chromium.org

[mfomitchev, 2015-10-29 21:35:59]

I chatted with sadrul, and he suggested simply adding dragged_window as an arg
to GetLocalProcessWindowAtPoint. We won't use it on any platforms other than
Android, but that's ok. Then we can have the _android implementation, and we
also don't need the global GetHost() method.

[bshe, 2015-10-30 16:31:05]

Description was changed from

==========
Introduce window_finder_impl.cc for android to use


BUG=507792
==========

to

==========
Introduces window_finder_android.cc and adds dragged_window parameters to
function GetLocalProcessWindowAtPoint


BUG=507792
==========

[bshe, 2015-10-30 16:31:44]

Description was changed from

==========
Introduces window_finder_android.cc and adds dragged_window parameters to
function GetLocalProcessWindowAtPoint


BUG=507792
==========

to

==========
Introduces window_finder_android.cc

Also adds dragged_window parameters to function GetLocalProcessWindowAtPoint


BUG=507792
==========

[bshe, 2015-10-30 16:37:38]

Patchset #3 (id:40001) has been deleted

[bshe, 2015-10-30 16:37:50]

Patchset #3 (id:60001) has been deleted

[bshe, 2015-10-30 16:41:37]

On 2015/10/29 21:35:59, mfomitchev wrote:
> I chatted with sadrul, and he suggested simply adding dragged_window as an arg
> to GetLocalProcessWindowAtPoint. We won't use it on any platforms other than
> Android, but that's ok. Then we can have the _android implementation, and we
> also don't need the global GetHost() method.

Thanks Mikhail and Sadrul! I have made the change in patchset 3. PTAL.

[mfomitchev, 2015-10-30 17:04:42]

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder.h (right):

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder.h:25: gfx::NativeWindow
dragged_window);
It's not clear how does "dragged window" fit into the context of this function
just by looking at the name of the function and the comments. Maybe we could
pass a root window instead and say this is just one of the root windows in the
comments? Still not ideal, but better IMHO.

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_ash.cc (right):

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_ash.cc:20: for
(aura::Window::Windows::const_reverse_iterator i =
It doesn't seem like this is the same functionality we had before. E.g.
previously we'd only go though children if window->layer()->type() wasn't
ui::LAYER_TEXTURED.

[bshe, 2015-10-30 19:38:46]

Patchset #4 (id:100001) has been deleted

[bshe, 2015-10-30 19:44:09]

Patchset #4 (id:120001) has been deleted

[bshe, 2015-10-30 19:55:27]

Patchset #4 (id:140001) has been deleted

[bshe, 2015-10-30 19:56:23]

Thanks for review!
PATL

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1805: gfx::NativeWindow
dragged_window =
On 2015/10/23 18:28:29, mfomitchev wrote:
> Seems like the right thing to do would be to put this implementation into
> GetLocalProcessWindowAtPoint() in window_finder_android.cc? As is we have two
> unconditional returns one after another on Android, so it wouldn't even
compile.

Added window_finder_android.cc

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_impl.cc (right):

https://codereview.chromium.org/1421083003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_impl.cc:16: gfx::NativeWindow window)
{
On 2015/10/23 18:28:29, mfomitchev wrote:
> Can we give a more descriptive name to "window"? root_window?

huh. Just realized the original function is a recursive function. So window is
not always root_window. I changed the logic in window_finder_ash.cc to fix the
bug.

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder.h (right):

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder.h:25: gfx::NativeWindow
dragged_window);
On 2015/10/30 17:04:42, mfomitchev wrote:
> It's not clear how does "dragged window" fit into the context of this function
> just by looking at the name of the function and the comments. Maybe we could
> pass a root window instead and say this is just one of the root windows in the
> comments? Still not ideal, but better IMHO.

renamed to root_window.

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/window_finder_ash.cc (right):

https://codereview.chromium.org/1421083003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/window_finder_ash.cc:20: for
(aura::Window::Windows::const_reverse_iterator i =
On 2015/10/30 17:04:42, mfomitchev wrote:
> It doesn't seem like this is the same functionality we had before. E.g.
> previously we'd only go though children if window->layer()->type() wasn't
> ui::LAYER_TEXTURED.

Good catch. I assumed it is OK to skip the layer type check as the windows with
these IDs are window
containers which should be contained in NOT_DRAW layers. But as some tests
failed, so it is apparently not the case.
I have changed the code so that we pass these IDs to the recursive function as
parameter. So we dont need
to duplicate the logic here.

[mfomitchev, 2015-10-30 20:37:34]

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1803: dragged_window ?
dragged_window->GetRootWindow() : nullptr;
Is it actually possible for dragged_window to be null?

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder.h (right):

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder.h:21: // |root_window| is currently
only used on Android platform. Other platforms
Should this be a TODO?

[bshe, 2015-10-30 20:57:06]

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1803: dragged_window ?
dragged_window->GetRootWindow() : nullptr;
On 2015/10/30 20:37:34, mfomitchev wrote:
> Is it actually possible for dragged_window to be null?

Yes. It sounds strange. But the original code seems to think so (line 1801).
I also find this comment here:
https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/widget/d...
It might suggest the dragged_window could become null as for desktop aura native
widget, content_window is the native window.

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder.h (right):

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder.h:21: // |root_window| is currently
only used on Android platform. Other platforms
On 2015/10/30 20:37:34, mfomitchev wrote:
> Should this be a TODO?

Done.

[mfomitchev, 2015-10-30 21:28:33]

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1803: dragged_window ?
dragged_window->GetRootWindow() : nullptr;
On 2015/10/30 20:57:05, bshe wrote:
> On 2015/10/30 20:37:34, mfomitchev wrote:
> > Is it actually possible for dragged_window to be null?
> 
> Yes. It sounds strange. But the original code seems to think so (line 1801).
> I also find this comment here:
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/widget/d...
> It might suggest the dragged_window could become null as for desktop aura
native
> widget, content_window is the native window.

Should we just bail and return null in this case?
GetLocalProcessWindowAtPointImpl will crash if root_window is null...

[bshe, 2015-11-02 13:46:43]

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1803: dragged_window ?
dragged_window->GetRootWindow() : nullptr;
On 2015/10/30 21:28:33, mfomitchev wrote:
> On 2015/10/30 20:57:05, bshe wrote:
> > On 2015/10/30 20:37:34, mfomitchev wrote:
> > > Is it actually possible for dragged_window to be null?
> > 
> > Yes. It sounds strange. But the original code seems to think so (line 1801).
> > I also find this comment here:
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ui/views/widget/d...
> > It might suggest the dragged_window could become null as for desktop aura
> native
> > widget, content_window is the native window.
> 
> Should we just bail and return null in this case?
> GetLocalProcessWindowAtPointImpl will crash if root_window is null...

I seems to me that it wont be null for aura android as we wont use the desktop
widget. But you are right, it is safer to add a check against it. Added in
window_finder_android.cc.

[mfomitchev, 2015-11-04 18:20:36]

https://codereview.chromium.org/1421083003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1818: return
GetLocalProcessWindowAtPoint(host_desktop_type_, screen_point, exclude,
nit: code style: each param on separate line (or all params on one line)

https://codereview.chromium.org/1421083003/diff/200001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1421083003/diff/200001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2376:
'browser/ui/views/tabs/window_finder_impl.cc',
This should be included only on ash and Android

[bshe, 2015-11-04 22:20:21]

https://codereview.chromium.org/1421083003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1818: return
GetLocalProcessWindowAtPoint(host_desktop_type_, screen_point, exclude,
On 2015/11/04 18:20:36, mfomitchev wrote:
> nit: code style: each param on separate line (or all params on one line)

Actually, "git cl format" formatted it for me...
I also believe this style is acceptable:
https://google.github.io/styleguide/cppguide.html#Function_Calls
So keep it as it is?

https://codereview.chromium.org/1421083003/diff/200001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1421083003/diff/200001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2376:
'browser/ui/views/tabs/window_finder_impl.cc',
On 2015/11/04 18:20:36, mfomitchev wrote:
> This should be included only on ash and Android

Done.

[mfomitchev, 2015-11-04 23:26:09]

LGTM

https://codereview.chromium.org/1421083003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1818: return
GetLocalProcessWindowAtPoint(host_desktop_type_, screen_point, exclude,
Ok, sgtm

[bshe, 2015-11-05 22:19:37]

bshe@chromium.org changed reviewers:
+ sky@chromium.org

[bshe, 2015-11-05 22:19:37]

+sky for owners
Thanks!

[sky, 2015-11-06 17:26:57]

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1803: dragged_window ?
dragged_window->GetRootWindow() : nullptr;
This is aura specific right? Can we instead pass in dragged_window and let the
implementations that want the root get it?

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_android.cc (right):

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_android.cc:10: gfx::NativeWindow
GetLocalProcessWindowAtPointImpl(
Move this into window_filter_impl.h

https://codereview.chromium.org/1421083003/diff/220001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1421083003/diff/220001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2968: ['use_ash==1 or OS=="android"', {
This is a subtle and confusing place to put this include. I would rather you
list window_finder_impl.cc with all sources and exclude where appropriate.

[bshe, 2015-11-06 21:08:43]

Patchset #8 (id:240001) has been deleted

[bshe, 2015-11-06 21:13:21]

Thanks for review. PTAL

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1803: dragged_window ?
dragged_window->GetRootWindow() : nullptr;
On 2015/11/06 17:26:56, sky wrote:
> This is aura specific right? Can we instead pass in dragged_window and let the
> implementations that want the root get it?

Done.

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_android.cc (right):

https://codereview.chromium.org/1421083003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_android.cc:10: gfx::NativeWindow
GetLocalProcessWindowAtPointImpl(
On 2015/11/06 17:26:56, sky wrote:
> Move this into window_filter_impl.h

Done.

https://codereview.chromium.org/1421083003/diff/220001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1421083003/diff/220001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2968: ['use_ash==1 or OS=="android"', {
On 2015/11/06 17:26:57, sky wrote:
> This is a subtle and confusing place to put this include. I would rather you
> list window_finder_impl.cc with all sources and exclude where appropriate.

I assume you mean include the file in "chrome_browser_ui_views_non_mac_sources".
It looks like non_mac_sources is used here and also when mac_views_browser is
true on mac platform. So I need to exclude them in two different places with
condition check for android and ash. It is still not easy to read.
Instead, I added a chrome_browser_ui_views_android_sources variable and the
condition checks seem easier to read. But
I need to duplicate window_finder_impl.* in this case. So not entirely sure this
approach is better. WDYT?

[bshe, 2015-11-06 21:14:39]

Patchset #8 (id:260001) has been deleted

[bshe, 2015-11-06 21:16:48]

Patchset #8 (id:280001) has been deleted

[sky, 2015-11-09 03:35:04]

LGTM with the following addressed.

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder.h (right):

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder.h:21: // TODO(bshe): |window| is
currently only used on Android platform. It is used
You don't actually document what |window| is. I would call it source or context,
and better document what it actually is.

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_impl.h (right):

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_impl.h:12: namespace aura {
You should be able to remove this forward declaration since you don't actually
use it here.

[bshe, 2015-11-09 18:25:36]

Thanks!

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder.h (right):

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder.h:21: // TODO(bshe): |window| is
currently only used on Android platform. It is used
On 2015/11/09 03:35:03, sky wrote:
> You don't actually document what |window| is. I would call it source or
context,
> and better document what it actually is.

Done.

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_impl.h (right):

https://codereview.chromium.org/1421083003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_impl.h:12: namespace aura {
On 2015/11/09 03:35:03, sky wrote:
> You should be able to remove this forward declaration since you don't actually
> use it here.

Done.

[bshe, 2015-11-09 18:32:43]

The CQ bit was checked by bshe@chromium.org

[bshe, 2015-11-09 18:32:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
mfomitchev@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1421083003/#ps320001
(title: "reviews")

[commit-bot: I haz the power, 2015-11-09 18:36:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421083003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421083003/320001

[commit-bot: I haz the power, 2015-11-09 19:43:14]

Committed patchset #9 (id:320001)

[commit-bot: I haz the power, 2015-11-09 19:44:18]

Patchset 9 (id:??) landed as
https://crrev.com/f6cef5f6e07fe5680de420ca0a1af9bb9bf3128f
Cr-Commit-Position: refs/heads/master@{#358632}