[ash] Don't remove an item when associated window is dragged

[ash] Don't remove an item when associated window is dragged

LauncherItem adding/removing is repeated when a Window is dragged because
it is reparented to DockedContainer during the dragging.
ShelfWindowWatcher should prevent this removing when window is dragging.

R=sky@chromium.org
BUG=NONE
TEST=ash_unittests --gtest_filter=ShelfWindowWatcherTest*.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243970

[simonhong, 2013-12-11 12:21:18]

Dear sky,
Please take a look.

[sky, 2013-12-12 00:49:59]

Can't you check for this state, meaning only do this if in the process of
dragging? Also, add test coverage.

[simonhong, 2013-12-12 05:30:40]

On 2013/12/12 00:49:59, sky wrote:
> Can't you check for this state, meaning only do this if in the process of
> dragging? Also, add test coverage.

I found that only dragging affects the behavior of ShelfWindowWatcher.
Other behaviors like activate/deactivate/maximize/minimize didn't affect.
I added a test for this.(ShelfWindowWatcherTest.DragWindow)
Please check again and many thanks!

[sky, 2013-12-12 16:41:43]

https://codereview.chromium.org/108313006/diff/40001/ash/shelf/shelf_window_w...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/40001/ash/shelf/shelf_window_w...
ash/shelf/shelf_window_watcher.cc:150: if
(observed_windows_.IsObserving(window))
If you do this, it means you need to better track window. In particular if
window gets added to another container it should not still have an item on the
shelf. Also, there should be a way for you to detect the window is in the
process of being dragged and only special case that.

[simonhong, 2013-12-13 02:03:45]

Dear sky,
Please check again!

https://codereview.chromium.org/108313006/diff/40001/ash/shelf/shelf_window_w...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/40001/ash/shelf/shelf_window_w...
ash/shelf/shelf_window_watcher.cc:150: if
(observed_windows_.IsObserving(window))
On 2013/12/12 16:41:43, sky wrote:
> If you do this, it means you need to better track window. In particular if
> window gets added to another container it should not still have an item on the
> shelf. Also, there should be a way for you to detect the window is in the
> process of being dragged and only special case that.

Checked window's dragging state and if so, an item is not removed.

[sky, 2013-12-14 00:17:20]

https://codereview.chromium.org/108313006/diff/70001/ash/shelf/shelf_window_w...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/70001/ash/shelf/shelf_window_w...
ash/shelf/shelf_window_watcher.cc:170: if (HasLauncherItemForWindow(window) &&
!IsDragging(window))
If the window is deleted while dragging how does this cleanup? You remove the
observer on 167, right? Please add test coverage for this.

[simonhong, 2013-12-16 02:24:15]

Dear sky,
Please check again.

https://codereview.chromium.org/108313006/diff/70001/ash/shelf/shelf_window_w...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/70001/ash/shelf/shelf_window_w...
ash/shelf/shelf_window_watcher.cc:170: if (HasLauncherItemForWindow(window) &&
!IsDragging(window))
On 2013/12/14 00:17:20, sky wrote:
> If the window is deleted while dragging how does this cleanup? You remove the
> observer on 167, right? Please add test coverage for this.

When a |window| is dragging its observer and item is not removed.
Instead, deleted |window| during the dragging is handled by
OnWindowDestroying().
For the test case, I found that WindowResizer can't handle deleted |window|
during the dragging. WindowResizer uses weak pointer of |window|. So I can't
simulate deleting a |window| before WindowResizer is destroyed for now. I think
it needs some refactoring for this.

[sky, 2013-12-16 21:11:53]

On Sun, Dec 15, 2013 at 6:24 PM,  <simonhong@chromium.org> wrote:
> Dear sky,
> Please check again.
>
>
>
>
https://codereview.chromium.org/108313006/diff/70001/ash/shelf/shelf_window_w...
> File ash/shelf/shelf_window_watcher.cc (right):
>
>
https://codereview.chromium.org/108313006/diff/70001/ash/shelf/shelf_window_w...
> ash/shelf/shelf_window_watcher.cc:170: if
> (HasLauncherItemForWindow(window) && !IsDragging(window))
> On 2013/12/14 00:17:20, sky wrote:
>>
>> If the window is deleted while dragging how does this cleanup? You
>
> remove the
>>
>> observer on 167, right? Please add test coverage for this.
>
>
> When a |window| is dragging its observer and item is not removed.
> Instead, deleted |window| during the dragging is handled by
> OnWindowDestroying().

