Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 7 months ago by jungkees Modified:
5 years, 6 months ago Reviewers:
michaeln, falken, ncarter (slow), nhiroki, kinuko, Mike West, Alexei Svitkine (slow), Ilya Sherman CC:
chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionService Worker: Add ServiceWorkerContainer.getRegistrations() method.
Implement ServiceWorkerContainer.getReistrations() method as per spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#navigator-service-worker-getRegistrations
In this patch, ServiceWorkerDatabase::GetRegistrationsForOrigin() has been
changed to additionally retrieving resources so a newly added
ServiceWorkerStorage::DiDGetRegistrations() can get all the stored real
registrations by calling ServiceWorkerStorage::GetOrCreateRegistration().
ServiceWorkerStorage::DidGetRegistrationsInfos() remained as-is used by
ServiceWorkerStorage::GetAllRegistrationsInfos() to get a vector of
ServiceWorkerRegistrationInfo objects.
Companion CL (Blink): https://codereview.chromium.org/1165363003/
Companion CL (Blink layout tests): https://codereview.chromium.org/1168393002/
BUG=478382
Committed: https://crrev.com/df6d1bf21f9b1e30c9d50a7697d6dc869c36097a
Cr-Commit-Position: refs/heads/master@{#333886}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use provider's origin in browser instead of passing client's url from renderer. #
Total comments: 3
Patch Set 3 : Refactor GetRegistrationsForOrigin to get real registrations. #
Total comments: 11
Patch Set 4 : Make GetRegistrationsForOrigin() handle nullptr for resource param. #
Total comments: 10
Patch Set 5 : Use scoped_refptr to ServiceWorkerRegistration and rename variables. #
Total comments: 16
Patch Set 6 : Add database unittests for reading resources list; Rebase to ToT. #
Total comments: 1
Patch Set 7 : Rebase to ToT. #
Total comments: 2
Patch Set 8 : Update tools/metrics/histograms/histograms.xml. #
Total comments: 3
Created: 5 years, 6 months ago
Messages
Total messages: 64 (20 generated)
jungkee.song@samsung.com changed reviewers: + falken@chromium.org, kinuko@chromium.org, nhiroki@chromium.org
PTAL
Generally looks good. https://codereview.chromium.org/1146913004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_dispatcher_host.cc:519: "ServiceWorkerDispatcherHost::OnGetRegistrations"); (nit: are all these TRACE EVENT's really useful? this is probably not a question to you but to the team?) https://codereview.chromium.org/1146913004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_dispatcher_host.cc:561: DCHECK_CURRENTLY_ON(BrowserThread::IO); nit: Why do you check this here rather than at the beginning of the method? https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... content/child/service_worker/service_worker_dispatcher.cc:188: const GURL& document_url, I have a feeling that this doesn't need to be (or shouldn't be) passed from the renderer but browser can get registrations for the provider's origin? https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... File content/child/service_worker/web_service_worker_provider_impl.h (right): https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... content/child/service_worker/web_service_worker_provider_impl.h:45: WebServiceWorkerGetRegistrationsCallbacks*); nit: indent
https://codereview.chromium.org/1146913004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_dispatcher_host.cc:519: "ServiceWorkerDispatcherHost::OnGetRegistrations"); On 2015/05/21 07:57:06, kinuko wrote: > (nit: are all these TRACE EVENT's really useful? this is probably not a question > to you but to the team?) Has any event profiling activity been going on with this? Just removed this particular TRACE, but I'll put some relevant TRACEs back if they are needed for profiling. Let me know. https://codereview.chromium.org/1146913004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_dispatcher_host.cc:561: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2015/05/21 07:57:06, kinuko wrote: > nit: Why do you check this here rather than at the beginning of the method? Was copied from OnGetRegistration() actually. Agreed and moved it to the beginning of the method. Done the same for OnGetRegistration(). https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... content/child/service_worker/service_worker_dispatcher.cc:188: const GURL& document_url, On 2015/05/21 07:57:06, kinuko wrote: > I have a feeling that this doesn't need to be (or shouldn't be) passed from the > renderer but browser can get registrations for the provider's origin? Agreed. Correct and much simpler! https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... File content/child/service_worker/web_service_worker_provider_impl.h (right): https://codereview.chromium.org/1146913004/diff/1/content/child/service_worke... content/child/service_worker/web_service_worker_provider_impl.h:45: WebServiceWorkerGetRegistrationsCallbacks*); On 2015/05/21 07:57:06, kinuko wrote: > nit: indent Just gone by removing the first parameter.
Thanks for the review kinuko@. New snapshot uploaded as such. PTAL.
michaeln@chromium.org changed reviewers: + michaeln@chromium.org
some drive by comments https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:1049: GetContext()->GetLiveRegistration(infos[i].registration_id); What about registrations that are stored but not "live"? I think the intent of the method is to return all registrations for an origin (is that right?). What about unittests for the new ServiceWorkerDispatcherHost method to cover picking up regs that are live vs not live and various error conditions.
https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:1049: GetContext()->GetLiveRegistration(infos[i].registration_id); On 2015/05/21 20:33:30, michaeln wrote: > What about registrations that are stored but not "live"? I think the intent of > the method is to return all registrations for an origin (is that right?). Yes, I think so. (Hadn't looked impl details yet) Probably we'd want to somehow reuse Storage::GetOrCreateRegistration. (Looks like this method could be somewhat heavyweight, we might be populating lots of registrations and versions but most of them would be quickly discarded... for initial cut I assume it should be fine though) > What about unittests for the new ServiceWorkerDispatcherHost method to cover > picking up regs that are live vs not live and various error conditions. +1
https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:1049: GetContext()->GetLiveRegistration(infos[i].registration_id); On 2015/05/22 06:19:06, kinuko wrote: > On 2015/05/21 20:33:30, michaeln wrote: > > What about registrations that are stored but not "live"? I think the intent of > > the method is to return all registrations for an origin (is that right?). > > Yes, I think so. (Hadn't looked impl details yet) > > Probably we'd want to somehow reuse Storage::GetOrCreateRegistration. > > (Looks like this method could be somewhat heavyweight, we might be populating > lots of registrations and versions but most of them would be quickly > discarded... for initial cut I assume it should be fine though) > Yes, I think I'd like to try something like: // callback: ServiceWorkerDispatcherHost::GetRegistrationsComplete // registration's type: std::vector<ServiceWorkerRegistration> ServiceWorkerStorage::DidGetRegistrations(callback, reg_data_vector, res_record_vector ..) for each reg_data in reg_data_vector: registration := GetLiveRegistration(reg_data.id) || GetOrCreateRegistration(reg_data, res_record_vector) registrations.push_back(registration) callback.Run(registrations) } I'll follow up on it. Will there be better way to do? > > What about unittests for the new ServiceWorkerDispatcherHost method to cover > > picking up regs that are live vs not live and various error conditions. > > +1 Will add this.
On 2015/05/22 13:26:44, jungkees wrote: > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_dispatcher_host.cc:1049: > GetContext()->GetLiveRegistration(infos[i].registration_id); > On 2015/05/22 06:19:06, kinuko wrote: > > On 2015/05/21 20:33:30, michaeln wrote: > > > What about registrations that are stored but not "live"? I think the intent > of > > > the method is to return all registrations for an origin (is that right?). > > > > Yes, I think so. (Hadn't looked impl details yet) > > > > Probably we'd want to somehow reuse Storage::GetOrCreateRegistration. > > > > (Looks like this method could be somewhat heavyweight, we might be populating > > lots of registrations and versions but most of them would be quickly > > discarded... for initial cut I assume it should be fine though) > > > > Yes, I think I'd like to try something like: > // callback: ServiceWorkerDispatcherHost::GetRegistrationsComplete > // registration's type: std::vector<ServiceWorkerRegistration> > ServiceWorkerStorage::DidGetRegistrations(callback, reg_data_vector, > res_record_vector ..) > for each reg_data in reg_data_vector: > registration := GetLiveRegistration(reg_data.id) || > GetOrCreateRegistration(reg_data, res_record_vector) > registrations.push_back(registration) > callback.Run(registrations) > } GetOrCreateRegistration seems to also perform GetLiveRegistration internally. I suspect you might want to refactor the storage methods a bit, rename the existing GetRegistrationsForOrigin to something like GetRegistrationInfosForOrigin and add a new method that retrieves registrations (but not registration infos)? michaeln@ and nhiroki@ may probably have better ideas though. > I'll follow up on it. Will there be better way to do? > > > > What about unittests for the new ServiceWorkerDispatcherHost method to cover > > > picking up regs that are live vs not live and various error conditions. > > > > +1 > > Will add this.
On 2015/05/25 07:13:13, kinuko wrote: > On 2015/05/22 13:26:44, jungkees wrote: > > > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... > > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > > > > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... > > content/browser/service_worker/service_worker_dispatcher_host.cc:1049: > > GetContext()->GetLiveRegistration(infos[i].registration_id); > > On 2015/05/22 06:19:06, kinuko wrote: > > > On 2015/05/21 20:33:30, michaeln wrote: > > > > What about registrations that are stored but not "live"? I think the > intent > > of > > > > the method is to return all registrations for an origin (is that right?). > > > > > > Yes, I think so. (Hadn't looked impl details yet) > > > > > > Probably we'd want to somehow reuse Storage::GetOrCreateRegistration. > > > > > > (Looks like this method could be somewhat heavyweight, we might be > populating > > > lots of registrations and versions but most of them would be quickly > > > discarded... for initial cut I assume it should be fine though) > > > > > > > Yes, I think I'd like to try something like: > > // callback: ServiceWorkerDispatcherHost::GetRegistrationsComplete > > // registration's type: std::vector<ServiceWorkerRegistration> > > ServiceWorkerStorage::DidGetRegistrations(callback, reg_data_vector, > > res_record_vector ..) > > for each reg_data in reg_data_vector: > > registration := GetLiveRegistration(reg_data.id) || > > GetOrCreateRegistration(reg_data, res_record_vector) > > registrations.push_back(registration) > > callback.Run(registrations) > > } > > GetOrCreateRegistration seems to also perform GetLiveRegistration internally. > > I suspect you might want to refactor the storage methods a bit, rename the > existing GetRegistrationsForOrigin to something like > GetRegistrationInfosForOrigin and add a new method that retrieves registrations > (but not registration infos)? michaeln@ and nhiroki@ may probably have better > ideas though. SGTM. Apparently, GetRegistrationsForOrigin is no longer used except tests, so we could directly change the method to retrieve real registrations (and we could rename GetAllRegistrations to GetAllRegistrationInfos).
On 2015/05/25 18:44:07, nhiroki wrote: > On 2015/05/25 07:13:13, kinuko wrote: > > On 2015/05/22 13:26:44, jungkees wrote: > > > > > > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... > > > File content/browser/service_worker/service_worker_dispatcher_host.cc > (right): > > > > > > > > > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service... > > > content/browser/service_worker/service_worker_dispatcher_host.cc:1049: > > > GetContext()->GetLiveRegistration(infos[i].registration_id); > > > On 2015/05/22 06:19:06, kinuko wrote: > > > > On 2015/05/21 20:33:30, michaeln wrote: > > > > > What about registrations that are stored but not "live"? I think the > > intent > > > of > > > > > the method is to return all registrations for an origin (is that > right?). > > > > > > > > Yes, I think so. (Hadn't looked impl details yet) > > > > > > > > Probably we'd want to somehow reuse Storage::GetOrCreateRegistration. > > > > > > > > (Looks like this method could be somewhat heavyweight, we might be > > populating > > > > lots of registrations and versions but most of them would be quickly > > > > discarded... for initial cut I assume it should be fine though) > > > > > > > > > > Yes, I think I'd like to try something like: > > > // callback: ServiceWorkerDispatcherHost::GetRegistrationsComplete > > > // registration's type: std::vector<ServiceWorkerRegistration> > > > ServiceWorkerStorage::DidGetRegistrations(callback, reg_data_vector, > > > res_record_vector ..) > > > for each reg_data in reg_data_vector: > > > registration := GetLiveRegistration(reg_data.id) || > > > GetOrCreateRegistration(reg_data, res_record_vector) > > > registrations.push_back(registration) > > > callback.Run(registrations) > > > } > > > > GetOrCreateRegistration seems to also perform GetLiveRegistration internally. > > > > I suspect you might want to refactor the storage methods a bit, rename the > > existing GetRegistrationsForOrigin to something like > > GetRegistrationInfosForOrigin and add a new method that retrieves > registrations > > (but not registration infos)? michaeln@ and nhiroki@ may probably have better > > ideas though. > > SGTM. Apparently, GetRegistrationsForOrigin is no longer used except tests, so > we could directly change the method to retrieve real registrations (and we could > rename GetAllRegistrations to GetAllRegistrationInfos). I'll work in that direction. Thanks for comments.
I'm having an access denied error when git cl upload to this CL. The companion Blink CL successfully went in in the same terminal shell. depot-tools-auth login to codereview site is done with no problem as well. Any clue? I'll continue tomorrow to resolve this uploading issue: ... 18 files changed, 486 insertions(+), 92 deletions(-) This branch is associated with issue 1146913004. Adding patch to that issue. Upload server: https://codereview.chromium.org (change with -s/--server) Title describing this patch set: Refactor GetRegistrationsForOrigin to get real registrations. Access to https://codereview.chromium.org is denied (server returned HTTP 403). $_
Maybe depot tools is confused about your login? I'd try to reauthenticate, look in chromium-dev mailing list for info about depot-tools-auth login https://codereview.chromium.org The method renames sgtm2.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
It was not an authentication error. Login to codereview was fine. I'd tried quite a few things. Uploading only the files that were in the previous patch sets worked fine actually. But when it comes with other files newly added in this new patch, it raised the access denied error. I suspect some of the newly changed files are filtered in the proxy and make a bad request or something like that. Finally I made it at home this time. PTAL.
Looks pretty good. It'd be nice to flesh out the CL description a bit more mentioning what was changed and why (e.g, why GetRegistrationsForOrigin returns resources now). https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1117: std::vector<ServiceWorkerRegistration*> regs; I think some naming tweaks are in order here, it's hard to understand at a glance registrations vs pushed_registrations vs regs. |registrations| → |registration_data| |pushed_registrationed| → |registration_ids| |regs| → |registrations| Maybe? https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1620: &resource_lists); if resource_lists is not used, does it make sense to pass a nullptr and make GetRegForOrigin handle that case? Would it avoid an extra read? https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1706: origin, ®istrations, &resource_lists); same with all these https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:256: typedef std::vector<ResourceList> ResourceListList; nit: It's subjective but feels nicer to me to just not typedef this. https://codereview.chromium.org/1146913004/diff/220001/content/child/service_... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/1146913004/diff/220001/content/child/service_... content/child/service_worker/service_worker_dispatcher.cc:454: return; JFYI: We're not suppsoed to write code like this, but since all functions in this file do let's keep it for consistency: https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...
I've just made the new patch set having addressed the comments. But again encountered the access denied (403) error when doing git cl upload. I'll upload it tonight. Thanks for the review in details. https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1117: std::vector<ServiceWorkerRegistration*> regs; The proposed names look clear. Just renamed them as such. https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1620: &resource_lists); On 2015/06/05 02:58:22, falken wrote: > if resource_lists is not used, does it make sense to pass a nullptr and make > GetRegForOrigin handle that case? Would it avoid an extra read? Yes, that's a better way. I'd mulled it over when I initially worked on. Addressed your comment. Thanks. https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:256: typedef std::vector<ResourceList> ResourceListList; Removed the typedef. Seems ListList sounded odd and the original expression is not that long, right? https://codereview.chromium.org/1146913004/diff/220001/content/child/service_... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/1146913004/diff/220001/content/child/service_... content/child/service_worker/service_worker_dispatcher.cc:454: return; Thanks for the pointer. So in this particular file, we don't want to change the behavior (just returning) for a release binary? Yeah, it seems we cannot even reject the promise as there's no callback in this case.
Uploaded a new snapshot. PTAL
some more drive-by comments https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_database.h:105: std::vector<std::vector<ResourceRecord>>* resource_lists); since this param may be null, please name it |opt_resources_lists| or something to indicate its not required https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1117: std::vector<ServiceWorkerRegistration*> registrations; This needs to be a vector of scoped_refptrs, otherwise if a callee makes a copy of the vector for later use, that would be bad. Also, if the registration is created in this loop, it will already have been deleted by the time the callee gets the vector. https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1119: for (auto it = (*registration_data).begin(); it != (*registration_data).end(); This loop probably could be easier to read and match more closely the loop in the very similar method below. int index = 0 for (const auto& registration_data : *registrations) { registration_ids.insert(registration_data.registration_id); registrations.push_back( GetOrCreateRegistration(registration_data, resource_lists->at(index++)); } https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1126: if (registration.get()) { no need for the null check, it's either gotten or created https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:56: typedef base::Callback<void(const std::vector<ServiceWorkerRegistration*>& std::vector<scoped_refptr<<ServiceWorkerRegistration>> see comments in the .cc file
Thanks for the review! Just uploaded a new snapshot having addressed the comments. PTAL
https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_database.h:105: std::vector<std::vector<ResourceRecord>>* resource_lists); Renamed it |opt_resources_list|. Note that I found resources_list is more relevant than resource_lists as other param definitions are already using resources to refer to the vector of ResourceRecord. Corrected the related param names in service_worker_storage.h/.cc as well. https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1117: std::vector<ServiceWorkerRegistration*> registrations; I had to be more careful about this indeed. Addressed the comment. Thanks. https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1119: for (auto it = (*registration_data).begin(); it != (*registration_data).end(); Neat! Addressed. Thanks. https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1126: if (registration.get()) { Right. This condition has been removed by replacing the loop code with the one your commented. https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:56: typedef base::Callback<void(const std::vector<ServiceWorkerRegistration*>& Addressed.
This is looking good. It seems missing some test coverage, can you add unittests for reading the resource lists? https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1146913004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:256: typedef std::vector<ResourceList> ResourceListList; On 2015/06/05 06:58:07, jungkees wrote: > Removed the typedef. Seems ListList sounded odd and the original expression is > not that long, right? Yea ListList is weird and generally it's easier for me to understand std::vector<XXX> than XXXList. https://codereview.chromium.org/1146913004/diff/220001/content/child/service_... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/1146913004/diff/220001/content/child/service_... content/child/service_worker/service_worker_dispatcher.cc:454: return; On 2015/06/05 06:58:07, jungkees wrote: > Thanks for the pointer. So in this particular file, we don't want to change the > behavior (just returning) for a release binary? Yeah, it seems we cannot even > reject the promise as there's no callback in this case. Right, we should either DCHECK() and not handle the failure, or handle the failure and not DCHECK. DCHECK means the failure is not possible. I think this particular file should be changed to conform to that guideline, but not in this patch. https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:457: for (itr->Seek(prefix); itr->Valid(); itr->Next(), ++index) { |index| is not used? https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:532: BadMessageReceived(); You have to rebase to ToT, this changed recently to ReceivedBadMessage(reason). https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.h:33: struct ServiceWorkerRegistrationInfo; nit: wrong alphabetical order https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:605: // Finding by origin should provide the same result iif origin is kScope. nit: not from this patch but s/iif/if https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:606: std::vector<scoped_refptr<ServiceWorkerRegistration>> registrations_origin; nit: also not from this patch but registrations_for_origin reads better https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:729: std::vector<std::vector<ResourceRecord>> resource_lists; Do we ever test the value of this?
https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:457: for (itr->Seek(prefix); itr->Valid(); itr->Next(), ++index) { No, it isn't. I removed it. https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:532: BadMessageReceived(); Rebasing to ToT done. I added two new error message entries at the end of the list in the enum as guided in bad_message.h file as follows: SWDH_GET_REGISTRATIONS_NO_HOST = 78, SWDH_GET_REGISTRATIONS_INVALID_ORIGIN = 79, https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.h (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.h:33: struct ServiceWorkerRegistrationInfo; Fixed. https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:605: // Finding by origin should provide the same result iif origin is kScope. Fixed. https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:606: std::vector<scoped_refptr<ServiceWorkerRegistration>> registrations_origin; Renamed all the occurrences. https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:729: std::vector<std::vector<ResourceRecord>> resource_lists; Thanks for spotting this. This value is not at all used in service_worker_storage files. I removed them.
Uploaded a new snapshot. PTAL
https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:606: std::vector<scoped_refptr<ServiceWorkerRegistration>> registrations_origin; On 2015/06/08 16:00:19, jungkees wrote: > Renamed all the occurrences. Sorry, I didn't realize there were multiple occurrences. Thanks for doing the renaming. https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:729: std::vector<std::vector<ResourceRecord>> resource_lists; On 2015/06/08 16:00:19, jungkees wrote: > Thanks for spotting this. This value is not at all used in > service_worker_storage files. I removed them. I was hoping we could add tests for the new functionality. That is, test that the resource_lists are populated with what you'd expect.
https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:729: std::vector<std::vector<ResourceRecord>> resource_lists; That bit is done in the service_worker_database_unittest.cc in which I added assertions as such. ServiceWorkerStorage::GetRegistrationsForOrigin() does not retrieve resources in itself.
lgtm https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:729: std::vector<std::vector<ResourceRecord>> resource_lists; On 2015/06/08 16:27:17, jungkees wrote: > That bit is done in the service_worker_database_unittest.cc in which I added > assertions as such. ServiceWorkerStorage::GetRegistrationsForOrigin() does not > retrieve resources in itself. Ah of course. Thanks! https://codereview.chromium.org/1146913004/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1146913004/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:489: HandleReadResult(FROM_HERE, status); Maybe we should do registrations->clear() here also, if this function returns error probably the output args should be empty.
Thanks for the review! Indeed I found that bad_message.h has been changed with a new CL landed after my latest patch set. I'll merge my change for the file and incorporate your last comment in the final patch set.
Uploaded a new snapshot rebased onto ToT and having the last comment (adding registrations->clear()) addressed.
The CQ bit was checked by jungkee.song@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/1146913004/#ps300001 (title: "Rebase to ToT.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146913004/300001
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...)
jungkee.song@samsung.com changed reviewers: + mkwst@chromium.org, nick@chromium.org
nick@, mkwst@, While committing this CL, I encountered a chromium-presubmit error missing LGTMs for 'content/browser/bad_message.h' and 'content/common/service_worker/service_worker_messages.h' files. This CL is adding ServiceWorkerContainer.getRegistrations() method as reviewed by Service Worker team. PTAL
IPC LGTM % nit. https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_me... File content/browser/bad_message.h (right): https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_me... content/browser/bad_message.h:24: // python tools/metrics/histograms/update_bad_message_reasons.py I don't think you did this.
Updated tools/metrics/histograms/histograms.xml. https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_me... File content/browser/bad_message.h (right): https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_me... content/browser/bad_message.h:24: // python tools/metrics/histograms/update_bad_message_reasons.py Updated. Thanks.
The CQ bit was checked by jungkee.song@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1146913004/#ps320001 (title: "Update tools/metrics/histograms/histograms.xml.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146913004/320001
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...)
kinuko@ Could you take a look at 'content/browser/bad_message.h'? And I'm not sure to whom I can ask a review of 'tools/metrics/histograms/histograms.xml' file. Is that one of the owners of src directory?
On 2015/06/10 15:29:54, jungkees wrote: > kinuko@ Could you take a look at 'content/browser/bad_message.h'? And I'm not > sure to whom I can ask a review of 'tools/metrics/histograms/histograms.xml' > file. Is that one of the owners of src directory? bad_message.h LGTM. You can ask isherman for a histograms.xml review.
lgtm/2 (reg: histograms.xml we often ask asvitkine or isherman to review) https://codereview.chromium.org/1146913004/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1136: it != installing_registrations_.end(); ++it) { nit: you could also use auto / range-based loop for this
jungkee.song@samsung.com changed reviewers: + asvitkine@chromium.org, isherman@chromium.org
asvitkine@ isherman@ Could you take a look at histograms.xml that has two additional entries by running update_bad_message_reasons.py? https://codereview.chromium.org/1146913004/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1136: it != installing_registrations_.end(); ++it) { Right. A thing is that I'm having uploading this patch set from within our proxy (still unknown 403 error), so this change will prevent me from committing it until the end of the day. Can I change this in a separate CL if you don't mind? Let me know.
https://codereview.chromium.org/1146913004/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1136: it != installing_registrations_.end(); ++it) { On 2015/06/11 01:03:24, jungkees wrote: > Right. A thing is that I'm having uploading this patch set from within our proxy > (still unknown 403 error), so this change will prevent me from committing it > until the end of the day. Can I change this in a separate CL if you don't mind? > Let me know. > np, works for me.
histograms.xml lgtm
The CQ bit was checked by jungkee.song@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146913004/320001
Message was sent while issue was closed.
Committed patchset #8 (id:320001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/df6d1bf21f9b1e30c9d50a7697d6dc869c36097a Cr-Commit-Position: refs/heads/master@{#333886} |
