New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 641535 link

Starred by 11 users

Issue metadata

Status: Verified
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Applist and shlef position not persist after turn off apps sync settings.

Project Member Reported by lgcheng@google.com, Aug 26 2016

Issue description

Version: Any.
OS: Chrome OS.

Applist position and shelf position not persist after turn off
settings->advanced sync settings->apps.

Reproduce:
Turn off settings->advanced sync settings->apps.

Change applist app positions/ shelf positions or pin/uppin apps.

Sign out and then sign in:

Expecting: local changes persist.

Observing: local changes not persist.

I suspect applist sync service miss start criteria. Our design is apps, apps settings, applist, arc apps should be controlled by settings->advanced sync settings->apps. But applist sync service still start even if we turn off settings->advanced sync settings->apps. So sync service will update local applist related information when system starts. And then applist sync service is turned off(need verify). But system start time sync update erase local modification after settings->advanced sync settings->apps is turned off.
 

Comment 1 by lgcheng@google.com, Aug 26 2016

Description: Show this description

Comment 2 by lgcheng@google.com, Aug 26 2016

Components: UI>Shell>Shelf Services>Sync
Project Member

Comment 3 by sheriffbot@chromium.org, Aug 28 2016

Labels: Hotlist-Google

Comment 4 by lgcheng@google.com, Aug 30 2016

Cc: omrilio@chromium.org
Cc: jhorwich@chromium.org kuscher@chromium.org
Owner: steve...@chromium.org
Status: Assigned (was: Untriaged)
Steven, could you have a look please?
Labels: -Pri-3 M-54 Pri-2
This is also observed on M53 btw it sounds like?
Cc: abodenha@chromium.org

Comment 8 by lgcheng@google.com, Aug 30 2016

Re #6 It happens on any branch...
Cc: steve...@chromium.org
Owner: khmel@chromium.org
There was a deliberate decision made to limit the complexity of the app list code by only persisting app list order through sync. We have never maintained position of app list items with sync disabled. We may have maintained shelf position and lost that when we integrated shelf position with app list position.

At one point there was an effort in Chrome to use sync data for things like this even with sync disabled (effectively syncing locally only). I am not sure what the status of that is.

We could probably fix just the shelf code by using the pre-existing settings for shelf ordering if app list sync is disabled.

->khmel@ who worked on the shelf order sync.

Comment 10 by dymp...@gmail.com, Aug 30 2016

Not sure if relevant to this issue but on Acer 15 and Asus Flip on Beta this is what's happening.

Flip - sync everything in settings > people > advanced sync settings
Acer 15 - no app sync but everything else

Shutdown both CBs

Restart Flip > make a change to shelf - unpin 2 apps
Restart Acer 15 > apps are un pinned from shelf because apps sync is turned on again
  


Acer 15 > turn off app sync again on 
Pin another app to shelf
App does not appear on Flip that is still on and signed in


Turn off Flip > restart > app is pinned to shelf
Acer 15 is still on > apps sync has been turned on again


Feedback sent from Acer 15  at 7:45pm EDT Aug 30



Comment 11 by khmel@chromium.org, Sep 16 2016

Cc: xiy...@chromium.org skuhne@chromium.org
 Issue 647804  has been merged into this issue.

Comment 12 by khmel@chromium.org, Sep 16 2016

Status: Started (was: Assigned)
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 19 2016

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

commit b8997b03c95df27570d0beab45cc42fe1df95c20
Author: khmel <khmel@chromium.org>
Date: Wed Oct 19 21:10:32 2016

Discard unused AppListPrefs code.

This CL discard unused code that monitors changes in app list model
but does not expose any functionality.

BUG= 641535 
Test=git cl try

