aura: Unify WindowTreeHost for some platforms

aura: Unify Ozone+Windows+Android WindowTreeHost

This effectively adds an Android WindowTreeHost.
For these three most platform-specific stuff
happens in PlatformWindow* and the WTH is mostly
a shim.

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

[no sievers, 2015-10-08 00:25:53]

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

[no sievers, 2015-10-08 00:26:00]

ptal

[mfomitchev, 2015-10-08 15:03:27]

+dnicoara for feedback on USE_OZONE ifdefs in window_tree_host_impl.cc

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
File ui/aura/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:45: WindowTreeHostImpl::WindowTreeHostWin(const
gfx::Rect& bounds)
fix constructor name

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:52: WindowTreeHostImpl::~WindowTreeHostWin() {
fix destructor name

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:114: if (cursor == current_cursor_)
native_cursor?
Perhaps we just move this if up and use it on all platforms? Then we'd have
reason to have current_cursor_ on all platforms.

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:143: #if !defined(USE_OZONE)
@dnicoara - do you know if we need this ifdef?

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:149: TRACE_EVENT0("input",
"WindowTreeHostOzone::DispatchEvent");
Fix class name in msg

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:151: #if defined(OS_WIN)
I don't think there's a good reason for this to be platform-specific - we can
just do it on all platforms?
@dnicoara - do you agree?

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:159: // TODO: this obviously shouldn't be here.
Heh..

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:174: #if !defined(USE_OZONE)
I am not sure we need this ifdef either...

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:191: #if !defined(USE_OZONE)
or this..

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
File ui/platform_window/platform_window.h (right):

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
ui/platform_window/platform_window.h:27: static scoped_ptr<PlatformWindow>
Create(PlatformWindowDelegate* delegate,
IMHO it's better just to do an ifdef in the constructor in  WindowTreeHostImpl.
There's a few problems with adding this method here. E.g. this is currently only
implemented for Windows and Ozone. We need this for other platforms if we are
going to do this. Also the new dependency on Ozone seems unfortunate.

[sadrul, 2015-10-08 15:18:10]

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
File ui/aura/window_tree_host_impl.h (right):

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.h:20: class AURA_EXPORT WindowTreeHostImpl
Because we have other impls of WindowTreeHost, this name may be a little
confusing. I suggest WindowTreeHostPlatform.

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
File ui/platform_window/platform_window.cc (right):

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
ui/platform_window/platform_window.cc:10: #include
"ui/ozone/public/ozone_platform.h"
We can't depend on ozone from here, since ozone also depends on
ui/platform_window. An earlier attempt to resolve this was to introduce a
PlatformWindowFactory (https://codereview.chromium.org/403263002/). Ultimately
for mandoline/mus, we went with #ifdefs
(https://code.google.com/p/chromium/codesearch#chromium/src/components/mus/vm/...).
Maybe we can just do that in WindowTreeHost too?

[no sievers, 2015-10-08 17:59:06]

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
File ui/aura/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:174: #if !defined(USE_OZONE)
On 2015/10/08 15:03:27, mfomitchev wrote:
> I am not sure we need this ifdef either...

So this comes down to whether someone can confirm that OnLostCapture() will be
called when it needs to on Ozone. Then we can use the |has_capture_|
optimization here and above which removes 3 ifdefs.

[no sievers, 2015-10-09 00:29:37]

Patchset #2 (id:20001) has been deleted

[no sievers, 2015-10-09 00:34:18]

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
File ui/aura/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:45: WindowTreeHostImpl::WindowTreeHostWin(const
gfx::Rect& bounds)
On 2015/10/08 15:03:27, mfomitchev wrote:
> fix constructor name

Done.

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:52: WindowTreeHostImpl::~WindowTreeHostWin() {
On 2015/10/08 15:03:27, mfomitchev wrote:
> fix destructor name

Done.

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:114: if (cursor == current_cursor_)
On 2015/10/08 15:03:27, mfomitchev wrote:
> native_cursor?
> Perhaps we just move this if up and use it on all platforms? Then we'd have
> reason to have current_cursor_ on all platforms. 

Done.

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:149: TRACE_EVENT0("input",
"WindowTreeHostOzone::DispatchEvent");
On 2015/10/08 15:03:27, mfomitchev wrote:
> Fix class name in msg

Done.

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:151: #if defined(OS_WIN)
On 2015/10/08 15:03:27, mfomitchev wrote:
> I don't think there's a good reason for this to be platform-specific - we can
> just do it on all platforms?
> @dnicoara - do you agree?

Done.

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
File ui/aura/window_tree_host_impl.h (right):

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.h:20: class AURA_EXPORT WindowTreeHostImpl
On 2015/10/08 15:18:10, sadrul wrote:
> Because we have other impls of WindowTreeHost, this name may be a little
> confusing. I suggest WindowTreeHostPlatform.

Done.

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
File ui/platform_window/platform_window.cc (right):

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
ui/platform_window/platform_window.cc:10: #include
"ui/ozone/public/ozone_platform.h"
On 2015/10/08 15:18:10, sadrul wrote:
> We can't depend on ozone from here, since ozone also depends on
> ui/platform_window. An earlier attempt to resolve this was to introduce a
> PlatformWindowFactory (https://codereview.chromium.org/403263002/). Ultimately
> for mandoline/mus, we went with #ifdefs
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/mus/vm/...).
> Maybe we can just do that in WindowTreeHost too?

Done.

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
File ui/platform_window/platform_window.h (right):

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
ui/platform_window/platform_window.h:27: static scoped_ptr<PlatformWindow>
Create(PlatformWindowDelegate* delegate,
On 2015/10/08 15:03:27, mfomitchev wrote:
> IMHO it's better just to do an ifdef in the constructor in 
WindowTreeHostImpl.
> There's a few problems with adding this method here. E.g. this is currently
only
> implemented for Windows and Ozone. We need this for other platforms if we are
> going to do this. Also the new dependency on Ozone seems unfortunate.

Done. For non-Ozone I left it with the implementation though, since ui_aura
doesn't really need to depend on that (i.e. window_android rather than just
platform_window).

[mfomitchev, 2015-10-09 13:56:54]

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
File ui/aura/window_tree_host_impl.cc (right):

https://codereview.chromium.org/1390883003/diff/1/ui/aura/window_tree_host_im...
ui/aura/window_tree_host_impl.cc:174: #if !defined(USE_OZONE)
On 2015/10/08 17:59:06, sievers wrote:
> On 2015/10/08 15:03:27, mfomitchev wrote:
> > I am not sure we need this ifdef either...
> 
> So this comes down to whether someone can confirm that OnLostCapture() will be
> called when it needs to on Ozone. Then we can use the |has_capture_|
> optimization here and above which removes 3 ifdefs.

Yeah, lets wait for @dnicoara to respond to these.

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
File ui/platform_window/platform_window.h (right):

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
ui/platform_window/platform_window.h:27: static scoped_ptr<PlatformWindow>
Create(PlatformWindowDelegate* delegate,
On 2015/10/09 00:34:18, sievers wrote:
> On 2015/10/08 15:03:27, mfomitchev wrote:
> > IMHO it's better just to do an ifdef in the constructor in 
> WindowTreeHostImpl.
> > There's a few problems with adding this method here. E.g. this is currently
> only
> > implemented for Windows and Ozone. We need this for other platforms if we
are
> > going to do this. Also the new dependency on Ozone seems unfortunate.
> 
> Done. For non-Ozone I left it with the implementation though, since ui_aura
> doesn't really need to depend on that (i.e. window_android rather than just
> platform_window).

Do you mean just in the header files? The Aura module itself would still need to
depend on /ui/platform_window/android, since WTHPlatform would instantiate it on
Android, otherwise we'll get a linking error when building Aura for Android.
Just like Aura depends on /ui/platform_window/win... On this note, I also had to
make Aura depend on //ui/platform_window/android:platform_window_java to make it
run - see https://codereview.chromium.org/1391893003/

Also, if we are going to make that ifdef !defined(USE_OZONE), we need to
implement Create on other platforms (X11, etc). Personally I don't think it's
worth the hassle.

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/BUILD.gn
File ui/aura/BUILD.gn (right):

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/BUILD.gn#newcod...
ui/aura/BUILD.gn:144: deps += [ "//ui/ozone" ]
this dep should remain ozone only

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp#newcod...
ui/aura/aura.gyp:141: ['OS=="win" or use_ozone==1', {
The preferred way is to include the file in the main list and then exclude it
based on the condition rather than the other way around. This probably applies
to GN as well.
https://chromium.googlesource.com/external/gyp/+/master/docs/UserDocumentatio...

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp#newcod...
ui/aura/aura.gyp:141: ['OS=="win" or use_ozone==1', {
Do you want to add android in a separate CL?

[dnicoara, 2015-10-09 15:04:22]

dnicoara@chromium.org changed reviewers:
+ dnicoara@chromium.org

[dnicoara, 2015-10-09 15:04:22]

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host_platform.cc:60: window_.reset();
Any reason this is needed? The destructor should clean this up automatically.

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host_platform.cc:150: #if !defined(USE_OZONE)
It should be safe to remove the #ifdef and call the compositor on Ozone

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host_platform.cc:178: #if !defined(USE_OZONE)
It should be safe to remove the #ifdef (same with the ones in
{Set,Release}Capture)

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host_platform.cc:196: #if !defined(USE_OZONE)
It should be safe to remove the #ifdef.

All of these methods don't have implementations on Ozone since they weren't
needed, hence they were left empty for simplicity/clarity.

[no sievers, 2015-10-09 19:30:42]

Patchset #3 (id:60001) has been deleted

[no sievers, 2015-10-09 19:31:51]

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp#newcod...
ui/aura/aura.gyp:141: ['OS=="win" or use_ozone==1', {
On 2015/10/09 13:56:54, mfomitchev wrote:
> The preferred way is to include the file in the main list and then exclude it
> based on the condition rather than the other way around. This probably applies
> to GN as well.
>
https://chromium.googlesource.com/external/gyp/+/master/docs/UserDocumentatio...

Done.

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp#newcod...
ui/aura/aura.gyp:141: ['OS=="win" or use_ozone==1', {
On 2015/10/09 13:56:54, mfomitchev wrote:
> Do you want to add android in a separate CL?

Well, might as well do it now that it's so straightforward. Done.

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host_platform.cc:60: window_.reset();
On 2015/10/09 15:04:22, dnicoara wrote:
> Any reason this is needed? The destructor should clean this up automatically.

Done.

[mfomitchev, 2015-10-09 20:11:49]

On 2015/10/09 19:31:51, sievers wrote:
> https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp
> File ui/aura/aura.gyp (right):
> 
>
https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp#newcod...
> ui/aura/aura.gyp:141: ['OS=="win" or use_ozone==1', {
> On 2015/10/09 13:56:54, mfomitchev wrote:
> > The preferred way is to include the file in the main list and then exclude
it
> > based on the condition rather than the other way around. This probably
applies
> > to GN as well.
> >
>
https://chromium.googlesource.com/external/gyp/+/master/docs/UserDocumentatio...
> 
> Done.
> 
>
https://codereview.chromium.org/1390883003/diff/40001/ui/aura/aura.gyp#newcod...
> ui/aura/aura.gyp:141: ['OS=="win" or use_ozone==1', {
> On 2015/10/09 13:56:54, mfomitchev wrote:
> > Do you want to add android in a separate CL?
> 
> Well, might as well do it now that it's so straightforward. Done.
> 
>
https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
> File ui/aura/window_tree_host_platform.cc (right):
> 
>
https://codereview.chromium.org/1390883003/diff/40001/ui/aura/window_tree_hos...
> ui/aura/window_tree_host_platform.cc:60: window_.reset();
> On 2015/10/09 15:04:22, dnicoara wrote:
> > Any reason this is needed? The destructor should clean this up
automatically.
> 
> Done.

Looks like you may have missed my response in
https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...

[no sievers, 2015-10-09 20:47:46]

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
File ui/platform_window/platform_window.h (right):

https://codereview.chromium.org/1390883003/diff/1/ui/platform_window/platform...
ui/platform_window/platform_window.h:27: static scoped_ptr<PlatformWindow>
Create(PlatformWindowDelegate* delegate,
On 2015/10/09 13:56:54, mfomitchev wrote:
> On 2015/10/09 00:34:18, sievers wrote:
> > On 2015/10/08 15:03:27, mfomitchev wrote:
> > > IMHO it's better just to do an ifdef in the constructor in 
> > WindowTreeHostImpl.
> > > There's a few problems with adding this method here. E.g. this is
currently
> > only
> > > implemented for Windows and Ozone. We need this for other platforms if we
> are
> > > going to do this. Also the new dependency on Ozone seems unfortunate.
> > 
> > Done. For non-Ozone I left it with the implementation though, since ui_aura
> > doesn't really need to depend on that (i.e. window_android rather than just
> > platform_window).
> 
> Do you mean just in the header files? The Aura module itself would still need
to
> depend on /ui/platform_window/android, since WTHPlatform would instantiate it
on
> Android, otherwise we'll get a linking error when building Aura for Android.
> Just like Aura depends on /ui/platform_window/win... On this note, I also had
to
> make Aura depend on //ui/platform_window/android:platform_window_java to make
it
> run - see https://codereview.chromium.org/1391893003/
> 
> Also, if we are going to make that ifdef !defined(USE_OZONE), we need to
> implement Create on other platforms (X11, etc). Personally I don't think it's
> worth the hassle.

Done.

[mfomitchev, 2015-10-09 21:24:18]

Looks pretty good!

https://codereview.chromium.org/1390883003/diff/100001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_win.cc (right):

https://codereview.chromium.org/1390883003/diff/100001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_win.cc:43: saved_window_style_ =
GetWindowLong(GetAcceleratedWidget(), GWL_STYLE);
hwnd = GetAcceleratedWidget() so that we don't call GetAcceleratedWidget() so
many times?

https://codereview.chromium.org/1390883003/diff/100001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (left):

https://codereview.chromium.org/1390883003/diff/100001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:44: #include "ui/aura/window_tree_host_win.h"
If we are removing window_tree_host_win, shouldn't we import
window_tree_host_platform?

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/BUILD.gn
File ui/aura/BUILD.gn (right):

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/BUILD.gn#newco...
ui/aura/BUILD.gn:140: if ((is_mac || use_x11) && !use_ozone) {
(!is_win && !use_ozone && !is_android)?
IMHO better to list all platforms that need this rather than all platforms that
don't, since the set of platforms may change.

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/BUILD.gn#newco...
ui/aura/BUILD.gn:151: "//ui/platform_window/android",
I also had to add "//ui/platform_window/android:platform_window_java" here to
make it run, otherwise it crashed at runtime...

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/aura.gyp#newco...
ui/aura/aura.gyp:143: ['(OS=="mac" or use_x11==1) and use_ozone==0', {
Same as for gn

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/aura.gyp#newco...
ui/aura/aura.gyp:151:
'../platform_window/android/android_window.gyp:android_window',
same as for gn

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.h (right):

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.h:27: protected:
Ozone WTH had this, but Windows WTH didn't, so probably better to keep these
public?

[no sievers, 2015-10-09 21:41:02]

sievers@chromium.org changed reviewers:
+ oshima@chromium.org

[no sievers, 2015-10-09 21:41:14]

+oshima for ash/*

https://codereview.chromium.org/1390883003/diff/100001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_win.cc (right):

https://codereview.chromium.org/1390883003/diff/100001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_win.cc:43: saved_window_style_ =
GetWindowLong(GetAcceleratedWidget(), GWL_STYLE);
On 2015/10/09 21:24:18, mfomitchev wrote:
> hwnd = GetAcceleratedWidget() so that we don't call GetAcceleratedWidget() so
> many times?

Done.

https://codereview.chromium.org/1390883003/diff/100001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (left):

https://codereview.chromium.org/1390883003/diff/100001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:44: #include "ui/aura/window_tree_host_win.h"
On 2015/10/09 21:24:18, mfomitchev wrote:
> If we are removing window_tree_host_win, shouldn't we import
> window_tree_host_platform?

This was unneeded here, I think. It only needs WindowTreeHost which is included
above.

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/aura.gyp#newco...
ui/aura/aura.gyp:151:
'../platform_window/android/android_window.gyp:android_window',
On 2015/10/09 21:24:18, mfomitchev wrote:
> same as for gn

gyp should be ok since it's transitive.

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/aura.gyp#newco...
ui/aura/aura.gyp:151:
'../platform_window/android/android_window.gyp:android_window',
On 2015/10/09 21:24:18, mfomitchev wrote:
> same as for gn

Done.

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.h (right):

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.h:27: protected:
On 2015/10/09 21:24:18, mfomitchev wrote:
> Ozone WTH had this, but Windows WTH didn't, so probably better to keep these
> public?

Done.

[no sievers, 2015-10-09 21:42:23]

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/BUILD.gn
File ui/aura/BUILD.gn (right):

https://codereview.chromium.org/1390883003/diff/100001/ui/aura/BUILD.gn#newco...
ui/aura/BUILD.gn:151: "//ui/platform_window/android",
On 2015/10/09 21:24:18, mfomitchev wrote:
> I also had to add "//ui/platform_window/android:platform_window_java" here to
> make it run, otherwise it crashed at runtime...

oops forgot this

[no sievers, 2015-10-09 22:30:06]

@sadrul: how about I remove the platform_window_win/x11/android targets? now
that there is a factory this can all just be part of 'platform_window' with
platform specific code like anywhere else in the codebase. i don't think the
impl targets need to be exposed. also they don't need to be exported now, only
the factory method needs to be. wdyt?

[sadrul, 2015-10-09 23:50:31]

On 2015/10/09 22:30:06, sievers wrote:
> @sadrul: how about I remove the platform_window_win/x11/android targets? now
> that there is a factory this can all just be part of 'platform_window' with
> platform specific code like anywhere else in the codebase. i don't think the
> impl targets need to be exposed. also they don't need to be exported now, only
> the factory method needs to be. wdyt?

I don't think we should do that. There should be a platform_window target that
does not have any implementations. Ozone, for example, should be able to use
that and provide its implementation of PlatformWindow, without bringing in any
of the 'default' impls.

We also should not have a PlatformWindow::Create(). We can have more than one
implementations in the same build (e.g. both stub and x11, where stub is used
for tests only), and it would be better to explicitly create the
platform-specific thing instead of using a PlatformWindow::Create().

[no sievers, 2015-10-09 23:55:32]

On 2015/10/09 23:50:31, sadrul wrote:
> On 2015/10/09 22:30:06, sievers wrote:
> > @sadrul: how about I remove the platform_window_win/x11/android targets? now
> > that there is a factory this can all just be part of 'platform_window' with
> > platform specific code like anywhere else in the codebase. i don't think the
> > impl targets need to be exposed. also they don't need to be exported now,
only
> > the factory method needs to be. wdyt?
> 
> I don't think we should do that. There should be a platform_window target that
> does not have any implementations. Ozone, for example, should be able to use
> that and provide its implementation of PlatformWindow, without bringing in any
> of the 'default' impls.
> 

Good point. Ozone only needs to depend on the interface then. So we maybe need
two targets.
But not one for each port.


> We also should not have a PlatformWindow::Create(). We can have more than one
> implementations in the same build (e.g. both stub and x11, where stub is used
> for tests only), and it would be better to explicitly create the
> platform-specific thing instead of using a PlatformWindow::Create().

You can still do that, even if you have a factory. But then you'd need to export
the impl.
Btw, StubWindow is unused.

[no sievers, 2015-10-09 23:57:10]

On 2015/10/09 23:55:32, sievers wrote:
> On 2015/10/09 23:50:31, sadrul wrote:
> > On 2015/10/09 22:30:06, sievers wrote:
> > > @sadrul: how about I remove the platform_window_win/x11/android targets?
now
> > > that there is a factory this can all just be part of 'platform_window'
with
> > > platform specific code like anywhere else in the codebase. i don't think
the
> > > impl targets need to be exposed. also they don't need to be exported now,
> only
> > > the factory method needs to be. wdyt?
> > 
> > I don't think we should do that. There should be a platform_window target
that
> > does not have any implementations. Ozone, for example, should be able to use
> > that and provide its implementation of PlatformWindow, without bringing in
any
> > of the 'default' impls.
> > 
> 
> Good point. Ozone only needs to depend on the interface then. So we maybe need
> two targets.
> But not one for each port.
> 

Either way, it doesn't have to be in this change. I just find the whole build
setup very verbose for this as it currently is. (And it's really just a single
class.)

[no sievers, 2015-10-10 00:02:24]

On 2015/10/09 23:50:31, sadrul wrote:
> On 2015/10/09 22:30:06, sievers wrote:
> > @sadrul: how about I remove the platform_window_win/x11/android targets? now
> > that there is a factory this can all just be part of 'platform_window' with
> > platform specific code like anywhere else in the codebase. i don't think the
> > impl targets need to be exposed. also they don't need to be exported now,
only
> > the factory method needs to be. wdyt?
> 
> I don't think we should do that. There should be a platform_window target that
> does not have any implementations. Ozone, for example, should be able to use
> that and provide its implementation of PlatformWindow, without bringing in any
> of the 'default' impls.
> 
> We also should not have a PlatformWindow::Create(). We can have more than one
> implementations in the same build (e.g. both stub and x11, where stub is used
> for tests only), and it would be better to explicitly create the
> platform-specific thing instead of using a PlatformWindow::Create().

Btw you suggested the factory :)

[sadrul, 2015-10-10 00:10:05]

On 2015/10/10 00:02:24, sievers wrote:
> On 2015/10/09 23:50:31, sadrul wrote:
> > On 2015/10/09 22:30:06, sievers wrote:
> > > @sadrul: how about I remove the platform_window_win/x11/android targets?
now
> > > that there is a factory this can all just be part of 'platform_window'
with
> > > platform specific code like anywhere else in the codebase. i don't think
the
> > > impl targets need to be exposed. also they don't need to be exported now,
> only
> > > the factory method needs to be. wdyt?
> > 
> > I don't think we should do that. There should be a platform_window target
that
> > does not have any implementations. Ozone, for example, should be able to use
> > that and provide its implementation of PlatformWindow, without bringing in
any
> > of the 'default' impls.
> > 
> > We also should not have a PlatformWindow::Create(). We can have more than
one
> > implementations in the same build (e.g. both stub and x11, where stub is
used
> > for tests only), and it would be better to explicitly create the
> > platform-specific thing instead of using a PlatformWindow::Create().
> 
> Btw you suggested the factory :)

I must have explained it wrong :) I suggested there was an attempt at using a
factory a while back, but ultimately, we ended up using the PlatformWindow
explicitly for mus/mandoline, which ends up being much simpler.

[no sievers, 2015-10-12 20:47:30]

@sadrul: I've reverted the factory. how's the latest patch set?

[no sievers, 2015-10-14 23:26:34]

On 2015/10/12 20:47:30, sievers wrote:
> @sadrul: I've reverted the factory. how's the latest patch set?

ping

[mfomitchev, 2015-10-15 15:00:57]

https://codereview.chromium.org/1390883003/diff/240001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/240001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:60: new
ui::OzonePlatform::GetInstance()->CreatePlatformWindow(this, bounds));
We need to get rid of "new" here. This is probably why the trybots are failing.

[no sievers, 2015-10-15 17:24:24]

https://codereview.chromium.org/1390883003/diff/240001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/240001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:60: new
ui::OzonePlatform::GetInstance()->CreatePlatformWindow(this, bounds));
On 2015/10/15 15:00:57, mfomitchev wrote:
> We need to get rid of "new" here. This is probably why the trybots are
failing.

Oops, thanks, done.

[no sievers, 2015-10-15 18:47:14]

Patchset #14 (id:300001) has been deleted

[sadrul, 2015-10-15 19:47:40]

I think some of the #idefs can be removed, and that would be cleaner. The rest
lgtm

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:7: 
Remove extra blank line.

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:116: ::SetCursor(cursor.platform());
Can this bit move into WinWindow() instead, and we always call
window_->SetCursor() without the #ifdef?

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:127: #endif
WinWindow::MoveCursorTo() doesn't do anything. So I think we can call
window_->MoveCursorTo() unconditionally here without the #ifdef.

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.h (right):

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.h:27: public:
not needed.

[oshima, 2015-10-15 21:56:31]

can you look into test failures on ozone build?

[no sievers, 2015-10-16 00:06:15]

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:137: if (bounds_.origin() !=
old_bounds.origin())
These two optimizations for redundant calls are causing the
linux_chromium_chromeos_ozone_rel_ng test failures.

[oshima, 2015-10-16 17:15:45]

On 2015/10/16 00:06:15, sievers wrote:
>
https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
> File ui/aura/window_tree_host_platform.cc (right):
> 
>
https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
> ui/aura/window_tree_host_platform.cc:137: if (bounds_.origin() !=
> old_bounds.origin())
> These two optimizations for redundant calls are causing the
> linux_chromium_chromeos_ozone_rel_ng test failures.

See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_tre...

[no sievers, 2015-10-16 18:26:15]

Ok thanks, added the device scale factor check similar to WTHX11. Tests are
passing now.

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:7: 
On 2015/10/15 19:47:40, sadrul wrote:
> Remove extra blank line.

Done.

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:116: ::SetCursor(cursor.platform());
On 2015/10/15 19:47:40, sadrul wrote:
> Can this bit move into WinWindow() instead, and we always call
> window_->SetCursor() without the #ifdef?

Done.

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:127: #endif
On 2015/10/15 19:47:39, sadrul wrote:
> WinWindow::MoveCursorTo() doesn't do anything. So I think we can call
> window_->MoveCursorTo() unconditionally here without the #ifdef.

Done.

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.h (right):

https://codereview.chromium.org/1390883003/diff/320001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.h:27: public:
On 2015/10/15 19:47:40, sadrul wrote:
> not needed.

Done.

[dnicoara, 2015-10-16 18:30:57]

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:7: #include <algorithm>
And this include can probably go to.

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:28: using std::max;
These don't seem to be used.

[oshima, 2015-10-16 18:31:42]

lgtm

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/aura.gyp#newco...
ui/aura/aura.gyp:151:
'../platform_window/android/android_window.gyp:android_window',
nit: indent

[no sievers, 2015-10-16 18:42:24]

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host_platform.cc (right):

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:7: #include <algorithm>
On 2015/10/16 18:30:57, dnicoara wrote:
> And this include can probably go to.

Done.

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:28: using std::max;
On 2015/10/16 18:30:57, dnicoara wrote:
> These don't seem to be used.

Done.

https://codereview.chromium.org/1390883003/diff/340001/ui/aura/window_tree_ho...
ui/aura/window_tree_host_platform.cc:28: using std::max;
On 2015/10/16 18:30:57, dnicoara wrote:
> These don't seem to be used.

Done.

[no sievers, 2015-10-16 19:16:08]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-10-16 19:16:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1390883003/#ps380001
(title: "win build")

[commit-bot: I haz the power, 2015-10-16 19:17:22]

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

[commit-bot: I haz the power, 2015-10-16 19:50:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-16 19:50:11]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[no sievers, 2015-10-16 19:56:23]

Patchset #17 (id:380001) has been deleted

[no sievers, 2015-10-16 19:57:25]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-10-16 19:57:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1390883003/#ps400001
(title: "win build")

[commit-bot: I haz the power, 2015-10-16 19:58:06]

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

[commit-bot: I haz the power, 2015-10-16 20:44:44]

Committed patchset #17 (id:400001)

[commit-bot: I haz the power, 2015-10-16 20:45:26]

Patchset 17 (id:??) landed as
https://crrev.com/1b4ef1a69b08dd2d779521118ddb1f3a285d4c83
Cr-Commit-Position: refs/heads/master@{#354585}