Creating default implementation for aura::client::ScreenPositionClient

Creating default implementation for aura::client::ScreenPositionClient

Making all implementations except for ash's use the new default base class.

I left the ash's implementation as is, since I am not sure if the display origin
is always the same as the root window origin. My suspicion is that it's not..

BUG=NONE

Committed: https://crrev.com/235ca91afbcc146c1d188b5de0f1c6070d745a8f
Cr-Commit-Position: refs/heads/master@{#302502}

[mfomitchev, 2014-10-28 20:58:30]

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

[mfomitchev, 2014-10-30 15:27:43]

Sadrul, can you please take a look?

[sadrul, 2014-10-30 16:09:11]

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
ui/wm/core/default_screen_position_client.cc:34: return window->type() ==
ui::wm::WINDOW_TYPE_POPUP;
Why special case POPUP here?

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
File ui/wm/core/default_screen_position_client.h (right):

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
ui/wm/core/default_screen_position_client.h:37:
DISALLOW_COPY_AND_ASSIGN(DefaultScreenPositionClient);
This should be in private:

[mfomitchev, 2014-10-30 20:12:54]

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
ui/wm/core/default_screen_position_client.cc:34: return window->type() ==
ui::wm::WINDOW_TYPE_POPUP;
On 2014/10/30 16:09:11, sadrul wrote:
> Why special case POPUP here?

I just copied this code from desktop_native_widget_aura.cc. I guess the
reasoning is that popus are always positioned in screen coordinates.. Should I
just leave this in DesktopScreenPositionClient and not have any such logic in
the default implementation?

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
File ui/wm/core/default_screen_position_client.h (right):

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
ui/wm/core/default_screen_position_client.h:37:
DISALLOW_COPY_AND_ASSIGN(DefaultScreenPositionClient);
On 2014/10/30 16:09:11, sadrul wrote:
> This should be in private:

Done.

[sadrul, 2014-10-30 20:20:07]

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
ui/wm/core/default_screen_position_client.cc:34: return window->type() ==
ui::wm::WINDOW_TYPE_POPUP;
On 2014/10/30 20:12:53, mfomitchev wrote:
> On 2014/10/30 16:09:11, sadrul wrote:
> > Why special case POPUP here?
> 
> I just copied this code from desktop_native_widget_aura.cc. I guess the
> reasoning is that popus are always positioned in screen coordinates.. Should I
> just leave this in DesktopScreenPositionClient and not have any such logic in
> the default implementation?

Yeah. I think we can do without PositionWindowInScreenCoordinates(). The desktop
implementation overrides ::SetBounds() anyway, and so it can do this check in
there. The default impl of ::SetBounds() below can just do window->SetBounds()

[mfomitchev, 2014-10-30 22:02:12]

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/686513004/diff/20001/ui/wm/core/default_scree...
ui/wm/core/default_screen_position_client.cc:34: return window->type() ==
ui::wm::WINDOW_TYPE_POPUP;
On 2014/10/30 20:20:07, sadrul wrote:
> On 2014/10/30 20:12:53, mfomitchev wrote:
> > On 2014/10/30 16:09:11, sadrul wrote:
> > > Why special case POPUP here?
> > 
> > I just copied this code from desktop_native_widget_aura.cc. I guess the
> > reasoning is that popus are always positioned in screen coordinates.. Should
I
> > just leave this in DesktopScreenPositionClient and not have any such logic
in
> > the default implementation?
> 
> Yeah. I think we can do without PositionWindowInScreenCoordinates(). The
desktop
> implementation overrides ::SetBounds() anyway, and so it can do this check in
> there. The default impl of ::SetBounds() below can just do window->SetBounds()

Done.

[mfomitchev, 2014-10-31 19:30:55]

Sadrul, can you PTAL? I made GetOrigin() an object method since it's used by
both Default and Desktop clients.

[sadrul, 2014-11-03 16:57:22]

LGTM

https://codereview.chromium.org/686513004/diff/100001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.h (right):

https://codereview.chromium.org/686513004/diff/100001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.h:33: virtual gfx::Point
GetOrigin(const aura::Window* root_window);
This doesn't need to be virtual?

Add a comment explaining that this returns the origin of the host
platform-window in system pixel coordinate (or alternatively, call this
GetOriginInSystemPixels()).

[mfomitchev, 2014-11-03 18:25:08]

https://codereview.chromium.org/686513004/diff/100001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.h (right):

https://codereview.chromium.org/686513004/diff/100001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.h:33: virtual gfx::Point
GetOrigin(const aura::Window* root_window);
Nope, it doesn't. I thought subclasses may want to override, but I guess until
there's a use case for that we can keep it non-virtual.

How about GetOriginInScreen() to make it consistent with the otehr methods? I
added a comment as well.

[mfomitchev, 2014-11-03 18:25:52]

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

[mfomitchev, 2014-11-03 18:25:53]

oshima@chromium.org: Please review changes in 

athena/screen/screen_manager_impl.cc

[sadrul, 2014-11-03 18:48:44]

https://codereview.chromium.org/686513004/diff/100001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.h (right):

https://codereview.chromium.org/686513004/diff/100001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.h:33: virtual gfx::Point
GetOrigin(const aura::Window* root_window);
On 2014/11/03 18:25:07, mfomitchev wrote:
> Nope, it doesn't. I thought subclasses may want to override, but I guess until
> there's a use case for that we can keep it non-virtual.
> 
> How about GetOriginInScreen() to make it consistent with the otehr methods? I
> added a comment as well.

Yep, sounds good.

[oshima, 2014-11-03 18:54:11]

Is there a bug for this? or can you describe the issue this CL fixes?

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2014

(or if this is copy, then please adjust similarity)

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.h (right):

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nuke (c)

[mfomitchev, 2014-11-03 19:52:23]

There's no bug. The issue is that there's no default implementation of
ScreenPositionClient, so it's implemented independently in various places in the
code with some code duplication. This CL provides such default implementation
and gets rid of the duplicated code. The trigger for this was that I needed a
ScreenPositionClient implementation for some views tests
(https://codereview.chromium.org/700563002/) and I didn't want to create yet
another implementation of ScreenPositionClient.

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2014/11/03 18:54:11, oshima wrote:
> 2014
> 
> (or if this is copy, then please adjust similarity)

Done.

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.h (right):

https://codereview.chromium.org/686513004/diff/120001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/11/03 18:54:11, oshima wrote:
> nuke (c)

Done.

[oshima, 2014-11-03 21:40:25]

thank you for clarification. lgtm.

https://codereview.chromium.org/686513004/diff/160001/ui/wm/core/default_scre...
File ui/wm/core/default_screen_position_client.cc (right):

https://codereview.chromium.org/686513004/diff/160001/ui/wm/core/default_scre...
ui/wm/core/default_screen_position_client.cc:26: return
gfx::ToFlooredPoint(gfx::ScalePoint(origin_in_pixels, 1 / scale));
i prefer to use float on both side (1.0f)

[mfomitchev, 2014-11-03 21:44:39]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-11-03 21:46:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/686513004/180001

[commit-bot: I haz the power, 2014-11-03 22:36:39]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-11-03 22:39:16]

Patchset 10 (id:??) landed as
https://crrev.com/235ca91afbcc146c1d188b5de0f1c6070d745a8f
Cr-Commit-Position: refs/heads/master@{#302502}