Reenable Tabdragging tests failing because of IsWindowPositionManaged()

Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*

Committed: https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c
Cr-Commit-Position: refs/heads/master@{#435638}

[afakhry, 2016-11-11 01:35:47]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 01:36:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 03:00:17]

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

[commit-bot: I haz the power, 2016-11-11 03:00:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2016-11-11 21:11:17]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 21:11:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-11-11 21:11:58]

Description was changed from

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

BUG=626761
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

to

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

[commit-bot: I haz the power, 2016-11-11 22:50:17]

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

[commit-bot: I haz the power, 2016-11-11 22:50:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2016-11-16 01:35:55]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 01:36:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-11-16 02:05:33]

Patchset #6 (id:100001) has been deleted

[afakhry, 2016-11-16 02:05:46]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 02:06:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 03:45:01]

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

[commit-bot: I haz the power, 2016-11-16 03:45:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[afakhry, 2016-11-16 18:18:22]

afakhry@chromium.org changed reviewers:
+ pkasting@chromium.org

[afakhry, 2016-11-16 18:18:23]

pkasting, please review. Thank you!

[Peter Kasting, 2016-11-17 01:32:49]

I don't really know the tab_drag_controller.cc code, so I don't actually know
what the change there does.  You should probably have sky review that.

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

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1118: move_loop_widget_ =
NULL;
Hmm.  This block of code looks similar to code in the destructor (which calls
RemoveObserver(), but only if |added_observer_to_move_loop_widget_| -- should we
be checking that elsewhere too?), as well as DragBrowserToNewTabStrip() (which
does all these things, but has some of them in an earlier #if ASH block --
should they indeed be ifdefed?).

This makes me wonder if these sorts of things should be pulled into some kind of
common "reset move loop widget" function that does the right magic voodoo.

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:814:
base::Unretained(this)));
Optional nit: We can save the repeated ugly PostTask() by doing something like
this:

  if (!CheckMaximized()) {  // Relies on |quit_| not being set yet
    quit_ = run_loop.QuitClosure();
    run_loop.Run();
  }

  ...

  bool CheckMaximized() {
    const bool maximized = window_->IsMaximized();
    if (maximized) {
      if (!quit_.is_null())
        quit_.Run();
    } else {
      base::MessageLoop::current()->task_runner()->PostTask(...
    }
    return maximized;
  }

This only breaks if Wait() can be called repeatedly for the same object.  To
avoid this, you could eliminate Wait() and move its code into the constructor
body, so callers cannot wait more than once.  (Or you could fix it, but that's
not actually necessary today.)

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:816:
CHECK(window_->IsMaximized());
Nit: CHECK in test code can causes tests to shut down uncleanly, which can leave
the bot in a bad state.  Generally something like ASSERT() is preferred, though
in this case I'd simply omit this line entirely, since you EXPECT it in the
callers already.

[afakhry, 2016-11-17 22:50:44]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[afakhry, 2016-11-17 22:51:04]

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

[afakhry, 2016-11-17 22:51:05]

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

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1118: move_loop_widget_ =
NULL;
On 2016/11/17 01:32:49, Peter Kasting wrote:
> Hmm.  This block of code looks similar to code in the destructor (which calls
> RemoveObserver(), but only if |added_observer_to_move_loop_widget_| -- should
we
> be checking that elsewhere too?), as well as DragBrowserToNewTabStrip() (which
> does all these things, but has some of them in an earlier #if ASH block --
> should they indeed be ifdefed?).
> 
> This makes me wonder if these sorts of things should be pulled into some kind
of
> common "reset move loop widget" function that does the right magic voodoo.

I'm also not very familiar with this code. So I agree we should involve +sky@ to
take a look at this.

I like your suggestion of having one function that does the magic. But I'm not
sure if it can be used everywhere instead of the current code that seemingly
does similar things (i.e. Not all of them do exactly the same thing. For
example, some depends on |added_observer_to_move_loop_widget_| some not. Some
set the WindowPositionManaged to true, some not. That #if ASH you mentioned). So
I think it would be better to defer to sky@ with this question.

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:814:
base::Unretained(this)));
On 2016/11/17 01:32:49, Peter Kasting wrote:
> Optional nit: We can save the repeated ugly PostTask() by doing something like
> this:
> 
>   if (!CheckMaximized()) {  // Relies on |quit_| not being set yet
>     quit_ = run_loop.QuitClosure();
>     run_loop.Run();
>   }
> 
>   ...
> 
>   bool CheckMaximized() {
>     const bool maximized = window_->IsMaximized();
>     if (maximized) {
>       if (!quit_.is_null())
>         quit_.Run();
>     } else {
>       base::MessageLoop::current()->task_runner()->PostTask(...
>     }
>     return maximized;
>   }
> 
> This only breaks if Wait() can be called repeatedly for the same object.  To
> avoid this, you could eliminate Wait() and move its code into the constructor
> body, so callers cannot wait more than once.  (Or you could fix it, but that's
> not actually necessary today.)

This is a great suggestion! Thanks! 
Done. 
I'm not quite clear on why calling Wait() on the same object breaks it? Can you
please clarify?

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:816:
CHECK(window_->IsMaximized());
On 2016/11/17 01:32:49, Peter Kasting wrote:
> Nit: CHECK in test code can causes tests to shut down uncleanly, which can
leave
> the bot in a bad state.  Generally something like ASSERT() is preferred,
though
> in this case I'd simply omit this line entirely, since you EXPECT it in the
> callers already.

Done. Removed.

[commit-bot: I haz the power, 2016-11-17 22:51:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2016-11-17 23:05:24]

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:814:
base::Unretained(this)));
On 2016/11/17 22:51:05, afakhry wrote:
> On 2016/11/17 01:32:49, Peter Kasting wrote:
> > Optional nit: We can save the repeated ugly PostTask() by doing something
like
> > this:
> > 
> >   if (!CheckMaximized()) {  // Relies on |quit_| not being set yet
> >     quit_ = run_loop.QuitClosure();
> >     run_loop.Run();
> >   }
> > 
> >   ...
> > 
> >   bool CheckMaximized() {
> >     const bool maximized = window_->IsMaximized();
> >     if (maximized) {
> >       if (!quit_.is_null())
> >         quit_.Run();
> >     } else {
> >       base::MessageLoop::current()->task_runner()->PostTask(...
> >     }
> >     return maximized;
> >   }
> > 
> > This only breaks if Wait() can be called repeatedly for the same object.  To
> > avoid this, you could eliminate Wait() and move its code into the
constructor
> > body, so callers cannot wait more than once.  (Or you could fix it, but
that's
> > not actually necessary today.)
> 
> This is a great suggestion! Thanks! 
> Done. 
> I'm not quite clear on why calling Wait() on the same object breaks it? Can
you
> please clarify?

After the first Wait(), |quit_| will remain initialized.  If Wait() is then
called a second time, when the window is already maximized, CheckMaximized()
will call quit_.Run() when the message loop is not running.  This seems bad
(reading the code, I think this causes the loop to immediately quit the next
time someone tries to run it).

[commit-bot: I haz the power, 2016-11-18 00:55:15]

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

[commit-bot: I haz the power, 2016-11-18 00:55:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[afakhry, 2016-11-18 16:45:36]

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/2494713003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:814:
base::Unretained(this)));
On 2016/11/17 23:05:24, Peter Kasting wrote:
> On 2016/11/17 22:51:05, afakhry wrote:
> > On 2016/11/17 01:32:49, Peter Kasting wrote:
> > > Optional nit: We can save the repeated ugly PostTask() by doing something
> like
> > > this:
> > > 
> > >   if (!CheckMaximized()) {  // Relies on |quit_| not being set yet
> > >     quit_ = run_loop.QuitClosure();
> > >     run_loop.Run();
> > >   }
> > > 
> > >   ...
> > > 
> > >   bool CheckMaximized() {
> > >     const bool maximized = window_->IsMaximized();
> > >     if (maximized) {
> > >       if (!quit_.is_null())
> > >         quit_.Run();
> > >     } else {
> > >       base::MessageLoop::current()->task_runner()->PostTask(...
> > >     }
> > >     return maximized;
> > >   }
> > > 
> > > This only breaks if Wait() can be called repeatedly for the same object. 
To
> > > avoid this, you could eliminate Wait() and move its code into the
> constructor
> > > body, so callers cannot wait more than once.  (Or you could fix it, but
> that's
> > > not actually necessary today.)
> > 
> > This is a great suggestion! Thanks! 
> > Done. 
> > I'm not quite clear on why calling Wait() on the same object breaks it? Can
> you
> > please clarify?
> 
> After the first Wait(), |quit_| will remain initialized.  If Wait() is then
> called a second time, when the window is already maximized, CheckMaximized()
> will call quit_.Run() when the message loop is not running.  This seems bad
> (reading the code, I think this causes the loop to immediately quit the next
> time someone tries to run it).

Yes, I also thought that's what you meant. I attempted to fix this in the new
patch by:

    if (!quit_.is_null())
      base::ResetAndReturn(&quit_).Run();

[Peter Kasting, 2016-11-18 19:05:01]

The unittest LGTM.  I'll leave sky to answer the questions we had about
refactoring the implementation.

https://codereview.chromium.org/2494713003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/2494713003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:832:
base::ResetAndReturn(&quit_).Run();
Nit: Consider commenting about why ResetAndReturn() is useful here.

[sky, 2016-11-19 14:55:03]

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

https://codereview.chromium.org/2494713003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1117:
SetWindowPositionManaged(move_loop_widget_->GetNativeWindow(), true);
Can you clarify why tab dragging code needs to explicitly call
SetWindowPositionManaged (anywhere in ths file)? It seems like this code should
be entirely isolated in ash. Ash knows the drag is happening by way of
RunMoveLoop() and if ash needs to disable anything at this point ash should key
of RunMoveLoop.

[afakhry, 2016-11-22 18:15:05]

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

https://codereview.chromium.org/2494713003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1117:
SetWindowPositionManaged(move_loop_widget_->GetNativeWindow(), true);
On 2016/11/19 14:55:03, sky wrote:
> Can you clarify why tab dragging code needs to explicitly call
> SetWindowPositionManaged (anywhere in ths file)? It seems like this code
should
> be entirely isolated in ash. Ash knows the drag is happening by way of
> RunMoveLoop() and if ash needs to disable anything at this point ash should
key
> of RunMoveLoop.

I was hoping that you would answer this question as I'm not familiar with this
code. I don't know why SetWindowPositionManaged() is called from several places
in this file. As you said, SetWindowPositionManaged() seems ASH specific ...
it's a no-op for non-ASH:
(https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co...).
The question is, can we unify things? There are many places where we remove the
observer from |move_loop_widget_| and clear it, but they're not all exactly the
same. Sometimes we do that only when |added_observer_to_move_loop_widget_| is
true, sometimes we call SetWindowPositionManaged(), and sometimes only #if
USE_ASH. See:
https://cs.chromium.org/search/?q=f:+tab_drag_controller.cc+move_loop_widget_...

[sky, 2016-11-22 20:56:17]

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

[sky, 2016-11-22 20:58:01]

+skuhne
Stefan added the code here: https://chromiumcodereview.appspot.com/11085053 .
Stefan, the question is can we move manipulating the window position managed
property from TabDragController into ash when ash starts the move loop?

[Mr4D (OOO till 08-26), 2016-11-23 16:04:48]

From glancing at the code I would say yes.

On Tue, Nov 22, 2016 at 12:58 PM, <sky@chromium.org> wrote:

> +skuhne
> Stefan added the code here: https://chromiumcodereview.
> appspot.com/11085053 .
> Stefan, the question is can we move manipulating the window position
> managed
> property from TabDragController into ash when ash starts the move loop?
>
> https://codereview.chromium.org/2494713003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[afakhry, 2016-11-23 23:53:19]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 23:54:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 01:56:32]

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

[commit-bot: I haz the power, 2016-11-24 01:56:33]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[afakhry, 2016-11-28 17:09:13]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 17:09:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-11-28 17:11:08]

