mash: Fold ShelfItemDelegateManager into ShelfModel

mash: Fold ShelfItemDelegateManager into ShelfModel

This eliminates some accesses of ash::Shell inside the shelf code, which is
necessary for porting the shelf to mus/mustash.

* ShelfModel now owns the ShelfItemDelegates
* Explicitly delete the ShelfItemDelegates during shutdown, because some of
the subclasses reach into ShelfModel during their destructors
* Move AppListShelfItemDelegate earlier, to WmShell::Initialize phase
* Fix ShelfAppBrowserTest.LauncherContextMenuVerifyCloseItemAppearance to use
the actual shelf ID of the browser shortcut, rather than using the magic
number "1" that can sometimes mean the app list item.

This also simplifies some of the unit tests for ChromeLauncherController
because they don't need to construct their own ShelfItemDelegateManager.

BUG=629990, 429870
TEST=ash_unittests, chrome unit_tests, manually test shelf pin/unpin/etc.

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

[James Cook, 2016-07-21 20:30:52]

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

[commit-bot: I haz the power, 2016-07-21 20:31:05]

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

[James Cook, 2016-07-21 20:33:30]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org, skuhne@chromium.org

[James Cook, 2016-07-21 20:33:31]

Please take a look:
msw, the whole thing
skuhne, chrome/browser/ui/ash/launcher/*

skuhne, can you also comment on whether we need the OnSetShelfItemDelegate
method anymore? I think the bug number in the comment was wrong, so I changed
it. It looks like maybe the root cause of the GetShelfIDForAppID crash
(http://crbug.com/429870) was opening settings from incognito
(http://crbug.com/430726)?

[commit-bot: I haz the power, 2016-07-21 21:30:01]

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

[commit-bot: I haz the power, 2016-07-21 21:30:02]

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

[msw, 2016-07-21 21:30:54]

Nice cleanup!

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/app_li...
File ash/common/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/app_li...
ash/common/shelf/app_list_shelf_item_delegate.cc:19: // Add the app list item to
the data model.
nit: s/data/shelf/?

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/app_li...
ash/common/shelf/app_list_shelf_item_delegate.cc:28: // The delegate manager
owns the newly created shelf item delegate.
nit: update comment (no more delegate manager)

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.cc:54: // Some ShelfItemDelegates access this model
in their destructors and hence
Would it be sufficient to inline this in the dtor? Seems so to me?

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.cc:180: if (ItemIndexByID(id) == -1)
Is checking for the item index useful? Consider this instead:
  if (id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end())
    return id_to_item_delegate_map_[id].get();
  return nullptr;

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model.h (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.h:35: void Shutdown();
nit: DestroyItemDelegates or similar?

https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:66: // ShelfItemDelegate subclasses it owns have complex
cleanup to run, so
Is this still needed? Don't stress too much looking/removing it now.

https://codereview.chromium.org/2171813004/diff/40001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/shelf/shelf_view.h#...
ash/shelf/shelf_view.h:50: class ShelfItemDelegateManager;
nit: remove (and do a 'git gs' to find others?)

https://codereview.chromium.org/2171813004/diff/40001/ash/test/test_shelf_del...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:10: #include "ash/common/wm_shell.h"
nit: remove; not needed.

https://codereview.chromium.org/2171813004/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1599: //
ash::ShelfItemDelegateManagerObserver:
nit: update (remove)

https://codereview.chromium.org/2171813004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1601: void
ChromeLauncherControllerImpl::OnSetShelfItemDelegate(
nit: reorder to match decl

[James Cook, 2016-07-22 01:11:32]

Description was changed from

==========
mash: Fold ShelfItemDelegateManager into ShelfModel

This eliminates some accesses of ash::Shell inside the shelf code, which is
necessary for porting the shelf to mus/mustash.

* ShelfModel now owns the ShelfItemDelegates
* Explicitly delete the ShelfItemDelegates during shutdown, because some of
the subclasses reach into ShelfModel during their destructors
* Move AppListShelfItemDelegate earlier, to WmShell::Initialize phase

This also simplifies some of the unit tests for ChromeLauncherController
because they don't need to construct their own ShelfItemDelegateManager.

BUG=629990,429870
TEST=ash_unittests, chrome unit_tests, manually test shelf pin/unpin/etc.
==========

to

==========
mash: Fold ShelfItemDelegateManager into ShelfModel

This eliminates some accesses of ash::Shell inside the shelf code, which is
necessary for porting the shelf to mus/mustash.

* ShelfModel now owns the ShelfItemDelegates
* Explicitly delete the ShelfItemDelegates during shutdown, because some of
the subclasses reach into ShelfModel during their destructors
* Move AppListShelfItemDelegate earlier, to WmShell::Initialize phase
* Fix ShelfAppBrowserTest.LauncherContextMenuVerifyCloseItemAppearance to use
the actual shelf ID of the browser shortcut, rather than using the magic
number "1" that can sometimes mean the app list item.

This also simplifies some of the unit tests for ChromeLauncherController
because they don't need to construct their own ShelfItemDelegateManager.

BUG=629990,429870
TEST=ash_unittests, chrome unit_tests, manually test shelf pin/unpin/etc.
==========

[James Cook, 2016-07-22 01:20:08]

msw, skuhne please take another look

+CC yiyix, just FYI I had to change
ShelfAppBrowserTest.LauncherContextMenuVerifyCloseItemAppearance. It was using
the magic number 1 for a "dummy" shelf item ID for the browser shortcut icon. In
production the app list item has ID 1, and a change in this CL moved the app
list item initialization earlier, so the test failed because it tried to reuse
that ID.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/app_li...
File ash/common/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/app_li...
ash/common/shelf/app_list_shelf_item_delegate.cc:19: // Add the app list item to
the data model.
On 2016/07/21 21:30:53, msw wrote:
> nit: s/data/shelf/?

Done.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/app_li...
ash/common/shelf/app_list_shelf_item_delegate.cc:28: // The delegate manager
owns the newly created shelf item delegate.
On 2016/07/21 21:30:53, msw wrote:
> nit: update comment (no more delegate manager)

Deleted comment since unique_ptr makes ownership transfer clear.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.cc:54: // Some ShelfItemDelegates access this model
in their destructors and hence
On 2016/07/21 21:30:53, msw wrote:
> Would it be sufficient to inline this in the dtor? Seems so to me?

I need to do this in an explicit cleanup pass from WmShell such that it will run
before ~WmShell and ~ShelfModel.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.cc:180: if (ItemIndexByID(id) == -1)
On 2016/07/21 21:30:53, msw wrote:
> Is checking for the item index useful? Consider this instead:
>   if (id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end())
>     return id_to_item_delegate_map_[id].get();
>   return nullptr;

Done. I wonder if this could be related to the shelf view title crash you
(speculatively) fixed in https://codereview.chromium.org/1994253004/ -- maybe
sometimes there's a delegate with no item.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model.h (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.h:35: void Shutdown();
On 2016/07/21 21:30:53, msw wrote:
> nit: DestroyItemDelegates or similar?

Done.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:66: // ShelfItemDelegate subclasses it owns have complex
cleanup to run, so
On 2016/07/21 21:30:53, msw wrote:
> Is this still needed? Don't stress too much looking/removing it now.

My worries about ARC items are the same as in the initial patch -- they do
complex cleanup on close.

https://codereview.chromium.org/2171813004/diff/40001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/shelf/shelf_view.h#...
ash/shelf/shelf_view.h:50: class ShelfItemDelegateManager;
On 2016/07/21 21:30:53, msw wrote:
> nit: remove (and do a 'git gs' to find others?)

Done here and everywhere.

https://codereview.chromium.org/2171813004/diff/40001/ash/test/test_shelf_del...
File ash/test/test_shelf_delegate.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/test/test_shelf_del...
ash/test/test_shelf_delegate.cc:10: #include "ash/common/wm_shell.h"
On 2016/07/21 21:30:54, msw wrote:
> nit: remove; not needed.

Done.

https://codereview.chromium.org/2171813004/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1599: //
ash::ShelfItemDelegateManagerObserver:
On 2016/07/21 21:30:54, msw wrote:
> nit: update (remove)

Done.

https://codereview.chromium.org/2171813004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1601: void
ChromeLauncherControllerImpl::OnSetShelfItemDelegate(
On 2016/07/21 21:30:54, msw wrote:
> nit: reorder to match decl

Done.

[James Cook, 2016-07-22 01:20:16]

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

[commit-bot: I haz the power, 2016-07-22 01:20:52]

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-07-22 01:33:32]

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

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

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

[msw, 2016-07-22 01:49:52]

lgtm with an open question about the need for DestroyItemDelegates.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.cc:54: // Some ShelfItemDelegates access this model
in their destructors and hence
On 2016/07/22 01:20:07, James Cook wrote:
> On 2016/07/21 21:30:53, msw wrote:
> > Would it be sufficient to inline this in the dtor? Seems so to me?
> 
> I need to do this in an explicit cleanup pass from WmShell such that it will
run
> before ~WmShell and ~ShelfModel.

It's not clear to me why this would need to happen before (instead of during)
~ShelfModel and ~WmShell. Do the ShelfItemDelegates access some other
Shelf/Shell state that is destroyed before ShelfModel/WmShell, but after
ShelfModel::Shutdown is called? Does inling fail some automated testing, surface
in some other manner, or is it just assumed given the existing pattern? It's
fine to leave as-is for now, but maybe worth further clarifying this need.

https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc#...
ash/common/wm_shell.cc:66: // ShelfItemDelegate subclasses it owns have complex
cleanup to run, so
On 2016/07/22 01:20:08, James Cook wrote:
> On 2016/07/21 21:30:53, msw wrote:
> > Is this still needed? Don't stress too much looking/removing it now.
> 
> My worries about ARC items are the same as in the initial patch -- they do
> complex cleanup on close.

Do you mean ArcAppDeferredLauncherItemController and/or
ArcPlaystoreShortcutLauncherItemController? those destructors look pretty
innocuous. What am I missing here?

[James Cook, 2016-07-22 16:03:32]

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

[commit-bot: I haz the power, 2016-07-22 16:03:52]

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

[James Cook, 2016-07-22 16:15:30]

On 2016/07/22 01:49:52, msw wrote:
> lgtm with an open question about the need for DestroyItemDelegates.
> 
>
https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
> File ash/common/shelf/shelf_model.cc (right):
> 
>
https://codereview.chromium.org/2171813004/diff/40001/ash/common/shelf/shelf_...
> ash/common/shelf/shelf_model.cc:54: // Some ShelfItemDelegates access this
model
> in their destructors and hence
> On 2016/07/22 01:20:07, James Cook wrote:
> > On 2016/07/21 21:30:53, msw wrote:
> > > Would it be sufficient to inline this in the dtor? Seems so to me?
> > 
> > I need to do this in an explicit cleanup pass from WmShell such that it will
> run
> > before ~WmShell and ~ShelfModel.
> 
> It's not clear to me why this would need to happen before (instead of during)
> ~ShelfModel and ~WmShell. Do the ShelfItemDelegates access some other
> Shelf/Shell state that is destroyed before ShelfModel/WmShell, but after
> ShelfModel::Shutdown is called? Does inling fail some automated testing,
surface
> in some other manner, or is it just assumed given the existing pattern? It's
> fine to leave as-is for now, but maybe worth further clarifying this need.

See below. It's probably fine, but in general, if ~A calls ~B which calls back
into A, that's a recipe for future problems since A is now in a partially
constructed state.

> https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc
> File ash/common/wm_shell.cc (right):
> 
>
https://codereview.chromium.org/2171813004/diff/40001/ash/common/wm_shell.cc#...
> ash/common/wm_shell.cc:66: // ShelfItemDelegate subclasses it owns have
complex
> cleanup to run, so
> On 2016/07/22 01:20:08, James Cook wrote:
> > On 2016/07/21 21:30:53, msw wrote:
> > > Is this still needed? Don't stress too much looking/removing it now.
> > 
> > My worries about ARC items are the same as in the initial patch -- they do
> > complex cleanup on close.
> 
> Do you mean ArcAppDeferredLauncherItemController and/or
> ArcPlaystoreShortcutLauncherItemController? those destructors look pretty
> innocuous. What am I missing here?

My primary concern is ArcAppDeferredLauncherItemController, which can attempt to
close an open app on teardown:
https://cs-staging.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/a...

This does many things:
https://cs-staging.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/a...

It accesses the profile (probably safe?), closes the launcher item, which might
create a new shortcut controller:
https://cs-staging.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/c...

It calls into an AppIconLoader:
https://cs-staging.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/c...

Which calls into an ArcPrefs:
https://cs-staging.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/a...

And I have no way of manually trying this, and as far as I can tell there are no
automated tests of this code. It's *probably* all safe, but if I'm wrong, or
anyone changes this code in the wrong way in the future, it will result in a
shutdown crash. These are hard to find and can result in profile data not being
saved, so they're pretty bad.  That's what I'm afraid of.

I'll try moving it into ~WmShell and run some tryjobs and see if anything
breaks. But even if nothing breaks, it doesn't mean it won't break in
production.

Do you think it's worth the risk?  If so, and if the tryjobs are clean, I can do
it.

[commit-bot: I haz the power, 2016-07-22 16:17:37]

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

[commit-bot: I haz the power, 2016-07-22 16:17:38]

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

[James Cook, 2016-07-22 16:31:44]

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

[commit-bot: I haz the power, 2016-07-22 16:32:01]

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

[msw, 2016-07-22 17:11:22]

On 2016/07/22 16:15:30, James Cook wrote:
> Do you think it's worth the risk?

No, it's sufficiently strange that I can't confidently say it's safe.
That said, it may be ok, and odd to pre-emptively fix a non-issue.
lgtm whichever way you go.

[James Cook, 2016-07-22 17:18:55]

I just spent some time talking to khmel@ who works on ARC UI and wrote some of
the shelf code. He says it's hard to test this code and he's not sure if my
patch is safe or not either.

I'm going to call the existing changes enough of a win that keeping a
potentially unnecessary piece of shutdown cleanup is worth in. :-(

[James Cook, 2016-07-22 17:19:07]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-07-22 17:19:07]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2171813004/#ps120001
(title: "explicitly destroy ShelfItemDelegates")

[commit-bot: I haz the power, 2016-07-22 17:19:38]

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-07-22 17:59:52]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-22 18:03:13]

Description was changed from

==========
mash: Fold ShelfItemDelegateManager into ShelfModel

This eliminates some accesses of ash::Shell inside the shelf code, which is
necessary for porting the shelf to mus/mustash.

* ShelfModel now owns the ShelfItemDelegates
* Explicitly delete the ShelfItemDelegates during shutdown, because some of
the subclasses reach into ShelfModel during their destructors
* Move AppListShelfItemDelegate earlier, to WmShell::Initialize phase
* Fix ShelfAppBrowserTest.LauncherContextMenuVerifyCloseItemAppearance to use
the actual shelf ID of the browser shortcut, rather than using the magic
number "1" that can sometimes mean the app list item.

This also simplifies some of the unit tests for ChromeLauncherController
because they don't need to construct their own ShelfItemDelegateManager.

BUG=629990,429870
TEST=ash_unittests, chrome unit_tests, manually test shelf pin/unpin/etc.
==========

to

==========
mash: Fold ShelfItemDelegateManager into ShelfModel

This eliminates some accesses of ash::Shell inside the shelf code, which is
necessary for porting the shelf to mus/mustash.

* ShelfModel now owns the ShelfItemDelegates
* Explicitly delete the ShelfItemDelegates during shutdown, because some of
the subclasses reach into ShelfModel during their destructors
* Move AppListShelfItemDelegate earlier, to WmShell::Initialize phase
* Fix ShelfAppBrowserTest.LauncherContextMenuVerifyCloseItemAppearance to use
the actual shelf ID of the browser shortcut, rather than using the magic
number "1" that can sometimes mean the app list item.

This also simplifies some of the unit tests for ChromeLauncherController
because they don't need to construct their own ShelfItemDelegateManager.

BUG=629990,429870
TEST=ash_unittests, chrome unit_tests, manually test shelf pin/unpin/etc.

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

[commit-bot: I haz the power, 2016-07-22 18:03:14]

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