Steven, is this related to the AppList data type?

Sadly we still don't use the AppList data type for pinning. See  issue 315820  for a related discussion.

Is this affecting all CHromebooks? 

This is not new behavior, and yes it affects all chromebooks.

There are a number of issues here, but fundamentally the way we sync the shelf items is cobbled together and largely broken.

 Issue 315820  was filed to track one possible solution.

I have suggested in the past that we could use the AppList sync type (AppListSyncableService) to include shelf data (e.g. a 'pinned' property and a shelf ordinal). AppList already handles OEM installed apps which makes it a good candidate.

This is not a quick / simple fix, but I would very much like to see happen. It will become a larger issue when we integrate Android apps with the app list and shelf (which we will presumably want to do).

Need to fix this for arc as well. +khmel who is working on a CL (https://codereview.chromium.org/2008363003/) try to address this.

Personally, I like the solution based on AppList sync to support app pins since it is the most flexible. Just not sure if we would make it in M53.

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

From a comment in the CL: https://codereview.chromium.org/2055553004/:

I am not entirely convinced that we need item_pin_by_policy. The logic ensuring that the policy apps are sorted first is complicated. I think we would be better off if we just created separate groups in the launcher and only paid attention to the local policy.


One question (and this is where a design doc would help): Do we allow the user to place user pinned apps between or before policy pinned apps?

If we do, then we only care about pinned_by_policy when we initially pin policy apps (i.e. they should be added before the first pinned non policy app).

If we do not, then we should just sort pinned_by_policy apps separately from regularly pinned apps when we build the launcher items.

WDYT?

Another question: Is the intention to use the IsValid() state of item_pin_ordinal to determine whether or not an app is pinned, or is that state stored separately?

+ dskaram for policy related question.

Re #12: I think he current status that does not allow user pins between policy pinned apps is not by design. It is so because we cannot properly implement it using one synced list of app pins. Ideally, policy pins can be mixed with user pins. The only difference is that policy pins are not allowed to be removed.

If answer to #14 is true,  this would simplify new pin implementation a lot. 

There is another Q for policy apps;

Scenario A.
  1. User has pin to App1.
  2. App1 is pinned by policy. 
  3. App1 is removed from the policy.
  4. Should we retain App1 in the pin shelf?

Scenario B.
  1. User has NO pin to App1.
  2. App1 is pinned by policy. 
  3. App1 is removed from the policy.
  4. Should we retain App1 in the pin shelf?

Can an app be removed from policy without it being uninstalled?

Even if so, it would seem reasonable to leave it pinned (but allow the user
to unpin it) in both cases. I don't think we should store historical case
for what sounds like an unlikely edge case.

My understanding is that the desire for the feature is to allow enterprises
to install and pin some apps by default, and that these apps should
(initially) show up before other apps.

Assuming that is the case, we should just add policy pinned apps before any
sync-pinned apps, which will keep the logic fairly simple. (One edge case
we may need to worry about is policy apps being pinned before app list sync
data arrives - in that case we will need to re-insert the policy pinned
apps in front of the sync pinned apps when the sync data arrives and then
add the policy pinned apps to the sync list).

So conclusion:

1. Pinned by policy apps may be moved in shelf by user with no restriction.
2. Pinned by policy apps may not be removed by user.
3. Pinned and then unpinned by policy apps stay in shelf and user may manually remove it.
4. If pinned by policy apps appear and they were not previously pinned then they go to the first position.
5. New pinned by policy apps appear in the same order as they were listed in prefs (if they were not present before).

Could you please confirm my understanding?

+tbuckley@ - Tom, can you get in touch with the appropriate stakeholders and confirm the desired behavior?

khmel@ - One thought I had that may (or may not) simplify the implementation: Until/unless a user moves a policy-pinned app, or moves a normal pinned app between policy-pinned app, there is no need to set or sync ordinals for policy-pinned apps. i.e. the logic can be:

* Add *unsynced* policy-pinned apps (i.e. policy pinned apps that do not have a pinned ordinal set) first.
* Add sync-pinned apps after.
* Set and sync the pinned ordinal for policy-pinned apps when they are moved or an app is inserted before them (if allowed).

That would have the advantage that "pinned and then unpinned by policy" apps (an edge case) would only remain pinned if the user moved them.

When you mention apps "pinned by policy", is that for the PinnedLauncherApps policy [1]? Its description makes it sound as though no other apps can be added, and the order is uneditable.

[1] http://dev.chromium.org/administrators/policy-list-3#PinnedLauncherApps

>> When you mention apps "pinned by policy", is that for the PinnedLauncherApps >> policy [1]

Yes, that is about.

That means we are not allowed to move pinned by policy apps and should keep legacy behavior.

Policy pinned apps were overriding user-pinned apps and that got fixed in  Issue 549083 . We simplified as much as possible by sticking all policy pinned apps at the start and not regarding too much the "unlikely edge cases".

Overall big +1 to keeping things super simple. Admins just care about the policy pinned apps reaching users. There's no real use case for user being able to re-order them, especially within their other personally pinned apps.



Some rough notes from discussions with UX are documented here: https://docs.google.com/document/d/1BuTdjN8168nD4i_FOn48IloV1ZOZTXe8XR6-LMgSDbE/edit

+fqj who did the actual implementation of the policy+user merged pins UX. 

I think it will be simpler to allow users to reorder policy pinned apps with other pinned apps. Otherwise we have to expressly prevent it in the UI. It sounds like that would be fine. (The initial ordering would put the policy pinned apps in front).

Yeah, I would prefer that. Having fixed pins that does not allow to be moved feels strange.

dskaram@, could you stamp from enterprise PM's perspective?

Re comments 24, 25, I am generally fine as the use case is admin wants to deliver a certain set of apps for their users and that is not affected by whether the apps can be moved around.

That said, the doc attached posed similar question to UX, and Jenn was clear on the answers.

https://docs.google.com/document/d/1BuTdjN8168nD4i_FOn48IloV1ZOZTXe8XR6-LMgSDbE/


Perhaps best to involve one of the UI folks here? My general preference when there is no strong opinion is to go with what's easier to implement.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b43aaa3b74115c142a51d7e16409bb2d9abafa8

commit 3b43aaa3b74115c142a51d7e16409bb2d9abafa8
Author: khmel <khmel@chromium.org>
Date: Tue Jun 21 02:39:46 2016

arc: Support pinned apps across Arc-enabled and Arc-disabled platforms.

This enables to work on different platforms and and have persistence
Arc app pins on Arc-enabled platforms.

BUG= 604871 
BUG=b/28017712
TEST=refactored and extended unit_tests passes
TEST=Manually on devices. Worked on 2 devices. One has Arc onboard and
     second does not. Pin/Unpin/Change pin order for Arc and legacy
     apps. Changes on one device reflected on another and order of
     legacy app is kept on both. Order of mix of legacy and arc apps
     is kept on Arc enabled device.

Review-Url: https://codereview.chromium.org/2055553004
Cr-Commit-Position: refs/heads/master@{#400876}

[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/chromeos/policy/configuration_policy_handler_chromeos_unittest.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/app_list/app_list_syncable_service.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/app_list/app_list_syncable_service.h
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/app_list/app_list_syncable_service_factory.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/app_list/app_list_syncable_service_factory.h
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/chrome_launcher_prefs.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/chrome_launcher_prefs.h
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.h
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/chrome_mash_shelf_controller.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/chrome/browser/ui/ash/launcher/launcher_controller_helper.h
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/sync/protocol/app_list_specifics.proto
[modify] https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8/sync/protocol/proto_value_conversions.cc

 Issue 315820  has been merged into this issue.

Let's verify this when we get a working M53 build 

This Works fine on non enrolled device. Verified 

Chrome 53.0.2785.123/8530.87.0. 

Marking verified as per comment #33