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

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.

Committed: https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8
Cr-Commit-Position: refs/heads/master@{#400876}

[khmel, 2016-06-09 21:40:21]

khmel@chromium.org changed reviewers:
+ skuhne@chromium.org, xiyuan@chromium.org

[khmel, 2016-06-09 21:40:22]

Hi Xiyuan and Stefan,

As discussed our pin model has to be based on sync model.

Some top level comments:
I tried different scenarios.

First I decided to implement separate pin model that was responsible for pins
only (syncer::TYPE_APP_PIN). I did prototype but found that server change was
required as well. This would complicate everything and after offline discussion
with Xiyuan we decided to extend app_list_sync_service. Pin position is relative
small update and this looks OK.

My next idea was to move app_list_sync_service to app_sync_service and left
there only low level synchronization and move app_list code to separate service
that uses underlying app_sync_service and similar for app_pin_service. However
this touches a lot of legacy code and complexity of CL is far from acceptable.
Anyway this could be task as separate CL if we decide to do so.

So now I have pin support in app_list_sync_service

PTAL.

https://codereview.chromium.org/2055553004/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (left):

https://codereview.chromium.org/2055553004/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:372: if
(arc::ArcAuthService::IsAllowedForProfile(helper->profile()) &&
This is related fix, we need Arc Support App unpinable. We only create pin on
Arc init to highlight the feature.

https://codereview.chromium.org/2055553004/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (left):

https://codereview.chromium.org/2055553004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:462: void
PersistChromeItemIndex(int index);
No longer used, it position specified by ordinal now.

https://codereview.chromium.org/2055553004/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:469: void
MoveChromeOrApplistToFinalPosition(
No longer used. App list is always at pos 0 and Chrome item can be positioned
using normal flow.

[xiyuan, 2016-06-09 21:51:22]

xiyuan@chromium.org changed reviewers:
+ stevenjb@chromium.org

[xiyuan, 2016-06-09 21:51:23]

+stevenjb original author of AppListSyncableService

[stevenjb, 2016-06-10 17:09:16]

Is there a crbug.com issue tracking this? This is a chrome change and not Arc
specific, we should track it in the chromium tracker.

We should link any design docs to that issue, and in particular can we make sure
that the "pinned by policy" property is explained (e.g. why we need it?). I
assume it is because a user may have a policy enforcing pinning on one device
and not another? It is not obvious what the desired behavior in that case is.

[khmel, 2016-06-10 17:12:31]

Description was changed from

==========
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=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.
==========

to

==========
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.
==========

[khmel, 2016-06-10 17:15:35]

On 2016/06/10 17:09:16, stevenjb wrote:
> Is there a http://crbug.com issue tracking this? This is a chrome change and
not Arc
> specific, we should track it in the chromium tracker.
> 
> We should link any design docs to that issue, and in particular can we make
sure
> that the "pinned by policy" property is explained (e.g. why we need it?). I
> assume it is because a user may have a policy enforcing pinning on one device
> and not another? It is not obvious what the desired behavior in that case is.

I added crbug to this issue. I followed legacy behavior how we handle policy
apps. There are some unit tests for this. I also did not change logic of any pin
unit tests. They pass with this new implementation.

Do you want me to write design doc prior to this CL?

[stevenjb, 2016-06-10 18:00:41]

Thanks for taking this on.

I only looked closely at the app_list_syncable_service changes and some of the
chrome_launcher_prefs changes.

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. I will add a similar comment to the tracking issuse and we
can discuss this more there.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:105:
item->item_pin_ordinal.ToInternalValue() : std::string());
We should modify the logic lines 102/103 to match this.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:248: }
This is unnecessary, we should just use
AppListSyncableServiceFactory::GetForProfile(profile) where we need it.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:901:
model_->SetItemPosition(app_item, sync_item->item_ordinal);
{}

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.h:64: bool
item_pin_by_policy;
item_pinned_by_policy

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.h:109: bool
GetPinByPolicy(const std::string& app_id);
GetPinnedByPolicy

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:356: std::vector<std::string>
GetPinnedAppsFromPrefsDepricated(
s/Depricated/Deprecated/, but also maybe use Legacy instead of Deprecated since
we won't be able to fully deprecate the pref store for several releases.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:413: // Helper to creates pin
position that stays before any synced app, even if
s/creates/create/

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:466: // time. In normal workflow
we have at least Chrom browser pin info.
Chrome

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:474: // Don't include apps that
are currently do not exist on device.
s/are//

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:481: sync_peer.first) !=
policy_apps.end();
If the app list gets large this could be expensive. We should use a set to store
the policy app ids (or sort the vector).

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:490: if (first_run) {
We should wrap this into a separate function, e.g.
if (first_run)
  return ImportLagacyPinnedApps(...);

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:502: legacy_pins.end());
We are already iterating over legacy_pins below, we should just use a
traditional iterator and erase the kPinnedAppsPlaceholder placeholder there.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:510: :
syncer::StringOrdinal::CreateInitialOrdinal();
nit: Just initialize last_position to
syncer::StringOrdinal::CreateInitialOrdinal()

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:513: policy_apps.end();
This will also avoid an N^2 operation if policy_apps is a set.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:526:
DCHECK(last_position.IsValid());
Just check this once above.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:534: // Sort pins according its
ordinals.
s/its/their/

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:549: // Validate that there have
correct position and fix if needed.
s/there have correct/they have a correct/

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.h:79: const std::string&
app_id_after);
These functions should really all be in a separate namespace, e.g.
ash::launcher. 'ash' is very large and the context of 'SetPinPosition' is not
necessarily obvious.

