mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Remove CLC::id_to_item_controller_map_; rely on ShelfModel instead.
Skip removing shelf items in CLC destruction; rely on ~ShelfModel instead.

Remove CLC::[Get|Set]ShelfItemDelegate helpers; inline ShelfModel calls.
Remove moot ShelfModelObserver::OnSetShelfItemDelegate.

Rename CLC::LauncherItemClosed -> RemoveShelfItem.
Rename test helper GetAppLaunchers -> GetPinnedAppIds.

Inline ShelfModel::RemoveShelfItemDelegate helper function.
Avoid GetItemIndexForType, use GetShelfIDForAppID instead.
Use ItemTypeIsPinned helper function.

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

Review-Url: https://codereview.chromium.org/2798173002
Cr-Commit-Position: refs/heads/master@{#463030}
Committed: https://chromium.googlesource.com/chromium/src/+/4a3c245c97370cf05dcbe6e9dae89936042293b1

[msw, 2017-04-05 20:56:53]

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

[commit-bot: I haz the power, 2017-04-05 20:57:24]

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-05 22:54:36]

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

[commit-bot: I haz the power, 2017-04-05 22:54:37]

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, 2017-04-07 18:56:57]

Description was changed from

==========
mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Support window destruction in AppWindowLauncherItemController.

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

to

==========
mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Remove CLC::id_to_item_controller_map_; rely on ShelfModel instead.
Remove CLC::[Get|Set]ShelfItemDelegate helpers; inline ShelfModel calls.
Remove moot ShelfModelObserver::OnSetShelfItemDelegate.

Rename CLC::LauncherItemClosed -> RemoveShelfItem.
Rename test helper GetAppLaunchers -> GetPinnedAppIds.

Inline ShelfModel::RemoveShelfItemDelegate helper function.
Avoid GetItemIndexForType, use GetShelfIDForAppID instead.
Use ItemTypeIsPinned helper function.

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

[msw, 2017-04-07 18:56:57]

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

[msw, 2017-04-07 19:14:16]

Description was changed from

==========
mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Remove CLC::id_to_item_controller_map_; rely on ShelfModel instead.
Remove CLC::[Get|Set]ShelfItemDelegate helpers; inline ShelfModel calls.
Remove moot ShelfModelObserver::OnSetShelfItemDelegate.

Rename CLC::LauncherItemClosed -> RemoveShelfItem.
Rename test helper GetAppLaunchers -> GetPinnedAppIds.

Inline ShelfModel::RemoveShelfItemDelegate helper function.
Avoid GetItemIndexForType, use GetShelfIDForAppID instead.
Use ItemTypeIsPinned helper function.

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

to

==========
mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Remove CLC::id_to_item_controller_map_; rely on ShelfModel instead.
Skip removing shelf items in CLC destruction; rely on ~ShelfModel instead.

Remove CLC::[Get|Set]ShelfItemDelegate helpers; inline ShelfModel calls.
Remove moot ShelfModelObserver::OnSetShelfItemDelegate.

Rename CLC::LauncherItemClosed -> RemoveShelfItem.
Rename test helper GetAppLaunchers -> GetPinnedAppIds.

Inline ShelfModel::RemoveShelfItemDelegate helper function.
Avoid GetItemIndexForType, use GetShelfIDForAppID instead.
Use ItemTypeIsPinned helper function.

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

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

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

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

Hey James, please take a look; thanks!

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

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-07 19:49:41]

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

[commit-bot: I haz the power, 2017-04-07 19:49:42]

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_...)

[James Cook, 2017-04-07 20:42:22]

Love it. LGTM assuming the compile failures in the browser test aren't anything
significant.

https://codereview.chromium.org/2798173002/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model_observer.h (left):

https://codereview.chromium.org/2798173002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model_observer.h:36: // TODO(skuhne): Find the real
reason for this problem and remove this fix.
Hooray!

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

https://codereview.chromium.org/2798173002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:664:
shelf_model->SetShelfItemDelegate(shelf_id, std::move(controller));
optional: I would be OK with keeping a CLC::SetShelfItemDelegate() that forwards
through to shelf_model->SSID(). OTOH, if you think owner()->SetItemStatus() will
move out of CLC, then this is definitely the right way to go.

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

