mash: Remove ShelfDelegate; move functions to ShelfModel.

mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ShelfDelegate, subclasses, accessors, etc.
Move ShelfDelegate responsibilities to ash::ShelfModel, add tests.

Move ChromeLauncherControllerImpl ownership to ChromeShellDelegate.
Add lifetime management functions: ShelfInit() and ShelfShutdown().
Apply similar ownership changes in tests; expose an AshTestHelper.

Remove ShelfDelegate responsibilities from ChromeLauncherController:
* Apply chrome-specific item info/delegates in ShelfItemAdded.
  - Support ShelfModel::SetItem calls amid ShelfItemAdded in ShelfView.
  - Register some item delegates early to avoid a default one.
* Cleanup chrome-side item info in ShelfItemRemoved.

TODO: Make ChromeLauncherControllerImpl lifetime more flexible.
TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.

BUG=557406, 698887
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2791463002
Cr-Commit-Position: refs/heads/master@{#465821}
Committed: https://chromium.googlesource.com/chromium/src/+/5138f3dd861b75ce64e9186b126c5fa8774a03e7

[msw, 2017-04-11 19:39:24]

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

[commit-bot: I haz the power, 2017-04-11 19:40:12]

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, 2017-04-11 19:44:24]

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

[commit-bot: I haz the power, 2017-04-11 19:44:25]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2017-04-11 21:47:30]

Patchset #2 (id:20001) has been deleted

[msw, 2017-04-11 21:47:39]

Patchset #3 (id:60001) has been deleted

[msw, 2017-04-12 19:19:18]

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

[commit-bot: I haz the power, 2017-04-12 19:20:20]

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, 2017-04-12 19:27:03]

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

[commit-bot: I haz the power, 2017-04-12 19:27:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2017-04-14 20:34:42]

Patchset #8 (id:180001) has been deleted

[msw, 2017-04-14 20:34:54]

Patchset #5 (id:120001) has been deleted

[msw, 2017-04-14 20:35:03]

Patchset #3 (id:80001) has been deleted

[msw, 2017-04-14 20:35:14]

Patchset #1 (id:1) has been deleted

[msw, 2017-04-14 20:35:43]

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

[commit-bot: I haz the power, 2017-04-14 20:36:06]

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, 2017-04-14 21:36:47]

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

[commit-bot: I haz the power, 2017-04-14 21:36:49]

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

[msw, 2017-04-17 20:11:10]

Description was changed from

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

BUG=557406
==========

to

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ash::ShelfDelegate; move its responsibilities to ash::ShelfModel.


TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.


BUG=557406,698887
==========

[msw, 2017-04-18 18:10:08]

Patchset #8 (id:260001) has been deleted

[msw, 2017-04-18 20:54:26]

Description was changed from

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ash::ShelfDelegate; move its responsibilities to ash::ShelfModel.


TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.


BUG=557406,698887
==========

to

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ShelfDelegate, subclasses, accessors, etc.
Move ShelfDelegate responsibilities to ash::ShelfModel.

Move ChromeLauncherControllerImpl ownership to ChromeShellDelegate.
Add lifetime management functions: ShelfInit() and ShelfShutdown().
Apply similar ownership changes in tests; expose an AshTestHelper.

Remove ShelfDelegate responsibilities from ChromeLauncherController:
* Apply chrome-specific item info/delegates in ShelfItemAdded.
  - Support ShelfModel::SetItem calls amid ShelfItemAdded in ShelfView.
  - Register some item delegates early to avoid a default one.
* Cleanup chrome-side item info in ShelfItemRemoved.

TODO: Make ChromeLauncherControllerImpl lifetime more flexible.
TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

[msw, 2017-04-18 20:54:26]

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

[msw, 2017-04-18 20:57:11]

Patchset #8 (id:280001) has been deleted

[msw, 2017-04-18 20:57:19]

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

[commit-bot: I haz the power, 2017-04-18 20:57:47]

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, 2017-04-18 21:16:09]

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

[commit-bot: I haz the power, 2017-04-18 21:16:10]

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

[msw, 2017-04-18 21:49:26]

Patchset #8 (id:300001) has been deleted

[msw, 2017-04-18 21:49:33]

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

[commit-bot: I haz the power, 2017-04-18 21:49:59]

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

[msw, 2017-04-18 22:53:48]

Hey James, please take a look; thanks!

