After uninstall, remove last item in Launcher folder

After uninstall, remove last  item in Launcher folder

App Launcher: After uninstalling second-to-last app in a folder, reparent
last app to top level and delete folder.

BUG=368111
TEST=Uninstall one app from a two-app folder.  The remaining app should be
moved to the main app list, and the folder deleted.

Committed: https://crrev.com/740ee2670a9303bf17cff1cec762cb429c4041a4
Cr-Commit-Position: refs/heads/master@{#323797}

[Greg Levin, 2015-03-14 03:41:34]

glevin@chromium.org changed reviewers:
+ jennyz@chromium.org

[Greg Levin, 2015-03-14 03:51:45]

Hey Jenny.  Since you worked on the very similar Issue 357100 (even though it
was eventually "Won't Fix"ed), could you take a look at this one?  Thanks!

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_main_view.cc:288: 
When "drag app out of folder" uses
RemoveLastItemFromReparentItemFolderIfNecessary() to reparent the last remaining
item and remove the folder, it removes both AppsGridViews as AppListItemList
observers, and then adds the top level one back when it's done.  I'll admit, I
don't totally understand why all this is necessary, but it will crash if any of
these steps is left out.

[jennyz, 2015-03-17 21:16:48]

Can we write a unit test case for this operation?

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model_...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model_...
ui/app_list/app_list_model_observer.h:33: // Triggered just after an item is
uninstalled & deleted to clean up parent.
parent -> |parent|.

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_main_view.cc:288: 
On 2015/03/14 03:51:45, Greg Levin wrote:
> When "drag app out of folder" uses
> RemoveLastItemFromReparentItemFolderIfNecessary() to reparent the last
remaining
> item and remove the folder, it removes both AppsGridViews as AppListItemList
> observers, and then adds the top level one back when it's done.  I'll admit, I
> don't totally understand why all this is necessary, but it will crash if any
of
> these steps is left out.

Yes, the views are the observers of its associated data model, when we make a
series changes to both data model and view, we don't want to data model changed
to cascading to all the views before the entire operation completes, and if the
associated data item is removed, it could cause crash as well. Typically, we
follow the sequence of: remove view observer from the data model, remove the
views if the model will be removed, make changes to data model, make the
necessary changes to the views, add the view observers back when all operations
are done.

[stevenjb, 2015-03-23 19:34:30]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2015-03-23 19:34:31]

I haven't looked at this code in a while, but I'm a little concerned about the
pattern here.

It looks to me like a View action (uninstall an item) is triggering a model
event, which then informs the View, which triggers another model event (remove
the folder).

This seems potentially unstable (and makes
AppListMainView::OnAppListChildItemUninstalled pretty complicated).

I think that we need to approach this differently from the seemingly similar
(but in some ways very different) case of the View removing the second-to-last
item from a folder.

I think this would be better if, on uninstall only, the model detected that the
second-to-last item in a folder was removed. This is more expensive since the
model only tracks parent_id not folders, so we need to iterate through all items
in the model, but I think that would be more robust. Then the model can unparent
the remaining item, and the view code should respond correctly as-is.

https://codereview.chromium.org/1003393002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/1003393002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.h:72: void RemoveItem(const
std::string& id, bool uninstalled);
I think this would be better as two separate methods.

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model.h
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model....
ui/app_list/app_list_model.h:111: // allows observer to clean up folder with a
single item remaining.
nit: observers, folders

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model_...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model_...
ui/app_list/app_list_model_observer.h:34: virtual void
OnAppListChildItemUninstalled(AppListFolderItem* parent) {}
This is actually problematic. Currently we only handle
OnAppListChildItemUninstalled in AppListMainView, but that may end up deleting
the folder (|parent|), invalidating the pointer. So if anything else observes
this and handles it (or if there are ever multiple views associated with the
model), this could cause a crash.

We should pass the folder_id here instead.

[Greg Levin, 2015-03-23 22:04:50]

