Implement ServiceWorkerClient attributes [2/3]

Implement ServiceWorkerClient attributes [2/3]

Patch dependency:
[1] Blink: https://crrev.com/783663002/
[2] Chromium: https://crrev.com/771103002/ (THIS PATCH)
[3] Blink: https://crrev.com/778703004/

Spec: http://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-client

BUG=436335, 437152
Committed: https://crrev.com/2ff1341a5b3dc65806b7c6b294240a3f05fe8a81
Cr-Commit-Position: refs/heads/master@{#307852}

[Kunihiko Sakamoto, 2014-12-02 09:04:14]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2014-12-08 02:01:04]

ksakamoto@chromium.org changed reviewers:
+ nhiroki@chromium.org

[nhiroki, 2014-12-08 05:45:09]

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

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:30: void add(int
client_request_id, int client_id) {
The first letter of function names should be capitalized.

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:999: if
(!callback->has_pending_requests())
This condition seems never fulfilled because only callbacks whose
|has_pending_requests()| is greater than 0 should be in
|get_client_info_callbacks|.

Did you mean "if (callback->has_pending_requests()) delete callback;"?

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1003:
callbacks->Clear();
After clearing |callbacks|, GetClientInfo{Success,Error} messages from documents
could reach NOTREACHED at 753 and 775... right?

[nhiroki, 2014-12-08 05:47:05]

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

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:999: if
(!callback->has_pending_requests())
On 2014/12/08 05:45:08, nhiroki wrote:
> This condition seems never fulfilled because only callbacks whose
> |has_pending_requests()| is greater than 0 should be in
> |get_client_info_callbacks|.

CORRECTION: |has_pending_requests()| is **true**
 
> Did you mean "if (callback->has_pending_requests()) delete callback;"?

[Kunihiko Sakamoto, 2014-12-08 06:43:18]

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

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:30: void add(int
client_request_id, int client_id) {
On 2014/12/08 05:45:09, nhiroki wrote:
> The first letter of function names should be capitalized.

Done.

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:999: if
(!callback->has_pending_requests())
On 2014/12/08 05:47:05, nhiroki wrote:
> On 2014/12/08 05:45:08, nhiroki wrote:
> > This condition seems never fulfilled because only callbacks whose
> > |has_pending_requests()| is greater than 0 should be in
> > |get_client_info_callbacks|.
> 
> CORRECTION: |has_pending_requests()| is **true**
>  
> > Did you mean "if (callback->has_pending_requests()) delete callback;"?
> 

callback->Cancel() above decrements pending_requests_.size() by 1 and makes
HasPendingRequests() false.
Since |callbacks| may have multiple references to a GetClientInfoCallback, this
condition makes sure that we delete callback exactly once.

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1003:
callbacks->Clear();
On 2014/12/08 05:45:08, nhiroki wrote:
> After clearing |callbacks|, GetClientInfo{Success,Error} messages from
documents
> could reach NOTREACHED at 753 and 775... right?

I see. Removed NOTREACHEDs.
I wonder if other callbacks that gets cleared in OnStopped() don't have the same
problem.

[nhiroki, 2014-12-08 07:09:33]

LGTM with nits

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

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:999: if
(!callback->has_pending_requests())
On 2014/12/08 06:43:18, Kunihiko Sakamoto wrote:
> On 2014/12/08 05:47:05, nhiroki wrote:
> > On 2014/12/08 05:45:08, nhiroki wrote:
> > > This condition seems never fulfilled because only callbacks whose
> > > |has_pending_requests()| is greater than 0 should be in
> > > |get_client_info_callbacks|.
> > 
> > CORRECTION: |has_pending_requests()| is **true**
> >  
> > > Did you mean "if (callback->has_pending_requests()) delete callback;"?
> > 
> 
> callback->Cancel() above decrements pending_requests_.size() by 1 and makes
> HasPendingRequests() false.
> Since |callbacks| may have multiple references to a GetClientInfoCallback,
this
> condition makes sure that we delete callback exactly once.

Ah sorry, I missed the cancel above. That makes sense.

https://codereview.chromium.org/771103002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:1003:
callbacks->Clear();
On 2014/12/08 06:43:18, Kunihiko Sakamoto wrote:
> On 2014/12/08 05:45:08, nhiroki wrote:
> > After clearing |callbacks|, GetClientInfo{Success,Error} messages from
> documents
> > could reach NOTREACHED at 753 and 775... right?
> 
> I see. Removed NOTREACHEDs.
> I wonder if other callbacks that gets cleared in OnStopped() don't have the
same
> problem.

I think other event completion messages (eg. ActivateEventFinished) are sent
from a worker and they shouldn't be sent after the worker stopped.

https://codereview.chromium.org/771103002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/771103002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:753: return;
How about having a comment about why we can ignore this case like line 930?

https://codereview.chromium.org/771103002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:772: if (!callback)
ditto.

[Kunihiko Sakamoto, 2014-12-08 07:46:59]

Thanks for the review!

https://codereview.chromium.org/771103002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/771103002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:753: return;
On 2014/12/08 07:09:33, nhiroki wrote:
> How about having a comment about why we can ignore this case like line 930?

Done.

https://codereview.chromium.org/771103002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:772: if (!callback)
On 2014/12/08 07:09:33, nhiroki wrote:
> ditto.

Done.

[Kunihiko Sakamoto, 2014-12-08 08:17:35]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-12-08 08:18:11]

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

[commit-bot: I haz the power, 2014-12-08 08:22:12]

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

[commit-bot: I haz the power, 2014-12-08 08:22:13]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kunihiko Sakamoto, 2014-12-08 08:32:29]