[commit-bot: I haz the power, 2017-04-18 23:02:59]

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

[commit-bot: I haz the power, 2017-04-18 23:03:00]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-19 00:23:47]

I like it. Mostly questions.

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_model....
ash/shelf/shelf_model.cc:112: }
It would be nice if the tests for these functions migrated to
shelf_model_unittests.cc. Can you either move existing tests or add some new
ones?

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1538: if (static_cast<int>(model_->items().size()) !=
view_model_->view_size())
Are you sure this is safe? Is it only for tests or in production also? It seems
like we could miss an update. Is ChromeLauncherController sending an update
twice?

https://codereview.chromium.org/2791463002/diff/320001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/ash/shell.cc#newcode418
ash/shell.cc:418: shell_delegate_->ShelfInit();
Aside: It's unfortunate that ShelfInit() code has to be written to support being
called multiple times. Hopefully that will get better with lifetime changes.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:519: return
LauncherContextMenu::Create(launcher_controller_.get(), item,
Does this still need a null check for launcher_controller_?

The timing is a little unclear to me.

If it's definitely not null, DCHECK it.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:364:
if (browser->is_app() && browser->is_type_popup() && model &&
I don't think you need the null check, since Shell creates ShelfController in
its constructor and that creates ShelfModel.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:343:
// A test ChromeLauncherControllerImpl sublcass that uses TestShelfController.
nit: subclass

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:393:
// ash::test::TestShellDelegate:
optional: It might be clearer if ShelfInit() and ShelfShutdown() were
pure-virtual, so sites like this would be forced to show that ShelfInit() is
doing nothing, which is unusual.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:78:
ash_test_helper()->set_test_shell_delegate(shell_delegate_);
Ugh, this is tricky. It would be nice if set_test_shell_delegate() took
unique_ptr<> and ShellInitParams::delegate was a unique_ptr also. WDYT?
Follow-up CL?

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:336: }  //
namespace
Hooray for anonymous namespace!

[msw, 2017-04-19 20:56:07]

Comments addressed; please take another look; thanks!

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_model....
ash/shelf/shelf_model.cc:112: }
On 2017/04/19 00:23:47, James Cook wrote:
> It would be nice if the tests for these functions migrated to
> shelf_model_unittests.cc. Can you either move existing tests or add some new
> ones?

Done, I added new tests to exercise the new ShelfModel functions. Most of the
existing CLCImpl tests are a bit less straightforward. This exposed one nuanced
issue that prompted me to change behavior: GetShelfIDForAppID should not check
the launch id of the item (since none was provided in the inquiry).

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1538: if (static_cast<int>(model_->items().size()) !=
view_model_->view_size())
On 2017/04/19 00:23:47, James Cook wrote:
> Are you sure this is safe? Is it only for tests or in production also? It
seems
> like we could miss an update. Is ChromeLauncherController sending an update
> twice?

This is used for production and I'm reasonably confident that it's safe. This
ShelfItemChanged override just updates existing item views (and cancels any
ongoing dragging). No view exists for this item yet, so no updating is needed.
ShelfView's ShelfItemAdded override will be called imminently to add the new
view, and that pulls the latest ShelfModel/ShelfItem info (and cancels any
ongoing dragging). CLC isn't sending any updates twice, it's just updating an
item while observers are being notified of the item's addition.

https://codereview.chromium.org/2791463002/diff/320001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/ash/shell.cc#newcode418
ash/shell.cc:418: shell_delegate_->ShelfInit();
On 2017/04/19 00:23:47, James Cook wrote:
> Aside: It's unfortunate that ShelfInit() code has to be written to support
being
> called multiple times. Hopefully that will get better with lifetime changes.

