Avoid using ScreenPositionController in ColoredWindowController's ctor

Avoid using ScreenPositionController in ColoredWindowController's ctor

The current code causes a crash if |parent| belongs to 2nd display.  We may want to ensure that |parent| shouldn't have kUsesScreenCoordinatesKey, but this CL rather fixes the caller's code as a quick fix.

BUG=155656
TEST=on goobuntu, run chrome with --aura-host-window-size='800+0-1000x800,800+900-800x600', move a window to 2nd display, click the maximize button, and make sure that the click doesn't cause any crashes.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162183

[sky, 2012-10-15 23:07:52]

https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
File ash/wm/workspace/colored_window_controller.cc (right):

https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
ash/wm/workspace/colored_window_controller.cc:68:
widget->SetBounds(parent->GetBoundsInScreen());
My assumption here is that the parent does not have screen coordinates enabled
so this shouldn't be necessary. When isn't that the case?

[Jun Mukai, 2012-10-15 23:17:55]

https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
File ash/wm/workspace/colored_window_controller.cc (right):

https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
ash/wm/workspace/colored_window_controller.cc:68:
widget->SetBounds(parent->GetBoundsInScreen());
On 2012/10/15 23:07:52, sky wrote:
> My assumption here is that the parent does not have screen coordinates enabled
> so this shouldn't be necessary. When isn't that the case?

I'm not sure if I understand your saying, but actually
NativeWidgetAura::SetBounds() always needs screen coordinates for SetBounds. 
See
http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/widget/native_wid...,
it checks the ScreenPositionClient for its root and looks for the actual display
for the specified (screen) bounds.

If the code here is correct, NativeWidgetAura::SetBounds() should call
ScreenPositionClient in a different way -- or probably this code should look
like:
widget->GetNativeWindow()->SetBounds(parent->bounds()) ?

[sky, 2012-10-15 23:41:32]

On Mon, Oct 15, 2012 at 4:17 PM,  <mukai@chromium.org> wrote:
>
>
https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
> File ash/wm/workspace/colored_window_controller.cc (right):
>
>
https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
> ash/wm/workspace/colored_window_controller.cc:68:
> widget->SetBounds(parent->GetBoundsInScreen());
> On 2012/10/15 23:07:52, sky wrote:
>>
>> My assumption here is that the parent does not have screen coordinates
>
> enabled
>>
>> so this shouldn't be necessary. When isn't that the case?
>
>
> I'm not sure if I understand your saying, but actually
> NativeWidgetAura::SetBounds() always needs screen coordinates for
> SetBounds.  See
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/widget/native_wid...,
> it checks the ScreenPositionClient for its root and looks for the actual
> display for the specified (screen) bounds.

Sorry for not being clear. Here's the code I meant:

void ScreenPositionController::SetBounds(aura::Window* window,
                                         const gfx::Rect& bounds,
                                         const gfx::Display& display) {
  DCHECK_NE(-1, display.id());
  if (!window->parent()->GetProperty(internal::kUsesScreenCoordinatesKey)) {
    window->SetBounds(bounds);
    return;
  }

I was assuming all the parents would have kUsesScreenCoordinatesKey
set to false, but I realize that isn' the case some times.

> If the code here is correct, NativeWidgetAura::SetBounds() should call
> ScreenPositionClient in a different way -- or probably this code should
> look like:
> widget->GetNativeWindow()->SetBounds(parent->bounds()) ?

I like this (with a comment). Make it that.

Thanks!

  -Scott

[Jun Mukai, 2012-10-16 00:10:13]

On 2012/10/15 23:41:32, sky wrote:
> On Mon, Oct 15, 2012 at 4:17 PM,  <mailto:mukai@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
> > File ash/wm/workspace/colored_window_controller.cc (right):
> >
> >
>
https://codereview.chromium.org/11150031/diff/1/ash/wm/workspace/colored_wind...
> > ash/wm/workspace/colored_window_controller.cc:68:
> > widget->SetBounds(parent->GetBoundsInScreen());
> > On 2012/10/15 23:07:52, sky wrote:
> >>
> >> My assumption here is that the parent does not have screen coordinates
> >
> > enabled
> >>
> >> so this shouldn't be necessary. When isn't that the case?
> >
> >
> > I'm not sure if I understand your saying, but actually
> > NativeWidgetAura::SetBounds() always needs screen coordinates for
> > SetBounds.  See
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/widget/native_wid...,
> > it checks the ScreenPositionClient for its root and looks for the actual
> > display for the specified (screen) bounds.
> 
> Sorry for not being clear. Here's the code I meant:
> 
> void ScreenPositionController::SetBounds(aura::Window* window,
>                                          const gfx::Rect& bounds,
>                                          const gfx::Display& display) {
>   DCHECK_NE(-1, display.id());
>   if (!window->parent()->GetProperty(internal::kUsesScreenCoordinatesKey)) {
>     window->SetBounds(bounds);
>     return;
>   }
> 
> I was assuming all the parents would have kUsesScreenCoordinatesKey
> set to false, but I realize that isn' the case some times.
> 
> > If the code here is correct, NativeWidgetAura::SetBounds() should call
> > ScreenPositionClient in a different way -- or probably this code should
> > look like:
> > widget->GetNativeWindow()->SetBounds(parent->bounds()) ?
> 
> I like this (with a comment). Make it that.
> 
> Thanks!
> 
>   -Scott

Oh, I've missed screen_position_controller's part.  And yeah, in that case
kUsesScreenCorrdinatesKey is set.  As far as I quickly browsed the code, parent
would be workspace_manager's contents_view_, which seems default_container, and
kUsesScreenCorrdinatesKey is set.  Also Workspace's explicitly set this property
to true in its ctor, desktop_workspace()->window() would have this property too.


Changed the code to use GetNativeWindow(), and added comments.

[sky, 2012-10-16 15:38:43]

LGTM

https://codereview.chromium.org/11150031/diff/5001/ash/wm/workspace/colored_w...
File ash/wm/workspace/colored_window_controller.cc (right):

https://codereview.chromium.org/11150031/diff/5001/ash/wm/workspace/colored_w...
ash/wm/workspace/colored_window_controller.cc:68: // Sometimes |parent| has
kUsesScreenCoordinatesKey which causes a crash
This is expected in the restore case. So, you can remove the TODO.

[Jun Mukai, 2012-10-16 16:20:28]

https://codereview.chromium.org/11150031/diff/5001/ash/wm/workspace/colored_w...
File ash/wm/workspace/colored_window_controller.cc (right):

https://codereview.chromium.org/11150031/diff/5001/ash/wm/workspace/colored_w...
ash/wm/workspace/colored_window_controller.cc:68: // Sometimes |parent| has
kUsesScreenCoordinatesKey which causes a crash
On 2012/10/16 15:38:44, sky wrote:
> This is expected in the restore case. So, you can remove the TODO.

Done.

[commit-bot: I haz the power, 2012-10-16 16:20:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/11150031/10001

[commit-bot: I haz the power, 2012-10-16 18:30:30]

Change committed as 162183