ksakamoto@chromium.org changed reviewers:
+ dcheng@chromium.org

[Kunihiko Sakamoto, 2014-12-08 08:32:29]

+dcheng
Daniel, could you take a look for IPC changes (service_worker_messages.h) ?

[mlamouri (slow - plz ping), 2014-12-08 10:56:27]

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

[mlamouri (slow - plz ping), 2014-12-08 10:56:28]

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:706: result.frame_type
= static_cast<RequestContextFrameType>(info.frameType);
Now that ServiceWorkerProviderHost is aware of the render_frame_id, how much of
those attributes you could get directly from the browser process? It could be
interesting if we could save a browser->renderer->browser ipc loop.

[Kunihiko Sakamoto, 2014-12-09 02:05:44]

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:706: result.frame_type
= static_cast<RequestContextFrameType>(info.frameType);
On 2014/12/08 10:56:28, Mounir Lamouri wrote:
> Now that ServiceWorkerProviderHost is aware of the render_frame_id, how much
of
> those attributes you could get directly from the browser process? It could be
> interesting if we could save a browser->renderer->browser ipc loop.

We have URL in browser process, and it'd be easy to save frameType in browser
since it doesn't change.

For visibilityState and focused, I don't think browser process has complete
information to calculate them.

So browser->renderer->browser IPC loop is inevitable here.

[Kunihiko Sakamoto, 2014-12-10 01:41:51]

Daniel, please take a look. Thanks!

[dcheng, 2014-12-10 06:37:52]

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:33: void Success(int
client_request_id, const ServiceWorkerClientInfo& info) {
Nit: I think Success, Error, Cancel would be clearer as OnSuccess, OnError,
OnCancel.

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:46: int request_id() {
return request_id_; }
Nit: const.

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:56: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:734:
GetClientInfoCallback* callback = new GetClientInfoCallback(request_id);
This is really tricky. This is basically manual refcounting. Is there no
practical way to have a 1:1 callback <=> controllee mapping?

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.h:335:
IDMap<GetClientInfoCallback> get_client_info_callbacks_;
Is it guaranteed that OnStopped() gets called before ~ServiceWorkerVersion?

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:706: result.frame_type
= static_cast<RequestContextFrameType>(info.frameType);
On 2014/12/09 at 02:05:43, Kunihiko Sakamoto wrote:
> On 2014/12/08 10:56:28, Mounir Lamouri wrote:
> > Now that ServiceWorkerProviderHost is aware of the render_frame_id, how much
of
> > those attributes you could get directly from the browser process? It could
be
> > interesting if we could save a browser->renderer->browser ipc loop.
> 
> We have URL in browser process, and it'd be easy to save frameType in browser
since it doesn't change.
> 
> For visibilityState and focused, I don't think browser process has complete
information to calculate them.
> 
> So browser->renderer->browser IPC loop is inevitable here.

re focused:
We definitely don't have enough browser-side state for focus atm, but the
out-of-process work requires that we move some parts of focus tracking to the
browser process. It's conceivable we could add enough bits to satisfy the use
case for SW as well.

re visibilityState:
I'm pretty sure this is browser-driven, so theoretically, we could track this in
the browser pretty easy (after all, the embedder has to tell Blink what the
visiblity state).

[Kunihiko Sakamoto, 2014-12-10 06:38:12]

ksakamoto@chromium.org changed reviewers:
+ mkwst@chromium.org

[Kunihiko Sakamoto, 2014-12-10 06:38:12]

+mkwst
Mike, would you mind reviewing this for the IPC part?

[Kunihiko Sakamoto, 2014-12-10 06:41:56]

ksakamoto@chromium.org changed reviewers:
- mkwst@chromium.org

[Kunihiko Sakamoto, 2014-12-10 06:41:56]

-mkwst
Oops, sorry for crossing in the post.
Thanks dcheng@ for the review :)