Acknowledged.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/chrome_shell_delegate.cc:519: return
LauncherContextMenu::Create(launcher_controller_.get(), item,
On 2017/04/19 00:23:47, James Cook wrote:
> Does this still need a null check for launcher_controller_?
> 
> The timing is a little unclear to me.
> 
> If it's definitely not null, DCHECK it.

I restored the null check and comment.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:364:
if (browser->is_app() && browser->is_type_popup() && model &&
On 2017/04/19 00:23:47, James Cook wrote:
> I don't think you need the null check, since Shell creates ShelfController in
> its constructor and that creates ShelfModel.

Done.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:343:
// A test ChromeLauncherControllerImpl sublcass that uses TestShelfController.
On 2017/04/19 00:23:47, James Cook wrote:
> nit: subclass

Done.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:393:
// ash::test::TestShellDelegate:
On 2017/04/19 00:23:47, James Cook wrote:
> optional: It might be clearer if ShelfInit() and ShelfShutdown() were
> pure-virtual, so sites like this would be forced to show that ShelfInit() is
> doing nothing, which is unusual.

I don't think it's terribly helpful, but I made them pure virtual on the
interface. TestShellDelegate does actually instantiate the ShelfInitializer in
its ShelfInit() override, and this subclass needs to do the same init, so I
would generally still omit an override from this TestShellDelegate subclass, but
do you want me to explicitly override it here and just call the base class
ShelfInit() function impl?

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc (right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:78:
ash_test_helper()->set_test_shell_delegate(shell_delegate_);
On 2017/04/19 00:23:47, James Cook wrote:
> Ugh, this is tricky. It would be nice if set_test_shell_delegate() took
> unique_ptr<> and ShellInitParams::delegate was a unique_ptr also. WDYT?
> Follow-up CL?

Seems reasonable, but outside the scope of this CL.

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:336: }  //
namespace
On 2017/04/19 00:23:47, James Cook wrote:
> Hooray for anonymous namespace!

Acknowledged.

[msw, 2017-04-19 20:56:15]

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

[commit-bot: I haz the power, 2017-04-19 20:56: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, 2017-04-19 21:00:54]

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

[commit-bot: I haz the power, 2017-04-19 21:00:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2017-04-19 21:09:22]

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

[commit-bot: I haz the power, 2017-04-19 21:09:58]

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

[James Cook, 2017-04-19 21:17:55]

LGTM!

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2791463002/diff/320001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:393:
// ash::test::TestShellDelegate:
On 2017/04/19 20:56:07, msw wrote:
> On 2017/04/19 00:23:47, James Cook wrote:
> > optional: It might be clearer if ShelfInit() and ShelfShutdown() were
> > pure-virtual, so sites like this would be forced to show that ShelfInit() is
> > doing nothing, which is unusual.
> 
> I don't think it's terribly helpful, but I made them pure virtual on the
> interface. TestShellDelegate does actually instantiate the ShelfInitializer in
> its ShelfInit() override, and this subclass needs to do the same init, so I
> would generally still omit an override from this TestShellDelegate subclass,
but
> do you want me to explicitly override it here and just call the base class
> ShelfInit() function impl?

This is fine.

https://codereview.chromium.org/2791463002/diff/360001/ash/shelf/shelf_model_...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2791463002/diff/360001/ash/shelf/shelf_model_...
ash/shelf/shelf_model_unittest.cc:333: const std::string app_id1("app_id1");
nit: maybe app_id2 for the second one (and/or app_id1 for the first one)?

https://codereview.chromium.org/2791463002/diff/360001/ash/shelf/shelf_model_...
ash/shelf/shelf_model_unittest.cc:436: }
Nice tests. I always like the comment/do something/expect stuff style.

[msw, 2017-04-19 21:29:28]

Description was changed from

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ShelfDelegate, subclasses, accessors, etc.
Move ShelfDelegate responsibilities to ash::ShelfModel.

Move ChromeLauncherControllerImpl ownership to ChromeShellDelegate.
Add lifetime management functions: ShelfInit() and ShelfShutdown().
Apply similar ownership changes in tests; expose an AshTestHelper.

Remove ShelfDelegate responsibilities from ChromeLauncherController:
* Apply chrome-specific item info/delegates in ShelfItemAdded.
  - Support ShelfModel::SetItem calls amid ShelfItemAdded in ShelfView.
  - Register some item delegates early to avoid a default one.
* Cleanup chrome-side item info in ShelfItemRemoved.

TODO: Make ChromeLauncherControllerImpl lifetime more flexible.
TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

to

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ShelfDelegate, subclasses, accessors, etc.
Move ShelfDelegate responsibilities to ash::ShelfModel, add tests.

Move ChromeLauncherControllerImpl ownership to ChromeShellDelegate.
Add lifetime management functions: ShelfInit() and ShelfShutdown().
Apply similar ownership changes in tests; expose an AshTestHelper.

Remove ShelfDelegate responsibilities from ChromeLauncherController:
* Apply chrome-specific item info/delegates in ShelfItemAdded.
  - Support ShelfModel::SetItem calls amid ShelfItemAdded in ShelfView.
  - Register some item delegates early to avoid a default one.
* Cleanup chrome-side item info in ShelfItemRemoved.

TODO: Make ChromeLauncherControllerImpl lifetime more flexible.
TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

[msw, 2017-04-19 21:33:13]

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

[msw, 2017-04-19 21:33:18]

Scott, please take a look for missing chrome subdir OWNERS review:
-chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc
-chrome/browser/extensions/bookmark_app_helper.cc
-chrome/test/base/browser_with_test_window_test.*
Feel free to review more, but that's what technically needs OWNERS review.

https://codereview.chromium.org/2791463002/diff/360001/ash/shelf/shelf_model_...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2791463002/diff/360001/ash/shelf/shelf_model_...
ash/shelf/shelf_model_unittest.cc:333: const std::string app_id1("app_id1");
On 2017/04/19 21:17:54, James Cook wrote:
> nit: maybe app_id2 for the second one (and/or app_id1 for the first one)?

Done.

https://codereview.chromium.org/2791463002/diff/360001/ash/shelf/shelf_model_...
ash/shelf/shelf_model_unittest.cc:436: }
On 2017/04/19 21:17:54, James Cook wrote:
> Nice tests. I always like the comment/do something/expect stuff style.

