Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416133002:
Implement local storage for App List in case app sync is off. (Closed)
DescriptionImplement 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.
Committed: https://crrev.com/f06c029695e6d3a62ea950c1ba0f60cf01c353a8
Cr-Commit-Position: refs/heads/master@{#427390}
Patch Set 1 #
Total comments: 4
Patch Set 2 : "app sync service" based solution #Patch Set 3 : cleanup #Patch Set 4 : added extra sync test and rebased #
Total comments: 10
Patch Set 5 : comments addressed #
Total comments: 4
Patch Set 6 : refactore initialization #
Total comments: 4
Patch Set 7 : comments addressed, fix race conditions in tests #
Total comments: 5
Patch Set 8 : method renamed #Messages
Total messages: 30 (9 generated)
khmel@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven, This is local pin model implementation for the case when sync is off for apps. PTAL, Thanks
Hmm. So we specifically didn't bother with this for AppList sync since it is such a tiny edge case. However, I can see where with shelf pinning the effects of not maintaining pin order would be much more obvious / frustrating. At one point sync was supposed to handle this (i.e. sync would still work locally when disabled). I don't know what the state of that effort is. Ideally we would handle this in AppListSyncableService and store that data in a local pref when sync is disabled, but I understand not wanting to make that large of a change. So, this looks fine once the comments are addressed. https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_launcher_prefs.cc:669: } This should just be: DCHECK(position.IsValid()); DCHECK / NOTREACHED should only be for situations that are logically impossible and serve primarily to document the code, and secondarily to catch bugs early if the logic is somehow incorrect. If it is logically possible but an error for this to occur, use LOG(ERROR) instead of NOTREACHED(). https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_launcher_prefs.cc:712: // is off. 'find a way' ? It looks like we currently don't handle inserting the play store item if app list sync is disabled?
Thank you Steven for review, When I was implementing the first version I was thinking that changing app_list_sync_service was not an option. In your comment you confirmed that app_list_sync_service based solution is preferable. I took a look and it seems that complexity is not too high and this solution is more beneficial because everything is hidden inside app list sync service. Could you please take a look this version? PTAL Thanks. https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_launcher_prefs.cc:669: } On 2016/10/14 20:27:25, stevenjb wrote: > This should just be: > DCHECK(position.IsValid()); > > DCHECK / NOTREACHED should only be for situations that are logically impossible > and serve primarily to document the code, and secondarily to catch bugs early if > the logic is somehow incorrect. > > If it is logically possible but an error for this to occur, use LOG(ERROR) > instead of NOTREACHED(). Yes, I agree with your point of view, thanks for explanation. https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_launcher_prefs.cc:712: // is off. On 2016/10/14 20:27:24, stevenjb wrote: > 'find a way' ? It looks like we currently don't handle inserting the play store > item if app list sync is disabled? With new approach this is not required.
On 2016/10/17 21:10:57, khmel wrote: > Thank you Steven for review, > > When I was implementing the first version I was thinking that changing > app_list_sync_service was not an option. In your comment you confirmed that > app_list_sync_service based solution is preferable. I took a look and it seems > that complexity is not too high and this solution is more beneficial because > everything is hidden inside app list sync service. > > Could you please take a look this version? > PTAL > > Thanks. > > https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... > File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): > > https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:669: } > On 2016/10/14 20:27:25, stevenjb wrote: > > This should just be: > > DCHECK(position.IsValid()); > > > > DCHECK / NOTREACHED should only be for situations that are logically > impossible > > and serve primarily to document the code, and secondarily to catch bugs early > if > > the logic is somehow incorrect. > > > > If it is logically possible but an error for this to occur, use LOG(ERROR) > > instead of NOTREACHED(). > > Yes, I agree with your point of view, thanks for explanation. > > https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:712: // is off. > On 2016/10/14 20:27:24, stevenjb wrote: > > 'find a way' ? It looks like we currently don't handle inserting the play > store > > item if app list sync is disabled? > > With new approach this is not required. Sorry if there was the impression that changing app_list_syncable_service was not an option, I definitely have no problem with that. Looking at the code though, it looks like we already do this in AppListPrefs (c/b/ui/app_list/app_list_prefs.cc). It looks like that was added later so I wasn't aware of it, sorry about that. It looks like we should just be able to add pin_position to AppListPrefs::AppListInfo?
On 2016/10/17 21:25:43, stevenjb wrote: > On 2016/10/17 21:10:57, khmel wrote: > > Thank you Steven for review, > > > > When I was implementing the first version I was thinking that changing > > app_list_sync_service was not an option. In your comment you confirmed that > > app_list_sync_service based solution is preferable. I took a look and it seems > > that complexity is not too high and this solution is more beneficial because > > everything is hidden inside app list sync service. > > > > Could you please take a look this version? > > PTAL > > > > Thanks. > > > > > https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... > > File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): > > > > > https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... > > chrome/browser/ui/ash/chrome_launcher_prefs.cc:669: } > > On 2016/10/14 20:27:25, stevenjb wrote: > > > This should just be: > > > DCHECK(position.IsValid()); > > > > > > DCHECK / NOTREACHED should only be for situations that are logically > > impossible > > > and serve primarily to document the code, and secondarily to catch bugs > early > > if > > > the logic is somehow incorrect. > > > > > > If it is logically possible but an error for this to occur, use LOG(ERROR) > > > instead of NOTREACHED(). > > > > Yes, I agree with your point of view, thanks for explanation. > > > > > https://codereview.chromium.org/2416133002/diff/1/chrome/browser/ui/ash/chrom... > > chrome/browser/ui/ash/chrome_launcher_prefs.cc:712: // is off. > > On 2016/10/14 20:27:24, stevenjb wrote: > > > 'find a way' ? It looks like we currently don't handle inserting the play > > store > > > item if app list sync is disabled? > > > > With new approach this is not required. > > Sorry if there was the impression that changing app_list_syncable_service was > not an option, I definitely have no problem with that. > > Looking at the code though, it looks like we already do this in AppListPrefs > (c/b/ui/app_list/app_list_prefs.cc). It looks like that was added later so I > wasn't aware of it, sorry about that. It looks like we should just be able to > add pin_position to AppListPrefs::AppListInfo? Hmm. Probably I misunderstand something but this looks as dead code. For example I can comment out all getters: // Gets the app list info for |id|. //std::unique_ptr<AppListInfo> GetAppListInfo(const std::string& id) const; // Gets a map of all AppListInfo objects in the prefs. //void GetAllAppListInfos(AppListInfoMap* out) const; /*static std::unique_ptr<AppListPrefs::AppListInfo> CreateAppListInfoFromDict( const base::DictionaryValue* item_dict); and Chrome builds without any problem. Do you think we can rely on this?
stevenjb@chromium.org changed reviewers: + calamity@chromium.org
It's possible that AppListPrefs was never actually used? The initial CL description for the file says "This will be used to populate the app list..." +calamity@ who added it initially. We should either use AppListPrefs, or remove it in favor of the changes in this CL (which can be done separately, but should be done first to reduce confusion).
Description was changed from ========== arc: Add shelf pin support in case app sync is off. This CL keeps local copy of current pin model state and in case app sync is off, use this local copy. Once sync is resumed, local copy is overriden and information form sync is used to restore pin positions. BUG=641535 TEST=Unit test added TEST=Manually on device. ========== to ========== 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. ==========
Hi, CL that discards AppListPrefs was landed and I rebased this CL and added one more sync test. PTAL
https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc (right): https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:31: const app_list::AppListSyncableService* service2) { This doesn't seem to be arc specific? https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:53: !item1->item_pin_ordinal.Equals(item2->item_pin_ordinal))) { If you use item1->item_ordinal.EqualsOrBothInvalid(item2->item_ordinal) then you can remove lines 41-44. https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:324: model_->SetFoldersEnabled(true); We will need to test this, but in theory with local pref storage we should be able to always set folders enabled to true. Previously we disabled folders if sync was enabled because we were not persisting folder state, but if this is working correctly there is no reason to disable it. https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:399: model_observer_.reset(new ModelObserver(this)); If we start observing the model here, don't we need to reset it in MergeDataAndStartSyncing (right after the GetModel() call) to make sure we do not process model events while building the model from sync data? Also these should probably be added to the bottom of InitFromLocalStorage instead? (Which may also need to reset the observer at the top of the function?
Thank you for your comments. PTAL https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc (right): https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:31: const app_list::AppListSyncableService* service2) { On 2016/10/20 17:04:46, stevenjb wrote: > This doesn't seem to be arc specific? Yes, AreSyncItemsMatch was expected. https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:53: !item1->item_pin_ordinal.Equals(item2->item_pin_ordinal))) { On 2016/10/20 17:04:46, stevenjb wrote: > If you use item1->item_ordinal.EqualsOrBothInvalid(item2->item_ordinal) > then you can remove lines 41-44. Thanks, I missed this functionality. https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:324: model_->SetFoldersEnabled(true); On 2016/10/20 17:04:46, stevenjb wrote: > We will need to test this, but in theory with local pref storage we should be > able to always set folders enabled to true. Previously we disabled folders if > sync was enabled because we were not persisting folder state, but if this is > working correctly there is no reason to disable it. Right, we can move it into CTOR and drop settings at other places. https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:399: model_observer_.reset(new ModelObserver(this)); On 2016/10/20 17:04:46, stevenjb wrote: > If we start observing the model here, don't we need to reset it in > MergeDataAndStartSyncing (right after the GetModel() call) to make sure we do > not process model events while building the model from sync data? Also these > should probably be added to the bottom of InitFromLocalStorage instead? (Which > may also need to reset the observer at the top of the function? With local storage support yes, we also need to reset observer in MergeDataAndStartSyncing. However I cannot do the same here, in InitLocalStorage because it is called from CTOR and at this moment check at line 364 fails. I considered moving InitLocalStorage to the BuildModel call but this is not compatible with accessing to pin position, which can be made regardless if model built or not.
https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:399: model_observer_.reset(new ModelObserver(this)); On 2016/10/20 18:00:06, khmel wrote: > On 2016/10/20 17:04:46, stevenjb wrote: > > If we start observing the model here, don't we need to reset it in > > MergeDataAndStartSyncing (right after the GetModel() call) to make sure we do > > not process model events while building the model from sync data? Also these > > should probably be added to the bottom of InitFromLocalStorage instead? (Which > > may also need to reset the observer at the top of the function? > > With local storage support yes, we also need to reset observer in > MergeDataAndStartSyncing. However I cannot do the same here, in InitLocalStorage > because it is called from CTOR and at this moment check at line 364 fails. I > considered moving InitLocalStorage to the BuildModel call but this is not > compatible with accessing to pin position, which can be made regardless if model > built or not. On 2016/10/20 18:00:06, khmel wrote: > On 2016/10/20 17:04:46, stevenjb wrote: > > If we start observing the model here, don't we need to reset it in > > MergeDataAndStartSyncing (right after the GetModel() call) to make sure we do > > not process model events while building the model from sync data? Also these > > should probably be added to the bottom of InitFromLocalStorage instead? (Which > > may also need to reset the observer at the top of the function? > > With local storage support yes, we also need to reset observer in > MergeDataAndStartSyncing. However I cannot do the same here, in InitLocalStorage > because it is called from CTOR and at this moment check at line 364 fails. I > considered moving InitLocalStorage to the BuildModel call but this is not > compatible with accessing to pin position, which can be made regardless if model > built or not. Is there any harm in requiring the model to be build before accessing the pin position, just to be consistent? It still seems strange to me to add the model observer here, since in AppListSyncableService::MergeDataAndStartSyncing we call GetModel() (likely for the first time), and then call model_observer_.reset() right after. https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc (right): https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:31: const app_list::AppListSyncableService* service2) { Ah. nit: Just 'SyncItemsMatch' is better, otherwise it should be 'DoSyncItemsMatch', but that could have multiple meanings. https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:314: if (switches::IsFolderUIEnabled()) This tests whether or not sync is enabled on the command line, but since folder persistence is no longer tied to sync, I think we can just always set this to true, right? (We can then probably remove SetFoldersEnabled entirely in a separate CL).
Thanks Steven for your comments. PTAL https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/2416133002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:399: model_observer_.reset(new ModelObserver(this)); On 2016/10/21 20:03:10, stevenjb wrote: > On 2016/10/20 18:00:06, khmel wrote: > > On 2016/10/20 17:04:46, stevenjb wrote: > > > If we start observing the model here, don't we need to reset it in > > > MergeDataAndStartSyncing (right after the GetModel() call) to make sure we > do > > > not process model events while building the model from sync data? Also these > > > should probably be added to the bottom of InitFromLocalStorage instead? > (Which > > > may also need to reset the observer at the top of the function? > > > > With local storage support yes, we also need to reset observer in > > MergeDataAndStartSyncing. However I cannot do the same here, in > InitLocalStorage > > because it is called from CTOR and at this moment check at line 364 fails. I > > considered moving InitLocalStorage to the BuildModel call but this is not > > compatible with accessing to pin position, which can be made regardless if > model > > built or not. > > On 2016/10/20 18:00:06, khmel wrote: > > On 2016/10/20 17:04:46, stevenjb wrote: > > > If we start observing the model here, don't we need to reset it in > > > MergeDataAndStartSyncing (right after the GetModel() call) to make sure we > do > > > not process model events while building the model from sync data? Also these > > > should probably be added to the bottom of InitFromLocalStorage instead? > (Which > > > may also need to reset the observer at the top of the function? > > > > With local storage support yes, we also need to reset observer in > > MergeDataAndStartSyncing. However I cannot do the same here, in > InitLocalStorage > > because it is called from CTOR and at this moment check at line 364 fails. I > > considered moving InitLocalStorage to the BuildModel call but this is not > > compatible with accessing to pin position, which can be made regardless if > model > > built or not. > > Is there any harm in requiring the model to be build before accessing the pin > position, just to be consistent? > > It still seems strange to me to add the model observer here, since in > AppListSyncableService::MergeDataAndStartSyncing we call GetModel() (likely for > the first time), and then call model_observer_.reset() right after. There is no harm to have model built before pin access. It seems that model is pure optional part of this service and it is only required when app launcher is created. Calling GetModel()from MergeDataAndStartSyncing is not needed. Probably we may address this (making model pure on demand) if we want in separate CL. Calling GetModel() during InitFromLocalStorage is inconvenient. When model is building it may(actually does) modify sync service by creating sync items if they don't exist. We track this changes and update local storage. This causes race condition when building the model before initing from prefs overrides prefs for certain entries with default values. Of cause it is possible to handle such changes by adding some internal flag but this could complicate the logic. IMHO, settings observer on model building makes sense. Before we did not do this due sync_processor was not set and we could do nothing. However now we also map changes to prefs and handling external updates is essential for us. I slightly refactored the logic to simplify everything and make it more readable: 1. Build model when extension service is ready only 2. InitFromLocalStorage is now step of building model. 3. Extract logic to update folders, setting the observer and notifying model updated to helper and call when required. WDYT? https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc (right): https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:31: const app_list::AppListSyncableService* service2) { On 2016/10/21 20:03:10, stevenjb wrote: > Ah. > nit: Just 'SyncItemsMatch' is better, otherwise it should be 'DoSyncItemsMatch', > but that could have multiple meanings. Sure! Sorry for confusion https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/2416133002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service.cc:314: if (switches::IsFolderUIEnabled()) On 2016/10/21 20:03:10, stevenjb wrote: > This tests whether or not sync is enabled on the command line, but since folder > persistence is no longer tied to sync, I think we can just always set this to > true, right? (We can then probably remove SetFoldersEnabled entirely in a > separate CL). > Agree, added TODO
This looks much more clear, thanks, and welcome to the AppListSyncableService code :) LGTM
khmel@chromium.org changed reviewers: + sky@chromium.org
Thank you :)! Adding sky@ to confirm c/b/prefs/browser_prefs.cc c/b/sync/test/integration/single_client_app_list_sync_test.cc PTAL
sky@chromium.org changed reviewers: + maxbogue@chromium.org
I'm not familiar with chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc . Please find a more local owner for that, randomly adding maxbogue. chrome/browser/prefs/browser_prefs.cc LGTM
Thank you sky@ for review! to: maxbogue@ PTAL c/b/sync/test/integration/single_client_app_list_sync_test.cc Thank you!
lgtm w/ a couple comments https://codereview.chromium.org/2416133002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc (right): https://codereview.chromium.org/2416133002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:53: class AppListSyncUpdateWaiter You may consider making this a subclass of StatusChangeChecker, which does the whole waiting bit for you, some better debug messaging, and a timeout. You'd just have to keep a boolean instead of the closure. I don't feel strongly about it though. https://codereview.chromium.org/2416133002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:169: for (const auto app_id : app_ids) { const auto& so you're not doing copies of strings? Or just const std::string& for maximum clarity.
Thank you all for review! I passed tests with latest structure and found 2 minor race conditions in tests. Steven, could you please confirm them? I added comments there. PS. It seems race conditions is possible to fix by using direct observer: https://cs.chromium.org/chromium/src/extensions/browser/notification_types.h?... However it is marked as deprecated and in this code we use ready().Post() instead. PTAL https://codereview.chromium.org/2416133002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc (right): https://codereview.chromium.org/2416133002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:53: class AppListSyncUpdateWaiter On 2016/10/24 19:36:50, maxbogue wrote: > You may consider making this a subclass of StatusChangeChecker, which does the > whole waiting bit for you, some better debug messaging, and a timeout. You'd > just have to keep a boolean instead of the closure. I don't feel strongly about > it though. That is good point, thank you! https://codereview.chromium.org/2416133002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:169: for (const auto app_id : app_ids) { On 2016/10/24 19:36:50, maxbogue wrote: > const auto& so you're not doing copies of strings? Or just const std::string& > for maximum clarity. Good point! Done https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:602: if (!app_service || !app_service->IsInited()) To Steven: Another race condition which is hard to solve in test due initialization order. https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:349: base::RunLoop run_loop; to Steven, this fix race condition when StartAppSyncService is called while model is not built.
lgtm w/ name change https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_syncable_service.h (right): https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_syncable_service.h:125: bool IsInited() const; IsInitialized() https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:602: if (!app_service || !app_service->IsInited()) On 2016/10/25 15:49:00, khmel wrote: > To Steven: Another race condition which is hard to solve in test due > initialization order. Acknowledged. https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2416133002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:349: base::RunLoop run_loop; On 2016/10/25 15:49:00, khmel wrote: > to Steven, this fix race condition when StartAppSyncService is called while > model is not built. Acknowledged.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org, sky@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2416133002/#ps140001 (title: "method renamed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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. Committed: https://crrev.com/f06c029695e6d3a62ea950c1ba0f60cf01c353a8 Cr-Commit-Position: refs/heads/master@{#427390} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f06c029695e6d3a62ea950c1ba0f60cf01c353a8 Cr-Commit-Position: refs/heads/master@{#427390} |
