mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdatePolicyPinnedAppsFromPrefs.
(via UpdateAppLaunchersFromPref, called on changes to prefs::kPolicyPinnedLauncherApps)
Inline ChromeLauncherController::IsPinnable in LauncherContextMenu.

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org

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

[msw, 2016-12-30 00:48:36]

Description was changed from

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org

mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.
==========

to

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdateAppLaunchersFromPref.
(already called on changes to prefs::kPolicyPinnedLauncherApps)

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org
==========

[msw, 2016-12-30 00:53:43]

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

[commit-bot: I haz the power, 2016-12-30 00:53:57]

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-12-30 01:21:35]

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

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

Dry run: This issue passed the CQ dry run.

[msw, 2016-12-30 01:26:46]

Hey James, please take a look; thanks!

[James Cook, 2017-01-04 17:32:17]

LGTM with nits. If pulling out the utility function in
ChromeLauncherControllerImpl turns out to be complex I'm happy to take another
look.

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_item...
File ash/common/shelf/shelf_item_delegate.h (left):

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_item...
ash/common/shelf/shelf_item_delegate.h:53: virtual bool CanPin() const = 0;
Wow, the old code was confusing. This should have been CanUnpin(). I like your
approach better.

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:1354: std::string app_id =
delegate_->GetAppIDForShelfID(model_->items()[index].id);
nit: move this down near 1357

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:402: return
item && !item->pinned_by_policy &&
nit: The !pinned_by_policy part is a bit confusing. This could use a comment
that it really means that the user can toggle pin/unpin state, or rename the
function to something like CanUserPin().

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1212: //
Update the policy-pinned flag for each item on the shelf.
Not your fault, but this function is too long. Please put your code in a utility
function and/or refactor this function into smaller pieces.

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1216:
AppListControllerDelegate::PIN_FIXED;
optional: It's pretty confusing that PIN_FIXED lives in
AppListControllerDelegate. Maybe it should be a raw enum or enum class in
chrome_launcher_types.h

[msw, 2017-01-04 22:06:07]

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

[commit-bot: I haz the power, 2017-01-04 22:06:52]

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

[msw, 2017-01-04 22:23:04]

Comments addressed; please take another look, thanks!

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_item...
File ash/common/shelf/shelf_item_delegate.h (left):

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_item...
ash/common/shelf/shelf_item_delegate.h:53: virtual bool CanPin() const = 0;
On 2017/01/04 17:32:17, James Cook wrote:
> Wow, the old code was confusing. This should have been CanUnpin(). I like your
> approach better.

Acknowledged.

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2608013002/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:1354: std::string app_id =
delegate_->GetAppIDForShelfID(model_->items()[index].id);
On 2017/01/04 17:32:17, James Cook wrote:
> nit: move this down near 1357

Done.

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:402: return
item && !item->pinned_by_policy &&
On 2017/01/04 17:32:17, James Cook wrote:
> nit: The !pinned_by_policy part is a bit confusing. This could use a comment
> that it really means that the user can toggle pin/unpin state, or rename the
> function to something like CanUserPin().

Moved into LauncherContextMenu::IsCommandIdEnabled and commented.

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1212: //
Update the policy-pinned flag for each item on the shelf.
On 2017/01/04 17:32:17, James Cook wrote:
> Not your fault, but this function is too long. Please put your code in a
utility
> function and/or refactor this function into smaller pieces.

Done.

https://codereview.chromium.org/2608013002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1216:
AppListControllerDelegate::PIN_FIXED;
On 2017/01/04 17:32:17, James Cook wrote:
> optional: It's pretty confusing that PIN_FIXED lives in
> AppListControllerDelegate. Maybe it should be a raw enum or enum class in
> chrome_launcher_types.h

Doesn't make sense to move the enum in this cl.

[commit-bot: I haz the power, 2017-01-04 22:43:34]

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

[commit-bot: I haz the power, 2017-01-04 22:43:35]

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-01-04 23:57:08]

still lgtm assuming the test failure isn't anything important

[msw, 2017-01-05 00:26:49]

Description was changed from

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdateAppLaunchersFromPref.
(already called on changes to prefs::kPolicyPinnedLauncherApps)

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org
==========

to

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdatePolicyPinnedAppsFromPrefs.
(via UpdateAppLaunchersFromPref, called on changes to
prefs::kPolicyPinnedLauncherApps)
Inline ChromeLauncherController::IsPinnable in LauncherContextMenu.

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org
==========

[msw, 2017-01-05 00:27:43]

On 2017/01/04 23:57:08, James Cook wrote:
> still lgtm assuming the test failure isn't anything important

The test wasn't constructing the shelf items correctly; fixed.
Feel free to take another look; I'll check the cq bit now.

[msw, 2017-01-05 00:27:44]

On 2017/01/04 23:57:08, James Cook wrote:
> still lgtm assuming the test failure isn't anything important

The test wasn't constructing the shelf items correctly; fixed.
Feel free to take another look; I'll check the cq bit now.

[msw, 2017-01-05 00:28:01]

The CQ bit was checked by msw@chromium.org

[msw, 2017-01-05 00:28:02]

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/2608013002/#ps60001
(title: "Fix test.")

[commit-bot: I haz the power, 2017-01-05 00:28:47]

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-01-05 01:00:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483576081976850,
"parent_rev": "601b69c04e34a9ced2929ae7e3b6ac7de6e77a14", "commit_rev":
"45016d94ffab82e7956b3837c78f84a6d380b83a"}

[commit-bot: I haz the power, 2017-01-05 01:00:51]

Description was changed from

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdatePolicyPinnedAppsFromPrefs.
(via UpdateAppLaunchersFromPref, called on changes to
prefs::kPolicyPinnedLauncherApps)
Inline ChromeLauncherController::IsPinnable in LauncherContextMenu.

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org
==========

to

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdatePolicyPinnedAppsFromPrefs.
(via UpdateAppLaunchersFromPref, called on changes to
prefs::kPolicyPinnedLauncherApps)
Inline ChromeLauncherController::IsPinnable in LauncherContextMenu.

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2608013002
==========

[commit-bot: I haz the power, 2017-01-05 01:00:52]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdatePolicyPinnedAppsFromPrefs.
(via UpdateAppLaunchersFromPref, called on changes to
prefs::kPolicyPinnedLauncherApps)
Inline ChromeLauncherController::IsPinnable in LauncherContextMenu.

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2608013002
==========

to

==========
mash: Replace ShelfItemDelegate::CanPin with ShelfItem::pinned_by_policy.

Remove CanPin (ash shelf item delegation back to chrome).
Add a pinned_by_policy flag on the ShelfItem struct itself instead.
Update flags in ChromeLauncherControllerImpl::UpdatePolicyPinnedAppsFromPrefs.
(via UpdateAppLaunchersFromPref, called on changes to
prefs::kPolicyPinnedLauncherApps)
Inline ChromeLauncherController::IsPinnable in LauncherContextMenu.

TODO: Revise the policy-pinning code to make this more efficient.

BUG=557406
TEST=No changes to policy-pinned app shelf items.
R=jamescook@chromium.org

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

[commit-bot: I haz the power, 2017-01-05 01:04:02]

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

[James Cook, 2017-01-05 02:35:49]

lgtm