Service Worker: Add Clients.get(id)

Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-method

Intent to implement and ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility: https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552310
Committed: https://crrev.com/0ca977be9dbe2582e8f18415bd9299efd36fd329
Cr-Commit-Position: refs/heads/master@{#376870}

[jungkees, 2016-01-04 07:30:08]

Patchset #4 (id:60001) has been deleted

[jungkees, 2016-01-04 07:39:16]

Description was changed from

==========
(WIP) Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

It's WIP. The work is not mature enough to be reviewed yet.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

BUG=552310
==========

to

==========
Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

BUG=552310
==========

[jungkees, 2016-01-04 07:39:16]

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

[jungkees, 2016-01-04 07:41:16]

I uploaded a snapshot ready for review. PTAL.

[zino, 2016-01-04 09:05:27]

This is a small feature but webexposed.

So, you need to send a intent to blink-dev first.

Thank you.

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

https://codereview.chromium.org/1439333002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:55: 
This was already defined in provider host[1] as well as I didn't find where this
is used.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

https://codereview.chromium.org/1439333002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1189: 
IMHO, Many functions are defined duplicately.
What's differences between OnGetClient and GetClient and OnGetClientFinished?
Even if you implement all of logic to get client in OnGetClient, it seems
enough. (for readibility)

https://codereview.chromium.org/1439333002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1809: 
I think you can reuse the provider_host->GetWindowClientInfo() method.

https://codereview.chromium.org/1439333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/1439333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/Clients.idl:11:
[CallWith=ScriptState] Promise<any> get(DOMString id);
This is webexposed.
So, you need to send a intent to implement and ship[1] to blink-dev and receive
lgtm x 3 from API owners.
If you are already done, please leave the link to CL description.

Also, I think it's better to add a runtime flag for this feature.

http://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process

https://codereview.chromium.org/1439333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp
(right):

https://codereview.chromium.org/1439333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp:84: //
TODO(jungkees): Use ServiceWorkerClient for typename S of
CallBackPromiseAdpater.
I'm not sure but you are already done?

[zino, 2016-01-04 09:08:11]

Sorry for confusing.

This CL isn't lgtm yet. My previous comments included l-g-t-m keyword.

[zino, 2016-01-04 09:09:26]

Sorry for confusing repeatedly.

This CL isn't l-g-t-m yet. My previous comments included l-g-t-m keyword. :-P

[falken, 2016-01-05 05:16:52]

Thanks for working on this. Some high-level comments:
- I recommend an "intent to implement and ship" for clientId and Clients.get().
That way you don't need feature flags.
- Firefox already implemented clientId and Clients.get(). Instead of making new
tests, let's re-use their tests to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

[zino, 2016-01-05 05:38:30]

Jungkee,

I think some logic has to move to new service_worker_client_utils.cc.
Please see: https://codereview.chromium.org/1535983002

[jungkees, 2016-01-05 06:59:26]

Thanks for the reviews, zino@, falken@. I'll address them.

> - I recommend an "intent to implement and ship" for clientId and
Clients.get().

Would one "intent to implement and ship" for both be fine? Or better with one
for each?

[falken, 2016-01-05 07:01:32]

On 2016/01/05 06:59:26, jungkees wrote:
> Thanks for the reviews, zino@, falken@. I'll address them.
> 
> > - I recommend an "intent to implement and ship" for clientId and
> Clients.get().
> 
> Would one "intent to implement and ship" for both be fine? Or better with one
> for each?

Just one is fine, this is a small API surface. However please resolve the spec
concern raised for clientId, before continuing with the intent to ship.

[jungkees, 2016-01-05 07:03:03]

Sure. Thanks!

[falken, 2016-01-26 01:51:28]

Is this ready for reviwe?

[jungkees, 2016-01-26 05:31:33]

The implementation is mostly done, but I've seen an error case with the imported
layout test as-is. I'll solve it after the f2f.

[falken, 2016-02-03 01:07:16]

On 2016/01/26 05:31:33, jungkees wrote:
> The implementation is mostly done, but I've seen an error case with the
imported
> layout test as-is. I'll solve it after the f2f.

Ping :)

[jungkees, 2016-02-03 05:07:48]

I was just working on it :) With the patch set 6, the crashing issue has been
resolved. I noticed I was trying to get the WindowClient info from the IO thread
instead of from the UI thread, and fixed it.

I'm still having a time out error with "clients-get-cross-origin.html" imported
from Mozilla. I'm working on it.

[jungkees, 2016-02-03 06:57:55]

Description was changed from

==========
Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

BUG=552310
==========

to

==========
Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552310
==========

[jungkees, 2016-02-03 06:57:55]

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

[jungkees, 2016-02-03 06:58:45]

I uploaded a new snapshot having resolved the layout test issues. PTAL

[nhiroki, 2016-02-03 09:40:00]