Acknowledged.

[msw, 2017-04-19 21:33:34]

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

[commit-bot: I haz the power, 2017-04-19 21:34:15]

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, 2017-04-19 22:40:53]

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

[commit-bot: I haz the power, 2017-04-19 22:40:55]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-19 23:54:25]

LGTM - I only looked at the miscellaneous files James isn't an owner of.

[msw, 2017-04-20 00:12:40]

The CQ bit was checked by msw@chromium.org

[msw, 2017-04-20 00:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2791463002/#ps380001
(title: "Address comment.")

[commit-bot: I haz the power, 2017-04-20 00:13:25]

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, 2017-04-20 00:22:11]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1492647160731490,
"parent_rev": "bd1281da74251c1116fad952dc794a107e5ba1e3", "commit_rev":
"5138f3dd861b75ce64e9186b126c5fa8774a03e7"}

[commit-bot: I haz the power, 2017-04-20 00:23:22]

Description was changed from

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ShelfDelegate, subclasses, accessors, etc.
Move ShelfDelegate responsibilities to ash::ShelfModel, add tests.

Move ChromeLauncherControllerImpl ownership to ChromeShellDelegate.
Add lifetime management functions: ShelfInit() and ShelfShutdown().
Apply similar ownership changes in tests; expose an AshTestHelper.

Remove ShelfDelegate responsibilities from ChromeLauncherController:
* Apply chrome-specific item info/delegates in ShelfItemAdded.
  - Support ShelfModel::SetItem calls amid ShelfItemAdded in ShelfView.
  - Register some item delegates early to avoid a default one.
* Cleanup chrome-side item info in ShelfItemRemoved.

TODO: Make ChromeLauncherControllerImpl lifetime more flexible.
TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org,sky@chromium.org
==========

to

==========
mash: Remove ShelfDelegate; move functions to ShelfModel.

Remove ShelfDelegate, subclasses, accessors, etc.
Move ShelfDelegate responsibilities to ash::ShelfModel, add tests.

Move ChromeLauncherControllerImpl ownership to ChromeShellDelegate.
Add lifetime management functions: ShelfInit() and ShelfShutdown().
Apply similar ownership changes in tests; expose an AshTestHelper.

Remove ShelfDelegate responsibilities from ChromeLauncherController:
* Apply chrome-specific item info/delegates in ShelfItemAdded.
  - Support ShelfModel::SetItem calls amid ShelfItemAdded in ShelfView.
  - Register some item delegates early to avoid a default one.
* Cleanup chrome-side item info in ShelfItemRemoved.

TODO: Make ChromeLauncherControllerImpl lifetime more flexible.
TODO: Move functions to (or add accessors on) ash::[mojom::]ShelfController.

BUG=557406,698887
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2791463002
Cr-Commit-Position: refs/heads/master@{#465821}
Committed:
https://chromium.googlesource.com/chromium/src/+/5138f3dd861b75ce64e9186b126c...
==========

[commit-bot: I haz the power, 2017-04-20 00:23:23]

Committed patchset #11 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/5138f3dd861b75ce64e9186b126c...