Here are fixes to all comments except Steven's general structural one.  I don't
need a LGTM on these... I'm just saving a "clean" draft of this version.  I'll
have a look at Steven's suggestions and see if I can rework this.

https://codereview.chromium.org/1003393002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/1003393002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.h:72: void RemoveItem(const
std::string& id, bool uninstalled);
On 2015/03/23 19:34:30, stevenjb wrote:
> I think this would be better as two separate methods.

Done.

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model.h
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model....
ui/app_list/app_list_model.h:111: // allows observer to clean up folder with a
single item remaining.
On 2015/03/23 19:34:30, stevenjb wrote:
> nit: observers, folders
> 

Done.

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model_...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model_...
ui/app_list/app_list_model_observer.h:33: // Triggered just after an item is
uninstalled & deleted to clean up parent.
On 2015/03/17 21:16:48, jennyz wrote:
> parent -> |parent|.

Since arg is now parent_id, I'm going to go with "folder" instead of a variable
name.

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/app_list_model_...
ui/app_list/app_list_model_observer.h:34: virtual void
OnAppListChildItemUninstalled(AppListFolderItem* parent) {}
On 2015/03/23 19:34:30, stevenjb wrote:
> This is actually problematic. Currently we only handle
> OnAppListChildItemUninstalled in AppListMainView, but that may end up deleting
> the folder (|parent|), invalidating the pointer. So if anything else observes
> this and handles it (or if there are ever multiple views associated with the
> model), this could cause a crash.
> 
> We should pass the folder_id here instead.

Done.

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/1003393002/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_main_view.cc:288: 
On 2015/03/17 21:16:48, jennyz wrote:
> On 2015/03/14 03:51:45, Greg Levin wrote:
> > When "drag app out of folder" uses
> > RemoveLastItemFromReparentItemFolderIfNecessary() to reparent the last
> remaining
> > item and remove the folder, it removes both AppsGridViews as AppListItemList
> > observers, and then adds the top level one back when it's done.  I'll admit,
I
> > don't totally understand why all this is necessary, but it will crash if any
> of
> > these steps is left out.
> 
> Yes, the views are the observers of its associated data model, when we make a
> series changes to both data model and view, we don't want to data model
changed
> to cascading to all the views before the entire operation completes, and if
the
> associated data item is removed, it could cause crash as well. Typically, we
> follow the sequence of: remove view observer from the data model, remove the
> views if the model will be removed, make changes to data model, make the
> necessary changes to the views, add the view observers back when all
operations
> are done.

Ok, that makes sense.  Thanks!

[Greg Levin, 2015-03-25 21:58:16]

Here's an updated (hopefully improved) version.  Please have another look!

> I think this would be better if, on uninstall only, the model detected
> that the second-to-last item in a folder was removed.

Done.  And you were right... I just had to move out that last item in the model,
and everything else took care of itself.  Thanks!

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_main_view.cc:276: }
This seems like a slightly awkward way to get this done, but we hit a debug
break (in the destructor of an observer) if we don't remove the folder's
AppItemList from its AppsGridView before it's removed, and I didn't see a better
way to call into the views from the model.

[stevenjb, 2015-03-25 22:24:01]

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.cc:227: DeleteItem(id);
What happens if this is the last item in a folder? Can DeleteItem cause the
folder to be deleted? It might be safer to save the folder_id and call
FindFolderItem(folder_id) after calling DeleteItem.

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.h:112: void DeleteUninstalledItem(const std::string&
id);
nit: Move this below DeleteItem here and in the .cc

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model_unittest.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model_unittest.cc:457:
folder1->item_list()->item_at(0)->position());
This is a little unclear, relying on the fact that moving an item also removes
items from the list making item_at(0) different each time.

None of these operations should delete the items constructed at the beginning of
this test. It might be more clear (and concise) to use CreateAndAddItem:
  AppListItem* item1 = model_.CreateAndAddItem("Item 0");
  ...
  model_.MoveItemToFolderAt(item1, folder_id, item1->position());

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model_unittest.cc:470: }
We should also add a test that creates a single item folder and uninstalls the
item.

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_main_view.cc:276: }
On 2015/03/25 21:58:16, Greg Levin wrote:
> This seems like a slightly awkward way to get this done, but we hit a debug
> break (in the destructor of an observer) if we don't remove the folder's
> AppItemList from its AppsGridView before it's removed, and I didn't see a
better
> way to call into the views from the model.