What happens if window is dragging and then moved into a container
that should not have a shelf item?

  -Scott

> For the test case, I found that WindowResizer can't handle deleted
> |window|
> during the dragging. WindowResizer uses weak pointer of |window|. So I
> can't
> simulate deleting a |window| before WindowResizer is destroyed for now.
> I think
> it needs some refactoring for this.
>
> https://codereview.chromium.org/108313006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2013-12-17 04:43:36]

Dear sky,
Re-parenting to container which should not have an item is handled in
OnWindowParentChanged().
And also covered by ShelfWindowWatcherTest.ReparentWindow.
Please check again.

[sky, 2013-12-17 15:49:56]

https://codereview.chromium.org/108313006/diff/110001/ash/shelf/shelf_window_...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/110001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.cc:201: (parent == docked_container &&
IsDragging(window)))
Can you just check IsDragging here and not care about dock container?

https://codereview.chromium.org/108313006/diff/110001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.cc:218: DCHECK(HasLauncherItemForWindow(window));
Why the DCHECK here?

[simonhong, 2013-12-18 07:13:41]

Dear sky,
I created RemovedWindowObserver to handle removed window from default container.
This observer handles the item of removed window from default container and
dragged window.
I think implementation of ShelfWindowWatcher is more easy to read.
Please check again!

https://codereview.chromium.org/108313006/diff/130001/ash/shelf/shelf_window_...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/130001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.cc:91: if ((IsDragging(window) && parent ==
docked_container))
We should check IsDragging() and its parent is |docked_container|.
If IsDragging() is only used, we can't judge |window| is re-parented to right
container(should have an item) or not.

[simonhong, 2013-12-19 19:14:43]

kindly ping..

[sky, 2013-12-20 00:05:35]

I'm gone until the 6th. If you can't wait you'll need to get another reviewer.

On Thu, Dec 19, 2013 at 11:14 AM,  <simonhong@chromium.org> wrote:
> kindly ping..
>
> https://codereview.chromium.org/108313006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonhong, 2014-01-07 12:55:06]

kindly ping..

[sky, 2014-01-08 15:01:49]

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.cc:190: observed_removed_windows_.Add(window);
Is there a reason this doesn't remove from observed_windows_? Seems like it
would be less error prone to only have one observer active at a time.

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
File ash/shelf/shelf_window_watcher.h (right):

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.h:59: // Observer of removed Window that has an
item.
Change comments to:
Used to track windows that are removed. See description of ... for more details.

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.h:102: void
StartObserveRemovedWindow(aura::Window* window);
StartObservingRemovedWindow

Used when a window is removed. During dragging a window may be removed and when
the drag completes added back. When this happens we don't want to remove the
launcher item. StartObservingRemovedWindow, if necessary, attaches and observer.
When done FinishObservingRemovedWindow() is invoked.

[simonhong, 2014-01-09 01:00:08]

Dear sky,
Please check again.

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
File ash/shelf/shelf_window_watcher.cc (right):

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.cc:190: observed_removed_windows_.Add(window);
On 2014/01/08 15:01:49, sky wrote:
> Is there a reason this doesn't remove from observed_windows_? Seems like it
> would be less error prone to only have one observer active at a time.

|window| is removed from |observed_windows_| at OnWillRemoveWindow()
before calling StartObservingRemovedWindow().

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
File ash/shelf/shelf_window_watcher.h (right):

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.h:59: // Observer of removed Window that has an
item.
On 2014/01/08 15:01:49, sky wrote:
> Change comments to:
> Used to track windows that are removed. See description of ... for more
details.

Done.

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.h:102: void
StartObserveRemovedWindow(aura::Window* window);
On 2014/01/08 15:01:49, sky wrote:
> StartObservingRemovedWindow
> 
> Used when a window is removed. During dragging a window may be removed and
when
> the drag completes added back. When this happens we don't want to remove the
> launcher item. StartObservingRemovedWindow, if necessary, attaches and
observer.
> When done FinishObservingRemovedWindow() is invoked.

Done.

https://codereview.chromium.org/108313006/diff/220001/ash/shelf/shelf_window_...
ash/shelf/shelf_window_watcher.h:106: void
FinishObserveRemovedWindow(aura::Window* window);
Changed to FinishObservingRemovedWindow().

[sky, 2014-01-09 16:00:28]

LGTM

[commit-bot: I haz the power, 2014-01-09 17:05:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhong@chromium.org/108313006/290001

[commit-bot: I haz the power, 2014-01-09 20:43:12]

Change committed as 243970