sky and Mr4D, PTAL. Thanks!

[commit-bot: I haz the power, 2016-11-28 18:24:32]

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

[commit-bot: I haz the power, 2016-11-28 18:24:33]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-28 18:30:43]

https://codereview.chromium.org/2494713003/diff/160001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/2494713003/diff/160001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler.cc:96:
window_state->set_window_position_managed(false);
How about adding test coverage of this to ash?

https://codereview.chromium.org/2494713003/diff/160001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler.cc:100:
window_state->set_window_position_managed(window_position_managed);
I think you should assume that if 'this' has been deleted the window_state is
also no longer valid. In other words, move this after line 102.

[afakhry, 2016-11-28 19:17:55]

Description was changed from

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

to

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves the setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

[afakhry, 2016-11-28 19:18:22]

Description was changed from

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves the setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

to

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

[afakhry, 2016-11-28 19:19:28]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 19:20:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-11-28 19:22:29]

https://codereview.chromium.org/2494713003/diff/160001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/2494713003/diff/160001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler.cc:96:
window_state->set_window_position_managed(false);
On 2016/11/28 18:30:42, sky wrote:
> How about adding test coverage of this to ash?

Done. Please check if what I added is enough or more is needed.

https://codereview.chromium.org/2494713003/diff/160001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler.cc:100:
window_state->set_window_position_managed(window_position_managed);
On 2016/11/28 18:30:42, sky wrote:
> I think you should assume that if 'this' has been deleted the window_state is
> also no longer valid. In other words, move this after line 102.