Review-Url: https://chromiumcodereview.appspot.com/2430753002
Cr-Commit-Position: refs/heads/master@{#426283}

[modify] https://crrev.com/b8997b03c95df27570d0beab45cc42fe1df95c20/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/b8997b03c95df27570d0beab45cc42fe1df95c20/chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc
[modify] https://crrev.com/b8997b03c95df27570d0beab45cc42fe1df95c20/chrome/browser/ui/BUILD.gn
[delete] https://crrev.com/ded4487943610f83fecc344b7fb0a9fdf9cf9a36/chrome/browser/ui/app_list/app_list_prefs.cc
[delete] https://crrev.com/ded4487943610f83fecc344b7fb0a9fdf9cf9a36/chrome/browser/ui/app_list/app_list_prefs.h
[delete] https://crrev.com/ded4487943610f83fecc344b7fb0a9fdf9cf9a36/chrome/browser/ui/app_list/app_list_prefs_factory.cc
[delete] https://crrev.com/ded4487943610f83fecc344b7fb0a9fdf9cf9a36/chrome/browser/ui/app_list/app_list_prefs_factory.h
[modify] https://crrev.com/b8997b03c95df27570d0beab45cc42fe1df95c20/chrome/browser/ui/app_list/app_list_syncable_service.cc
[modify] https://crrev.com/b8997b03c95df27570d0beab45cc42fe1df95c20/chrome/browser/ui/app_list/app_list_syncable_service.h
[delete] https://crrev.com/ded4487943610f83fecc344b7fb0a9fdf9cf9a36/chrome/browser/ui/app_list/model_pref_updater.cc
[delete] https://crrev.com/ded4487943610f83fecc344b7fb0a9fdf9cf9a36/chrome/browser/ui/app_list/model_pref_updater.h
[delete] https://crrev.com/ded4487943610f83fecc344b7fb0a9fdf9cf9a36/chrome/browser/ui/app_list/model_pref_updater_unittest.cc
[modify] https://crrev.com/b8997b03c95df27570d0beab45cc42fe1df95c20/chrome/test/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 2 2016

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

commit fdae64e689c5c1424d5da8f827e9d1d10d905c1c
Author: khmel <khmel@chromium.org>
Date: Wed Nov 02 21:39:56 2016

arc: Fix default app availability reporting.

This solves the case when App list model is created before
ArcAppListPrefs and notifications about defualt apps are
not dispatched.

BUG= 641535 
TEST=Locally on device.

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

[modify] https://crrev.com/fdae64e689c5c1424d5da8f827e9d1d10d905c1c/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
[modify] https://crrev.com/fdae64e689c5c1424d5da8f827e9d1d10d905c1c/chrome/browser/ui/app_list/arc/arc_app_list_prefs.h

Project Member

Comment 16 by bugdroid1@chromium.org, Nov 3 2016

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

commit eca5057f03c04b7dbb0764fa090aac36fbd9731c
Author: khmel <khmel@chromium.org>
Date: Thu Nov 03 16:54:45 2016

Implement local storage for App List in case app sync is off.

This CL keeps local copy of current app list service and in
case app sync is off, date from local storage is used to
init app list model. Once sync is resumed, local copy is
overriden and information form sync is used.

BUG= 641535 
TEST=Unit and sync tests added
TEST=Manually on device.

TBR=reviewer@chromium.org
Reland: http://crrev.com/2416133002
(cherry picked from commit f06c029695e6d3a62ea950c1ba0f60cf01c353a8)

Review-Url: https://codereview.chromium.org/2416133002
Review-Url: https://codereview.chromium.org/2468413005
Cr-Original-Commit-Position: refs/heads/master@{#427390}
Cr-Commit-Position: refs/heads/master@{#429621}

[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc
[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/browser/ui/app_list/app_list_syncable_service.cc
[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/browser/ui/app_list/app_list_syncable_service.h
[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/browser/ui/ash/chrome_launcher_prefs.cc
[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/common/pref_names.cc
[modify] https://crrev.com/eca5057f03c04b7dbb0764fa090aac36fbd9731c/chrome/common/pref_names.h

Labels: -M-54 M-55
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 7 2016

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

commit 43ff56fd70f2ae8827bf8caa80b16f94040c4f0e
Author: kinaba <kinaba@chromium.org>
Date: Mon Nov 07 09:09:11 2016

Revert of arc: Fix default app availability reporting. (patchset #1 id:1 of https://codereview.chromium.org/2468973006/ )

Reason for revert:
It's breaking all ARC CTS autotests b/32660818,
and I highly suspect it is also affecting the Chrome OS CQ crbug.com/662451.

BUG=b/32660818

Original issue's description:
> arc: Fix default app availability reporting.
>
> This solves the case when App list model is created before
> ArcAppListPrefs and notifications about defualt apps are
> not dispatched.
>
> BUG= 641535 
> TEST=Locally on device.
>
> Committed: https://crrev.com/fdae64e689c5c1424d5da8f827e9d1d10d905c1c
> Cr-Commit-Position: refs/heads/master@{#429408}

TBR=xiyuan@chromium.org,hidehiko@chromium.org,khmel@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 641535 

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

[modify] https://crrev.com/43ff56fd70f2ae8827bf8caa80b16f94040c4f0e/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
[modify] https://crrev.com/43ff56fd70f2ae8827bf8caa80b16f94040c4f0e/chrome/browser/ui/app_list/arc/arc_app_list_prefs.h

FYI - after bisecting, the failures of cheets_PerformanceAppTest test was also from the same 'Fix default app availability reporting' patch.
Cc: cywang@chromium.org
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 17 2016

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

commit d8b8866329c222f332f28e5b24807748a0dedb2a
Author: khmel <khmel@chromium.org>
Date: Thu Nov 17 00:25:47 2016

Reland of arc: Fix default app availability reporting. (patchset #1 id:1 of https://codereview.chromium.org/2477323002/ )

Reason for revert:
Arc integration tests were fixed and now it is possible to reland reverted CL

Original issue's description:
> Revert of arc: Fix default app availability reporting. (patchset #1 id:1 of https://codereview.chromium.org/2468973006/ )
>
> Reason for revert:
> It's breaking all ARC CTS autotests b/32660818,
> and I highly suspect it is also affecting the Chrome OS CQ crbug.com/662451.
>
> BUG=b/32660818
>
> Original issue's description:
> > arc: Fix default app availability reporting.
> >
> > This solves the case when App list model is created before
> > ArcAppListPrefs and notifications about defualt apps are
> > not dispatched.
> >
> > BUG= 641535 
> > TEST=Locally on device.
> >
> > Committed: https://crrev.com/fdae64e689c5c1424d5da8f827e9d1d10d905c1c
> > Cr-Commit-Position: refs/heads/master@{#429408}
>
> TBR=xiyuan@chromium.org,hidehiko@chromium.org,khmel@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 641535 
>
> Committed: https://crrev.com/43ff56fd70f2ae8827bf8caa80b16f94040c4f0e
> Cr-Commit-Position: refs/heads/master@{#430238}

TBR=xiyuan@chromium.org,hidehiko@chromium.org,kinaba@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=b/32660818

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

[modify] https://crrev.com/d8b8866329c222f332f28e5b24807748a0dedb2a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
[modify] https://crrev.com/d8b8866329c222f332f28e5b24807748a0dedb2a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.h

Labels: -M-55
Status: Fixed (was: Started)

Comment 24 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58
Status: Verified (was: Fixed)
9334.23.0 / 58.0.3029.39

Sign in to add a comment