[Kunihiko Sakamoto, 2014-12-10 08:38:57]

Patchset #7 (id:140001) has been deleted

[Kunihiko Sakamoto, 2014-12-10 09:10:48]

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:33: void Success(int
client_request_id, const ServiceWorkerClientInfo& info) {
On 2014/12/10 06:37:52, dcheng wrote:
> Nit: I think Success, Error, Cancel would be clearer as OnSuccess, OnError,
> OnCancel.

Done.

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:56: };
On 2014/12/10 06:37:52, dcheng wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:734:
GetClientInfoCallback* callback = new GetClientInfoCallback(request_id);
On 2014/12/10 06:37:52, dcheng wrote:
> This is really tricky. This is basically manual refcounting. Is there no
> practical way to have a 1:1 callback <=> controllee mapping?

I splitted GetClientInfoCallback in 2 classes (per-controllee part and
refcounted aggregator).
Does this look better?

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/771103002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.h:335:
IDMap<GetClientInfoCallback> get_client_info_callbacks_;
On 2014/12/10 06:37:52, dcheng wrote:
> Is it guaranteed that OnStopped() gets called before ~ServiceWorkerVersion?

Now ~ServiceWorkerVersion frees up all the callbacks.

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/771103002/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:706: result.frame_type
= static_cast<RequestContextFrameType>(info.frameType);
On 2014/12/10 06:37:52, dcheng wrote:
> On 2014/12/09 at 02:05:43, Kunihiko Sakamoto wrote:
> > On 2014/12/08 10:56:28, Mounir Lamouri wrote:
> > > Now that ServiceWorkerProviderHost is aware of the render_frame_id, how
much
> of
> > > those attributes you could get directly from the browser process? It could
> be
> > > interesting if we could save a browser->renderer->browser ipc loop.
> > 
> > We have URL in browser process, and it'd be easy to save frameType in
browser
> since it doesn't change.
> > 
> > For visibilityState and focused, I don't think browser process has complete
> information to calculate them.
> > 
> > So browser->renderer->browser IPC loop is inevitable here.
> 
> re focused:
> We definitely don't have enough browser-side state for focus atm, but the
> out-of-process work requires that we move some parts of focus tracking to the
> browser process. It's conceivable we could add enough bits to satisfy the use
> case for SW as well.
> 
> re visibilityState:
> I'm pretty sure this is browser-driven, so theoretically, we could track this
in
> the browser pretty easy (after all, the embedder has to tell Blink what the
> visiblity state).

Thanks for the info. Added a TODO comment so that we can revisit it later.

[dcheng, 2014-12-10 20:17:12]

IPC changes LGTM with nits addressed.

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:31: :
request_id_(request_id)
I think this should be indented another two spaces.

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:32: ,
pending_requests_(pending_requests) {}
Chrome convention is to put the "," on the previous line.

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:61:
scoped_refptr<GetClientDocumentsCallback>& callback)
Blink doesn't allow mutable refs so this should be const scoped_refptr<...>&
callback

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:62: :
client_id_(client_id)
Ditto on indenting.

[Kunihiko Sakamoto, 2014-12-11 01:20:31]

Thanks!

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:31: :
request_id_(request_id)
On 2014/12/10 20:17:12, dcheng wrote:
> I think this should be indented another two spaces.

Done.

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:32: ,
pending_requests_(pending_requests) {}
On 2014/12/10 20:17:12, dcheng wrote:
> Chrome convention is to put the "," on the previous line.

Done.

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:61:
scoped_refptr<GetClientDocumentsCallback>& callback)
On 2014/12/10 20:17:12, dcheng wrote:
> Blink doesn't allow mutable refs so this should be const scoped_refptr<...>&
> callback

Done.

https://codereview.chromium.org/771103002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:62: :
client_id_(client_id)
On 2014/12/10 20:17:12, dcheng wrote:
> Ditto on indenting.

Done.

[Kunihiko Sakamoto, 2014-12-11 01:20:47]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-12-11 01:23:11]

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

[Kunihiko Sakamoto, 2014-12-11 01:54:28]

The CQ bit was unchecked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2014-12-11 02:08:26]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-12-11 02:10:42]

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

[commit-bot: I haz the power, 2014-12-11 03:38:19]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2014-12-11 03:39:06]

Patchset 10 (id:??) landed as
https://crrev.com/2ff1341a5b3dc65806b7c6b294240a3f05fe8a81
Cr-Commit-Position: refs/heads/master@{#307852}