mash: Move LauncherItemController to ash, rename ShelfItemDelegate.

mash: Move LauncherItemController to ash, rename ShelfItemDelegate.

Move chrome's LauncherItemController to ash and rename it.
Remove the ChromeLauncherController member; use the static getter.
Use it for existing ash subclasses and in ShelfModel's ownership.
(this will help eliminate CLC's |id_to_item_controller_map_|).

Fix up users and simplify old crud out of tests.

BUG=557406
TEST=Automated; no shelf item behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2791803002
Cr-Commit-Position: refs/heads/master@{#461964}
Committed: https://chromium.googlesource.com/chromium/src/+/e73759e7b2870df901e45294e19bdce9a47a60f6

[msw, 2017-03-31 23:51:51]

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

[msw, 2017-03-31 23:51:56]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-31 23:52:40]

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-01 00:56:19]

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

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

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-03 23:23:46]

Patchset #3 (id:60001) has been deleted

[msw, 2017-04-03 23:23:54]

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

[commit-bot: I haz the power, 2017-04-03 23:24:19]

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-04 01:33:35]

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

[commit-bot: I haz the power, 2017-04-04 01:33:36]

Dry run: This issue passed the CQ dry run.

[msw, 2017-04-04 02:51:43]

Description was changed from

==========
mash: Move LauncherItemController to ash, rename ShelfItemDelegate.

BUG=
==========

to

==========
mash: Move LauncherItemController to ash, rename ShelfItemDelegate.

Move chrome's LauncherItemController to ash and rename it.
Remove the ChromeLauncherController member; use the static getter.
Use it for existing ash subclasses and in ShelfModel's ownership.
(this will help eliminate CLC's |id_to_item_controller_map_|).

Fix up users and simplify old crud out of tests.

BUG=557406
TEST=Automated; no shelf item behavior changes.
R=jamescook@chromium.org
==========

[msw, 2017-04-04 02:51:43]

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

[msw, 2017-04-04 02:54:23]

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

[commit-bot: I haz the power, 2017-04-04 02:54:51]

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-04 02:55:09]

msw@chromium.org changed reviewers:
- khmel@chromium.org

[msw, 2017-04-04 02:55:10]

Hey James, please take a look; thanks!

[commit-bot: I haz the power, 2017-04-04 04:45:16]

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

[commit-bot: I haz the power, 2017-04-04 04:45:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[James Cook, 2017-04-04 15:34:45]

Looks good but exceeds my nit threshold. :-)

Also, I'm a little bummed that all ChromeLauncherController access is via the
singleton instance, since that makes it harder to test with dependency
injection. However, I'm OK with it because none of the existing tests use
dependency injection.

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_application_menu_model.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_application_menu_model.h:12: #include
"ash/public/cpp/shelf_item_delegate.h"
Can you forward declare this now?

