Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
229233003:
Fix AppListSyncableService::ModelObserver actions (Closed)
|
Created:
6 years, 8 months ago by stevenjb Modified:
6 years, 8 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix AppListSyncableService::ModelObserver actions
This addresses two bugs:
* Updates from the Model should be ignored while an item is being added.
* Folder deletions from the Model should be ignored since the model
may not contain items that are not installed on the device, and we
check for empty folders on item removal already.
BUG=361671, 361721
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262890
Patch Set 1 #Patch Set 2 : . #
Total comments: 5
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:175: return; What if the update is not triggered by adding item, ie, a valid one, will sync miss that update?
https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:175: return; On 2014/04/09 21:31:08, jennyz wrote: > What if the update is not triggered by adding item, ie, a valid one, will sync > miss that update? This is all synchronous, so adding_item_ would be NULL for any update that is not a side effect of adding the item.
https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:166: return; When users logs out and in again, the syncable service to prune the empty folder after sync is done. How do we decide if the folder is empty? Will it know the items not valid on this device but existing on other device? https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:175: return; On 2014/04/09 21:48:16, stevenjb wrote: > On 2014/04/09 21:31:08, jennyz wrote: > > What if the update is not triggered by adding item, ie, a valid one, will sync > > miss that update? > This is all synchronous, so adding_item_ would be NULL for any update that is > not a side effect of adding the item. > Sounds good.
https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:166: return; On 2014/04/09 22:00:32, jennyz wrote: > When users logs out and in again, the syncable service to prune the empty folder > after sync is done. How do we decide if the folder is empty? Will it know the > items not valid on this device but existing on other device? AppListSyncableService has reliable information about folder state, so on startup and any time we remove an item we call PruneEmptySyncFolders() and it will remove sync folders. (My previous thinking that we only did that on startup was incorrect, we do that when an item is removed also). AppListModel does not know about items that are not installed, so it might remove a folder prematurely, which is why we want to ignore this for folders.
On 2014/04/09 22:17:16, stevenjb wrote: > https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... > File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): > > https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... > chrome/browser/ui/app_list/app_list_syncable_service.cc:166: return; > On 2014/04/09 22:00:32, jennyz wrote: > > When users logs out and in again, the syncable service to prune the empty > folder > > after sync is done. How do we decide if the folder is empty? Will it know the > > items not valid on this device but existing on other device? > > AppListSyncableService has reliable information about folder state, so on > startup and any time we remove an item we call PruneEmptySyncFolders() and it > will remove sync folders. (My previous thinking that we only did that on startup > was incorrect, we do that when an item is removed also). > > AppListModel does not know about items that are not installed, so it might > remove a folder prematurely, which is why we want to ignore this for folders. So in this case, for the following use case, on Link, a folder contains TimeScape and 2 other common apps, on yoshi, the folder shows only the 2 common apps, if user drag one of them out on yoshi, the folder will be gone; while on Link, the folder should still be there will only Timescape in it, since the folder is not deleted with this change. This looks good to me.
On 2014/04/09 22:34:17, jennyz wrote: > On 2014/04/09 22:17:16, stevenjb wrote: > > > https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... > > File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): > > > > > https://codereview.chromium.org/229233003/diff/20001/chrome/browser/ui/app_li... > > chrome/browser/ui/app_list/app_list_syncable_service.cc:166: return; > > On 2014/04/09 22:00:32, jennyz wrote: > > > When users logs out and in again, the syncable service to prune the empty > > folder > > > after sync is done. How do we decide if the folder is empty? Will it know > the > > > items not valid on this device but existing on other device? > > > > AppListSyncableService has reliable information about folder state, so on > > startup and any time we remove an item we call PruneEmptySyncFolders() and it > > will remove sync folders. (My previous thinking that we only did that on > startup > > was incorrect, we do that when an item is removed also). > > > > AppListModel does not know about items that are not installed, so it might > > remove a folder prematurely, which is why we want to ignore this for folders. > So in this case, for the following use case, on Link, a folder contains > TimeScape and 2 other common apps, on yoshi, the folder shows only the 2 common > apps, if user drag one of them out on yoshi, the folder will be gone; while on > Link, the folder should still be there will only Timescape in it, since the > folder is not deleted with this change. This looks good to me. Correct.
lgtm
The other way to repro this is to uninstall an app from a folder with two apps before the second app has installed. (Easiest to do if one app is a default app and the other is a large app from the store). When the second app installs, currently it will show up in a new, unnamed folder since we deleted the sync item. With this fix, we will keep the sync item (since AppListSyncableService knows about the other item) and retain the folder name and location when the store item shows up. On Wed, Apr 9, 2014 at 3:42 PM, <stevenjb@chromium.org> wrote: > On 2014/04/09 22:34:17, jennyz wrote: > >> On 2014/04/09 22:17:16, stevenjb wrote: >> > >> > > https://codereview.chromium.org/229233003/diff/20001/ > chrome/browser/ui/app_list/app_list_syncable_service.cc > >> > File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): >> > >> > >> > > https://codereview.chromium.org/229233003/diff/20001/ > chrome/browser/ui/app_list/app_list_syncable_service.cc#newcode166 > >> > chrome/browser/ui/app_list/app_list_syncable_service.cc:166: return; >> > On 2014/04/09 22:00:32, jennyz wrote: >> > > When users logs out and in again, the syncable service to prune the >> empty >> > folder >> > > after sync is done. How do we decide if the folder is empty? Will it >> know >> the >> > > items not valid on this device but existing on other device? >> > >> > AppListSyncableService has reliable information about folder state, so >> on >> > startup and any time we remove an item we call PruneEmptySyncFolders() >> and >> > it > >> > will remove sync folders. (My previous thinking that we only did that on >> startup >> > was incorrect, we do that when an item is removed also). >> > >> > AppListModel does not know about items that are not installed, so it >> might >> > remove a folder prematurely, which is why we want to ignore this for >> > folders. > >> So in this case, for the following use case, on Link, a folder contains >> TimeScape and 2 other common apps, on yoshi, the folder shows only the 2 >> > common > >> apps, if user drag one of them out on yoshi, the folder will be gone; >> while on >> Link, the folder should still be there will only Timescape in it, since >> the >> folder is not deleted with this change. This looks good to me. >> > Correct. > > > > https://codereview.chromium.org/229233003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/229233003/20001
Message was sent while issue was closed.
Change committed as 262890 |
