Make cancelling of app list correctly clean up folder UI.

Make cancelling of app list correctly clean up folder UI.

This CL makes a cancelled drag clean up the folder UI. Previously cancelled
drags would not burst the folder background "bubble", leaving the folder name
UI hidden.

This CL extracts an AppsGridViewFolderDelegate from AppListFolderView
in order to facilitate the testing of this functionality.

The bug also mentions a view getting stuck but this patch does not
address that part of the issue.

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

[calamity, 2014-03-25 23:47:32]

FYI, the actual fix is at apps_grid_view.cc:~660.

[tapted, 2014-03-26 02:53:53]

+jennyz

Nice change. lgtm % nits, but wait for jenny to have a look too.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_folder_view.h (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_folder_view.h:98: OVERRIDE;
nit: I think "OVERRIDE" isn't meant to appear on a line by itself but can't
remember where I saw this guideline :/. there's some discussion here:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Y1hJFezInw4/N9Ru1...

But maybe clang-format is the culprit here.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:598: if
(IsDraggingForReprentInHiddenGridView()) {
nit: doesn't need curlies

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:659: if (folder_delegate_ &&
!IsDraggingForReprentInHiddenGridView())
there's a typo in this function name: Reprent -> Reparent

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:444: AppsGridViewFolderDelegate*
folder_delegate_;
I think this needs a more verbose comment, to say it can be NULL, and that only
the GridViews used inside folders have a folder_delegate.

I wonder if we could even inherit from AppsGridView for this... but that would
be something to explore in a follow-up.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view_folder_delegate.h (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_folder_delegate.h:23: class APP_LIST_EXPORT
AppsGridViewFolderDelegate {
all classes in headers should have a class comment.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view_unittest.cc (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_unittest.cc:195: OVERRIDE {}
nit: OVERRIDE looks lonely, unless clang-format disagrees.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_unittest.cc:213: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

[jennyz, 2014-03-26 22:09:38]

Nice refactoring, but it will crash the chrome on chromeos, see my comments
inline. If you added the code I suggest, it should burst the folder ink bubble,
but I don't see how this fixes the problem that the dragged item stuck at the
same place when the launcher opens again.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_folder_view.h (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_folder_view.h:98: OVERRIDE;
On 2014/03/26 02:53:54, tapted wrote:
> nit: I think "OVERRIDE" isn't meant to appear on a line by itself but can't
> remember where I saw this guideline :/. there's some discussion here:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Y1hJFezInw4/N9Ru1...
> 
> But maybe clang-format is the culprit here.
+1, having OVERRIDE on a separate line looks a little weird to me.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:659: if (folder_delegate_ &&
!IsDraggingForReprentInHiddenGridView())
This change will crash chrome on chromeos, since this code path is called from
the stack destroying the app launcher bubble when esc is pressed, which is
originated from AppListView::Close(). At this point AppListFolderView has been
destroyed, thus, crash!

This exposes a bug in the ContentView::CancelDrag(). We did not cancel drag in
the folder view's grid view. We need to add the following code in the after the
existing lines, the it will works fine with your change:
  if (apps_container_view_->app_list_folder_view()
          ->items_grid_view()
          ->has_dragged_view()) {
    apps_container_view_->app_list_folder_view()->items_grid_view()->EndDrag(
        true);
  }

[jennyz, 2014-03-26 22:20:53]

On 2014/03/26 22:09:38, jennyz wrote:
> Nice refactoring, but it will crash the chrome on chromeos, see my comments
> inline. If you added the code I suggest, it should burst the folder ink
bubble,
> but I don't see how this fixes the problem that the dragged item stuck at the
> same place when the launcher opens again.
> 
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
> File ui/app_list/views/app_list_folder_view.h (right):
> 
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
> ui/app_list/views/app_list_folder_view.h:98: OVERRIDE;
> On 2014/03/26 02:53:54, tapted wrote:
> > nit: I think "OVERRIDE" isn't meant to appear on a line by itself but can't
> > remember where I saw this guideline :/. there's some discussion here:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Y1hJFezInw4/N9Ru1...
> > 
> > But maybe clang-format is the culprit here.
> +1, having OVERRIDE on a separate line looks a little weird to me.
> 
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
> File ui/app_list/views/apps_grid_view.cc (right):
> 
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
> ui/app_list/views/apps_grid_view.cc:659: if (folder_delegate_ &&
> !IsDraggingForReprentInHiddenGridView())
> This change will crash chrome on chromeos, since this code path is called from
> the stack destroying the app launcher bubble when esc is pressed, which is
> originated from AppListView::Close(). At this point AppListFolderView has been
> destroyed, thus, crash!
> 
> This exposes a bug in the ContentView::CancelDrag(). We did not cancel drag in
> the folder view's grid view. We need to add the following code in the after
the
> existing lines, the it will works fine with your change:
>   if (apps_container_view_->app_list_folder_view()
>           ->items_grid_view()
>           ->has_dragged_view()) {
>     apps_container_view_->app_list_folder_view()->items_grid_view()->EndDrag(
>         true);
>   }

I have uploaded a patch for fixing ContentView::CancelDrag().
https://codereview.chromium.org/213253008/

[calamity, 2014-03-27 01:59:38]

On 2014/03/26 22:09:38, jennyz wrote:
> Nice refactoring, but it will crash the chrome on chromeos, see my comments
> inline. If you added the code I suggest, it should burst the folder ink
bubble,
> but I don't see how this fixes the problem that the dragged item stuck at the
> same place when the launcher opens again.
> 
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
> File ui/app_list/views/app_list_folder_view.h (right):
> 
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
> ui/app_list/views/app_list_folder_view.h:98: OVERRIDE;
> On 2014/03/26 02:53:54, tapted wrote:
> > nit: I think "OVERRIDE" isn't meant to appear on a line by itself but can't
> > remember where I saw this guideline :/. there's some discussion here:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Y1hJFezInw4/N9Ru1...
> > 
> > But maybe clang-format is the culprit here.
> +1, having OVERRIDE on a separate line looks a little weird to me.

clang-format =(

>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
> File ui/app_list/views/apps_grid_view.cc (right):
> 
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
> ui/app_list/views/apps_grid_view.cc:659: if (folder_delegate_ &&
> !IsDraggingForReprentInHiddenGridView())
> This change will crash chrome on chromeos, since this code path is called from
> the stack destroying the app launcher bubble when esc is pressed, which is
> originated from AppListView::Close(). At this point AppListFolderView has been
> destroyed, thus, crash!
> 
> This exposes a bug in the ContentView::CancelDrag(). We did not cancel drag in
> the folder view's grid view. We need to add the following code in the after
the
> existing lines, the it will works fine with your change:
>   if (apps_container_view_->app_list_folder_view()
>           ->items_grid_view()
>           ->has_dragged_view()) {
>     apps_container_view_->app_list_folder_view()->items_grid_view()->EndDrag(
>         true);
>   }

What happens on Windows is that the SynchronousDrag is canceled by ESC, without
closing the app launcher. This means that the drag shouldn't result in a model
update, (but should end the drag), so EndDrag(true) is appropriate here. I think
the right thing to do is to have an EndDragType enum (DROP, CANCEL, CLOSE) and
do send through the correct value from ContentsView::Close(). WDYT?

[calamity, 2014-03-27 03:06:49]

On 2014/03/27 01:59:38, calamity wrote:
> On 2014/03/26 22:09:38, jennyz wrote:
> > Nice refactoring, but it will crash the chrome on chromeos, see my comments
> > inline. If you added the code I suggest, it should burst the folder ink
> bubble,
> > but I don't see how this fixes the problem that the dragged item stuck at
the
> > same place when the launcher opens again.
> > 
> >
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
> > File ui/app_list/views/app_list_folder_view.h (right):
> > 
> >
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
> > ui/app_list/views/app_list_folder_view.h:98: OVERRIDE;
> > On 2014/03/26 02:53:54, tapted wrote:
> > > nit: I think "OVERRIDE" isn't meant to appear on a line by itself but
can't
> > > remember where I saw this guideline :/. there's some discussion here:
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Y1hJFezInw4/N9Ru1...
> > > 
> > > But maybe clang-format is the culprit here.
> > +1, having OVERRIDE on a separate line looks a little weird to me.
> 
> clang-format =(
> 
> >
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
> > File ui/app_list/views/apps_grid_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
> > ui/app_list/views/apps_grid_view.cc:659: if (folder_delegate_ &&
> > !IsDraggingForReprentInHiddenGridView())
> > This change will crash chrome on chromeos, since this code path is called
from
> > the stack destroying the app launcher bubble when esc is pressed, which is
> > originated from AppListView::Close(). At this point AppListFolderView has
been
> > destroyed, thus, crash!
> > 
> > This exposes a bug in the ContentView::CancelDrag(). We did not cancel drag
in
> > the folder view's grid view. We need to add the following code in the after
> the
> > existing lines, the it will works fine with your change:
> >   if (apps_container_view_->app_list_folder_view()
> >           ->items_grid_view()
> >           ->has_dragged_view()) {
> >    
apps_container_view_->app_list_folder_view()->items_grid_view()->EndDrag(
> >         true);
> >   }
> 
> What happens on Windows is that the SynchronousDrag is canceled by ESC,
without
> closing the app launcher. This means that the drag shouldn't result in a model
> update, (but should end the drag), so EndDrag(true) is appropriate here. I
think
> the right thing to do is to have an EndDragType enum (DROP, CANCEL, CLOSE) and
> do send through the correct value from ContentsView::Close(). WDYT?

Oh wups, I didn't realize your change would deal with the ChromeOS case
completely. Disregard the EndDragType stuff.

As for the other part of the bug, I'm not able to repro the view getting stuck
on the latest canary. I'm also planning to write something to reset the app
list's state when it Show()s because it currently comes up with the folder view
if that's what it was dismissed with (and the folder view isn't properly cleaned
up by Hide()).

[calamity, 2014-03-27 05:14:44]

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_folder_view.h (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_folder_view.h:98: OVERRIDE;
On 2014/03/26 22:09:38, jennyz wrote:
> On 2014/03/26 02:53:54, tapted wrote:
> > nit: I think "OVERRIDE" isn't meant to appear on a line by itself but can't
> > remember where I saw this guideline :/. there's some discussion here:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Y1hJFezInw4/N9Ru1...
> > 
> > But maybe clang-format is the culprit here.
> +1, having OVERRIDE on a separate line looks a little weird to me.

clang-format =(

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:598: if
(IsDraggingForReprentInHiddenGridView()) {
On 2014/03/26 02:53:54, tapted wrote:
> nit: doesn't need curlies

Done.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:659: if (folder_delegate_ &&
!IsDraggingForReprentInHiddenGridView())
On 2014/03/26 02:53:54, tapted wrote:
> there's a typo in this function name: Reprent -> Reparent
> 

Done.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:659: if (folder_delegate_ &&
!IsDraggingForReprentInHiddenGridView())
On 2014/03/26 22:09:38, jennyz wrote:
> This change will crash chrome on chromeos, since this code path is called from
> the stack destroying the app launcher bubble when esc is pressed, which is
> originated from AppListView::Close(). At this point AppListFolderView has been
> destroyed, thus, crash!
> 
> This exposes a bug in the ContentView::CancelDrag(). We did not cancel drag in
> the folder view's grid view. We need to add the following code in the after
the
> existing lines, the it will works fine with your change:
>   if (apps_container_view_->app_list_folder_view()
>           ->items_grid_view()
>           ->has_dragged_view()) {
>     apps_container_view_->app_list_folder_view()->items_grid_view()->EndDrag(
>         true);
>   }

Cool, thanks for fixing that.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:444: AppsGridViewFolderDelegate*
folder_delegate_;
On 2014/03/26 02:53:54, tapted wrote:
> I think this needs a more verbose comment, to say it can be NULL, and that
only
> the GridViews used inside folders have a folder_delegate.
> 
> I wonder if we could even inherit from AppsGridView for this... but that would
> be something to explore in a follow-up.

Done.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view_folder_delegate.h (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_folder_delegate.h:23: class APP_LIST_EXPORT
AppsGridViewFolderDelegate {
On 2014/03/26 02:53:54, tapted wrote:
> all classes in headers should have a class comment.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments

Done.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view_unittest.cc (right):

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_unittest.cc:195: OVERRIDE {}
On 2014/03/26 02:53:54, tapted wrote:
> nit: OVERRIDE looks lonely, unless clang-format disagrees.

All hail our benevolent clang-format overlord.

https://codereview.chromium.org/210583004/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_unittest.cc:213: };
On 2014/03/26 02:53:54, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

[jennyz, 2014-03-27 16:53:34]

The cl is good and it will burst the folder ink bubble when drag is canceled,
but I don't see how this fixes the problem that the dragged item stuck at the
same place when the launcher opens again. Can you add some comment in cl to
explain?

[calamity, 2014-03-27 23:51:31]

On 2014/03/27 16:53:34, jennyz wrote:
> The cl is good and it will burst the folder ink bubble when drag is canceled,
> but I don't see how this fixes the problem that the dragged item stuck at the
> same place when the launcher opens again. Can you add some comment in cl to
> explain?

I'm not able to repro the item getting stuck on the latest canary.

[jennyz, 2014-03-27 23:54:46]

On 2014/03/27 23:51:31, calamity wrote:
> On 2014/03/27 16:53:34, jennyz wrote:
> > The cl is good and it will burst the folder ink bubble when drag is
canceled,
> > but I don't see how this fixes the problem that the dragged item stuck at
the
> > same place when the launcher opens again. Can you add some comment in cl to
> > explain?
> 
> I'm not able to repro the item getting stuck on the latest canary.

Please explain this in cl description.

[jennyz, 2014-03-27 23:54:49]

lgtm

[calamity, 2014-03-27 23:55:24]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:58:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/210583004/150001

[cmp, 2014-03-30 16:34:14]

The CQ bit was unchecked by cmp@chromium.org

[cmp, 2014-03-30 16:37:05]

The CQ bit was checked by cmp@chromium.org

[commit-bot: I haz the power, 2014-03-30 16:37:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/210583004/150001

[commit-bot: I haz the power, 2014-03-30 17:02:39]

Change committed as 260436