Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
981893002:
Lazily build the app list model. (Closed)
|
Created:
5 years, 9 months ago by calamity Modified:
5 years, 8 months ago CC:
chromium-reviews, tim+watch_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org, zea+watch_chromium.org, tfarina, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, chromium-apps-reviews_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@fix_set_enabled Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLazily build the app list model.
This CL makes the AppListSyncableService build the app list model when
it is requested instead of building it when the extensions system is
ready.
This fixes an issue where the sync system was being brought up by the
app list on Chrome launch, likely causing startup slowdown.
BUG=462429
Committed: https://crrev.com/acd0831886805f0d3027dc75ba5bf38867b929c0
Cr-Commit-Position: refs/heads/master@{#322731}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : address comments #
Total comments: 5
Patch Set 3 : rebase, address comments #Patch Set 4 : revert ALSL #Messages
Total messages: 39 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
calamity@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc:100: AppListServiceLinux::GetInstance()->Init(initial_profile); This is necessary to fix a test. There's some weird jazz happening in AppListServiceImpl::OnProfileWillBeRemoved(). It uses local state as storage for the current app list profile but there are 2 AppListServiceImpls that exist at once. AppListServiceAsh and AppListServicePlatform both change the local state and whoever gets the notification second doesn't set their view delegate's profile to NULL.
tapted@chromium.org changed reviewers: + stevenjb@chromium.org
Adding stevenjb, since he's the most familiar with this code. Some questions - sync isn't really my area, but I like the approach. https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (left): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:251: if (extension_system->extension_service() && How do you know this logic isn't needed any more? (maybe `git annotate` to see why it exists - I think it was something obscure) I guess model() would have just returned an empty model in those cases. But could something call GetModel() too soon and die? Or: build an empty model, but then never receive any signal that the model is ready to be populated? https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:473: model_->DeleteItem(id); What about all these calls internally that access |model_|? Can sync start (flare?) for something else without anything calling GetModel() and populating the initial set? https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc:100: AppListServiceLinux::GetInstance()->Init(initial_profile); On 2015/03/10 06:18:51, calamity wrote: > This is necessary to fix a test. There's some weird jazz happening in > AppListServiceImpl::OnProfileWillBeRemoved(). > > It uses local state as storage for the current app list profile but there are 2 > AppListServiceImpls that exist at once. AppListServiceAsh and > AppListServicePlatform both change the local state and whoever gets the > notification second doesn't set their view delegate's profile to NULL. What's the test? (e.g. how do you know it's not a "real" problem?). And will windows have this problem as well? Note that on Windows the ash instance is only initialized when metro mode is running.
This lgtm, but we'll want to test on Chrome OS to ensure that this doesn't introduce noticeable lag (although that may be less of an issue with the new UI). https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (left): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:251: if (extension_system->extension_service() && On 2015/03/10 19:19:54, tapted wrote: > How do you know this logic isn't needed any more? (maybe `git annotate` to see > why it exists - I think it was something obscure) > > I guess model() would have just returned an empty model in those cases. But > could something call GetModel() too soon and die? Or: build an empty model, but > then never receive any signal that the model is ready to be populated? Since AppListSyncableService is a context keyed service, it might be initialized before or after the extension service, so we needed to initialize the model either here or once the extension service has been created and we are notified (although that notification appears deprecated, I'm not familiar with the situation there). Nothing should actually access the model before the extension service is ready however. We could add some DCHECKs to BuildModel() to make that clear. (Otherwise we will crash in ExtensionAppModelBuilder::BuildModel). https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:286: BuildModel(); nit: GetModel(); https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:473: model_->DeleteItem(id); On 2015/03/10 19:19:54, tapted wrote: > What about all these calls internally that access |model_|? Can sync start > (flare?) for something else without anything calling GetModel() and populating > the initial set? No sync events will get called before MergeDataAndStartSyncing, and I don't think that MergeDataAndStartSyncing() will get called until SyncStarted() gets called from BuildModel(). However, I'm not 100% certain of that, so we may want to check with the sync team and/or call GetModel() at the top of MergeDataAndStartSyncing(). (Or maybe DCHECK(model_) if we're reasonably sure MergeDataAndStartSyncing won't get called first).
zea@chromium.org changed reviewers: + zea@chromium.org
https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:473: model_->DeleteItem(id); On 2015/03/10 19:56:06, stevenjb wrote: > On 2015/03/10 19:19:54, tapted wrote: > > What about all these calls internally that access |model_|? Can sync start > > (flare?) for something else without anything calling GetModel() and populating > > the initial set? > No sync events will get called before MergeDataAndStartSyncing, and I don't > think that MergeDataAndStartSyncing() will get called until SyncStarted() gets > called from BuildModel(). However, I'm not 100% certain of that, so we may want > to check with the sync team and/or call GetModel() at the top of > MergeDataAndStartSyncing(). (Or maybe DCHECK(model_) if we're reasonably sure > MergeDataAndStartSyncing won't get called first). > Sync can start up at any time, whether or not GetModel/SyncStarted is called here. Keep in mind other datatypes have this same logic and can tell sync to start. So MergeDataAndStartSyncing could happen at any time. You're right that no sync events will happen until MergeDataAndStartSyncing is called though.
https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (left): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:251: if (extension_system->extension_service() && On 2015/03/10 19:56:06, stevenjb wrote: > On 2015/03/10 19:19:54, tapted wrote: > > How do you know this logic isn't needed any more? (maybe `git annotate` to see > > why it exists - I think it was something obscure) > > > > I guess model() would have just returned an empty model in those cases. But > > could something call GetModel() too soon and die? Or: build an empty model, > but > > then never receive any signal that the model is ready to be populated? > > Since AppListSyncableService is a context keyed service, it might be initialized > before or after the extension service, so we needed to initialize the model > either here or once the extension service has been created and we are notified > (although that notification appears deprecated, I'm not familiar with the > situation there). > > Nothing should actually access the model before the extension service is ready > however. We could add some DCHECKs to BuildModel() to make that clear. > (Otherwise we will crash in ExtensionAppModelBuilder::BuildModel). DCHECKs added. https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:286: BuildModel(); On 2015/03/10 19:56:06, stevenjb wrote: > nit: GetModel(); Done. https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:473: model_->DeleteItem(id); Hmm. I don't know if I should ensure the model is built in MergeDataAndStartSyncing. Chances are, it was always built before it in the past. I don't know if there would be problems if it wasn't built when MDASS is called. I'm not in Sydney now so it's a bit tricky to check. https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc:100: AppListServiceLinux::GetInstance()->Init(initial_profile); On 2015/03/10 19:19:54, tapted wrote: > On 2015/03/10 06:18:51, calamity wrote: > > This is necessary to fix a test. There's some weird jazz happening in > > AppListServiceImpl::OnProfileWillBeRemoved(). > > > > It uses local state as storage for the current app list profile but there are > 2 > > AppListServiceImpls that exist at once. AppListServiceAsh and > > AppListServicePlatform both change the local state and whoever gets the > > notification second doesn't set their view delegate's profile to NULL. > > What's the test? (e.g. how do you know it's not a "real" problem?). And will > windows have this problem as well? > > Note that on Windows the ash instance is only initialized when metro mode is > running. It is a 'real' problem. I guess I should fix it before I land this. I think AppListServiceAsh just shouldn't be changing local_state_ for that case. The test is AppListServiceImplTest.DeletingProfileUpdatesViewDelegate. Why is the AppListServiceAsh always created for linux? Seems weird.
https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:473: model_->DeleteItem(id); On 2015/03/12 21:23:22, calamity wrote: > Hmm. I don't know if I should ensure the model is built in > MergeDataAndStartSyncing. Chances are, it was always built before it in the > past. I don't know if there would be problems if it wasn't built when MDASS is > called. I'm not in Sydney now so it's a bit tricky to check. If you're concerned about a race here, Sync has the ability to hold off on calling MergeDataAndStartSyncing until some condition is met via the datatype controller logic. See for example the search_engine_data_type_controller.cc. You'd need to create an app_list_data_type_controller to do this.
I kicked the tires, everything seems to work fine.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/981893002/#ps60001 (title: "address comments")
The CQ bit was unchecked by calamity@chromium.org
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981893002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:473: model_->DeleteItem(id); On 2015/03/12 21:23:22, calamity wrote: > Hmm. I don't know if I should ensure the model is built in > MergeDataAndStartSyncing. Chances are, it was always built before it in the > past. I don't know if there would be problems if it wasn't built when MDASS is > called. I'm not in Sydney now so it's a bit tricky to check. Perhaps just GetModel() with a comment at the start of MergeDataAndStartSyncing()? https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc:100: AppListServiceLinux::GetInstance()->Init(initial_profile); On 2015/03/12 21:23:22, calamity wrote: > On 2015/03/10 19:19:54, tapted wrote: > > On 2015/03/10 06:18:51, calamity wrote: > > > This is necessary to fix a test. There's some weird jazz happening in > > > AppListServiceImpl::OnProfileWillBeRemoved(). > > > > > > It uses local state as storage for the current app list profile but there > are > > 2 > > > AppListServiceImpls that exist at once. AppListServiceAsh and > > > AppListServicePlatform both change the local state and whoever gets the > > > notification second doesn't set their view delegate's profile to NULL. > > > > What's the test? (e.g. how do you know it's not a "real" problem?). And will > > windows have this problem as well? > > > > Note that on Windows the ash instance is only initialized when metro mode is > > running. > > It is a 'real' problem. I guess I should fix it before I land this. I think > AppListServiceAsh just shouldn't be changing local_state_ for that case. The > test is AppListServiceImplTest.DeletingProfileUpdatesViewDelegate. Why is the > AppListServiceAsh always created for linux? Seems weird. I think for a while there was an `Open Ash` option from the Hotdog menu, which didn't trigger a restart. Whereas on Windows, to get to Ash you need to restart chrome (so Ash is only sometimes-initialized). https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:256: DCHECK(extension_system_->extension_service() && If we're unsure about an obscure startup sequence, we could make this a CHECK (with a TODO to make it a DCHECK after a dev channel release). https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc (right): https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc:100: AppListServiceLinux::GetInstance()->Init(initial_profile); Did you want to make this same fix in app_list_service_win.cc's InitAll(..) ? I'm not sure there's coverage in metro mode for the app list integration tests, which is the place that would care.
https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:473: model_->DeleteItem(id); On 2015/03/20 11:47:14, tapted (travelling) wrote: > On 2015/03/12 21:23:22, calamity wrote: > > Hmm. I don't know if I should ensure the model is built in > > MergeDataAndStartSyncing. Chances are, it was always built before it in the > > past. I don't know if there would be problems if it wasn't built when MDASS is > > called. I'm not in Sydney now so it's a bit tricky to check. > > Perhaps just GetModel() with a comment at the start of > MergeDataAndStartSyncing()? Done. https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_syncable_service.cc:256: DCHECK(extension_system_->extension_service() && On 2015/03/20 11:47:15, tapted (travelling) wrote: > If we're unsure about an obscure startup sequence, we could make this a CHECK > (with a TODO to make it a DCHECK after a dev channel release). Done. https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc (right): https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc:100: AppListServiceLinux::GetInstance()->Init(initial_profile); On 2015/03/20 11:47:15, tapted (travelling) wrote: > Did you want to make this same fix in app_list_service_win.cc's InitAll(..) ? > I'm not sure there's coverage in metro mode for the app list integration tests, > which is the place that would care. I put up another CL to make this change unnecessary. https://codereview.chromium.org/1020133002/ I think it's valid... Does the metro mode app list have profile switching? If so, I guess it's not really valid...
https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc (right): https://codereview.chromium.org/981893002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/linux/app_list_service_linux.cc:100: AppListServiceLinux::GetInstance()->Init(initial_profile); On 2015/03/23 05:14:49, calamity wrote: > On 2015/03/20 11:47:15, tapted (travelling) wrote: > > Did you want to make this same fix in app_list_service_win.cc's InitAll(..) ? > > I'm not sure there's coverage in metro mode for the app list integration > tests, > > which is the place that would care. > > I put up another CL to make this change unnecessary. > > https://codereview.chromium.org/1020133002/ > > I think it's valid... Does the metro mode app list have profile switching? If > so, I guess it's not really valid... all lgtm if the bots are happy with this reverted, now that r322322 has landed.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/981893002/#ps80001 (title: "rebase, address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981893002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/981893002/#ps120001 (title: "revert ALSL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981893002/120001
zea@ can you PTAL at c/b/sync? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sync LGTM (in the future, it's okay to TBR for this kind of renaming of callsites)
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981893002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/acd0831886805f0d3027dc75ba5bf38867b929c0 Cr-Commit-Position: refs/heads/master@{#322731} |