This is indeed very awkward. It's also unclear to me exactly what this is doing.
It seems to be making an assumption that the active view is going to be deleted,
but that isn't necessarily the case is it? A sync event could uninstall an app
in a different folder, in which case the SetItemList seems like it would be bad.

Instead, I think that the model needs to signal when a specific folder is going
to be deleted so that the view can simply close it if it is open/visible. Does
that make sense?

[Greg Levin, 2015-03-26 19:45:11]

Still not sure if AppListMainView::OnSingleItemFolderWillBeDeleted() is exactly
what you're looking for, but at least it's getting closer.  Please have another
look.  Thanks!

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.cc:227: DeleteItem(id);
On 2015/03/25 22:24:00, stevenjb wrote:
> What happens if this is the last item in a folder? Can DeleteItem cause the
> folder to be deleted? It might be safer to save the folder_id and call
> FindFolderItem(folder_id) after calling DeleteItem.

Yes, good call.

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.h:112: void DeleteUninstalledItem(const std::string&
id);
On 2015/03/25 22:24:01, stevenjb wrote:
> nit: Move this below DeleteItem here and in the .cc

Done.

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model_unittest.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model_unittest.cc:457:
folder1->item_list()->item_at(0)->position());
On 2015/03/25 22:24:01, stevenjb wrote:
> This is a little unclear, relying on the fact that moving an item also removes
> items from the list making item_at(0) different each time.
> 
> None of these operations should delete the items constructed at the beginning
of
> this test. It might be more clear (and concise) to use CreateAndAddItem:
>   AppListItem* item1 = model_.CreateAndAddItem("Item 0");
>   ...
>   model_.MoveItemToFolderAt(item1, folder_id, item1->position());

Done.
(I find the current version more unclear, but only because I don't understand
how "position" encodes information.  My new code wouldn't work if "position"
were "index", but it's clearly something else.)

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model_unittest.cc:470: }
On 2015/03/25 22:24:01, stevenjb wrote:
> We should also add a test that creates a single item folder and uninstalls the
> item.

Done.

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_main_view.cc:276: }
On 2015/03/25 22:24:01, stevenjb wrote:
> On 2015/03/25 21:58:16, Greg Levin wrote:
> > This seems like a slightly awkward way to get this done, but we hit a debug
> > break (in the destructor of an observer) if we don't remove the folder's
> > AppItemList from its AppsGridView before it's removed, and I didn't see a
> better
> > way to call into the views from the model.
> 
> This is indeed very awkward. It's also unclear to me exactly what this is
doing.
> It seems to be making an assumption that the active view is going to be
deleted,
> but that isn't necessarily the case is it? A sync event could uninstall an app
> in a different folder, in which case the SetItemList seems like it would be
bad.
> 
> Instead, I think that the model needs to signal when a specific folder is
going
> to be deleted so that the view can simply close it if it is open/visible. Does
> that make sense?

Done?  I'm still calling SetItemList(nullptr), because simply closing the open
folder does not remove the AppsGridView as an observer of the folder's
AppListItemList (which is about to be deleted).  However, I am verifying that
the folder to be deleted is actually the currently displayed folder.

[stevenjb, 2015-03-30 17:23:59]