https://codereview.chromium.org/2055553004/diff/20001/sync/protocol/app_list_...
File sync/protocol/app_list_specifics.proto (right):

https://codereview.chromium.org/2055553004/diff/20001/sync/protocol/app_list_...
sync/protocol/app_list_specifics.proto:55: optional bool item_pin_by_policy = 8;
item_pinned_by_policy, but I am not sure we need this, see my top level comment.

[khmel, 2016-06-10 22:08:11]

Hi Steven,

Thanks a lot for your comments.

I've updated the code.
So based on discussion in crbug we are not allowed to change the order of pinned
by policy app. So legacy design is still valid.

I dropped pinned_by_policy flag. It was needed to support following scenario:
  1. User has no pin to App1.
  2. App1 is pinned by policy. 
  3. App1 is removed from the policy.
  4. App1 pin is removed from shelf.
Now 4 is not true. I am not sure how this is important. I also needed to update
ChromeLauncherControllerTest, Policy to fit with this change.

PS I've started design doc but it may take a time.

PTAL

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:105:
item->item_pin_ordinal.ToInternalValue() : std::string());
On 2016/06/10 18:00:40, stevenjb wrote:
> We should modify the logic lines 102/103 to match this.

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:248: }
On 2016/06/10 18:00:40, stevenjb wrote:
> This is unnecessary, we should just use
> AppListSyncableServiceFactory::GetForProfile(profile) where we need it.