(I like to avoid recursive include of mojom headers when I can.)

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_model.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_model.h:13: #include
"ash/public/cpp/shelf_item_delegate.h"
Can you forward declare this now?

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_model_observer.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_model_observer.h:40: // TODO(skuhne): Find the real
reason for this problem and remove this fix.
Aside: I wonder if this problem will go away. At some point might be worth
deleting this hack, adding a CHECK somewhere, and seeing what happens.

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_model_observer.h:42: ShelfItemDelegate* item_delegate) =
0;
forward declare

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
File ash/public/cpp/shelf_item_delegate.cc (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
ash/public/cpp/shelf_item_delegate.cc:11: shelf_id_(0),
nit: Is there a constant for invalid?

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
File ash/public/cpp/shelf_item_delegate.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
ash/public/cpp/shelf_item_delegate.h:26: class ASH_PUBLIC_EXPORT
ShelfItemDelegate : public mojom::ShelfItemDelegate {
Just curious, any particular reason ash::ShelfItemDelegate continues to derive
from ash::mojom::ShelfItemDelegate? Easier to convert the code in chrome that
derived from LauncherItemController? Keeps the idea that mojom::Foo is
implemented by Foo?

It just feels a bit odd that it doesn't do anything with the mojom methods.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:51: //
experiment goes away.
Aside: I wonder if this "experiment" is still around.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:78: virtual
ash::ShelfID CreateAppLauncherItem(ash::ShelfItemDelegate* controller,
kinda weird that these item delegates are named "controller". Maybe
item_delegate?

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:93: // TODO(skuhne):
Pass in scoped_ptr to make ownership clear.
Is now a good time to resolve this TODO?

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:205: virtual
ash::ShelfItemDelegate* GetLauncherItemController(
Likewise, continuing to call this GetLauncherItemController() is weird. This
needs to be renamed, either in this CL or a followup.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:330:
ash::ShelfItemDelegate* controller) {
ditto on all these locals named "controller"

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:2297:
// TODO(crbug.com/654622): This test breaks with a non-null static instance.
Woo hoo, these are going away!

[James Cook, 2017-04-04 15:35:22]

Also, I am in an all-day meeting today. I'll try to check mail after lunch, but
I can't guarantee a response until end of day.

[msw, 2017-04-04 18:53:09]

Comments addressed, please take another look; thanks!

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_application_menu_model.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_application_menu_model.h:12: #include
"ash/public/cpp/shelf_item_delegate.h"
On 2017/04/04 15:34:44, James Cook (OOO) wrote:
> Can you forward declare this now?
> 
> (I like to avoid recursive include of mojom headers when I can.)

Done for ShelfItemDelegate, but we need shelf.mojom.h for mojom::MenuItemPtr.

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_model.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_model.h:13: #include
"ash/public/cpp/shelf_item_delegate.h"
On 2017/04/04 15:34:44, James Cook (OOO) wrote:
> Can you forward declare this now?

Done.

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_model_observer.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_model_observer.h:40: // TODO(skuhne): Find the real
reason for this problem and remove this fix.
On 2017/04/04 15:34:44, James Cook (OOO) wrote:
> Aside: I wonder if this problem will go away. At some point might be worth
> deleting this hack, adding a CHECK somewhere, and seeing what happens.

I plan to remove the map in my next CL, that should make this moot.

https://codereview.chromium.org/2791803002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_model_observer.h:42: ShelfItemDelegate* item_delegate) =
0;
On 2017/04/04 15:34:44, James Cook (OOO) wrote:
> forward declare

Done.

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
File ash/public/cpp/shelf_item_delegate.cc (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
ash/public/cpp/shelf_item_delegate.cc:11: shelf_id_(0),
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> nit: Is there a constant for invalid?

Done. (kInvalidShelfID)

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
File ash/public/cpp/shelf_item_delegate.h (right):

https://codereview.chromium.org/2791803002/diff/100001/ash/public/cpp/shelf_i...
ash/public/cpp/shelf_item_delegate.h:26: class ASH_PUBLIC_EXPORT
ShelfItemDelegate : public mojom::ShelfItemDelegate {
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> Just curious, any particular reason ash::ShelfItemDelegate continues to derive
> from ash::mojom::ShelfItemDelegate? Easier to convert the code in chrome that
> derived from LauncherItemController? Keeps the idea that mojom::Foo is
> implemented by Foo?
> 
> It just feels a bit odd that it doesn't do anything with the mojom methods.

Good point; we could use composition instead of inheritance. That'd be a bit
much for this CL, but worth considering for a followup. For now, this CL helps
reduce ChromeLauncherContorllerImpl's responsibilities.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:51: //
experiment goes away.
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> Aside: I wonder if this "experiment" is still around.

No idea, I can followup as needed.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:78: virtual
ash::ShelfID CreateAppLauncherItem(ash::ShelfItemDelegate* controller,
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> kinda weird that these item delegates are named "controller". Maybe
> item_delegate?

Done.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:93: // TODO(skuhne):
Pass in scoped_ptr to make ownership clear.
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> Is now a good time to resolve this TODO?

Done, but what a pain... this wasn't simple.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:205: virtual
ash::ShelfItemDelegate* GetLauncherItemController(
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> Likewise, continuing to call this GetLauncherItemController() is weird. This
> needs to be renamed, either in this CL or a followup.

Done. Sadly, these renames take time...

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:330:
ash::ShelfItemDelegate* controller) {
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> ditto on all these locals named "controller"

Done.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:2297:
// TODO(crbug.com/654622): This test breaks with a non-null static instance.
On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> Woo hoo, these are going away!

Acknowledged. (there is one left for an ARC test...)

[msw, 2017-04-04 18:53:17]

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

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

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-04 19:20:40]

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

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

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-04 20:46:02]

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

[commit-bot: I haz the power, 2017-04-04 20:46:03]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-04 23:57:27]

LGTM. Nice CL.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:93: // TODO(skuhne):
Pass in scoped_ptr to make ownership clear.
On 2017/04/04 18:53:09, msw wrote:
> On 2017/04/04 15:34:45, James Cook (OOO) wrote:
> > Is now a good time to resolve this TODO?
> 
> Done, but what a pain... this wasn't simple.

Thanks for doing it.

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2791803002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:351: //
Existing controller is destroyed and replaced by registering again.
Nice to see junk like this going away.

[msw, 2017-04-05 00:10:22]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2017-04-05 00:11:06]

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-05 03:18:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491351022585650,
"parent_rev": "b4087e5552eff901d0b4183f86ef4f045e0a876c", "commit_rev":
"e73759e7b2870df901e45294e19bdce9a47a60f6"}

[commit-bot: I haz the power, 2017-04-05 03:19:53]

Description was changed from

==========
mash: Move LauncherItemController to ash, rename ShelfItemDelegate.

Move chrome's LauncherItemController to ash and rename it.
Remove the ChromeLauncherController member; use the static getter.
Use it for existing ash subclasses and in ShelfModel's ownership.
(this will help eliminate CLC's |id_to_item_controller_map_|).

Fix up users and simplify old crud out of tests.

BUG=557406
TEST=Automated; no shelf item behavior changes.
R=jamescook@chromium.org
==========

to

==========
mash: Move LauncherItemController to ash, rename ShelfItemDelegate.

Move chrome's LauncherItemController to ash and rename it.
Remove the ChromeLauncherController member; use the static getter.
Use it for existing ash subclasses and in ShelfModel's ownership.
(this will help eliminate CLC's |id_to_item_controller_map_|).

Fix up users and simplify old crud out of tests.

BUG=557406
TEST=Automated; no shelf item behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2791803002
Cr-Commit-Position: refs/heads/master@{#461964}
Committed:
https://chromium.googlesource.com/chromium/src/+/e73759e7b2870df901e45294e19b...
==========

[commit-bot: I haz the power, 2017-04-05 03:19:54]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/e73759e7b2870df901e45294e19b...