(I reviewed this CL on behalf of falken@ because he will be ooo for a while)

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:233:
client_info.client_uuid = host->client_uuid();
It looks like |client_info.client_uuid| is already set at the ctor of
SWClientInfo. Can you remove this line?

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:262:
info.client_uuid = base::get<2>(it);
ditto: |client_uuid| would be already set.

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:398:
provider_host->GetWindowClientInfo(callback);
"base::Bind(&DidGetClient(callback))" would be consistent with other callsites.

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:408:
client_info.last_focus_time = base::TimeTicks();
You may want to call the ctor instead of assigning these fields one-by-one.

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.h (right):

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.h:54: const
std::string& id,
I'd prefer "client_uuid" to "id" because this class handles some ids
(process_id, frame_id) and other classes (eg. ServiceWorkerClientInfo) name this
"client_uuid".

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_client_info.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
content/common/service_worker/service_worker_client_info.cc:18:
last_focus_time(base::TimeTicks()) {}
Generally, having multiple ctors is not so good because we can easily forget to
update them all when we add a new parameter in the class/struct and that results
in uninitialized parameter bugs. To avoid that, how about using "Delegated
Constructor"[1] like this?

ServiceWorkerClientInfo::ServiceWorkerClientInfo()
    : ServiceWorkerClientInfo(std::string(), ...,
WebServiceWorkerClientTypeLast) {}

[1] http://chromium-cpp.appspot.com/#core-whitelist

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
content/common/service_worker/service_worker_messages.h:261: std::string)
/* client_uuid */

https://codereview.chromium.org/1439333002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:848:
ToWebServiceWorkerClientInfo(client)));
Can you declare and initialize |info| at once?

  scoped_ptr<blink::WebServiceWorkerClientInfo> info(new
WebSWClientInfo(ToWebServiceWorkerClientInfo(client)));

