mash: Refactor ChromeLauncherController's ShelfDelegate code.

mash: Refactor ChromeLauncherController's ShelfDelegate code.

Simplify Chrome's ShelfDelegate code and rely on ShelfModel more.
(should help extract ShelfDelegate functionality from Chrome)
Make similar changes to TestShelfDelegate (rely on ShelfModel).

Remove marginally useful ShelfDelegate::HasShelfIDToAppIDMapping.
Inline Unpin, Pin, DoPinAppWithID, and DoUnpinAppWithID helper functions.
Inline TogglePinned functionality in LauncherContextMenu.

Rename UnpinAndUpdatePrefs->UnpinShelfItemInternal, nix pref changes.
(inline pref changes in PinAppWithID and UnpinAppWithID for now)

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

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

[msw, 2017-03-22 22:47:51]

Description was changed from

==========
mash: Refactor ChromeLauncherController's ShelfDelegate code.

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

to

==========
mash: Refactor ChromeLauncherController's ShelfDelegate code.

Simplify Chrome's ShelfDelegate code and rely on ShelfModel more.
(should help extract ShelfDelegate functionality from Chrome)

Remove marginally useful ShelfDelegate::HasShelfIDToAppIDMapping.
Inline ChromeLauncherControllerImpl::[Unpin|Pin]() helper functions.
Also inline DoPinAppWithID and DoUnpinAppWithID helper functions.

Rename UnpinAndUpdatePrefs->UnpinShelfItemInternal, nix pref changes.
(inline pref changes in PinAppWithID and UnpinAppWithID for now)
Inline TogglePinned functionality in LauncherContextMenu.

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

[msw, 2017-03-22 22:47:51]

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

[msw, 2017-03-22 22:49:21]

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

[commit-bot: I haz the power, 2017-03-22 22:50:16]

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-03-22 22:53:32]

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

[commit-bot: I haz the power, 2017-03-22 22:53:33]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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...)

[msw, 2017-03-22 23:04:16]

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

[commit-bot: I haz the power, 2017-03-22 23:05:31]

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

[msw, 2017-03-22 23:18:35]

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

[commit-bot: I haz the power, 2017-03-22 23:18:44]

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-03-23 00:07:18]

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

[commit-bot: I haz the power, 2017-03-23 00:07:19]

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-03-23 01:08:24]

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

[commit-bot: I haz the power, 2017-03-23 01:09:04]

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

[msw, 2017-03-23 01:10:16]

Description was changed from

==========
mash: Refactor ChromeLauncherController's ShelfDelegate code.

Simplify Chrome's ShelfDelegate code and rely on ShelfModel more.
(should help extract ShelfDelegate functionality from Chrome)

Remove marginally useful ShelfDelegate::HasShelfIDToAppIDMapping.
Inline ChromeLauncherControllerImpl::[Unpin|Pin]() helper functions.
Also inline DoPinAppWithID and DoUnpinAppWithID helper functions.

Rename UnpinAndUpdatePrefs->UnpinShelfItemInternal, nix pref changes.
(inline pref changes in PinAppWithID and UnpinAppWithID for now)
Inline TogglePinned functionality in LauncherContextMenu.

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

to

==========
mash: Refactor ChromeLauncherController's ShelfDelegate code.

Simplify Chrome's ShelfDelegate code and rely on ShelfModel more.
(should help extract ShelfDelegate functionality from Chrome)
Make similar changes to TestShelfDelegate (rely on ShelfModel).

Remove marginally useful ShelfDelegate::HasShelfIDToAppIDMapping.
Inline Unpin, Pin, DoPinAppWithID, and DoUnpinAppWithID helper functions.
Inline TogglePinned functionality in LauncherContextMenu.

Rename UnpinAndUpdatePrefs->UnpinShelfItemInternal, nix pref changes.
(inline pref changes in PinAppWithID and UnpinAppWithID for now)

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

[commit-bot: I haz the power, 2017-03-23 01:51:18]

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

[commit-bot: I haz the power, 2017-03-23 01:51:19]

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-03-23 02:39:09]

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

[commit-bot: I haz the power, 2017-03-23 02:39:33]

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-03-23 02:55:33]

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

[commit-bot: I haz the power, 2017-03-23 02:55:34]

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

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

Hey James, please take a look; thanks!

[James Cook, 2017-03-23 16:43:50]

I love it. One question about sync, otherwise just nits.

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

https://codereview.chromium.org/2771663002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_delegate.h:20: // Get the shelf ID from an application
ID.
nit: Maybe document that it returns kInvalidShelfId if the app doesn't exist
(since there's no HasShelfId function anymore)

https://codereview.chromium.org/2771663002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_delegate.h:33: // Get the application ID for a given
shelf ID.
nit: What does it return for a bogus/invalid shelf id?

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

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:353: void
ChromeLauncherControllerImpl::UnpinShelfItemInternal(ash::ShelfID id) {
nit: move to match header order (it's short, I don't care about the git blame
history loss)

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:806: const
std::string& app_id,
Not for this CL: WDYT of converting this to use AppLaunchId()?

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:810:
ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);
Wow, this is terrible. It implies that app_id is always an ARC id. At a minimum
this function needs to be renamed to something like UpdateShelfAppIdForArc() or
MaybeGetArcAppId(). Not for this CL, tho. I commented on the bug.

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:848:
SetItemType(shelf_id, ash::TYPE_PINNED_APP);
Do you need to trigger sync after this?

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:858: }
beautiful function, btw. I like the blocks with single-line comments on each.