Sorry, I just added this helper similar to some other factory I worked with.
Removed

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:901:
model_->SetItemPosition(app_item, sync_item->item_ordinal);
On 2016/06/10 18:00:40, stevenjb wrote:
> {}

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.h (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.h:64: bool
item_pin_by_policy;
On 2016/06/10 18:00:40, stevenjb wrote:
> item_pinned_by_policy

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.h:109: bool
GetPinByPolicy(const std::string& app_id);
On 2016/06/10 18:00:40, stevenjb wrote:
> GetPinnedByPolicy

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:356: std::vector<std::string>
GetPinnedAppsFromPrefsDepricated(
On 2016/06/10 18:00:41, stevenjb wrote:
> s/Depricated/Deprecated/, but also maybe use Legacy instead of Deprecated
since
> we won't be able to fully deprecate the pref store for several releases.

Legacy sounds better, thanks

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:413: // Helper to creates pin
position that stays before any synced app, even if
On 2016/06/10 18:00:40, stevenjb wrote:
> s/creates/create/

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:466: // time. In normal workflow
we have at least Chrom browser pin info.
On 2016/06/10 18:00:41, stevenjb wrote:
> Chrome

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:474: // Don't include apps that
are currently do not exist on device.
On 2016/06/10 18:00:40, stevenjb wrote:
> s/are//

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:490: if (first_run) {
On 2016/06/10 18:00:41, stevenjb wrote:
> We should wrap this into a separate function, e.g.
> if (first_run)
>   return ImportLagacyPinnedApps(...);

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:502: legacy_pins.end());
On 2016/06/10 18:00:41, stevenjb wrote:
> We are already iterating over legacy_pins below, we should just use a
> traditional iterator and erase the kPinnedAppsPlaceholder placeholder there.

Done.
Refactored GetPinnedAppsFromPrefsLegacy not to return kPinnedAppsPlaceholder,
GetPinnedAppsFromPrefsLegacy is now private helper and this won't affect other
code.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:510: :
syncer::StringOrdinal::CreateInitialOrdinal();
On 2016/06/10 18:00:40, stevenjb wrote:
> nit: Just initialize last_position to
> syncer::StringOrdinal::CreateInitialOrdinal()

That is nicer, thanks

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:513: policy_apps.end();
On 2016/06/10 18:00:41, stevenjb wrote:
> This will also avoid an N^2 operation if policy_apps is a set.

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:526:
DCHECK(last_position.IsValid());
On 2016/06/10 18:00:41, stevenjb wrote:
> Just check this once above.

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:534: // Sort pins according its
ordinals.
On 2016/06/10 18:00:40, stevenjb wrote:
> s/its/their/

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:549: // Validate that there have
correct position and fix if needed.
On 2016/06/10 18:00:40, stevenjb wrote:
> s/there have correct/they have a correct/

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:549: // Validate that there have
correct position and fix if needed.
On 2016/06/10 18:00:40, stevenjb wrote:
> s/there have correct/they have a correct/

Done.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.h:79: const std::string&
app_id_after);
On 2016/06/10 18:00:41, stevenjb wrote:
> These functions should really all be in a separate namespace, e.g.
> ash::launcher. 'ash' is very large and the context of 'SetPinPosition' is not
> necessarily obvious.

Done.

https://codereview.chromium.org/2055553004/diff/20001/sync/protocol/app_list_...
File sync/protocol/app_list_specifics.proto (right):

https://codereview.chromium.org/2055553004/diff/20001/sync/protocol/app_list_...
sync/protocol/app_list_specifics.proto:55: optional bool item_pin_by_policy = 8;
On 2016/06/10 18:00:41, stevenjb wrote:
> item_pinned_by_policy, but I am not sure we need this, see my top level
comment.

Done

[khmel, 2016-06-14 15:44:44]

Hi,

Gentle ping,
Do we have any stopper not to review it?

PTAL

[xiyuan, 2016-06-14 21:42:51]

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:29: bool
used_in_testing = false;
nit: used_in_testing -> use_in_testing to be consistent with SetUseInTesting.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:33: void
AppListSyncableServiceFactory::SetUseInTesting() {
nit: either move this down after BuildInstanceFor, or move it up in header file
so that the order in header and cc file matches.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:226: class AppList {
nit: Can we get a better name? AppList collides with, errr AppList. How about
call it AppTracker?

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:608: for (const auto& app_id :
policy_apps.app_list()) {
Would this be enforced every time? If a user moves a policy pinned app, would
the new position be kept after this code runs again (e.g. on the next login)?

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

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:340:
s->extension_service()->Init();
Line 338-340 could be just

extension_service_->Init();

right?

[stevenjb, 2016-06-14 22:44:09]

This looks pretty close. Just one subtle sync issue, and one suggestion: allow
policy pinned apps to be re-ordered and preserve that ordering.

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2055553004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:248: }
On 2016/06/10 22:08:10, khmel wrote:
> On 2016/06/10 18:00:40, stevenjb wrote:
> > This is unnecessary, we should just use
> > AppListSyncableServiceFactory::GetForProfile(profile) where we need it.
> 
> Sorry, I just added this helper similar to some other factory I worked with.
> Removed

It's not inherently wrong, but we do try to go by a "less code is better"
policy, so adding a helper just to save an extra include usually fails that
litmus test, unless the getter is used widely.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:69:
item->item_pin_ordinal = syncer::StringOrdinal(specifics.item_pin_ordinal());
An unfortunate side effect of this is that if an older version of chrome updates
a sync item (e.g. because it was moved) it would inadvertantly unpin the item.

Looking back through the sync code, I think this is what we want for both
item_ordinal and item_pin_ordinal:

if (specifics.has_item_pin_ordinal())
  item->item_pin_ordinal = syncer::StringOrdinal(specifics.item_pin_ordinal());

That way if the value is unset (vs. empty), any existing state will not be
overwritten.

(For item_ordinal those are synonymous since item_ordinal.empty() has no special
meaning, whereas with item_pin_ordinal, empty() means unpinned)

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:229: ~AppList() {}
nit: In a file local helper class, we can omit the trivial
constructor/destructor and DISALLOW_...

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:394: }
nit: This could be a member of AppList.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:404: const auto* pinned =
prefs->GetList(prefs::kPinnedLauncherApps);
nit: pinned_apps

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:434: }
nit: This would be a bit clearer as:

const base::DictionaryValue* app_pref = nullptr;
if (!pinned->GetDictionary(i, &app_pref)) {
  LOG(ERROR) ...
  continue;
}
apps.MaybeAddAppFromPref(app_pref, helper)

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:470: if (prefs_->IsSyncing()) {
nit: invert + early exit (and modify comment)

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:481: syncer::StringOrdinal
CreateFirstPinPosition(Profile* profile) {
nit: GetFirstPinPosition
s/create/get/ in the comment
(create sounds like it constructs something more complicated than just an
ordinal)

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:500: syncer::StringOrdinal
CreateLastPinPosition(Profile* profile) {
Ditto

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:606: // Validate that there have
a correct position and fix if needed.
s/there/they/

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:608: for (const auto& app_id :
policy_apps.app_list()) {
On 2016/06/14 21:42:50, xiyuan wrote:
> Would this be enforced every time? If a user moves a policy pinned app, would
> the new position be kept after this code runs again (e.g. on the next login)?

If we put the policy pinned apps in front every time without preventing them
from being re-ordered in the UI, that would be confusing I think.

In the issue description, it was suggested: "+1 to keeping things super simple".

My suggestion would be to simplify the logic below as follows:

* If the app is already in pin_infos (because its pinned position was synced),
continue (i.e. leave it).
* Otherwise put it before the first item.
* Move the chrome browser app insertion below so that it will be added in front
of policy apps if it doesn't exist.

Does that work / make sense?

[khmel, 2016-06-15 17:01:21]

Hi Steven and Xiyaun,

Thanks for your comment.

I updated CL:
Rebased (uff).
Nits and style changes.
Pin by policy apps are movable.
Add and extend unit tests

PTAL

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:69:
item->item_pin_ordinal = syncer::StringOrdinal(specifics.item_pin_ordinal());
On 2016/06/14 22:44:08, stevenjb wrote:
> An unfortunate side effect of this is that if an older version of chrome
updates
> a sync item (e.g. because it was moved) it would inadvertantly unpin the item.
> 
> Looking back through the sync code, I think this is what we want for both
> item_ordinal and item_pin_ordinal:
> 
> if (specifics.has_item_pin_ordinal())
>   item->item_pin_ordinal =
syncer::StringOrdinal(specifics.item_pin_ordinal());
> 
> That way if the value is unset (vs. empty), any existing state will not be
> overwritten.
> 
> (For item_ordinal those are synonymous since item_ordinal.empty() has no
special
> meaning, whereas with item_pin_ordinal, empty() means unpinned)

That is great point. I missed this possibility, thanks! Also extended unit_tests

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:29: bool
used_in_testing = false;
On 2016/06/14 21:42:50, xiyuan wrote:
> nit: used_in_testing -> use_in_testing to be consistent with SetUseInTesting.

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:33: void
AppListSyncableServiceFactory::SetUseInTesting() {
On 2016/06/14 21:42:50, xiyuan wrote:
> nit: either move this down after BuildInstanceFor, or move it up in header
file
> so that the order in header and cc file matches.

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:226: class AppList {
On 2016/06/14 21:42:50, xiyuan wrote:
> nit: Can we get a better name? AppList collides with, errr AppList. How about
> call it AppTracker?

Agree, quite confusing :)

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:229: ~AppList() {}
On 2016/06/14 22:44:09, stevenjb wrote:
> nit: In a file local helper class, we can omit the trivial
> constructor/destructor and DISALLOW_...

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:394: }
On 2016/06/14 22:44:08, stevenjb wrote:
> nit: This could be a member of AppList.

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:404: const auto* pinned =
prefs->GetList(prefs::kPinnedLauncherApps);
On 2016/06/14 22:44:09, stevenjb wrote:
> nit: pinned_apps

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:434: }
On 2016/06/14 22:44:09, stevenjb wrote:
> nit: This would be a bit clearer as:
> 
> const base::DictionaryValue* app_pref = nullptr;
> if (!pinned->GetDictionary(i, &app_pref)) {
>   LOG(ERROR) ...
>   continue;
> }
> apps.MaybeAddAppFromPref(app_pref, helper)

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:470: if (prefs_->IsSyncing()) {
On 2016/06/14 22:44:09, stevenjb wrote:
> nit: invert + early exit (and modify comment)

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:481: syncer::StringOrdinal
CreateFirstPinPosition(Profile* profile) {
On 2016/06/14 22:44:09, stevenjb wrote:
> nit: GetFirstPinPosition
> s/create/get/ in the comment
> (create sounds like it constructs something more complicated than just an
> ordinal)

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:500: syncer::StringOrdinal
CreateLastPinPosition(Profile* profile) {
On 2016/06/14 22:44:09, stevenjb wrote:
> Ditto

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:606: // Validate that there have
a correct position and fix if needed.
On 2016/06/14 22:44:08, stevenjb wrote:
> s/there/they/

Done.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:608: for (const auto& app_id :
policy_apps.app_list()) {
On 2016/06/14 21:42:50, xiyuan wrote:
> Would this be enforced every time? If a user moves a policy pinned app, would
> the new position be kept after this code runs again (e.g. on the next login)?

This is was done for unmovable pinned by policy apps, as legacy. I changed this
version to movable pinned by policy apps. I think we want to change legacy
design. Based on this assumption your comments make sense.

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

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:340:
s->extension_service()->Init();
On 2016/06/14 21:42:50, xiyuan wrote:
> Line 338-340 could be just
> 
> extension_service_->Init();
> 
> right?

Absolutely :)

[xiyuan, 2016-06-15 17:26:25]

lgtm

Thank you for taking on this.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:248:
DISALLOW_COPY_AND_ASSIGN(AppList);
Why removing this?

[khmel, 2016-06-15 17:29:14]

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:248:
DISALLOW_COPY_AND_ASSIGN(AppList);
On 2016/06/15 17:26:25, xiyuan wrote:
> Why removing this?

As Steven recommended in comment above. I would personally leave DISALLOW, this
class is not suppose to be copied.

[xiyuan, 2016-06-15 17:30:22]

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:248:
DISALLOW_COPY_AND_ASSIGN(AppList);
On 2016/06/15 17:29:14, khmel wrote:
> On 2016/06/15 17:26:25, xiyuan wrote:
> > Why removing this?
> 
> As Steven recommended in comment above. I would personally leave DISALLOW,
this
> class is not suppose to be copied.

I see. Never mind then. I am fine either way.

[stevenjb, 2016-06-15 17:53:52]

Looking great, thanks! Just a couple small comments.

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:248:
DISALLOW_COPY_AND_ASSIGN(AppList);
On 2016/06/15 17:30:22, xiyuan wrote:
> On 2016/06/15 17:29:14, khmel wrote:
> > On 2016/06/15 17:26:25, xiyuan wrote:
> > > Why removing this?
> > 
> > As Steven recommended in comment above. I would personally leave DISALLOW,
> this
> > class is not suppose to be copied.
> 
> I see. Never mind then. I am fine either way.

I forget whether we can use DISALLOW without an explicit constructor/destructor.
Maybe we can if it is not virtual? If so, that's fine.

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:69: if
(specifics.has_item_pin_ordinal()) {
Let's use this pattern for item_ordinal also for consistency.

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:630: }));
This is a bit lengthy and expensive for a DCHECK. We could use
std::binary_search to make it faster, but frankly this is something that we are
better off ensuring in a test instead.

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:634: // Now put it to the front.
nit: s/put/move/

[khmel, 2016-06-15 18:26:23]

PTAL

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:248:
DISALLOW_COPY_AND_ASSIGN(AppList);
On 2016/06/15 17:53:52, stevenjb wrote:
> On 2016/06/15 17:30:22, xiyuan wrote:
> > On 2016/06/15 17:29:14, khmel wrote:
> > > On 2016/06/15 17:26:25, xiyuan wrote:
> > > > Why removing this?
> > > 
> > > As Steven recommended in comment above. I would personally leave DISALLOW,
> > this
> > > class is not suppose to be copied.
> > 
> > I see. Never mind then. I am fine either way.
> 
> I forget whether we can use DISALLOW without an explicit
constructor/destructor.
> Maybe we can if it is not virtual? If so, that's fine.

I tested, we still need CTOR for DISALLOW, so skipping DISALLOW  makes sense.

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:69: if
(specifics.has_item_pin_ordinal()) {
On 2016/06/15 17:53:52, stevenjb wrote:
> Let's use this pattern for item_ordinal also for consistency.

Done.

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:630: }));
On 2016/06/15 17:53:52, stevenjb wrote:
> This is a bit lengthy and expensive for a DCHECK. We could use
> std::binary_search to make it faster, but frankly this is something that we
are
> better off ensuring in a test instead.

agree, removed.

https://codereview.chromium.org/2055553004/diff/60001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:634: // Now put it to the front.
On 2016/06/15 17:53:52, stevenjb wrote:
> nit: s/put/move/

Done.

https://codereview.chromium.org/2055553004/diff/80001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2055553004/diff/80001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:2379:
TEST_F(ChromeLauncherControllerImplTest, ImportLegacyPin) {
Added this test but forgot to run :)

[stevenjb, 2016-06-15 18:33:39]

LGTM !

[Mr4D (OOO till 08-26), 2016-06-15 19:01:45]

LGTM! And many thanks for all your work on this!

[khmel, 2016-06-15 19:20:54]

khmel@chromium.org changed reviewers:
+ zea@chromium.org

[khmel, 2016-06-15 19:20:54]

Hi,

Thanks a lot for your reviews and discussions!

Adding 
zea@chromium.org (sync/protocol/app_list_specifics.proto)
pam@chromium.org (chrome/browser/prefs/browser_prefs.cc)

PTAL

[khmel, 2016-06-15 19:21:17]

khmel@chromium.org changed reviewers:
+ pam@chromium.org

[khmel, 2016-06-15 19:21:49]

PTAL

[Nicolas Zea, 2016-06-15 20:56:41]

https://codereview.chromium.org/2055553004/diff/80001/sync/protocol/app_list_...
File sync/protocol/app_list_specifics.proto (right):

https://codereview.chromium.org/2055553004/diff/80001/sync/protocol/app_list_...
sync/protocol/app_list_specifics.proto:52: optional string item_pin_ordinal = 7;
Could you update proto_value_conversions.cc to reflect this field as well?

[khmel, 2016-06-15 21:05:14]

Thanks Nicolas for comments.

PTAL

https://codereview.chromium.org/2055553004/diff/80001/sync/protocol/app_list_...
File sync/protocol/app_list_specifics.proto (right):

https://codereview.chromium.org/2055553004/diff/80001/sync/protocol/app_list_...
sync/protocol/app_list_specifics.proto:52: optional string item_pin_ordinal = 7;
On 2016/06/15 20:56:41, Nicolas Zea wrote:
> Could you update proto_value_conversions.cc to reflect this field as well?

Done

[Nicolas Zea, 2016-06-20 07:16:19]

sync lgtm

[khmel, 2016-06-20 15:49:04]

khmel@chromium.org changed reviewers:
+ sky@chromium.org
- pam@chromium.org

[khmel, 2016-06-20 15:49:05]

Thanks Nicolas for review.

Scott, could you please review trivial change in

chrome/browser/prefs/browser_prefs.cc

PTAL

[sky, 2016-06-20 19:16:51]

LGTM

[khmel, 2016-06-20 20:40:38]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-06-20 20:40:39]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
stevenjb@chromium.org, skuhne@chromium.org, zea@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2055553004/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2016-06-20 20:41:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055553004/120001

[commit-bot: I haz the power, 2016-06-20 20:57:50]

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

[commit-bot: I haz the power, 2016-06-20 20:57:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[khmel, 2016-06-20 22:57:53]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-06-20 22:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
stevenjb@chromium.org, zea@chromium.org, skuhne@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2055553004/#ps140001
(title: "browser_tests compilation fix")

[commit-bot: I haz the power, 2016-06-20 22:58:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055553004/140001

[commit-bot: I haz the power, 2016-06-20 23:13:27]

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

[commit-bot: I haz the power, 2016-06-20 23:13:29]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[khmel, 2016-06-21 01:41:22]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-06-21 01:41:23]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
stevenjb@chromium.org, zea@chromium.org, skuhne@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2055553004/#ps160001
(title: "chrome_mash_shelf_controller.cc update due namespace renaming")

[commit-bot: I haz the power, 2016-06-21 01:41:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055553004/160001

[commit-bot: I haz the power, 2016-06-21 02:40:03]

Description was changed from

==========
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.
==========

to

==========
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.
==========

[commit-bot: I haz the power, 2016-06-21 02:40:04]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-21 02:41:51]

Description was changed from

==========
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.
==========

to

==========
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.

Committed: https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8
Cr-Commit-Position: refs/heads/master@{#400876}
==========

[commit-bot: I haz the power, 2016-06-21 02:41:53]

Patchset 9 (id:??) landed as
https://crrev.com/3b43aaa3b74115c142a51d7e16409bb2d9abafa8
Cr-Commit-Position: refs/heads/master@{#400876}