Or, directly pass it to the callback?

  callbacks->onSuccess(adoptWebPtr(new
WebSWClientInfo(ToWebServiceWorkerClientInfo(client)))

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-frame.html
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-frame.html:4:
fetch("clientId")
Indents are not necessary.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:36:
window.addEventListener("message", on_message, false);
"..." -> '...'

How about inlining 'on_message' handler here?

window.addEventListener("message", function(e) {
  ...
});

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:40:
console.error('invalid origin: ' + e.origin);
Can you use assert_equals() instead of console.error?

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/Clients.idl:11:
[CallWith=ScriptState] Promise<any> get(DOMString id);
Can you disable this using the runtime flag until ready to ship?
http://www.chromium.org/blink/runtime-enabled-features

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:26:
client = ServiceWorkerClient::create(*webClient);
Can you verify the passed |webClient| type using switch statement?



if (!webClient)
    return nullptr;

switch (webClient->clientType) {
case WebServiceWorkerClientTypeWindow:
    return ServiceWorkerWindowClient::create(*webClient);
case WebServiceWorkerClientTypeWorker:
case WebServiceWorkerClientTypeSharedWorker:
    return ServiceWorkerClient::create(*webClient);
case WebServiceWorkerClientTypeAll:
case WebServiceWorkerClientTypeLast:
    ASSERT_NOT_REACHED();
    return nullptr;
}

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp:74: if
(client->uuid == "") {
|client->uuid.isEmpty()| could be a bit more readable.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp:108:
// FIXME: May be null due to worker termination: http://crbug.com/413518.
"FIXME" comment is deprecated:
http://www.chromium.org/blink/coding-style#TOC-Comments

Can you use "TODO(name):" style instead?

[jungkees, 2016-02-03 14:15:08]

Thanks a lot for your review, nhiroki@! I uploaded a new snapshot having
addressed those comments. PTAL

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:233:
client_info.client_uuid = host->client_uuid();
Ah.. yes. I forgot to remove this after changing the ctor to take |client_uuid|.
Done.

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:262:
info.client_uuid = base::get<2>(it);
On 2016/02/03 09:39:59, nhiroki (ooo until Feb. 4) wrote:
> ditto: |client_uuid| would be already set.

Done.

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:398:
provider_host->GetWindowClientInfo(callback);
Right. Addressed.

Out of my own curiosity, which purpose would be more relevant with the
consistency concern here: explicitly calling DidGetClient or explicitly using
base::Bind for an async API (even though the type of |callback| is already
base::Callback<>)?

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:408:
client_info.last_focus_time = base::TimeTicks();
On 2016/02/03 09:39:59, nhiroki (ooo until Feb. 4) wrote:
> You may want to call the ctor instead of assigning these fields one-by-one.

Done.

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.h (right):

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.h:54: const
std::string& id,
Agreed. Addressed.

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_client_info.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
content/common/service_worker/service_worker_client_info.cc:18:
last_focus_time(base::TimeTicks()) {}
Thanks for the information and the pointer! Done.

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/1439333002/diff/140001/content/common/service...
content/common/service_worker/service_worker_messages.h:261: std::string)
Addressed. Thanks!

https://codereview.chromium.org/1439333002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:848:
ToWebServiceWorkerClientInfo(client)));
On 2016/02/03 09:39:59, nhiroki (ooo until Feb. 4) wrote:
> Can you declare and initialize |info| at once?
> 
>   scoped_ptr<blink::WebServiceWorkerClientInfo> info(new
> WebSWClientInfo(ToWebServiceWorkerClientInfo(client)));
> 
> Or, directly pass it to the callback?
> 
>   callbacks->onSuccess(adoptWebPtr(new
> WebSWClientInfo(ToWebServiceWorkerClientInfo(client)))

Done.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-frame.html
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-frame.html:4:
fetch("clientId")
On 2016/02/03 09:39:59, nhiroki (ooo until Feb. 4) wrote:
> Indents are not necessary.

Done.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:36:
window.addEventListener("message", on_message, false);
On 2016/02/03 09:40:00, nhiroki (ooo until Feb. 4) wrote:
> "..." -> '...'
> 
> How about inlining 'on_message' handler here?
> 
> window.addEventListener("message", function(e) {
>   ...
> });
> 

Done.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:40:
console.error('invalid origin: ' + e.origin);
On 2016/02/03 09:40:00, nhiroki (ooo until Feb. 4) wrote:
> Can you use assert_equals() instead of console.error?

Done.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/Clients.idl:11:
[CallWith=ScriptState] Promise<any> get(DOMString id);
I added ServiceWorkerClientsGetID flag. As the intent to ship is done, I put the
status to stable. If I need to change it to experimental, please let me know.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:26:
client = ServiceWorkerClient::create(*webClient);
Addressed. Thanks!

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp
(right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp:74: if
(client->uuid == "") {
Yes. Addressed.

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp:108:
// FIXME: May be null due to worker termination: http://crbug.com/413518.
On 2016/02/03 09:40:00, nhiroki (ooo until Feb. 4) wrote:
> "FIXME" comment is deprecated:
> http://www.chromium.org/blink/coding-style#TOC-Comments
> 
> Can you use "TODO(name):" style instead?

Done.

[Mike West, 2016-02-04 18:17:44]

I'll defer the serviceworker pieces of the review to nhiroki@, et al. The IPC
changes look reasonable, and once you're confident that it does what you need
and you won't be changing that part, I'm happy to stamp it.

[zino, 2016-02-05 06:21:44]

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:395:
provider_host->GetWindowClientInfo(base::Bind(&DidGetClient, callback));
If you use GetClientInfoCallback, you can pass callback directly without
DidGetClient().

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.h (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.h:30: using
ClientCallback = base::Callback<void(const ServiceWorkerClientInfo&)>;
nit: There was the same callback type in service_worker_provider_host.h. You
could use it.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1038: void
ServiceWorkerVersion::OnGetClientFinished(
If you modify this callback to take client_uuid, you can set uuid to client info
here.
(e.g. ServiceWorkerVersion::OnOpenWindowFinished() : 
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...)

Then, you can avoid these changes:
  https://codereview.chromium.org/1439333002/patch/160001/170003

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:21:
return nullptr;
nit: indentation

[nhiroki, 2016-02-08 06:27:03]

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/Clients.idl:11:
[CallWith=ScriptState] Promise<any> get(DOMString id);
On 2016/02/03 14:15:07, jungkees wrote:
> I added ServiceWorkerClientsGetID flag. As the intent to ship is done, I put
the
> status to stable. If I need to change it to experimental, please let me know.

Thank you for adding it. 'stable' should be ok, but I'd prefer 'experimental'
until we have confidence that the feature is stable enough and ready to ship.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.h (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.h:51: // Gets a
client matched by |id|. |callback| is called with the client
nit: |id| -> |client_uuid|

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1029: void
ServiceWorkerVersion::OnGetClient(int request_id, const std::string& id) {
id -> client_uuid

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1031: request_id, "id",
id);
ditto.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1043: request_id, "The
retrieved client", &client);
|&client| might not be very useful for investigating trace information. 

ServiceWorkerVersion::OnGetClient() goes through different code paths based on
the client type, so adding the client type could be useful to identify a slow
path.

  TRACE_EVENT_ASYNC_END1("ServiceWorker", "ServiceWorkerVersion::OnGetClient",
request_id, "client_type", client.client_type);

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:549: void
OnGetClient(int request_id, const std::string& id);
nit: can you add a blank line after this?

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:25:
myOriginClientId = frame1.contentDocument.body.textContent;
I suspect that |myOriginClientId| is null (or empty string?) because frame's
content is rendered by a navigation request that does not have a client id yet.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:8: var
host_info = get_host_info();
never used?

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:17:
.then(function(registration) {
Can you register "registration.unregister()" with completion_callbacks?

add_completion_callback(function() { registration.unregister(); });

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:23:
.then(function(frame1) {
ditto: add_completion_callback(function() { frame1.remove(); });

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:32:
frame = frame2;
ditto: add_completion_callback(function() { frame2.remove(); });

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:36:
clientIds.push(clientId);
Can you add 'invalid-id' into |clientIds| here? Listing all test cases in the
html side would be more readable.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:49:
window.removeEventListener("message", get_client_id);
Can you use '' instead of ""?

"Use single quotes for string literals."
http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:52:
window.addEventListener("message", get_client_id, false);
ditto.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:38:
window.addEventListener('message', function on_message(e) {
"function on_message(e)" -> "function(e)"

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:3:
if (e.clientId === null) {
Can you add a comment like this?

// On navigation, the client id should be null.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:15:
if (e.data.message == 'get_client_ids') {
The first if-block is almost equal to the second else-if-block. Can you merge
them? For example, posting an array that contains only one client id from
clients-get-other-origin.html for the 'get_other_client_id' case.

[jungkees, 2016-02-12 15:03:22]

Thanks for the reivew! While having worked on the layout test fix, I noticed I
missed filtering out the off-origin-client match. I uploaded a new snapshot
having addressed them and the other comments. PTAL

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/1439333002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/Clients.idl:11:
[CallWith=ScriptState] Promise<any> get(DOMString id);
Agreed. Addressed.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:395:
provider_host->GetWindowClientInfo(base::Bind(&DidGetClient, callback));
On 2016/02/05 06:21:44, zino wrote:
> If you use GetClientInfoCallback, you can pass callback directly without
> DidGetClient().

Done.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.h (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.h:30: using
ClientCallback = base::Callback<void(const ServiceWorkerClientInfo&)>;
Removed ClientCallback in favor of
ServiceWorkerProviderHost::GetClientInfoCallback as suggested.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.h:51: // Gets a
client matched by |id|. |callback| is called with the client
On 2016/02/08 06:27:02, nhiroki (slow) wrote:
> nit: |id| -> |client_uuid|

Done.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1029: void
ServiceWorkerVersion::OnGetClient(int request_id, const std::string& id) {
On 2016/02/08 06:27:02, nhiroki (slow) wrote:
> id -> client_uuid

Done.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1031: request_id, "id",
id);
On 2016/02/08 06:27:02, nhiroki (slow) wrote:
> ditto.

Done.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1038: void
ServiceWorkerVersion::OnGetClientFinished(
If I change GetClientInfoCallback to take |client_uuid|,
ServiceWorkerProviderHost::Focus and
ServiceWorkerProviderHost::GetWindowClientInfo should be changed as they use
PostTaskAndReplyWithResult with a GetClientInfoCallback-type callback which
expects a single client info argument. I just retained my previous change that
initialize |client_uuid| of the ServiceWorkerClientInfo object in its
constructor when a value is given.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1043: request_id, "The
retrieved client", &client);
Addressed. Thanks.

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1439333002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:549: void
OnGetClient(int request_id, const std::string& id);
On 2016/02/08 06:27:02, nhiroki (slow) wrote:
> nit: can you add a blank line after this?

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:25:
myOriginClientId = frame1.contentDocument.body.textContent;
Sorry I missed this. I changed it to load 'resources/clients-get-frame.html'
instead of 'resources/blank.html' so we can intercept a fetch (from the fetch()
request in clients-get-frame.html) with a non-null clientId.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:8: var
host_info = get_host_info();
Removed it.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:17:
.then(function(registration) {
On 2016/02/08 06:27:02, nhiroki (slow) wrote:
> Can you register "registration.unregister()" with completion_callbacks?
> 
> add_completion_callback(function() { registration.unregister(); });
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:23:
.then(function(frame1) {
On 2016/02/08 06:27:02, nhiroki (slow) wrote:
> ditto: add_completion_callback(function() { frame1.remove(); });

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:32:
frame = frame2;
On 2016/02/08 06:27:03, nhiroki (slow) wrote:
> ditto: add_completion_callback(function() { frame2.remove(); });

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:36:
clientIds.push(clientId);
On 2016/02/08 06:27:03, nhiroki (slow) wrote:
> Can you add 'invalid-id' into |clientIds| here? Listing all test cases in the
> html side would be more readable.

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:49:
window.removeEventListener("message", get_client_id);
On 2016/02/08 06:27:03, nhiroki (slow) wrote:
> Can you use '' instead of ""?
> 
> "Use single quotes for string literals."
> http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:52:
window.addEventListener("message", get_client_id, false);
On 2016/02/08 06:27:02, nhiroki (slow) wrote:
> ditto.

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:38:
window.addEventListener('message', function on_message(e) {
On 2016/02/08 06:27:03, nhiroki (slow) wrote:
> "function on_message(e)" -> "function(e)"

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:3:
if (e.clientId === null) {
On 2016/02/08 06:27:03, nhiroki (slow) wrote:
> Can you add a comment like this?
> 
> // On navigation, the client id should be null.

Done.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:15:
if (e.data.message == 'get_client_ids') {
I merged them but posted undefined instead of [undefined] as [undefined] doesn't
seem to be structured cloned across the message passing.

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
(right):

https://codereview.chromium.org/1439333002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:21:
return nullptr;
On 2016/02/05 06:21:44, zino wrote:
> nit: indentation

Done.

[nhiroki, 2016-02-16 06:09:31]

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:398:
provider_host->GetWindowClientInfo(callback);
On 2016/02/03 14:15:07, jungkees wrote:
> Right. Addressed.
> 
> Out of my own curiosity, which purpose would be more relevant with the
> consistency concern here: explicitly calling DidGetClient or explicitly using
> base::Bind for an async API (even though the type of |callback| is already
> base::Callback<>)?

Sorry, I missed your reply and miswrote the code snippet.
"base::Bind(&DidGetClient, callback)" is correct.

I meant the former, that is, "explicitly calling DidGetClient()". Going through
the same code path on completion would be good in terms of extensibility. Let's
say we want to release something at the end of GetClient(), we may assume
DidGetClient() is always called on completion and attempt to release the
resource there but unfortunately it can never be called in some cases... this
may cause resource leaks.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:235: void
OnGetWindowClientOnUI(
nit: please insert a blank line before this line.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:236: const
base::Tuple<int, int, std::string>& client_info,
Is this necessary to be a Tuple? Can we separate this?

void OnGetWindowClientOnUI(
    int process_id,
    int frame_id,
    const std::string& client_uuid,
    const GURL& script_url,
    ...)

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:238: const
ServiceWorkerProviderHost::GetClientInfoCallback& callback) {
Can you add DCHECK_CURRENTLY_ON(BrowserThread::UI); here?

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:407:
DidGetClient(callback, ServiceWorkerClientInfo());
GetClient() sometimes calls the callback synchronously, and at other times calls
it asynchronously. Generally, we should avoid such a mixed use and make them
async calls like following code (we've already done the same thing in
GetClients() though... I'll fix it later).

BrowserThread::PostTask(
    BrowserThread::PostTask, FROM_HERE,
    base::Bind(&DidGetClient, callback, ServiceWorkerClientInfo()));

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:418:
controller->script_url(), base::Bind(callback)));
If you don't call DidGetClient in this async case, DidGetClient would be
pointless because it just check the running thread and obviously sync cases run
on the valid thread. How about completely removing DidGetClient?

// At line 414
BrowserThread::PostTask(
    BrowserThread::UI, FROM_HERE,
    base::Bind(&OnGetWindowClientOnUI,
               base::MakeTuple(...),
               controller->script_url(),
               callback);

// At line 407, 424, 433
BrowserThread::PostTask(
    BrowserThread::IO, FROM_HERE,
    base::Bind(callback, ServiceWorkerClientInfo()));

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:424:
DidGetClient(callback, ServiceWorkerClientInfo());
ditto: sync call vs. async call

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:433:
DidGetClient(callback, client_info);
ditto: sync call vs. async call

https://codereview.chromium.org/1439333002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/1439333002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:19:
Promise.all(
e.waitUntil(Promise.all(...)); ?

(ExtendableMessageEvent was recently enabled in LayoutTests by [1])

[1] https://codereview.chromium.org/1658073002/

[jungkees, 2016-02-16 16:28:58]

I uploaded a new snapshot having addressed the comments. PTAL

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:398:
provider_host->GetWindowClientInfo(callback);
> I meant the former, that is, "explicitly calling DidGetClient()". Going
through
> the same code path on completion would be good in terms of extensibility.
Let's
> say we want to release something at the end of GetClient(), we may assume
> DidGetClient() is always called on completion and attempt to release the
> resource there but unfortunately it can never be called in some cases... this
> may cause resource leaks.

Right. Good point. Addressed.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:235: void
OnGetWindowClientOnUI(
On 2016/02/16 06:09:31, nhiroki (slow) wrote:
> nit: please insert a blank line before this line.

Done.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:236: const
base::Tuple<int, int, std::string>& client_info,
I was just trying to align it to |clients_info| passed to
OnGetWindowClientsOnUI, but agree separating it to those params explicitly makes
it clearer to understand. Addressed.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:238: const
ServiceWorkerProviderHost::GetClientInfoCallback& callback) {
On 2016/02/16 06:09:31, nhiroki (slow) wrote:
> Can you add DCHECK_CURRENTLY_ON(BrowserThread::UI); here?

Done.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:407:
DidGetClient(callback, ServiceWorkerClientInfo());
Thanks for the good point. Addressed.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:418:
controller->script_url(), base::Bind(callback)));
> it just check the running thread and obviously sync cases run on the valid
thread.

Agreed to this point. Removed DidGetClient and replaced the code as suggested.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:424:
DidGetClient(callback, ServiceWorkerClientInfo());
On 2016/02/16 06:09:31, nhiroki (slow) wrote:
> ditto: sync call vs. async call

Done.

https://codereview.chromium.org/1439333002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:433:
DidGetClient(callback, client_info);
On 2016/02/16 06:09:31, nhiroki (slow) wrote:
> ditto: sync call vs. async call

Done.

https://codereview.chromium.org/1439333002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/1439333002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:19:
Promise.all(
Ahh.. Great! Addressed.

[nhiroki, 2016-02-17 02:40:28]

Looks good :)

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:418:
controller->script_url().GetOrigin()) {
This origin check should be done before line 408.

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:423: 
Can you add DCHECK here?

DCHECK(provider_host->client_type() == blink::WebServiceWorkerClientTypeWorker
|| provider_host->client_type() ==
blink::WebServiceWorkerClientTypeSharedWorker);

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:965: // When
Clients.matchAll() is called on the script evaluation phase, the
s/matchAll()/get()/

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:13:
'resources/clients-get-other-origin.html';
For improving consistency, can you rename this file to
"clients-get-other-origin-frame.html" or "clients-get-cross-origin-frame.html"?

If you choose the former, please also rename
"serviceworker/clients-get-cross-origin.html" to
"serviceworker/clients-get-other-origin.html".

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:14:
var myOriginClientId;
my_origin_client_id

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:35:
.then(function(clientId) {
client_id

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:10: var
clientIds = [];
"Write variable identifiers and function names in lowercase separated with
underscores."
http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:27:
.then(function(clientId) {
client_id

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:36:
.then(function(clientId) {
ditto.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:58:
['visible', true, new URL(scope + '#1', location).toString(), 'nested'],
"normalizeURL(scope) + '#1'" might work?

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:68:
service_worker_unregister_and_done(t, scope);
You don't have to explicitly run 'unregister' here because you already added it
as a completion callback at line 16.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:22:
registration = reg;
|registration| is not used. Did you mean to use it for unregistration at line
50?

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:40:
var otherOriginClientIds = [];
other_origin_client_ids

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:16:
var clientIds = e.data.clientIds;
client_ids

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:20:
clientIds.map(function(clientId) {
client_id

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:31:
method get
I guess you don't have to change this because the feature is still behind a flag
(not exposed to the stable build).

[jungkees, 2016-02-17 14:01:13]

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

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:418:
controller->script_url().GetOrigin()) {
I moved this origin check to before line 408 and removed the origin check in
OnGetWindowClientOnUI which would be redundant, otherwise.

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:423: 
On 2016/02/17 02:40:28, nhiroki wrote:
> Can you add DCHECK here?
> 
> DCHECK(provider_host->client_type() == blink::WebServiceWorkerClientTypeWorker
> || provider_host->client_type() ==
> blink::WebServiceWorkerClientTypeSharedWorker);

Done.

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1439333002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:965: // When
Clients.matchAll() is called on the script evaluation phase, the
On 2016/02/17 02:40:28, nhiroki wrote:
> s/matchAll()/get()/

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:13:
'resources/clients-get-other-origin.html';
Yes, that's better. I renamed them so with *cross-origin*.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:14:
var myOriginClientId;
On 2016/02/17 02:40:28, nhiroki wrote:
> my_origin_client_id

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get-cross-origin.html:35:
.then(function(clientId) {
On 2016/02/17 02:40:28, nhiroki wrote:
> client_id

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:10: var
clientIds = [];
Sorry for having missed them. Thanks for the pointer. Addressed.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:27:
.then(function(clientId) {
On 2016/02/17 02:40:28, nhiroki wrote:
> client_id

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:36:
.then(function(clientId) {
On 2016/02/17 02:40:28, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:58:
['visible', true, new URL(scope + '#1', location).toString(), 'nested'],
Yes, that works. Addressed.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:68:
service_worker_unregister_and_done(t, scope);
Without this line, it ends up being a timeout error. So, I just left it as of
now. I'll figure out what causes that error.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:22:
registration = reg;
Yes, I removed the getRegistration() in line 50 and changed it to use
|registration| instead.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-other-origin.html:40:
var otherOriginClientIds = [];
On 2016/02/17 02:40:28, nhiroki wrote:
> other_origin_client_ids

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:16:
var clientIds = e.data.clientIds;
On 2016/02/17 02:40:28, nhiroki wrote:
> client_ids

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/clients-get-worker.js:20:
clientIds.map(function(clientId) {
On 2016/02/17 02:40:28, nhiroki wrote:
> client_id

Done.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:31:
method get
I see. So I guess it can be added later in line with changing the value of the
feature status in RuntimeEnabledFeatures.in when it gets stable. I'm curious
whether the same string in
"/third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt"
doesn't have to be removed.

[zino, 2016-02-17 18:38:14]

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:68:
service_worker_unregister_and_done(t, scope);
On 2016/02/17 14:01:12, jungkees wrote:
> Without this line, it ends up being a timeout error. So, I just left it as of
> now. I'll figure out what causes that error.

I think you can use the "promise_test" instead of async_test.
If so, you can remove this line.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:31:
method get
On 2016/02/17 14:01:13, jungkees wrote:
> I see. So I guess it can be added later in line with changing the value of the
> feature status in RuntimeEnabledFeatures.in when it gets stable. I'm curious
> whether the same string in
>
"/third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt"
> doesn't have to be removed.

Yes, currently, the feature is behind a runtime flag in stable mode but it's
already exposed for test runner. (or experimental)

https://codereview.chromium.org/1439333002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:412:
BrowserThread::PostTask(
Couldn't we use "provider_host->GetWindowClientInfo(callback);" simply?

https://codereview.chromium.org/1439333002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_client_info.cc (right):

https://codereview.chromium.org/1439333002/diff/220001/content/common/service...
content/common/service_worker/service_worker_client_info.cc:22: const
std::string& client_uuid,
After this change, I think we can remove setting client_uuid in some callbacks.
It's duplicated unnecessarily.
 - ServiceWorkerVersion::OnFocusClientFinished
 - ServiceWorkerVersion::OnNavigateClientFinished
 - ServiceWorkerVersion::OnOpenWindowFinished

https://codereview.chromium.org/1439333002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
(right):

https://codereview.chromium.org/1439333002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:33: }
I think we should return null or something even if passing 'switch' statement.

[nhiroki, 2016-02-18 01:40:27]

LGTM after jinho.bang@'s comments are addressed. Thank you for working on this
:)

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:1:
<!DOCTYPE html>
This tests only the case where there are window clients. Can you add a test case
for worker clients?

(This CL is getting bigger and longer, so a separate CL would be OK.)

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:68:
service_worker_unregister_and_done(t, scope);
On 2016/02/17 18:38:13, zino wrote:
> On 2016/02/17 14:01:12, jungkees wrote:
> > Without this line, it ends up being a timeout error. So, I just left it as
of
> > now. I'll figure out what causes that error.

Did you call "t.done()"?

> I think you can use the "promise_test" instead of async_test.
> If so, you can remove this line.

promise_test() sgtm.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:31:
method get
On 2016/02/17 18:38:13, zino wrote:
> On 2016/02/17 14:01:13, jungkees wrote:
> > I see. So I guess it can be added later in line with changing the value of
the
> > feature status in RuntimeEnabledFeatures.in when it gets stable.
> > I'm curious whether the same string in
> >
>
"/third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt"
> > doesn't have to be removed.
> 
> Yes, currently, the feature is behind a runtime flag in stable mode but it's
> already exposed for test runner. (or experimental)

That's right. Thank you for your follow-up.

https://codereview.chromium.org/1439333002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
(right):

https://codereview.chromium.org/1439333002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:33: }
On 2016/02/17 18:38:14, zino wrote:
> I think we should return null or something even if passing 'switch' statement.

Agree. IIRC, MSVC may complain if there is not 'default' or such a return.

[nhiroki, 2016-02-18 02:09:19]

The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 02:09:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1439333002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1439333002/220001

[nhiroki, 2016-02-18 02:18:09]

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:1:
<!DOCTYPE html>
On 2016/02/18 01:40:27, nhiroki wrote:
> This tests only the case where there are window clients. Can you add a test
case
> for worker clients?
> 
> (This CL is getting bigger and longer, so a separate CL would be OK.)

FYI: I think it's ok to ship this feature (to make the flag stable) when we add
a test for worker clients and confirm this feature does not cause any crashes on
the chromium waterfall.

[commit-bot: I haz the power, 2016-02-18 02:30:49]

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

[commit-bot: I haz the power, 2016-02-18 02:30:50]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[nhiroki, 2016-02-18 03:08:07]

fyi: mkwst@, I think you can now start to review this CL:

- content/common/service_worker/service_worker_messages.h
- third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
- third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.{cpp,h}

[jungkees, 2016-02-18 15:02:20]

I uploaded a new snapshot having addressed zino@'s comments. nhiroki@, zino@,
PTAL again before committing it.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:1:
<!DOCTYPE html>
Okay. I'll do that in a separate CL.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:1:
<!DOCTYPE html>
I will do so.

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:68:
service_worker_unregister_and_done(t, scope);
Correct it by using "promise_test".

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(right):

https://codereview.chromium.org/1439333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:31:
method get
Understood. Thanks.

https://codereview.chromium.org/1439333002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/1439333002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_client_utils.cc:412:
BrowserThread::PostTask(
Yes, that's effectively the same call flow between the IO and the UI thread and
simplifying the code. Addressed. Thanks.

https://codereview.chromium.org/1439333002/diff/220001/content/common/service...
File content/common/service_worker/service_worker_client_info.cc (right):

https://codereview.chromium.org/1439333002/diff/220001/content/common/service...
content/common/service_worker/service_worker_client_info.cc:22: const
std::string& client_uuid,
Good point. I refactored those functions a bit according to the suggestion.
Thanks.

https://codereview.chromium.org/1439333002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp
(right):

https://codereview.chromium.org/1439333002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:33: }
This actually causes a compile time error in some of the bots. Fixed.

[nhiroki, 2016-02-19 05:18:32]

https://codereview.chromium.org/1439333002/diff/240001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1439333002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:895:
callbacks->onSuccess(adoptWebPtr(new blink::WebServiceWorkerClientInfo(
It looks like this fails compile steps on Win bot. Maybe it's caused by ADL
failure[1]. Adding "blink::" prefix to "adoptWebPtr" may fix it.

[1] https://en.wikipedia.org/wiki/Argument-dependent_name_lookup

(It'd be great if you could fix other call sites in this file, too)

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:38:
channel.port1.onmessage = t.step_func(on_message);
This test may finish before |on_message| is fired. You need to promisify the
message handler and wait until the promise is settled like this:


.then(function(client_id) {
    var channel = new MessageChannel();
    var saw_message = new Promise(function(resolve) {
        channel1.port1.onmessage = resolve;
      });
    frame.contentWindow.navigator.serviceWorker.controller.postMessage(...);
    return saw_message;
  })
.then(function(event) {
    assert_equals(event.data.length, 3);
    assert_array_equals(event.data[0], expected[0]);
    assert_array_equals(event.data[1], expected[1]);
    assert_equals(event.data[2], expected[2]);
  });

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(left):

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:771:
PASS Verify the interface of ServiceWorkerGlobalScope
This change fails global-interface-listing-service-worker.html. Can you revert
this?

[jungkees, 2016-02-19 05:30:02]

nhiroki@, thanks for the comments. Actually, I'm having a 403 error in uploading
snapshots for this specific CL from my work env. I'll upload the fix tonight.
Sorry for the unwanted latency.

[Mike West, 2016-02-19 06:45:00]

On 2016/02/18 at 03:08:07, nhiroki wrote:
> fyi: mkwst@, I think you can now start to review this CL:
> 
> - content/common/service_worker/service_worker_messages.h
> - third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
> - third_party/WebKit/Source/web/ServiceWorkerGlobalScopeClientImpl.{cpp,h}

These LGTM, thanks.

[jungkees, 2016-02-19 16:31:43]

I'm figuring out the reason for the timeout error encountered again in
clients-get.html after promisifying the message handler. I'll update.

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:38:
channel.port1.onmessage = t.step_func(on_message);
That's right. I'm working on this. Actually having tried this promisifying the
message handler, I'm having a timeout error again. I think I need a bit of more
time to find the reason.

[jungkees, 2016-02-22 05:23:56]

The timeout error encountered during the layout test update turned out to be
caused by dereferencing a nullptr in GetCallback::onSuccess
(ServiceWorkerClients.cpp). I'll upload a new patchset including the resolution
for this error.

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:38:
channel.port1.onmessage = t.step_func(on_message);
It was not an error in this test case. GetCallback::onSuccess
(ServiceWorkerClients.cpp) didn't properly handle the case where it's passed a
null client. Adding a null check solved it.

[jungkees, 2016-02-22 14:05:09]

I uploaded a new snapshot having addressed the comments. PTAL

https://codereview.chromium.org/1439333002/diff/240001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1439333002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:895:
callbacks->onSuccess(adoptWebPtr(new blink::WebServiceWorkerClientInfo(
Addressed. Also, I changed this part of the code to be the same as other
onSuccess call sites that takes a WebServiceWorkerClientInfo object. That change
is to avoid crashing on DCHECK in ToWebServiceWorkerClientInfo when the
client_uuid of the passed client is an empty string.

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html
(right):

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/clients-get.html:38:
channel.port1.onmessage = t.step_func(on_message);
Promisified the block and fixed the renderer crash on a null client.

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(left):

https://codereview.chromium.org/1439333002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:771:
PASS Verify the interface of ServiceWorkerGlobalScope
On 2016/02/19 05:18:32, nhiroki (slow until Feb. 24) wrote:
> This change fails global-interface-listing-service-worker.html. Can you revert
> this?

Done.

[nhiroki, 2016-02-22 15:59:09]

lgtm

https://codereview.chromium.org/1439333002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp
(right):

https://codereview.chromium.org/1439333002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp:74: if
(!client || client->uuid.isEmpty()) {
According to the assertion newly added at line 897 in
service_worker_context_client.cc, |client->uuid.isEmpty()| should never be
satisfied. If it's correct, can you remove this condition?

[jungkees, 2016-02-22 23:08:25]

Addressed the comment. Thanks!

https://codereview.chromium.org/1439333002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp
(right):

https://codereview.chromium.org/1439333002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClients.cpp:74: if
(!client || client->uuid.isEmpty()) {
Yes, that's right. I removed the latter condition. Thanks!

[jungkees, 2016-02-22 23:08:39]

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

[jungkees, 2016-02-22 23:08:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
jinho.bang@samsung.com, mkwst@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1439333002/#ps280001
(title: "Remove unnecessary condition")

[commit-bot: I haz the power, 2016-02-22 23:11:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1439333002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1439333002/280001

[zino, 2016-02-22 23:33:38]

non-owner lgtm

[commit-bot: I haz the power, 2016-02-23 00:36:41]

Description was changed from

==========
Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552310
==========

to

==========
Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552310
==========

[commit-bot: I haz the power, 2016-02-23 00:36:43]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-02-23 00:38:18]

Description was changed from

==========
Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552310
==========

to

==========
Service Worker: Add Clients.get(id)

As per the latest spec update, this CL implements Clients.get(id) method.

Spec discussion:
https://github.com/slightlyoff/ServiceWorker/issues/723#issuecomment-151326898
Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-get-...

Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XlBHOIOCjUk

The layout tests for this CL are mostly taken from Mozilla's work instead of
making new ones to ensure compatibility:
https://bugzilla.mozilla.org/show_bug.cgi?id=1222464

BUG=552310

Committed: https://crrev.com/0ca977be9dbe2582e8f18415bd9299efd36fd329
Cr-Commit-Position: refs/heads/master@{#376870}
==========

[commit-bot: I haz the power, 2016-02-23 00:38:20]

Patchset 14 (id:??) landed as
https://crrev.com/0ca977be9dbe2582e8f18415bd9299efd36fd329
Cr-Commit-Position: refs/heads/master@{#376870}