Much better. One suggestion then (assuming it works OK) I think we are good.

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model_unittest.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model_unittest.cc:457:
folder1->item_list()->item_at(0)->position());
On 2015/03/26 19:45:11, Greg Levin wrote:
> On 2015/03/25 22:24:01, stevenjb wrote:
> > This is a little unclear, relying on the fact that moving an item also
removes
> > items from the list making item_at(0) different each time.
> > 
> > None of these operations should delete the items constructed at the
beginning
> of
> > this test. It might be more clear (and concise) to use CreateAndAddItem:
> >   AppListItem* item1 = model_.CreateAndAddItem("Item 0");
> >   ...
> >   model_.MoveItemToFolderAt(item1, folder_id, item1->position());
> 
> Done.
> (I find the current version more unclear, but only because I don't understand
> how "position" encodes information.  My new code wouldn't work if "position"
> were "index", but it's clearly something else.)

'position' is a StringOrdinal, which uses strings like 'a', 'b' to provide
ordering that allows something to be inserted between 'a' and 'b' at 'am'.

https://codereview.chromium.org/1003393002/diff/60001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/1003393002/diff/60001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.cc:290: // Issue 368111: Upon uninstall of
2nd-to-last folder item, reparent last item
nit: crbug.com/368111:

https://codereview.chromium.org/1003393002/diff/60001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.cc:295: OnSingleItemFolderWillBeDeleted(folder_id));
In DeleteItem() we call OnAppListItemWillBeDeleted. This should get called for
folders too. It seems like that method should be able to check for the case of a
folder being deleted and handle any special view logic there.

[Greg Levin, 2015-03-30 23:08:52]

Good suggestion... that cleaned things up a lot!
Please have another look!

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model_unittest.cc (right):

https://codereview.chromium.org/1003393002/diff/40001/ui/app_list/app_list_mo...
ui/app_list/app_list_model_unittest.cc:457:
folder1->item_list()->item_at(0)->position());
On 2015/03/30 17:23:59, stevenjb wrote:
> On 2015/03/26 19:45:11, Greg Levin wrote:
> > On 2015/03/25 22:24:01, stevenjb wrote:
> > > This is a little unclear, relying on the fact that moving an item also
> removes
> > > items from the list making item_at(0) different each time.
> > > 
> > > None of these operations should delete the items constructed at the
> beginning
> > of
> > > this test. It might be more clear (and concise) to use CreateAndAddItem:
> > >   AppListItem* item1 = model_.CreateAndAddItem("Item 0");
> > >   ...
> > >   model_.MoveItemToFolderAt(item1, folder_id, item1->position());
> > 
> > Done.
> > (I find the current version more unclear, but only because I don't
understand
> > how "position" encodes information.  My new code wouldn't work if "position"
> > were "index", but it's clearly something else.)
> 
> 'position' is a StringOrdinal, which uses strings like 'a', 'b' to provide
> ordering that allows something to be inserted between 'a' and 'b' at 'am'.

Ok, thanks.  I'll ask for more detail if I ever need to dig deeper into it.

https://codereview.chromium.org/1003393002/diff/60001/ui/app_list/app_list_mo...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/1003393002/diff/60001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.cc:290: // Issue 368111: Upon uninstall of
2nd-to-last folder item, reparent last item
On 2015/03/30 17:23:59, stevenjb wrote:
> nit: crbug.com/368111:

Done.

https://codereview.chromium.org/1003393002/diff/60001/ui/app_list/app_list_mo...
ui/app_list/app_list_model.cc:295: OnSingleItemFolderWillBeDeleted(folder_id));
On 2015/03/30 17:23:59, stevenjb wrote:
> In DeleteItem() we call OnAppListItemWillBeDeleted. This should get called for
> folders too. It seems like that method should be able to check for the case of
> a folder being deleted and handle any special view logic there.

Done.  Thanks for pointing this out.  It turns out that a small modification in
AppsGridView::OnFolderItemRemoved() (called from
AppListFolderView::OnAppListItemWillBeDeleted()) was all that was needed, I just
hadn't found the right spot before.

[stevenjb, 2015-04-02 20:08:01]

LGTM!

[jennyz, 2015-04-03 18:14:25]

lgtm

[Greg Levin, 2015-04-03 18:39:42]

The CQ bit was checked by glevin@chromium.org

[commit-bot: I haz the power, 2015-04-03 18:40:15]

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

[commit-bot: I haz the power, 2015-04-03 19:45:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-03 20:40:12]

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