Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(280)

Issue 1146913004: Service Worker: Add ServiceWorkerContainer.getRegistrations() method. (Closed)

Created:
5 years, 7 months ago by jungkees
Modified:
5 years, 6 months ago
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.

Description

Service 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+528 lines, -104 lines) Patch
M content/browser/bad_message.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_context_core.cc View 1 2 3 4 5 1 chunk +2 lines, -4 lines 0 comments Download
M content/browser/service_worker/service_worker_context_wrapper.cc View 1 2 3 4 5 2 chunks +3 lines, -4 lines 0 comments Download
M content/browser/service_worker/service_worker_database.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_database.cc View 1 2 3 4 5 6 5 chunks +20 lines, -3 lines 0 comments Download
M content/browser/service_worker/service_worker_database_unittest.cc View 1 2 3 4 5 5 chunks +42 lines, -4 lines 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host.h View 1 2 3 4 5 4 chunks +13 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host.cc View 1 2 3 4 5 6 chunks +125 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_dispatcher_host_unittest.cc View 1 2 3 4 5 3 chunks +46 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_storage.h View 1 2 3 4 3 chunks +18 lines, -13 lines 0 comments Download
M content/browser/service_worker/service_worker_storage.cc View 1 2 3 4 5 7 chunks +69 lines, -30 lines 3 comments Download
M content/browser/service_worker/service_worker_storage_unittest.cc View 1 2 3 4 5 8 chunks +56 lines, -44 lines 0 comments Download
M content/child/service_worker/service_worker_dispatcher.h View 1 6 chunks +20 lines, -0 lines 0 comments Download
M content/child/service_worker/service_worker_dispatcher.cc View 1 2 3 4 5 5 chunks +78 lines, -0 lines 0 comments Download
M content/child/service_worker/web_service_worker_provider_impl.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/child/service_worker/web_service_worker_provider_impl.cc View 1 1 chunk +6 lines, -0 lines 0 comments Download
M content/common/service_worker/service_worker_messages.h View 1 2 3 4 5 3 chunks +20 lines, -0 lines 0 comments Download
M content/common/service_worker/service_worker_types.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/common/service_worker/service_worker_types.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 64 (20 generated)
jungkees
PTAL
5 years, 7 months ago (2015-05-21 05:18:49 UTC) #2
kinuko
Generally looks good. https://codereview.chromium.org/1146913004/diff/1/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/1/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode519 content/browser/service_worker/service_worker_dispatcher_host.cc:519: "ServiceWorkerDispatcherHost::OnGetRegistrations"); (nit: are all these TRACE ...
5 years, 7 months ago (2015-05-21 07:57:06 UTC) #3
jungkees
https://codereview.chromium.org/1146913004/diff/1/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/1/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode519 content/browser/service_worker/service_worker_dispatcher_host.cc:519: "ServiceWorkerDispatcherHost::OnGetRegistrations"); On 2015/05/21 07:57:06, kinuko wrote: > (nit: are ...
5 years, 7 months ago (2015-05-21 13:46:25 UTC) #4
jungkees
Thanks for the review kinuko@. New snapshot uploaded as such. PTAL.
5 years, 7 months ago (2015-05-21 13:48:32 UTC) #5
michaeln
some drive by comments https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode1049 content/browser/service_worker/service_worker_dispatcher_host.cc:1049: GetContext()->GetLiveRegistration(infos[i].registration_id); What about registrations that ...
5 years, 7 months ago (2015-05-21 20:33:31 UTC) #7
kinuko
https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode1049 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 ...
5 years, 7 months ago (2015-05-22 06:19:06 UTC) #8
jungkees
https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode1049 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 ...
5 years, 7 months ago (2015-05-22 13:26:44 UTC) #9
kinuko
On 2015/05/22 13:26:44, jungkees wrote: > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/1146913004/diff/20001/content/browser/service_worker/service_worker_dispatcher_host.cc#newcode1049 > ...
5 years, 7 months ago (2015-05-25 07:13:13 UTC) #10
nhiroki
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_worker/service_worker_dispatcher_host.cc ...
5 years, 7 months ago (2015-05-25 18:44:07 UTC) #11
jungkees
On 2015/05/25 18:44:07, nhiroki wrote: > On 2015/05/25 07:13:13, kinuko wrote: > > On 2015/05/22 ...
5 years, 7 months ago (2015-05-26 00:52:43 UTC) #12
jungkees
I'm having an access denied error when git cl upload to this CL. The companion ...
5 years, 7 months ago (2015-05-27 12:35:49 UTC) #13
michaeln
Maybe depot tools is confused about your login? I'd try to reauthenticate, look in chromium-dev ...
5 years, 6 months ago (2015-05-27 18:28:15 UTC) #14
jungkees
It was not an authentication error. Login to codereview was fine. I'd tried quite a ...
5 years, 6 months ago (2015-05-28 12:48:22 UTC) #24
jungkees
5 years, 6 months ago (2015-05-28 12:48:48 UTC) #25
falken
Looks pretty good. It'd be nice to flesh out the CL description a bit more ...
5 years, 6 months ago (2015-06-05 02:58:22 UTC) #26
jungkees
I've just made the new patch set having addressed the comments. But again encountered the ...
5 years, 6 months ago (2015-06-05 06:58:07 UTC) #27
jungkees
Uploaded a new snapshot. PTAL
5 years, 6 months ago (2015-06-05 12:53:57 UTC) #28
michaeln
some more drive-by comments https://codereview.chromium.org/1146913004/diff/240001/content/browser/service_worker/service_worker_database.h File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/service_worker/service_worker_database.h#newcode105 content/browser/service_worker/service_worker_database.h:105: std::vector<std::vector<ResourceRecord>>* resource_lists); since this param ...
5 years, 6 months ago (2015-06-05 21:50:42 UTC) #29
jungkees
Thanks for the review! Just uploaded a new snapshot having addressed the comments. PTAL
5 years, 6 months ago (2015-06-06 13:55:01 UTC) #30
jungkees
https://codereview.chromium.org/1146913004/diff/240001/content/browser/service_worker/service_worker_database.h File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1146913004/diff/240001/content/browser/service_worker/service_worker_database.h#newcode105 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 ...
5 years, 6 months ago (2015-06-06 13:55:12 UTC) #31
falken
This is looking good. It seems missing some test coverage, can you add unittests for ...
5 years, 6 months ago (2015-06-08 04:59:10 UTC) #32
jungkees
https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_database.cc File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_database.cc#newcode457 content/browser/service_worker/service_worker_database.cc:457: for (itr->Seek(prefix); itr->Valid(); itr->Next(), ++index) { No, it isn't. ...
5 years, 6 months ago (2015-06-08 16:00:19 UTC) #33
jungkees
Uploaded a new snapshot. PTAL
5 years, 6 months ago (2015-06-08 16:05:23 UTC) #34
falken
https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_storage_unittest.cc File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_storage_unittest.cc#newcode606 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 ...
5 years, 6 months ago (2015-06-08 16:14:58 UTC) #35
jungkees
https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_storage_unittest.cc File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_storage_unittest.cc#newcode729 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 ...
5 years, 6 months ago (2015-06-08 16:27:17 UTC) #36
falken
lgtm https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_storage_unittest.cc File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/1146913004/diff/260001/content/browser/service_worker/service_worker_storage_unittest.cc#newcode729 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: > ...
5 years, 6 months ago (2015-06-09 07:37:51 UTC) #37
jungkees
Thanks for the review! Indeed I found that bad_message.h has been changed with a new ...
5 years, 6 months ago (2015-06-09 07:52:26 UTC) #38
jungkees
Uploaded a new snapshot rebased onto ToT and having the last comment (adding registrations->clear()) addressed.
5 years, 6 months ago (2015-06-09 14:58:36 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146913004/300001
5 years, 6 months ago (2015-06-10 07:46:37 UTC) #42
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/69746)
5 years, 6 months ago (2015-06-10 07:53:36 UTC) #44
jungkees
nick@, mkwst@, While committing this CL, I encountered a chromium-presubmit error missing LGTMs for 'content/browser/bad_message.h' ...
5 years, 6 months ago (2015-06-10 08:17:48 UTC) #46
Mike West
IPC LGTM % nit. https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_message.h File content/browser/bad_message.h (right): https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_message.h#newcode24 content/browser/bad_message.h:24: // python tools/metrics/histograms/update_bad_message_reasons.py I don't ...
5 years, 6 months ago (2015-06-10 09:34:19 UTC) #47
jungkees
Updated tools/metrics/histograms/histograms.xml. https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_message.h File content/browser/bad_message.h (right): https://codereview.chromium.org/1146913004/diff/300001/content/browser/bad_message.h#newcode24 content/browser/bad_message.h:24: // python tools/metrics/histograms/update_bad_message_reasons.py Updated. Thanks.
5 years, 6 months ago (2015-06-10 14:10:28 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146913004/320001
5 years, 6 months ago (2015-06-10 14:10:59 UTC) #51
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/69794)
5 years, 6 months ago (2015-06-10 14:17:30 UTC) #53
jungkees
kinuko@ Could you take a look at 'content/browser/bad_message.h'? And I'm not sure to whom I ...
5 years, 6 months ago (2015-06-10 15:29:54 UTC) #54
ncarter (slow)
On 2015/06/10 15:29:54, jungkees wrote: > kinuko@ Could you take a look at 'content/browser/bad_message.h'? And ...
5 years, 6 months ago (2015-06-10 18:42:41 UTC) #55
kinuko
lgtm/2 (reg: histograms.xml we often ask asvitkine or isherman to review) https://codereview.chromium.org/1146913004/diff/320001/content/browser/service_worker/service_worker_storage.cc File content/browser/service_worker/service_worker_storage.cc (right): ...
5 years, 6 months ago (2015-06-11 00:03:36 UTC) #56
jungkees
asvitkine@ isherman@ Could you take a look at histograms.xml that has two additional entries by ...
5 years, 6 months ago (2015-06-11 01:03:24 UTC) #58
kinuko
https://codereview.chromium.org/1146913004/diff/320001/content/browser/service_worker/service_worker_storage.cc File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1146913004/diff/320001/content/browser/service_worker/service_worker_storage.cc#newcode1136 content/browser/service_worker/service_worker_storage.cc:1136: it != installing_registrations_.end(); ++it) { On 2015/06/11 01:03:24, jungkees ...
5 years, 6 months ago (2015-06-11 02:12:21 UTC) #59
Ilya Sherman
histograms.xml lgtm
5 years, 6 months ago (2015-06-11 03:25:52 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146913004/320001
5 years, 6 months ago (2015-06-11 03:38:16 UTC) #62
commit-bot: I haz the power
Committed patchset #8 (id:320001)
5 years, 6 months ago (2015-06-11 03:52:19 UTC) #63
commit-bot: I haz the power
5 years, 6 months ago (2015-06-11 03:53:22 UTC) #64
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/df6d1bf21f9b1e30c9d50a7697d6dc869c36097a
Cr-Commit-Position: refs/heads/master@{#333886}

Powered by Google App Engine
This is Rietveld 408576698