Done.

[Mr4D (OOO till 08-26), 2016-11-28 19:32:56]

Please see comment. beside that lgtm

https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler_unittest.cc (right):

https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler_unittest.cc:83:
window_state->window_position_managed();
I might be mistaken, but is there any case where this returns true? You might
want to add a unit test which adds a case where this could be tested.

[commit-bot: I haz the power, 2016-11-28 21:28:35]

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

[commit-bot: I haz the power, 2016-11-28 21:28:36]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-28 21:31:50]

https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:823:
base::MessageLoop::current()->task_runner()->PostTask(
It's generally preferable to install a listener that is notified when the change
happens rather than polling. Is there a way to observer the change?

[afakhry, 2016-11-28 22:31:37]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 22:33:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2016-11-28 22:54:25]

https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
File ash/wm/toplevel_window_event_handler_unittest.cc (right):

https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
ash/wm/toplevel_window_event_handler_unittest.cc:83:
window_state->window_position_managed();
On 2016/11/28 19:32:56, Mr4D wrote:
> I might be mistaken, but is there any case where this returns true? You might
> want to add a unit test which adds a case where this could be tested.

Thanks for this note. Actually this test is not very accurate. I need to verify
that window position auto management is disabled inside the move run loop.
Please take a look at the new test I added.

