Fix popup window repositioning in Desktop Chrome.

Fix popup window repositioning in Desktop Chrome.

We need to use Widget::SetBounds in place of Widget::SetSize as the position of the window
may have changed.

BUG=374705
TEST=Covered by views_unittest DesktopAuraWidgetTest.TopLevelOwnedPopupRepositionTest

Committed: https://crrev.com/0393576a8c18cb92aecdc78e417aee3df4ec5d94
Cr-Commit-Position: refs/heads/master@{#313146}

[ananta, 2015-01-23 02:39:26]

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

[sky, 2015-01-23 17:14:57]

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:148: // The position
of the window may have changed. Hence we use SetBounds in
I'm confused. Isn't child_window_ (or window here) a child of the widget?

[sky, 2015-01-23 17:38:51]

Ok, I get why you need this.

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
(right):

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:412:
EXPECT_EQ(popup_window.top_level_widget()->GetWindowBoundsInScreen(),
new_pos should be first (argument order is expected, actual).

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:414:
RunPendingMessages();
Why the RunPendingMessages?

[ananta, 2015-01-23 19:37:12]

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
(right):

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:412:
EXPECT_EQ(popup_window.top_level_widget()->GetWindowBoundsInScreen(),
On 2015/01/23 17:38:50, sky wrote:
> new_pos should be first (argument order is expected, actual).

Done.

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:414:
RunPendingMessages();
On 2015/01/23 17:38:50, sky wrote:
> Why the RunPendingMessages?

Stupid cut paste. Removed the first RunPendingMessages from this test and the
TopLevelOwnedPopupResizeTest test. We need the second one to ensure that the
popup window is destroyed before we enter the dtor of the
DesktopAuraTopLevelWindowTest class

[sky, 2015-01-23 20:29:21]

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
(right):

https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:414:
RunPendingMessages();
On 2015/01/23 19:37:11, ananta wrote:
> On 2015/01/23 17:38:50, sky wrote:
> > Why the RunPendingMessages?
> 
> Stupid cut paste. Removed the first RunPendingMessages from this test and the
> TopLevelOwnedPopupResizeTest test. We need the second one to ensure that the
> popup window is destroyed before we enter the dtor of the
> DesktopAuraTopLevelWindowTest class

Why do we need to wait for the destruction?

[ananta, 2015-01-23 20:33:06]

On 2015/01/23 20:29:21, sky wrote:
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:414:
> RunPendingMessages();
> On 2015/01/23 19:37:11, ananta wrote:
> > On 2015/01/23 17:38:50, sky wrote:
> > > Why the RunPendingMessages?
> > 
> > Stupid cut paste. Removed the first RunPendingMessages from this test and
the
> > TopLevelOwnedPopupResizeTest test. We need the second one to ensure that the
> > popup window is destroyed before we enter the dtor of the
> > DesktopAuraTopLevelWindowTest class
> 
> Why do we need to wait for the destruction?

The DesktopAuraTopLevelWindowTest class removes the observer in the
OnWindowDestroying callback. If we don't wait
then the dtor of the WindowObserver fires a DCHECK that it is still being
observed.

[ananta, 2015-01-24 00:58:57]

On 2015/01/23 20:33:06, ananta wrote:
> On 2015/01/23 20:29:21, sky wrote:
> >
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> > File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> > ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:414:
> > RunPendingMessages();
> > On 2015/01/23 19:37:11, ananta wrote:
> > > On 2015/01/23 17:38:50, sky wrote:
> > > > Why the RunPendingMessages?
> > > 
> > > Stupid cut paste. Removed the first RunPendingMessages from this test and
> the
> > > TopLevelOwnedPopupResizeTest test. We need the second one to ensure that
the
> > > popup window is destroyed before we enter the dtor of the
> > > DesktopAuraTopLevelWindowTest class
> > 
> > Why do we need to wait for the destruction?
> 
> The DesktopAuraTopLevelWindowTest class removes the observer in the
> OnWindowDestroying callback. If we don't wait
> then the dtor of the WindowObserver fires a DCHECK that it is still being
> observed.

Added support for optional synchronous mode in the DesktopAuraTopLevelWindowTest
class. This is in the form
of a flag use_async_mode_ which is set to true by default. The two tests
TopLevelOwnedPopupRepositionTest and TopLevelOwnedPopupResizeTest don't need
async support. They set it to false and we don't need to call RunPendingMessages
in these tests

[ananta, 2015-01-24 01:08:27]

On 2015/01/24 00:58:57, ananta wrote:
> On 2015/01/23 20:33:06, ananta wrote:
> > On 2015/01/23 20:29:21, sky wrote:
> > >
> >
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> > > File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> > > ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:414:
> > > RunPendingMessages();
> > > On 2015/01/23 19:37:11, ananta wrote:
> > > > On 2015/01/23 17:38:50, sky wrote:
> > > > > Why the RunPendingMessages?
> > > > 
> > > > Stupid cut paste. Removed the first RunPendingMessages from this test
and
> > the
> > > > TopLevelOwnedPopupResizeTest test. We need the second one to ensure that
> the
> > > > popup window is destroyed before we enter the dtor of the
> > > > DesktopAuraTopLevelWindowTest class
> > > 
> > > Why do we need to wait for the destruction?
> > 
> > The DesktopAuraTopLevelWindowTest class removes the observer in the
> > OnWindowDestroying callback. If we don't wait
> > then the dtor of the WindowObserver fires a DCHECK that it is still being
> > observed.
> 
> Added support for optional synchronous mode in the
DesktopAuraTopLevelWindowTest
> class. This is in the form
> of a flag use_async_mode_ which is set to true by default. The two tests
> TopLevelOwnedPopupRepositionTest and TopLevelOwnedPopupResizeTest don't need
> async support. They set it to false and we don't need to call
RunPendingMessages
> in these tests

The top_level_widget_ in the general case is destroyed by the
DesktopNativeWidgetTopLevelHandler class

[ananta, 2015-01-24 04:00:51]

On 2015/01/24 01:08:27, ananta wrote:
> On 2015/01/24 00:58:57, ananta wrote:
> > On 2015/01/23 20:33:06, ananta wrote:
> > > On 2015/01/23 20:29:21, sky wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> > > > File ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/871813003/diff/20001/ui/views/widget/desktop_...
> > > > ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc:414:
> > > > RunPendingMessages();
> > > > On 2015/01/23 19:37:11, ananta wrote:
> > > > > On 2015/01/23 17:38:50, sky wrote:
> > > > > > Why the RunPendingMessages?
> > > > > 
> > > > > Stupid cut paste. Removed the first RunPendingMessages from this test
> and
> > > the
> > > > > TopLevelOwnedPopupResizeTest test. We need the second one to ensure
that
> > the
> > > > > popup window is destroyed before we enter the dtor of the
> > > > > DesktopAuraTopLevelWindowTest class
> > > > 
> > > > Why do we need to wait for the destruction?
> > > 
> > > The DesktopAuraTopLevelWindowTest class removes the observer in the
> > > OnWindowDestroying callback. If we don't wait
> > > then the dtor of the WindowObserver fires a DCHECK that it is still being
> > > observed.
> > 
> > Added support for optional synchronous mode in the
> DesktopAuraTopLevelWindowTest
> > class. This is in the form
> > of a flag use_async_mode_ which is set to true by default. The two tests
> > TopLevelOwnedPopupRepositionTest and TopLevelOwnedPopupResizeTest don't need
> > async support. They set it to false and we don't need to call
> RunPendingMessages
> > in these tests
> 
> The top_level_widget_ in the general case is destroyed by the
> DesktopNativeWidgetTopLevelHandler class

Please hold off on the review. I will refactor the test classes and upload a new
patch.

[sky, 2015-01-24 18:51:47]

While I definitely want the test classes cleaned up, that shouldn't hold up your
landing the fix. I'm LGTM ing. A separate clean up pass of the test code would
be awesome!

[ananta, 2015-01-26 21:11:40]

On 2015/01/24 18:51:47, sky wrote:
> While I definitely want the test classes cleaned up, that shouldn't hold up
your
> landing the fix. I'm LGTM ing. A separate clean up pass of the test code would
> be awesome!

ok thanks. Will upload a new patch for the tests.

[ananta, 2015-01-26 21:11:52]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-01-26 21:12:18]

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

[commit-bot: I haz the power, 2015-01-26 21:52:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-26 21:54:18]

Patchset 5 (id:??) landed as
https://crrev.com/0393576a8c18cb92aecdc78e417aee3df4ec5d94
Cr-Commit-Position: refs/heads/master@{#313146}