[msw, 2017-03-23 18:08:10]

Please take another look; thanks!

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

https://codereview.chromium.org/2771663002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_delegate.h:20: // Get the shelf ID from an application
ID.
On 2017/03/23 16:43:49, James Cook wrote:
> nit: Maybe document that it returns kInvalidShelfId if the app doesn't exist
> (since there's no HasShelfId function anymore)

Done.

https://codereview.chromium.org/2771663002/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_delegate.h:33: // Get the application ID for a given
shelf ID.
On 2017/03/23 16:43:49, James Cook wrote:
> nit: What does it return for a bogus/invalid shelf id?

Documented that this should return an empty string in that case.

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

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:353: void
ChromeLauncherControllerImpl::UnpinShelfItemInternal(ash::ShelfID id) {
On 2017/03/23 16:43:49, James Cook wrote:
> nit: move to match header order (it's short, I don't care about the git blame
> history loss)

Left as-is. The ordering in this file is a mess... I'm not sure where to put it.
(I'd put it at the end, but that's where AppIconLoaderDelegate overrides live)

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:806: const
std::string& app_id,
On 2017/03/23 16:43:49, James Cook wrote:
> Not for this CL: WDYT of converting this to use AppLaunchId()?

I'm very much in favor of that; I thought about doing it in this CL.

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:810:
ArcAppWindowLauncherController::GetShelfAppIdFromArcAppId(app_id);
On 2017/03/23 16:43:49, James Cook wrote:
> Wow, this is terrible. It implies that app_id is always an ARC id. At a
minimum
> this function needs to be renamed to something like UpdateShelfAppIdForArc()
or
> MaybeGetArcAppId(). Not for this CL, tho. I commented on the bug.

Yeah, this is a bad pattern. Yury said he'll work on it.

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:848:
SetItemType(shelf_id, ash::TYPE_PINNED_APP);
On 2017/03/23 16:43:50, James Cook wrote:
> Do you need to trigger sync after this?

Good catch! I missed that! Fixed (but maybe I ruined the function's beauty)

https://codereview.chromium.org/2771663002/diff/100001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:858: }
On 2017/03/23 16:43:49, James Cook wrote:
> beautiful function, btw. I like the blocks with single-line comments on each.

Acknowledged.

[msw, 2017-03-23 18:08:36]

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

[commit-bot: I haz the power, 2017-03-23 18:09:26]

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-03-23 18:49:40]

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

[commit-bot: I haz the power, 2017-03-23 18:49:41]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-23 19:05:34]

LGTM

[msw, 2017-03-23 19:15:56]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2017-03-23 19:16:49]

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-03-23 19:25:27]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490296556864200,
"parent_rev": "0c1ab0868b12be571311df463894f06e47c8e62a", "commit_rev":
"bedb0ae2fde426fac9960793663e674d4fe26637"}

[commit-bot: I haz the power, 2017-03-23 19:26:11]

Description was changed from

==========
mash: Refactor ChromeLauncherController's ShelfDelegate code.

Simplify Chrome's ShelfDelegate code and rely on ShelfModel more.
(should help extract ShelfDelegate functionality from Chrome)
Make similar changes to TestShelfDelegate (rely on ShelfModel).

Remove marginally useful ShelfDelegate::HasShelfIDToAppIDMapping.
Inline Unpin, Pin, DoPinAppWithID, and DoUnpinAppWithID helper functions.
Inline TogglePinned functionality in LauncherContextMenu.

Rename UnpinAndUpdatePrefs->UnpinShelfItemInternal, nix pref changes.
(inline pref changes in PinAppWithID and UnpinAppWithID for now)

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

to

==========
mash: Refactor ChromeLauncherController's ShelfDelegate code.

Simplify Chrome's ShelfDelegate code and rely on ShelfModel more.
(should help extract ShelfDelegate functionality from Chrome)
Make similar changes to TestShelfDelegate (rely on ShelfModel).

Remove marginally useful ShelfDelegate::HasShelfIDToAppIDMapping.
Inline Unpin, Pin, DoPinAppWithID, and DoUnpinAppWithID helper functions.
Inline TogglePinned functionality in LauncherContextMenu.

Rename UnpinAndUpdatePrefs->UnpinShelfItemInternal, nix pref changes.
(inline pref changes in PinAppWithID and UnpinAppWithID for now)

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

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

[commit-bot: I haz the power, 2017-03-23 19:26:12]

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