Service Worker: Add ServiceWorkerContainer.getRegistrations() method.

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}

[jungkees, 2015-05-21 05:17:15]

jungkee.song@samsung.com changed reviewers:
+ falken@chromium.org, kinuko@chromium.org, nhiroki@chromium.org

[jungkees, 2015-05-21 05:18:49]

PTAL

[kinuko, 2015-05-21 07:57:06]

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

[jungkees, 2015-05-21 13:46:25]

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.

[jungkees, 2015-05-21 13:48:32]

Thanks for the review kinuko@.
New snapshot uploaded as such. PTAL.

[michaeln, 2015-05-21 20:33:30]

michaeln@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2015-05-21 20:33:31]

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.

[kinuko, 2015-05-22 06:19:06]

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

[jungkees, 2015-05-22 13:26:44]

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.

[kinuko, 2015-05-25 07:13:13]

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.

[nhiroki, 2015-05-25 18:44:07]

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).

[jungkees, 2015-05-26 00:52:43]

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.

[jungkees, 2015-05-27 12:35:49]

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).
$_

[michaeln, 2015-05-27 18:28:15]

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.

[jungkees, 2015-05-28 07:57:14]

Patchset #3 (id:40001) has been deleted

[jungkees, 2015-05-28 08:03:57]

Patchset #3 (id:60001) has been deleted

[jungkees, 2015-05-28 08:10:07]

Patchset #3 (id:80001) has been deleted

[jungkees, 2015-05-28 08:15:40]

Patchset #3 (id:100001) has been deleted

[jungkees, 2015-05-28 08:39:15]

Patchset #3 (id:120001) has been deleted

[jungkees, 2015-05-28 08:39:24]

Patchset #3 (id:140001) has been deleted

[jungkees, 2015-05-28 08:40:03]

Patchset #3 (id:160001) has been deleted

[jungkees, 2015-05-28 08:47:25]

Patchset #3 (id:180001) has been deleted

[jungkees, 2015-05-28 09:14:54]

Patchset #3 (id:200001) has been deleted

[jungkees, 2015-05-28 12:48:22]

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.

[falken, 2015-06-05 02:58:22]

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,
&registrations, &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...

[jungkees, 2015-06-05 06:58:07]

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.

[jungkees, 2015-06-05 12:53:57]

Uploaded a new snapshot. PTAL

[michaeln, 2015-06-05 21:50:42]

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

[jungkees, 2015-06-06 13:55:01]

Thanks for the review!
Just uploaded a new snapshot having addressed the comments. PTAL

[jungkees, 2015-06-06 13:55:12]

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.

[falken, 2015-06-08 04:59:10]

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?

[jungkees, 2015-06-08 16:00:19]

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.

[jungkees, 2015-06-08 16:05:23]

Uploaded a new snapshot. PTAL

[falken, 2015-06-08 16:14:58]

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.

[jungkees, 2015-06-08 16:27:17]

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.

[falken, 2015-06-09 07:37:51]

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.

[jungkees, 2015-06-09 07:52:26]

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.

[jungkees, 2015-06-09 14:58:36]

Uploaded a new snapshot rebased onto ToT and having the last comment (adding
registrations->clear()) addressed.

[jungkees, 2015-06-10 07:46:25]

The CQ bit was checked by jungkee.song@samsung.com

[jungkees, 2015-06-10 07:46:26]

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.")

[commit-bot: I haz the power, 2015-06-10 07:46:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146913004/300001

[commit-bot: I haz the power, 2015-06-10 07:53:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 07:53:36]

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...)

[jungkees, 2015-06-10 08:13:18]

jungkee.song@samsung.com changed reviewers:
+ mkwst@chromium.org, nick@chromium.org

[jungkees, 2015-06-10 08:17:48]

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

[Mike West, 2015-06-10 09:34:19]

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.

[jungkees, 2015-06-10 14:10:28]

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.

[jungkees, 2015-06-10 14:10:41]

The CQ bit was checked by jungkee.song@samsung.com

[jungkees, 2015-06-10 14:10:42]

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.")

[commit-bot: I haz the power, 2015-06-10 14:10:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146913004/320001

[commit-bot: I haz the power, 2015-06-10 14:17:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 14:17:30]

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...)

[jungkees, 2015-06-10 15:29:54]

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?

[ncarter (slow), 2015-06-10 18:42:41]

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.

[kinuko, 2015-06-11 00:03:36]

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

[jungkees, 2015-06-11 00:54:14]

jungkee.song@samsung.com changed reviewers:
+ asvitkine@chromium.org, isherman@chromium.org

[jungkees, 2015-06-11 01:03:24]

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.

[kinuko, 2015-06-11 02:12:21]

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.

[Ilya Sherman, 2015-06-11 03:25:52]

histograms.xml lgtm

[jungkees, 2015-06-11 03:38:08]

The CQ bit was checked by jungkee.song@samsung.com

[commit-bot: I haz the power, 2015-06-11 03:38:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146913004/320001

[commit-bot: I haz the power, 2015-06-11 03:52:19]

Committed patchset #8 (id:320001)

[commit-bot: I haz the power, 2015-06-11 03:53:22]

Patchset 8 (id:??) landed as
https://crrev.com/df6d1bf21f9b1e30c9d50a7697d6dc869c36097a
Cr-Commit-Position: refs/heads/master@{#333886}