Gather the ServiceWorker client information in the browser process.

Gather the ServiceWorker client information in the browser process.

This is also adding some logic about empty and invalid
ServiceWorkerClientInfo.

BUG=450634
Committed: https://crrev.com/5d6924925a291a7cf394a685772251c83a63f99f
Cr-Commit-Position: refs/heads/master@{#313982}

Committed: https://crrev.com/ed180d9ace6284cb2b91843e70e93b57a71b6e80
Cr-Commit-Position: refs/heads/master@{#314636}

[mlamouri (slow - plz ping), 2015-01-23 15:28:11]

mlamouri@chromium.org changed reviewers:
+ michaeln@chromium.org, nasko@chromium.org

[mlamouri (slow - plz ping), 2015-01-23 15:28:12]

nasko@, could you review changes in:
 - content/common/DEPS
 - content/common/service_worker/service_worker_messages.h

michaeln@, could you review changes in:
 - content/*/service_worker/*

Thanks! :)

[mlamouri (slow - plz ping), 2015-01-23 17:20:58]

-nasko@ who is going to be super busy next week.

+tsepez@ to do the security review

[mlamouri (slow - plz ping), 2015-01-23 17:21:17]

mlamouri@chromium.org changed reviewers:
+ tsepez@chromium.org
- nasko@chromium.org

[Tom Sepez, 2015-01-23 17:51:43]

https://codereview.chromium.org/871013003/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_traits.h (right):

https://codereview.chromium.org/871013003/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_traits.h:17: template <>
This is a step in the wrong direction, as we want to eliminate hand-written
traits and the possibility for errors that the introduce. This brings us
directly into conflict between making IPC easy and doing the information hiding
that C++ folks like.  Do you absolutely need info hiding here?  Otherwise making
them public and continuing to use macros avoids IPC issues.

[mlamouri (slow - plz ping), 2015-01-23 18:41:07]

https://codereview.chromium.org/871013003/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_traits.h (right):

https://codereview.chromium.org/871013003/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_traits.h:17: template <>
On 2015/01/23 at 17:51:43, Tom Sepez wrote:
> This is a step in the wrong direction, as we want to eliminate hand-written
traits and the possibility for errors that the introduce. This brings us
directly into conflict between making IPC easy and doing the information hiding
that C++ folks like.  Do you absolutely need info hiding here?  Otherwise making
them public and continuing to use macros avoids IPC issues.

I guess I could have all the properties public for the sake of IPC but still
recommend to not use them directly.

michaeln@, WDYT?

[michaeln, 2015-01-26 22:09:46]

looking pretty good

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:53:
DCHECK(render_frame_host);
Its hard to be sure this is guaranteed to hold true, I suspect it is not.

Any number of messages could have been processed on the UI thread before this
one to get client info comes thru. Some of which probably could delete the frame
host.

Is there a GetClientInfoCallback return value that means N/A?

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:58:
render_frame_host->GetLastCommittedURL(),
There's a chance frame can contain a different document at the time we get here.
The lifetimes of the old and new documents that reside in a frame being
navigated overlap. A couple different ways the mismatch can happen.

At the beginning of a docs life, the client association happens prior to the
document being committed, in that case the committed url can reflect the doc
that is being replaced by a new navigation.

At the end of a docs life, I think an old Document can linger after a new doc
has been committed to the frame. In that case, the url will reflect the new
page.

It might make sense to filter out 'uncommitted' documents from this list?

I think tightening up the Frame vs Document identity'isms could come later.

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:45: virtual
~GetClientDocumentsCallback() {
can you add a DCHECK here for the IO thead?

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:46: if
(version_->running_status() == RUNNING) {
What gaurantees version_ is still valid? Since host->GetClientInfo is an async
method, i'm not so sure this is a safe assumption. Is there a reason to not use
a scoped_refptr<Version> here?

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1078:
DCHECK(!info.IsEmpty());
i think you may need to allow for, and filter out, empty infos to deal with
clients that go poof between asking for the info and getting an answer

https://codereview.chromium.org/871013003/diff/20001/content/common/service_w...
File content/common/service_worker/service_worker_types.h (left):

https://codereview.chromium.org/871013003/diff/20001/content/common/service_w...
content/common/service_worker/service_worker_types.h:206: struct
ServiceWorkerClientInfo {
I think keep this as a struct would be fine.

[mlamouri (slow - plz ping), 2015-01-27 15:03:08]

tsepez@, I no longer have custom traits.
michaeln@, I applied your comments. I'm not sure what you were expecting me to
do for the frame being navigated so I went ahead and added a check but it might
be less than what you were expecting.

PTAL.

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:53:
DCHECK(render_frame_host);
On 2015/01/26 at 22:09:46, michaeln wrote:
> Its hard to be sure this is guaranteed to hold true, I suspect it is not.
> 
> Any number of messages could have been processed on the UI thread before this
one to get client info comes thru. Some of which probably could delete the frame
host.
> 
> Is there a GetClientInfoCallback return value that means N/A?

An empty ServiceWorkerClientInfo should do.

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:58:
render_frame_host->GetLastCommittedURL(),
On 2015/01/26 at 22:09:46, michaeln wrote:
> There's a chance frame can contain a different document at the time we get
here. The lifetimes of the old and new documents that reside in a frame being
navigated overlap. A couple different ways the mismatch can happen.
> 
> At the beginning of a docs life, the client association happens prior to the
document being committed, in that case the committed url can reflect the doc
that is being replaced by a new navigation.
> 
> At the end of a docs life, I think an old Document can linger after a new doc
has been committed to the frame. In that case, the url will reflect the new
page.
> 
> It might make sense to filter out 'uncommitted' documents from this list?
> 
> I think tightening up the Frame vs Document identity'isms could come later.

I'm not sure what you are suggesting I do here. Knowing if the frame is being
navigated would require observing it - do we want that? I could pass an
|expected_url| to the method and check that the committed url matches the
expected url. If it doesn't, it means that a navigation occurred in between. If
it does, it's probably good enough?

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:45: virtual
~GetClientDocumentsCallback() {
On 2015/01/26 at 22:09:46, michaeln wrote:
> can you add a DCHECK here for the IO thead?

Done.

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:46: if
(version_->running_status() == RUNNING) {
On 2015/01/26 at 22:09:46, michaeln wrote:
> What gaurantees version_ is still valid? Since host->GetClientInfo is an async
method, i'm not so sure this is a safe assumption. Is there a reason to not use
a scoped_refptr<Version> here?

Done.

https://codereview.chromium.org/871013003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1078:
DCHECK(!info.IsEmpty());
On 2015/01/26 at 22:09:46, michaeln wrote:
> i think you may need to allow for, and filter out, empty infos to deal with
clients that go poof between asking for the info and getting an answer

Done.

https://codereview.chromium.org/871013003/diff/20001/content/common/service_w...
File content/common/service_worker/service_worker_types.h (left):

https://codereview.chromium.org/871013003/diff/20001/content/common/service_w...
content/common/service_worker/service_worker_types.h:206: struct
ServiceWorkerClientInfo {
On 2015/01/26 at 22:09:46, michaeln wrote:
> I think keep this as a struct would be fine.

Moved back to a struct but still in its own file with some changes so there are
meaningful IsEmpty() and IsValid() methods.

[Tom Sepez, 2015-01-27 17:54:13]

Messages LGTM.

[michaeln, 2015-01-27 23:12:57]

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:55:
render_frame_host->GetLastCommittedURL() != expected_url) {
Not sure this works in history.pushState() use cases, does a pushState'd url get
committed? The value of the expected_url is the url that brought the document
into being.

> I'm not sure what you are suggesting I do here. 

I hadn't suggested anything, was just noticing the raciness. I think for the
time being don't even try to handle it (at least not in this CL). If we knew the
provider_id on the UI thread, that would be great, we could use that value
instead of the docurl value. We'd only get 'infos' back about the currently
committed clients and we'd only get 'infos' back about clients we actually are
interested in.

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:57: // |version_| must
outlive this callback.
comment not really needed anymore

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:857:
weak_factory_.GetWeakPtr(), it.GetCurrentKey(), callback));
Just attended the code prple speed meeting and afterwards overheard pkasting in
the hallway talking about how coalescing flurries of tasks can help. With that
in mind, it may be better to build up an array of <pid, frameid, docurl> tuples
and send one task over to the UI thread to retrieve the full clientinfo structs
for each and reply with one task back over to the IO thread.

That could make the refcounted GetClientDocumentsCallback class whose dtor
sneakily completes the larger call no longer needed.

Also gathering all the infos in one turn of the UI loop helps with providing a
consistent view of the state a particular moment in time.

Wdyt?

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
File content/common/service_worker/service_worker_client_info.cc (right):

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_client_info.cc:16: url(GURL()),
empty GURL() value isn't needed in the init list

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_client_info.cc:25: :
client_id(kInvalidServiceWorkerClientId),
indents are off here

Foo::Foo(
    int x,
    int y);
    : x_(x),
      y_(y) {}

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_client_info.cc:35: url.is_empty()
&&
are all of these required for IsEmpty() test or would url.is_empty() suffice?

[mlamouri (slow - plz ping), 2015-01-28 11:35:05]

PTAL

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:55:
render_frame_host->GetLastCommittedURL() != expected_url) {
On 2015/01/27 at 23:12:56, michaeln wrote:
> Not sure this works in history.pushState() use cases, does a pushState'd url
get committed? The value of the expected_url is the url that brought the
document into being.
> 
> > I'm not sure what you are suggesting I do here. 
> 
> I hadn't suggested anything, was just noticing the raciness. I think for the
time being don't even try to handle it (at least not in this CL). If we knew the
provider_id on the UI thread, that would be great, we could use that value
instead of the docurl value. We'd only get 'infos' back about the currently
committed clients and we'd only get 'infos' back about clients we actually are
interested in.

As far as I know, we commit history.pushState() indeed. I've removed that check
and left a TODO.
I've also added a check in SWVersion because I guess we might end up having a
navigation that got committed and the frame URL would no longer be the one we
are expecting. I think it's no big deal if we return a non controlled but same
origin WindowClient but we shouldn't return a cross origin one.

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:57: // |version_| must
outlive this callback.
On 2015/01/27 at 23:12:56, michaeln wrote:
> comment not really needed anymore

Removed.

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:857:
weak_factory_.GetWeakPtr(), it.GetCurrentKey(), callback));
On 2015/01/27 at 23:12:56, michaeln wrote:
> Just attended the code prple speed meeting and afterwards overheard pkasting
in the hallway talking about how coalescing flurries of tasks can help. With
that in mind, it may be better to build up an array of <pid, frameid, docurl>
tuples and send one task over to the UI thread to retrieve the full clientinfo
structs for each and reply with one task back over to the IO thread.
> 
> That could make the refcounted GetClientDocumentsCallback class whose dtor
sneakily completes the larger call no longer needed.
> 
> Also gathering all the infos in one turn of the UI loop helps with providing a
consistent view of the state a particular moment in time.
> 
> Wdyt?

That sounds good. I would be happy to do that but do you mind if I do that
refactoring in a follow-up? This CL is blocking a couple of other CLs and this
is a bit orthogonal to the original point. WDYT?

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
File content/common/service_worker/service_worker_client_info.cc (right):

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_client_info.cc:16: url(GURL()),
On 2015/01/27 at 23:12:57, michaeln wrote:
> empty GURL() value isn't needed in the init list

Removed. I wanted to have it here to be explicit but don't mind removing it.

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_client_info.cc:25: :
client_id(kInvalidServiceWorkerClientId),
On 2015/01/27 at 23:12:56, michaeln wrote:
> indents are off here
> 
> Foo::Foo(
>     int x,
>     int y);
>     : x_(x),
>       y_(y) {}

I will never get used to that :)

https://codereview.chromium.org/871013003/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_client_info.cc:35: url.is_empty()
&&
On 2015/01/27 at 23:12:56, michaeln wrote:
> are all of these required for IsEmpty() test or would url.is_empty() suffice?

It's hard to have strong requirements there because it would anyway be possible
to set the parameters independently to each other but it would also be possible
to call the ctor with an empty URL. The idea is to check whether something
changed. I don't mind either way but I think the intent is clearer that way.

[michaeln, 2015-01-28 20:26:21]

lgtm

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/871013003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:857:
weak_factory_.GetWeakPtr(), it.GetCurrentKey(), callback));
On 2015/01/28 11:35:05, Mounir Lamouri wrote:
> On 2015/01/27 at 23:12:56, michaeln wrote:
> > Just attended the code prple speed meeting and afterwards overheard pkasting
> in the hallway talking about how coalescing flurries of tasks can help. With
> that in mind, it may be better to build up an array of <pid, frameid, docurl>
> tuples and send one task over to the UI thread to retrieve the full clientinfo
> structs for each and reply with one task back over to the IO thread.
> > 
> > That could make the refcounted GetClientDocumentsCallback class whose dtor
> sneakily completes the larger call no longer needed.
> > 
> > Also gathering all the infos in one turn of the UI loop helps with providing
a
> consistent view of the state a particular moment in time.
> > 
> > Wdyt?
> 
> That sounds good. I would be happy to do that but do you mind if I do that
> refactoring in a follow-up? This CL is blocking a couple of other CLs and this
> is a bit orthogonal to the original point. WDYT?

sgtm, TODO?

[mlamouri (slow - plz ping), 2015-01-29 07:12:16]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2015-01-29 07:12:17]

TODO added.

Jochen, could you have a look at the content/common/DEPS change?

[jochen (gone - plz use gerrit), 2015-01-29 15:18:01]

https://codereview.chromium.org/871013003/diff/100001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/871013003/diff/100001/content/common/DEPS#new...
content/common/DEPS:35:
"+third_party/WebKit/public/platform/WebServiceWorkerClientsInfo.h",
this is not an POD/enum header, so we can't add it here.

https://codereview.chromium.org/871013003/diff/100001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/871013003/diff/100001/content/content_common....
content/content_common.gypi:489:
'common/service_worker/service_worker_client_info.h',
don't you need to also update the BUILD.gn file?

[mlamouri (slow - plz ping), 2015-01-29 17:57:46]

This CL no longer changes DEPS so I no longer need your review jochen. Nice
work! ;)

https://codereview.chromium.org/871013003/diff/100001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/871013003/diff/100001/content/common/DEPS#new...
content/common/DEPS:35:
"+third_party/WebKit/public/platform/WebServiceWorkerClientsInfo.h",
On 2015/01/29 at 15:18:00, jochen (slow) wrote:
> this is not an POD/enum header, so we can't add it here.

Removed.

https://codereview.chromium.org/871013003/diff/100001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/871013003/diff/100001/content/content_common....
content/content_common.gypi:489:
'common/service_worker/service_worker_client_info.h',
On 2015/01/29 at 15:18:00, jochen (slow) wrote:
> don't you need to also update the BUILD.gn file?

I think content/common/BUILD.gn re-uses private_common_sources. It should be
fine.

[mlamouri (slow - plz ping), 2015-01-29 17:57:53]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-29 17:58:13]

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

[commit-bot: I haz the power, 2015-01-29 18:01:46]

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

[commit-bot: I haz the power, 2015-01-29 18:01:47]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2015-01-30 19:43:01]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-30 19:43:29]

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

[commit-bot: I haz the power, 2015-01-30 20:39:49]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-01-30 20:41:57]

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

[horo, 2015-01-31 10:02:21]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/893783002/ by horo@chromium.org.

The reason for reverting is: This patch is causing
http/tests/serviceworker/clients-getall.html to fail on the mac bots:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt....

[mlamouri (slow - plz ping), 2015-02-02 15:46:18]

The following CL should fix how we know whether a frame is focused:
https://codereview.chromium.org/898443003

[mlamouri (slow - plz ping), 2015-02-04 20:14:17]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-02-04 20:14:58]

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

[commit-bot: I haz the power, 2015-02-04 21:20:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-04 21:21:28]

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