https://codereview.chromium.org/2798173002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:471: return id
== ash::kInvalidShelfID ? GetShelfIDForAppID(kChromeAppId) : id;
nit: maybe a comment here about why invalid id means browser shortcut? (I
presume it's to show the white dot under the browser shortcut when |contents| is
not due to a web app.

[msw, 2017-04-07 22:02:16]

Take another look if you're interested; I fixed mostly trivial browser test
compile errors and expectations, and fixed empty app_id handling in
GetShelfIDForAppIDAndLaunchID (it was matching empty to the app list... I might
give the app list item a fake/placeholder app id in the future).

https://codereview.chromium.org/2798173002/diff/40001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model_observer.h (left):

https://codereview.chromium.org/2798173002/diff/40001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model_observer.h:36: // TODO(skuhne): Find the real
reason for this problem and remove this fix.
On 2017/04/07 20:42:22, James Cook wrote:
> Hooray!

Acknowledged.

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

https://codereview.chromium.org/2798173002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:664:
shelf_model->SetShelfItemDelegate(shelf_id, std::move(controller));
On 2017/04/07 20:42:22, James Cook wrote:
> optional: I would be OK with keeping a CLC::SetShelfItemDelegate() that
forwards
> through to shelf_model->SSID(). OTOH, if you think owner()->SetItemStatus()
will
> move out of CLC, then this is definitely the right way to go.

Both belong on our ShelfController ash mojo interface in the long run, but we
might just use ShelfModel for a model mirroring approach. I'll keep these using
ShelfModel for now; unfortunately there will likely be some churn either way.
lmk if you'd prefer that I restore the CLC helpers; it is a bit of a toss up.

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

https://codereview.chromium.org/2798173002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:471: return id
== ash::kInvalidShelfID ? GetShelfIDForAppID(kChromeAppId) : id;
On 2017/04/07 20:42:22, James Cook wrote:
> nit: maybe a comment here about why invalid id means browser shortcut? (I
> presume it's to show the white dot under the browser shortcut when |contents|
is
> not due to a web app.

Done.

[msw, 2017-04-07 22:02:22]

The CQ bit was checked by msw@chromium.org

[msw, 2017-04-07 22:02:23]

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/2798173002/#ps60001
(title: "Address comment; fix browser tests.")

[commit-bot: I haz the power, 2017-04-07 22:03:28]

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-07 22:56:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491602542837860,
"parent_rev": "46652c871ed3fd21804bc01886b9a7cf6e57009e", "commit_rev":
"4a3c245c97370cf05dcbe6e9dae89936042293b1"}

[commit-bot: I haz the power, 2017-04-07 22:57:24]

Description was changed from

==========
mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Remove CLC::id_to_item_controller_map_; rely on ShelfModel instead.
Skip removing shelf items in CLC destruction; rely on ~ShelfModel instead.

Remove CLC::[Get|Set]ShelfItemDelegate helpers; inline ShelfModel calls.
Remove moot ShelfModelObserver::OnSetShelfItemDelegate.

Rename CLC::LauncherItemClosed -> RemoveShelfItem.
Rename test helper GetAppLaunchers -> GetPinnedAppIds.

Inline ShelfModel::RemoveShelfItemDelegate helper function.
Avoid GetItemIndexForType, use GetShelfIDForAppID instead.
Use ItemTypeIsPinned helper function.

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

to

==========
mash: Remove ChromeLauncherController's |id_to_item_controller_map_|.

Remove CLC::id_to_item_controller_map_; rely on ShelfModel instead.
Skip removing shelf items in CLC destruction; rely on ~ShelfModel instead.

Remove CLC::[Get|Set]ShelfItemDelegate helpers; inline ShelfModel calls.
Remove moot ShelfModelObserver::OnSetShelfItemDelegate.

Rename CLC::LauncherItemClosed -> RemoveShelfItem.
Rename test helper GetAppLaunchers -> GetPinnedAppIds.

Inline ShelfModel::RemoveShelfItemDelegate helper function.
Avoid GetItemIndexForType, use GetShelfIDForAppID instead.
Use ItemTypeIsPinned helper function.

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

Review-Url: https://codereview.chromium.org/2798173002
Cr-Commit-Position: refs/heads/master@{#463030}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a3c245c97370cf05dcbe6e9dae8...
==========

[commit-bot: I haz the power, 2017-04-07 22:57:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4a3c245c97370cf05dcbe6e9dae8...

[James Cook, 2017-04-07 23:12:18]

still lgtm

[James Cook, 2017-04-08 04:41:22]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2800263002/ by jamescook@chromium.org.

The reason for reverting is: Seems to be causing unit test failures on Linux
Chrome OS debug builder, see crbug.com/709729

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....