arc: Dont sync app list change caused by Arc opt out.

arc: Dont sync app list change caused by Arc opt out.

Arc Opt out was recently changed and it is now local change. This
causes the problem with syncing app list info because Arc apps are
removed from local app list model, shelf and these changes are sent
to sync. As result, if user runs another Arc device, Arc apps there
will be removed from app list and pin. Solution is not to sync
changes, caused by Arc opt out.

TEST=Manually on device, opt out Arc on one device and Arc apps are
     not removed from shelf and app list on another running device.
     Re-enable Arc again and Arc apps are restrored at the same
     positions in shelf and App list.
TEST=Extended and updated unit_tests
BUG=b/31163918
BUG=642864
Committed: https://crrev.com/c9726f4e10331b7c91542b3e35b22a2c483fd758
Cr-Commit-Position: refs/heads/master@{#416043}

[khmel, 2016-08-31 20:31:48]

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

[khmel, 2016-08-31 20:31:48]

Hi Steven and Xiyuan,

PTAL

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:224: if
(item->GetItemType() == ArcAppItem::kItemType) {
I tried first not to modify AppListSync but OnAppListItemWillBeDeleted is quite
generic interface and used widely, modifying (adding additional flag or skipping
to call) it causes a lot of changes. So putting check here for Arc item here is
quite small.

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

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:990:
DoUnpinAppWithID(app_id, false);
It seems we can skip resetting pin position for uninstalled items. These sync
items will be automatically deleted as app list model change.

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

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:348:
arc_test_.SetUp(profile());
Arc profile should be set before accessing app list sync service. Otherwise it
will be started without Arc builder.

[stevenjb, 2016-08-31 21:10:49]

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_model_builder.h (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_model_builder.h:52: // Removes an app based
on app id.
We need to document |local_change| here.

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:224: if
(item->GetItemType() == ArcAppItem::kItemType) {
On 2016/08/31 20:31:48, khmel wrote:
> I tried first not to modify AppListSync but OnAppListItemWillBeDeleted is
quite
> generic interface and used widely, modifying (adding additional flag or
skipping
> to call) it causes a lot of changes. So putting check here for Arc item here
is
> quite small.

This seems reasonable. We already have arc related dependencies here, although
we should add an explicit dependency on arc_auth_service.h in this file. (It is
included through arc_app_list_prefs.h, which is unintuitive, we should consider
moving ArcAuthService::Observer to its own header in a separate CL).

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_model_builder.cc (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_model_builder.cc:58: RemoveApp(app_id,
!auth_service->IsArcEnabled());
This isn't a very intuitive value for 'local_change'. I would rename the
'local_change' parameter to 'unsynced_change' and add a comment here.

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

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:386:
UnpinAndSync(id, false);
This is a little dangerous, if we missed any Unpin(id) calls the behavior will
change to not sync. I would remove the Unpin function.

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:395:
ash::launcher::RemovePinPosition(profile_, GetAppIDForShelfID(id));
This looks odd. It is not clear why |sync_change| affects the call to 
RemovePinPosition.

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:990:
DoUnpinAppWithID(app_id, false);
On 2016/08/31 20:31:48, khmel wrote:
> It seems we can skip resetting pin position for uninstalled items. These sync
> items will be automatically deleted as app list model change.

Can we add a comment to that effect here?

[xiyuan, 2016-08-31 21:43:09]

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:224: if
(item->GetItemType() == ArcAppItem::kItemType) {
nit: fix indent

[khmel, 2016-09-01 00:03:03]

Thanks for your comments!

PTAL

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_model_builder.h (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_model_builder.h:52: // Removes an app based
on app id.
On 2016/08/31 21:10:49, stevenjb wrote:
> We need to document |local_change| here.

Done.

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:224: if
(item->GetItemType() == ArcAppItem::kItemType) {
On 2016/08/31 21:10:49, stevenjb wrote:
> On 2016/08/31 20:31:48, khmel wrote:
> > I tried first not to modify AppListSync but OnAppListItemWillBeDeleted is
> quite
> > generic interface and used widely, modifying (adding additional flag or
> skipping
> > to call) it causes a lot of changes. So putting check here for Arc item here
> is
> > quite small.
> 
> This seems reasonable. We already have arc related dependencies here, although
> we should add an explicit dependency on arc_auth_service.h in this file. (It
is
> included through arc_app_list_prefs.h, which is unintuitive, we should
consider
> moving ArcAuthService::Observer to its own header in a separate CL).
> 
>  

Thanks, will create additional CL to move ArcAuthService::Observer into separate
file (probably ArcAppListPrefs::Observer is also good candidate to do so)

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:224: if
(item->GetItemType() == ArcAppItem::kItemType) {
On 2016/08/31 21:43:09, xiyuan wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_model_builder.cc (right):

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_model_builder.cc:58: RemoveApp(app_id,
!auth_service->IsArcEnabled());
On 2016/08/31 21:10:49, stevenjb wrote:
> This isn't a very intuitive value for 'local_change'. I would rename the
> 'local_change' parameter to 'unsynced_change' and add a comment here.

That is better, thanks

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

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:386:
UnpinAndSync(id, false);
On 2016/08/31 21:10:49, stevenjb wrote:
> This is a little dangerous, if we missed any Unpin(id) calls the behavior will
> change to not sync. I would remove the Unpin function.

Ops, misprint here and true should be used (it seems Unpin for shelf id is only
used browser_test but I still cannot remove it).

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:395:
ash::launcher::RemovePinPosition(profile_, GetAppIDForShelfID(id));
On 2016/08/31 21:10:49, stevenjb wrote:
> This looks odd. It is not clear why |sync_change| affects the call to 
> RemovePinPosition.

I renamed this function and argument for better explanation. Also added more
comments.

https://codereview.chromium.org/2295343002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:990:
DoUnpinAppWithID(app_id, false);
On 2016/08/31 21:10:49, stevenjb wrote:
> On 2016/08/31 20:31:48, khmel wrote:
> > It seems we can skip resetting pin position for uninstalled items. These
sync
> > items will be automatically deleted as app list model change.
> 
> Can we add a comment to that effect here?

Done.

[stevenjb, 2016-09-01 16:01:07]

lgtm w/ minor change

https://codereview.chromium.org/2295343002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right):

https://codereview.chromium.org/2295343002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:214: void
DoUnpinAppWithID(const std::string& app_id, bool sync_change);
update_prefs

[xiyuan, 2016-09-01 18:06:48]

lgtm

[khmel, 2016-09-01 20:02:59]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-09-01 20:02:59]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2295343002/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2016-09-01 20:03:39]

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-09-01 20:48:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-01 20:55:54]

Description was changed from

==========
arc: Dont sync app list change caused by Arc opt out.

Arc Opt out was recently changed and it is now local change. This
causes the problem with syncing app list info because Arc apps are
removed from local app list model, shelf and these changes are sent
to sync. As result, if user runs another Arc device, Arc apps there
will be removed from app list and pin. Solution is not to sync
changes, caused by Arc opt out.

TEST=Manually on device, opt out Arc on one device and Arc apps are
     not removed from shelf and app list on another running device.
     Re-enable Arc again and Arc apps are restrored at the same
     positions in shelf and App list.
TEST=Extended and updated unit_tests
BUG=b/31163918
BUG=642864
==========

to

==========
arc: Dont sync app list change caused by Arc opt out.

Arc Opt out was recently changed and it is now local change. This
causes the problem with syncing app list info because Arc apps are
removed from local app list model, shelf and these changes are sent
to sync. As result, if user runs another Arc device, Arc apps there
will be removed from app list and pin. Solution is not to sync
changes, caused by Arc opt out.

TEST=Manually on device, opt out Arc on one device and Arc apps are
     not removed from shelf and app list on another running device.
     Re-enable Arc again and Arc apps are restrored at the same
     positions in shelf and App list.
TEST=Extended and updated unit_tests
BUG=b/31163918
BUG=642864

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

[commit-bot: I haz the power, 2016-09-01 20:55:55]

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