https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:823:
base::MessageLoop::current()->task_runner()->PostTask(
On 2016/11/28 21:31:50, sky wrote:
> It's generally preferable to install a listener that is notified when the
change
> happens rather than polling. Is there a way to observer the change?

Yes, that's the first thing I tried. I wasn't however able to find a
platform-independent way to observe changes in this property.
There is for example WMStateWaiter
(https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...),
but it's only for X11.

[sky, 2016-11-28 23:33:36]

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

[sky, 2016-11-28 23:34:45]

On 2016/11/28 22:54:25, afakhry wrote:
>
https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
> File ash/wm/toplevel_window_event_handler_unittest.cc (right):
> 
>
https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
> ash/wm/toplevel_window_event_handler_unittest.cc:83:
> window_state->window_position_managed();
> On 2016/11/28 19:32:56, Mr4D wrote:
> > I might be mistaken, but is there any case where this returns true? You
might
> > want to add a unit test which adds a case where this could be tested.
> 
> Thanks for this note. Actually this test is not very accurate. I need to
verify
> that window position auto management is disabled inside the move run loop.
> Please take a look at the new test I added.
> 
>
https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
> File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
> (right):
> 
>
https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
> chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:823:
> base::MessageLoop::current()->task_runner()->PostTask(
> On 2016/11/28 21:31:50, sky wrote:
> > It's generally preferable to install a listener that is notified when the
> change
> > happens rather than polling. Is there a way to observer the change?
> 
> Yes, that's the first thing I tried. I wasn't however able to find a
> platform-independent way to observe changes in this property.
> There is for example WMStateWaiter
>
(https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...),
> but it's only for X11.

Is there something that is called after the minimize that makes it way to
DesktopNativeWidgetAura and then widget? Perhaps the visibility or something
like?
+erg in case he has ideas. We're looking for a way to observe when the X11
window has become restored from a minimize.

[Elliot Glaysher, 2016-11-28 23:43:19]

On 2016/11/28 23:34:45, sky wrote:
> On 2016/11/28 22:54:25, afakhry wrote:
> >
>
https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
> > File ash/wm/toplevel_window_event_handler_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
> > ash/wm/toplevel_window_event_handler_unittest.cc:83:
> > window_state->window_position_managed();
> > On 2016/11/28 19:32:56, Mr4D wrote:
> > > I might be mistaken, but is there any case where this returns true? You
> might
> > > want to add a unit test which adds a case where this could be tested.
> > 
> > Thanks for this note. Actually this test is not very accurate. I need to
> verify
> > that window position auto management is disabled inside the move run loop.
> > Please take a look at the new test I added.
> > 
> >
>
https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
> > File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
> > chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:823:
> > base::MessageLoop::current()->task_runner()->PostTask(
> > On 2016/11/28 21:31:50, sky wrote:
> > > It's generally preferable to install a listener that is notified when the
> > change
> > > happens rather than polling. Is there a way to observer the change?
> > 
> > Yes, that's the first thing I tried. I wasn't however able to find a
> > platform-independent way to observe changes in this property.
> > There is for example WMStateWaiter
> >
>
(https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...),
> > but it's only for X11.
> 
> Is there something that is called after the minimize that makes it way to
> DesktopNativeWidgetAura and then widget? Perhaps the visibility or something
> like?
> +erg in case he has ideas. We're looking for a way to observe when the X11
> window has become restored from a minimize.

Most desktop environments implement the _NET_WM standard. There is a property on
windows called "_NET_WM_STATE" and we listen for x11 events relating to that in
DesktopWindowTreeHostX11::DispatchEvent(). (See "case PropertyNotify:".)
DWTHX11::OnWMStateUpdated() takes that value and theoretically propagates it,
but it doesn't look like there's an observer that gets dispatched.

[commit-bot: I haz the power, 2016-11-29 00:27:50]

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

[commit-bot: I haz the power, 2016-11-29 00:27:52]

Dry run: This issue passed the CQ dry run.

[afakhry, 2016-11-29 00:35:04]

On 2016/11/28 23:43:19, Elliot Glaysher wrote:
> On 2016/11/28 23:34:45, sky wrote:
> > On 2016/11/28 22:54:25, afakhry wrote:
> > >
> >
>
https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
> > > File ash/wm/toplevel_window_event_handler_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2494713003/diff/180001/ash/wm/toplevel_window...
> > > ash/wm/toplevel_window_event_handler_unittest.cc:83:
> > > window_state->window_position_managed();
> > > On 2016/11/28 19:32:56, Mr4D wrote:
> > > > I might be mistaken, but is there any case where this returns true? You
> > might
> > > > want to add a unit test which adds a case where this could be tested.
> > > 
> > > Thanks for this note. Actually this test is not very accurate. I need to
> > verify
> > > that window position auto management is disabled inside the move run loop.
> > > Please take a look at the new test I added.
> > > 
> > >
> >
>
https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
> > > File
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2494713003/diff/180001/chrome/browser/ui/view...
> > >
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:823:
> > > base::MessageLoop::current()->task_runner()->PostTask(
> > > On 2016/11/28 21:31:50, sky wrote:
> > > > It's generally preferable to install a listener that is notified when
the
> > > change
> > > > happens rather than polling. Is there a way to observer the change?
> > > 
> > > Yes, that's the first thing I tried. I wasn't however able to find a
> > > platform-independent way to observe changes in this property.
> > > There is for example WMStateWaiter
> > >
> >
>
(https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...),
> > > but it's only for X11.
> > 
> > Is there something that is called after the minimize that makes it way to
> > DesktopNativeWidgetAura and then widget? Perhaps the visibility or something
> > like?
> > +erg in case he has ideas. We're looking for a way to observe when the X11
> > window has become restored from a minimize.
> 
> Most desktop environments implement the _NET_WM standard. There is a property
on
> windows called "_NET_WM_STATE" and we listen for x11 events relating to that
in
> DesktopWindowTreeHostX11::DispatchEvent(). (See "case PropertyNotify:".)
> DWTHX11::OnWMStateUpdated() takes that value and theoretically propagates it,
> but it doesn't look like there's an observer that gets dispatched.

Just to clarify, we're looking for a way to observe when the window becomes
maximized on all platforms not just X11. For X11, we have the WMStateWaiter
mentioned earlier
(https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...).
We also have WindowStateObserver::OnPostWindowStateTypeChange(), but that's only
ash.

[afakhry, 2016-11-30 20:49:16]

Friendly ping.

[sky, 2016-11-30 22:03:57]

To be clear, you're pining erg here, right?

On Wed, Nov 30, 2016 at 12:49 PM,  <afakhry@chromium.org> wrote:
> Friendly ping.
>
> https://codereview.chromium.org/2494713003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2016-11-30 22:15:00]

On 2016/11/30 22:03:57, sky wrote:
> To be clear, you're pining erg here, right?
> 
> On Wed, Nov 30, 2016 at 12:49 PM,  <mailto:afakhry@chromium.org> wrote:
> > Friendly ping.
> >
> > https://codereview.chromium.org/2494713003/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

(It was not clear that I was being pinged; I'm not sure what I'm supposed to be
responding too.)

[afakhry, 2016-11-30 23:40:34]

On 2016/11/30 22:15:00, Elliot Glaysher wrote:
> On 2016/11/30 22:03:57, sky wrote:
> > To be clear, you're pining erg here, right?
> > 
> > On Wed, Nov 30, 2016 at 12:49 PM,  <mailto:afakhry@chromium.org> wrote:
> > > Friendly ping.
> > >
> > > https://codereview.chromium.org/2494713003/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> (It was not clear that I was being pinged; I'm not sure what I'm supposed to
be
> responding too.)

Sorry. To clarify, I just need feedback on the new test I added in
toplevel_window_event_handler_unittest.cc, as well as a suggestion on how to
replace polling for a window getting maximized with a platform-independent
observer that works on ash, Windows, and cocoa. I found the X11-specific
WMStateWaiter
[https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc?q=WMStateWaiter&sq=package:chromium&l=48&dr=CSs],
and the ash-specific WindowStateObserver
[https://cs.chromium.org/chromium/src/ash/common/wm/window_state_observer.h?dr=CSs&q=WindowStateObserver::OnPostWindowStateTypeChange&sq=package:chromium&l=35].
Is there something else that works in all platforms?

[Elliot Glaysher, 2016-11-30 23:59:28]

On 2016/11/30 23:40:34, afakhry wrote:
> On 2016/11/30 22:15:00, Elliot Glaysher wrote:
> > On 2016/11/30 22:03:57, sky wrote:
> > > To be clear, you're pining erg here, right?
> > > 
> > > On Wed, Nov 30, 2016 at 12:49 PM,  <mailto:afakhry@chromium.org> wrote:
> > > > Friendly ping.
> > > >
> > > > https://codereview.chromium.org/2494713003/
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > 
> > 
> > (It was not clear that I was being pinged; I'm not sure what I'm supposed to
> be
> > responding too.)
> 
> Sorry. To clarify, I just need feedback on the new test I added in
> toplevel_window_event_handler_unittest.cc, as well as a suggestion on how to
> replace polling for a window getting maximized with a platform-independent
> observer that works on ash, Windows, and cocoa. I found the X11-specific
> WMStateWaiter
>
[https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc?q=WMStateWaiter&sq=package:chromium&l=48&dr=CSs],
> and the ash-specific WindowStateObserver
>
[https://cs.chromium.org/chromium/src/ash/common/wm/window_state_observer.h?dr=CSs&q=WindowStateObserver::OnPostWindowStateTypeChange&sq=package:chromium&l=35].
> Is there something else that works in all platforms?

I'm unaware of anyway to do this in a cross-platform way. (As you've found,
WMStateWaiter will do what you want on X11, but only on X11). And the code where
that's handled (DWTHX11::OnWMStateUpdated()) isn't sending an observer message
when these state changes. You could add an observer interface, but to do this in
a cross platform way would require adding it to the rest of the window tree
hosts.

As for the WindowPositionAutoManagement test, assuming you've spammed the test
over and over again to check for flakiness, I don't see anything obviously wrong
with it. (I'm a bit nervous about the spinning up a nested runloop multiple
times in a test, but it probably doesn't matter since that should only run in an
ash environment. Just do the repeated test to check for flakiness; I'm probably
just over pattern matching.)

[afakhry, 2016-12-01 02:24:56]

On 2016/11/30 23:59:28, Elliot Glaysher wrote:
> On 2016/11/30 23:40:34, afakhry wrote:
> > On 2016/11/30 22:15:00, Elliot Glaysher wrote:
> > > On 2016/11/30 22:03:57, sky wrote:
> > > > To be clear, you're pining erg here, right?
> > > > 
> > > > On Wed, Nov 30, 2016 at 12:49 PM,  <mailto:afakhry@chromium.org> wrote:
> > > > > Friendly ping.
> > > > >
> > > > > https://codereview.chromium.org/2494713003/
> > > > 
> > > > -- 
> > > > You received this message because you are subscribed to the Google
Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > 
> > > 
> > > (It was not clear that I was being pinged; I'm not sure what I'm supposed
to
> > be
> > > responding too.)
> > 
> > Sorry. To clarify, I just need feedback on the new test I added in
> > toplevel_window_event_handler_unittest.cc, as well as a suggestion on how to
> > replace polling for a window getting maximized with a platform-independent
> > observer that works on ash, Windows, and cocoa. I found the X11-specific
> > WMStateWaiter
> >
>
[https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc?q=WMStateWaiter&sq=package:chromium&l=48&dr=CSs],
> > and the ash-specific WindowStateObserver
> >
>
[https://cs.chromium.org/chromium/src/ash/common/wm/window_state_observer.h?dr=CSs&q=WindowStateObserver::OnPostWindowStateTypeChange&sq=package:chromium&l=35].
> > Is there something else that works in all platforms?
> 
> I'm unaware of anyway to do this in a cross-platform way. (As you've found,
> WMStateWaiter will do what you want on X11, but only on X11). And the code
where
> that's handled (DWTHX11::OnWMStateUpdated()) isn't sending an observer message
> when these state changes. You could add an observer interface, but to do this
in
> a cross platform way would require adding it to the rest of the window tree
> hosts.
> 
> As for the WindowPositionAutoManagement test, assuming you've spammed the test
> over and over again to check for flakiness, I don't see anything obviously
wrong
> with it. (I'm a bit nervous about the spinning up a nested runloop multiple
> times in a test, but it probably doesn't matter since that should only run in
an
> ash environment. Just do the repeated test to check for flakiness; I'm
probably
> just over pattern matching.)

Thanks for your feedback. The MaximizedBrowserWindowWaiter that polls the
window's IsMaximized() state is only used in the file
tab_drag_controller_interactive_uitest.cc. We just needed a way to wait for the
window to become maximized as it happens asynchronously. The other newly added
test WindowPositionAutoManagement is a separate issue testing that during the
drag, the window's auto position management is disabled and restored to what it
was previously when the drag completes.

@sky, what do you think the way forward should be? Thanks!

[sky, 2016-12-01 15:53:07]

Ok, LGTM

[afakhry, 2016-12-01 16:53:47]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-12-01 16:53:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2494713003/#ps200001
(title: "Adding a better ash test.")

[commit-bot: I haz the power, 2016-12-01 16:54:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 17:33:44]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480611227364710,
"parent_rev": "d70d7bdcb3ad783076cb32b8147f7d748c716123", "commit_rev":
"4542c6b48c558e881770d487e0b765834dc87c23"}

[commit-bot: I haz the power, 2016-12-01 17:34:08]

Description was changed from

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

to

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

[commit-bot: I haz the power, 2016-12-01 17:34:10]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-12-01 17:37:53]

Description was changed from

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*
==========

to

==========
Reenable Tabdragging tests failing because of IsWindowPositionManaged()

It used to be exiting the move loop and clearing the move_loop_widget_ before
EndDrag() had a chance to set the window position to manageable again.

This CL adds a maximized browser window waiter, and moves setting the window
position managed logic to ash.

BUG=626761, 331924
TEST=interactive_ui_tests --gtest_filter=TabDragging*

Committed: https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c
Cr-Commit-Position: refs/heads/master@{#435638}
==========

[commit-bot: I haz the power, 2016-12-01 17:37:55]

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

[Ken Rockot(use gerrit already), 2016-12-01 19:48:25]

On 2016/12/01 at 17:37:55, commit-bot wrote:
> Patchset 10 (id:??) landed as
https://crrev.com/c813d7634e5c6ba6c808e326778b0cdb4f6e773c
> Cr-Commit-Position: refs/heads/master@{#435638}

Any chance this is causing ash_unittests failures?

https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...

[findit-for-me, 2016-12-01 20:31:45]

FYI: Findit identified this CL at revision 435638 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[afakhry, 2016-12-01 20:43:00]

On 2016/12/01 20:31:45, findit-for-me wrote:
> FYI: Findit identified this CL at revision 435638 as the culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

I will land a follow-up fix to track the window deletion. Feel free to revert if
it's urgent.

[afakhry, 2016-12-01 20:58:55]

On 2016/12/01 20:43:00, afakhry wrote:
> On 2016/12/01 20:31:45, findit-for-me wrote:
> > FYI: Findit identified this CL at revision 435638 as the culprit for
> > failures in the build cycles as shown on:
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> 
> I will land a follow-up fix to track the window deletion. Feel free to revert
if
> it's urgent.

CL with fix is here: https://codereview.chromium.org/2540143004

[Ken Rockot(use gerrit already), 2016-12-01 22:06:41]

On 2016/12/01 at 20:58:55, afakhry wrote:
> On 2016/12/01 20:43:00, afakhry wrote:
> > On 2016/12/01 20:31:45, findit-for-me wrote:
> > > FYI: Findit identified this CL at revision 435638 as the culprit for
> > > failures in the build cycles as shown on:
> > >
> >
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> > 
> > I will land a follow-up fix to track the window deletion. Feel free to
revert if
> > it's urgent.
> 
> CL with fix is here: https://codereview.chromium.org/2540143004

If you can't land the fix very soon, can we please revert this? That is
typically how we deal with offending CLs.

[Ken Rockot(use gerrit already), 2016-12-01 22:38:44]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/2550533002/ by rockot@chromium.org.

The reason for reverting is: Causing failures in ash_unittests. See CL comments
for details. If you have a fix, please reland with the fix applied..