[ServiceWorker] Add wpt tests to verify Client.type as 'worker'

[ServiceWorker] Add wpt tests to verify Client.type as 'worker'

Currently we have wpt tests verifying Clients.{get,matchAll} can return
clients with type 'window' or 'sharedworker' correctly, this CL adds
support for another type 'worker'.

BUG=705685
TEST=blink_tests

Review-Url: https://codereview.chromium.org/2884843004
Cr-Commit-Position: refs/heads/master@{#474954}
Committed: https://chromium.googlesource.com/chromium/src/+/2d88b05db6842a4cc1391dc30ed604588c147041

[leonhsl(Using Gerrit), 2017-05-16 02:47:38]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 02:48:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-05-16 03:07:55]

leon.han@intel.com changed reviewers:
+ falken@chromium.org

[leonhsl(Using Gerrit), 2017-05-16 03:07:56]

PTAL with my inline questions,, Thanks!

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:40:
frame.contentWindow.postMessage('StartWorker', '*', [channel.port2]);
Let frame(controlled by our sw) start the worker, because I found that if the
worker was started from main frame(uncontrolled by our sw), it would not be
controlled by our sw either.
# This is shown at
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/worker-interception.https-expected.txt,
which is not implemented by Chromium for now?

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:58:
assert_equals(e.data.length, 3);
The current problem is that we always get e.data as undefined.. I tried to print
out the worker id we got at line48, it turns out to be a valid id, but seems in
our sw clients-get-worker.js calling Clients.get() for this worker id always
fail. Any hints? Or something silly I'm doing?

[commit-bot: I haz the power, 2017-05-16 03:37:59]

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

[commit-bot: I haz the power, 2017-05-16 03:38:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[falken, 2017-05-16 04:51:35]

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:40:
frame.contentWindow.postMessage('StartWorker', '*', [channel.port2]);
On 2017/05/16 03:07:56, leonhsl wrote:
> Let frame(controlled by our sw) start the worker, because I found that if the
> worker was started from main frame(uncontrolled by our sw), it would not be
> controlled by our sw either.
> # This is shown at
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/worker-interception.https-expected.txt,
> which is not implemented by Chromium for now?

I see. We shouldn't add this workaround for a Chromium bug in the WPT. Instead
the WPT should be written as normal and we should have an expectation for
Chromium's failure. 

In order to have test coverage, what we could do is have the WPT asserting
normal things and then a special version called
chromium-clients-get-client-types.html in LayoutTests/http/tests/serviceworker
with the workaround added.

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:58:
assert_equals(e.data.length, 3);
On 2017/05/16 03:07:56, leonhsl wrote:
> The current problem is that we always get e.data as undefined.. I tried to
print
> out the worker id we got at line48, it turns out to be a valid id, but seems
in
> our sw clients-get-worker.js calling Clients.get() for this worker id always
> fail. Any hints? Or something silly I'm doing?

It sounds like it could be a bug. I'm confused why e.data is undefined instead
of the missing client being undefined in the e.data array. Could you dig in
more?

[leonhsl(Using Gerrit), 2017-05-18 09:36:30]

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:40:
frame.contentWindow.postMessage('StartWorker', '*', [channel.port2]);
On 2017/05/16 04:51:35, falken wrote:
> On 2017/05/16 03:07:56, leonhsl wrote:
> > Let frame(controlled by our sw) start the worker, because I found that if
the
> > worker was started from main frame(uncontrolled by our sw), it would not be
> > controlled by our sw either.
> > # This is shown at
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/worker-interception.https-expected.txt,
> > which is not implemented by Chromium for now?
> 
> I see. We shouldn't add this workaround for a Chromium bug in the WPT. Instead
> the WPT should be written as normal and we should have an expectation for
> Chromium's failure. 
> 
> In order to have test coverage, what we could do is have the WPT asserting
> normal things and then a special version called
> chromium-clients-get-client-types.html in LayoutTests/http/tests/serviceworker
> with the workaround added.

Got it and I agree such process. I will do so once I can get the test PASS.

https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:58:
assert_equals(e.data.length, 3);
On 2017/05/16 04:51:35, falken wrote:
> On 2017/05/16 03:07:56, leonhsl wrote:
> > The current problem is that we always get e.data as undefined.. I tried to
> print
> > out the worker id we got at line48, it turns out to be a valid id, but seems
> in
> > our sw clients-get-worker.js calling Clients.get() for this worker id always
> > fail. Any hints? Or something silly I'm doing?
> 
> It sounds like it could be a bug. I'm confused why e.data is undefined instead
> of the missing client being undefined in the e.data array. Could you dig in
> more?

I'm checking around this, found that console.log() in test html/javascript can
output log into the test report, but seems console.log() in sw javascript can't.
Where can we find those log text? :)

[falken, 2017-05-19 09:09:59]

On 2017/05/18 09:36:30, leonhsl wrote:
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
> (right):
> 
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:40:
> frame.contentWindow.postMessage('StartWorker', '*', [channel.port2]);
> On 2017/05/16 04:51:35, falken wrote:
> > On 2017/05/16 03:07:56, leonhsl wrote:
> > > Let frame(controlled by our sw) start the worker, because I found that if
> the
> > > worker was started from main frame(uncontrolled by our sw), it would not
be
> > > controlled by our sw either.
> > > # This is shown at
> > >
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/worker-interception.https-expected.txt,
> > > which is not implemented by Chromium for now?
> > 
> > I see. We shouldn't add this workaround for a Chromium bug in the WPT.
Instead
> > the WPT should be written as normal and we should have an expectation for
> > Chromium's failure. 
> > 
> > In order to have test coverage, what we could do is have the WPT asserting
> > normal things and then a special version called
> > chromium-clients-get-client-types.html in
LayoutTests/http/tests/serviceworker
> > with the workaround added.
> 
> Got it and I agree such process. I will do so once I can get the test PASS.
> 
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:58:
> assert_equals(e.data.length, 3);
> On 2017/05/16 04:51:35, falken wrote:
> > On 2017/05/16 03:07:56, leonhsl wrote:
> > > The current problem is that we always get e.data as undefined.. I tried to
> > print
> > > out the worker id we got at line48, it turns out to be a valid id, but
seems
> > in
> > > our sw clients-get-worker.js calling Clients.get() for this worker id
always
> > > fail. Any hints? Or something silly I'm doing?
> > 
> > It sounds like it could be a bug. I'm confused why e.data is undefined
instead
> > of the missing client being undefined in the e.data array. Could you dig in
> > more?
> 
> I'm checking around this, found that console.log() in test html/javascript can
> output log into the test report, but seems console.log() in sw javascript
can't.
> Where can we find those log text? :)

console.error() outputs into the test report, you can click the "stderr"
checkbox in the test results page to see them.

[leonhsl(Using Gerrit), 2017-05-19 10:51:00]

On 2017/05/19 09:09:59, falken wrote:
> On 2017/05/18 09:36:30, leonhsl wrote:
> >
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
> > File
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:40:
> > frame.contentWindow.postMessage('StartWorker', '*', [channel.port2]);
> > On 2017/05/16 04:51:35, falken wrote:
> > > On 2017/05/16 03:07:56, leonhsl wrote:
> > > > Let frame(controlled by our sw) start the worker, because I found that
if
> > the
> > > > worker was started from main frame(uncontrolled by our sw), it would not
> be
> > > > controlled by our sw either.
> > > > # This is shown at
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/worker-interception.https-expected.txt,
> > > > which is not implemented by Chromium for now?
> > > 
> > > I see. We shouldn't add this workaround for a Chromium bug in the WPT.
> Instead
> > > the WPT should be written as normal and we should have an expectation for
> > > Chromium's failure. 
> > > 
> > > In order to have test coverage, what we could do is have the WPT asserting
> > > normal things and then a special version called
> > > chromium-clients-get-client-types.html in
> LayoutTests/http/tests/serviceworker
> > > with the workaround added.
> > 
> > Got it and I agree such process. I will do so once I can get the test PASS.
> > 
> >
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:58:
> > assert_equals(e.data.length, 3);
> > On 2017/05/16 04:51:35, falken wrote:
> > > On 2017/05/16 03:07:56, leonhsl wrote:
> > > > The current problem is that we always get e.data as undefined.. I tried
to
> > > print
> > > > out the worker id we got at line48, it turns out to be a valid id, but
> seems
> > > in
> > > > our sw clients-get-worker.js calling Clients.get() for this worker id
> always
> > > > fail. Any hints? Or something silly I'm doing?
> > > 
> > > It sounds like it could be a bug. I'm confused why e.data is undefined
> instead
> > > of the missing client being undefined in the e.data array. Could you dig
in
> > > more?
> > 
> > I'm checking around this, found that console.log() in test html/javascript
can
> > output log into the test report, but seems console.log() in sw javascript
> can't.
> > Where can we find those log text? :)
> 
> console.error() outputs into the test report, you can click the "stderr"
> checkbox in the test results page to see them.

Yeah clients-get-client-types.https.html logs can be found there, but there has
no logs output by the sw script clients-get-worker.js.

[falken, 2017-05-22 04:12:20]

On 2017/05/19 10:51:00, leonhsl wrote:
> On 2017/05/19 09:09:59, falken wrote:
> > On 2017/05/18 09:36:30, leonhsl wrote:
> > >
> >
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
> > > File
> > >
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
> > >
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:40:
> > > frame.contentWindow.postMessage('StartWorker', '*', [channel.port2]);
> > > On 2017/05/16 04:51:35, falken wrote:
> > > > On 2017/05/16 03:07:56, leonhsl wrote:
> > > > > Let frame(controlled by our sw) start the worker, because I found that
> if
> > > the
> > > > > worker was started from main frame(uncontrolled by our sw), it would
not
> > be
> > > > > controlled by our sw either.
> > > > > # This is shown at
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/worker-interception.https-expected.txt,
> > > > > which is not implemented by Chromium for now?
> > > > 
> > > > I see. We shouldn't add this workaround for a Chromium bug in the WPT.
> > Instead
> > > > the WPT should be written as normal and we should have an expectation
for
> > > > Chromium's failure. 
> > > > 
> > > > In order to have test coverage, what we could do is have the WPT
asserting
> > > > normal things and then a special version called
> > > > chromium-clients-get-client-types.html in
> > LayoutTests/http/tests/serviceworker
> > > > with the workaround added.
> > > 
> > > Got it and I agree such process. I will do so once I can get the test
PASS.
> > > 
> > >
> >
>
https://codereview.chromium.org/2884843004/diff/1/third_party/WebKit/LayoutTe...
> > >
> >
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:58:
> > > assert_equals(e.data.length, 3);
> > > On 2017/05/16 04:51:35, falken wrote:
> > > > On 2017/05/16 03:07:56, leonhsl wrote:
> > > > > The current problem is that we always get e.data as undefined.. I
tried
> to
> > > > print
> > > > > out the worker id we got at line48, it turns out to be a valid id, but
> > seems
> > > > in
> > > > > our sw clients-get-worker.js calling Clients.get() for this worker id
> > always
> > > > > fail. Any hints? Or something silly I'm doing?
> > > > 
> > > > It sounds like it could be a bug. I'm confused why e.data is undefined
> > instead
> > > > of the missing client being undefined in the e.data array. Could you dig
> in
> > > > more?
> > > 
> > > I'm checking around this, found that console.log() in test html/javascript
> can
> > > output log into the test report, but seems console.log() in sw javascript
> > can't.
> > > Where can we find those log text? :)
> > 
> > console.error() outputs into the test report, you can click the "stderr"
> > checkbox in the test results page to see them.
> 
> Yeah clients-get-client-types.https.html logs can be found there, but there
has
> no logs output by the sw script clients-get-worker.js.

You might need to run the test and open it in content shell/chromium itself. To
do that, you can run Tools/Scripts/run-blink-wptserve -v. See also
Tools/Scripts/run-blink-wptserve --help output.

[leonhsl(Using Gerrit), 2017-05-24 03:55:31]

Hi, falken@, Thanks a lot for the information sharing about
Tools/Scripts/run-blink-wptserve, I did not know such a good tool before!

But after started the server and when I tried to open
https://localhost:8444/xxxx, chrome/content_shell always complain: 
'
Your connection is not private
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
NET::ERR_CERT_AUTHORITY_INVALID

Subject: 127.0.0.1
Issuer: web-platform-tests
Expires on: Jan 4, 2025
Current date: May 24, 2017
PEM encoded chain:
-----BEGIN CERTIFICATE-----
MIIDnTCCAoWgAwIBAgIBAjANBgkqhkiG9w0BAQsFADAdMRswGQYDVQQDDBJ3ZWIt
cGxhdGZvcm0tdGVzdHMwHhcNMTYxMDE4MDgzNjUzWhcNMjUwMTA0MDgzNjUzWjAU
MRIwEAYDVQQDDAkxMjcuMC4wLjEwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK
AoIBAQCqvVXRb5YIi49yvc5wLAj1KItMA7p4rdNeqSaHX0XOfW7MGPlzyzTg1lC8
R1Fhky1PGMVhd55Oiz/rNvC5HKTIPnMOyP2aMswiKEYAyANKRkK7DQbADocbypG4
hH3JOEGovvlGz5568lwSmJrrNP4YjuFN+AapQwAtu4vsrfk+noJbgzLwRy9Mqt7m
gycMszsF2e7uGvLNOdgVOjqdYx5j7Xer2QKxY7GgYklKQ8WfzOMdf1SSn15K1yd4
MsH6M1hmhgtEOnWmBS9B0zvL9lKB08kNe5kiDrkY+EmzmbNZJM8RkLu7aQnvmvmK
MuCoXDCXdZ8LTICxo0De5Y/ZtyNvAgMBAAGjgfAwge0wCQYDVR0TBAIwADAdBgNV
HQ4EFgQUoSHga/OmG32M8AUzYccdfUdtZpkwHwYDVR0jBBgwFoAUTmUNO5KzWks0
dfspJspTa6FlFX4wCwYDVR0PBAQDAgXgMBMGA1UdJQQMMAoGCCsGAQUFBwMBMH4G
A1UdEQR3MHWCCTEyNy4wLjAuMYINd3d3LjEyNy4wLjAuMYIheG4tLW44ajZkczUz
bHd3a3JxaHYyOGEuMTI3LjAuMC4xghZ4bi0tbHZlLTZsYWQuMTI3LjAuMC4xgg53
d3cyLjEyNy4wLjAuMYIOd3d3MS4xMjcuMC4wLjEwDQYJKoZIhvcNAQELBQADggEB
AERHvmu41zR5rOemWl/oK3/JqQQa1XWn1j73mLPrN+19ljdEDHyt6YrGUGlWY5E/
K7SJDPcDrmmxsNYAM6Z8g9tW0OnY/FO+rbg+PsTlIs3Q66eldfEDur1JMgsbty9W
qJgUQ1jUIwV8uRMAhW5w5VpFH55KwAZykzgkndLZgpnIX8LG8vlTSeHLE3KqB+jz
g6b0TCimlHzX5EKw3tbsDeKvL81ExLbLVH3XHzJNlzHHbhMUDqH4buS07hWD/X4p
5HsGNdLjmm55qqGikWWYSEYbMzarJsJ2GJSC6sCgyp6xmF+YTdccibTpcg36t6nC
acMK7vM+/ZCHyAwzMWnhwJk=
-----END CERTIFICATE-----

'

I tried some ways to authorize the certificate for localhost, but can't succeed.
Tried to put above CERTIFICATE to a .pem file and import it in chrome
Settings-->Manage certificates, but the import always fails.

Do you have any idea on this? Thanks!

[falken, 2017-05-24 05:32:09]

On 2017/05/24 03:55:31, leonhsl wrote:
> Hi, falken@, Thanks a lot for the information sharing about
> Tools/Scripts/run-blink-wptserve, I did not know such a good tool before!
> 
> But after started the server and when I tried to open
> https://localhost:8444/xxxx, chrome/content_shell always complain: 
> '
> Your connection is not private
> XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
> NET::ERR_CERT_AUTHORITY_INVALID
> 
> Subject: 127.0.0.1
> Issuer: web-platform-tests
> Expires on: Jan 4, 2025
> Current date: May 24, 2017
> PEM encoded chain:
> -----BEGIN CERTIFICATE-----
> MIIDnTCCAoWgAwIBAgIBAjANBgkqhkiG9w0BAQsFADAdMRswGQYDVQQDDBJ3ZWIt
> cGxhdGZvcm0tdGVzdHMwHhcNMTYxMDE4MDgzNjUzWhcNMjUwMTA0MDgzNjUzWjAU
> MRIwEAYDVQQDDAkxMjcuMC4wLjEwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK
> AoIBAQCqvVXRb5YIi49yvc5wLAj1KItMA7p4rdNeqSaHX0XOfW7MGPlzyzTg1lC8
> R1Fhky1PGMVhd55Oiz/rNvC5HKTIPnMOyP2aMswiKEYAyANKRkK7DQbADocbypG4
> hH3JOEGovvlGz5568lwSmJrrNP4YjuFN+AapQwAtu4vsrfk+noJbgzLwRy9Mqt7m
> gycMszsF2e7uGvLNOdgVOjqdYx5j7Xer2QKxY7GgYklKQ8WfzOMdf1SSn15K1yd4
> MsH6M1hmhgtEOnWmBS9B0zvL9lKB08kNe5kiDrkY+EmzmbNZJM8RkLu7aQnvmvmK
> MuCoXDCXdZ8LTICxo0De5Y/ZtyNvAgMBAAGjgfAwge0wCQYDVR0TBAIwADAdBgNV
> HQ4EFgQUoSHga/OmG32M8AUzYccdfUdtZpkwHwYDVR0jBBgwFoAUTmUNO5KzWks0
> dfspJspTa6FlFX4wCwYDVR0PBAQDAgXgMBMGA1UdJQQMMAoGCCsGAQUFBwMBMH4G
> A1UdEQR3MHWCCTEyNy4wLjAuMYINd3d3LjEyNy4wLjAuMYIheG4tLW44ajZkczUz
> bHd3a3JxaHYyOGEuMTI3LjAuMC4xghZ4bi0tbHZlLTZsYWQuMTI3LjAuMC4xgg53
> d3cyLjEyNy4wLjAuMYIOd3d3MS4xMjcuMC4wLjEwDQYJKoZIhvcNAQELBQADggEB
> AERHvmu41zR5rOemWl/oK3/JqQQa1XWn1j73mLPrN+19ljdEDHyt6YrGUGlWY5E/
> K7SJDPcDrmmxsNYAM6Z8g9tW0OnY/FO+rbg+PsTlIs3Q66eldfEDur1JMgsbty9W
> qJgUQ1jUIwV8uRMAhW5w5VpFH55KwAZykzgkndLZgpnIX8LG8vlTSeHLE3KqB+jz
> g6b0TCimlHzX5EKw3tbsDeKvL81ExLbLVH3XHzJNlzHHbhMUDqH4buS07hWD/X4p
> 5HsGNdLjmm55qqGikWWYSEYbMzarJsJ2GJSC6sCgyp6xmF+YTdccibTpcg36t6nC
> acMK7vM+/ZCHyAwzMWnhwJk=
> -----END CERTIFICATE-----
> 
> '
> 
> I tried some ways to authorize the certificate for localhost, but can't
succeed.
> Tried to put above CERTIFICATE to a .pem file and import it in chrome
> Settings-->Manage certificates, but the import always fails.
> 
> Do you have any idea on this? Thanks!

Yea if you can, just navigate to http://127.0.0.1:8001. If the test is not
changing origins, it should just work.

If you must go to an https URL I think you need to use a flag like
--allow-insecure-localhost or --ignore-certificate-errors.

[leonhsl(Using Gerrit), 2017-05-24 05:53:11]

Thank you very much for all the great help!

Now I've figured out what happened around this CL:

1,
After registered/activated an in-scope SW, if I create a new
Worker(in-scope-url) from an *uncontrolled* frame, the Worker should be
controlled by the SW and have a client type as 'Worker'.
But currently in Chromium this Worker turns out to be uncontrolled.
Can I add such a test and corresponding expectation into wpt
clients-get-client-types.https.html in this CL? Then I'd like to focus how to
fix it.

Although wpt worker-interception.https.html also has a test creating new
Worker(in-scope-url) from an *uncontrolled* frame, but it aims to verify that
the in-scope-url fetch request will be intercepted by the SW.

2,
After registered/activated an in-scope SW, if I create a new
Worker(in-scope-url) from an *controlled* frame, the Worker should be controlled
by the SW and have a client type as 'Worker'.
Currently in Chromium, although this Worker can get controlled by the SW, I've
confirmed that it has exactly the same client id with the frame starting it!
This is why ps#1 failed.
Can I also add a test&expectation firstly in this CL and then focus on fixing
it?

Thanks!

[leonhsl(Using Gerrit), 2017-05-25 08:12:50]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 08:13:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 09:58:22]

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

[commit-bot: I haz the power, 2017-05-25 09:58:23]

Dry run: This issue passed the CQ dry run.

[falken, 2017-05-26 01:49:25]

Thanks for digging in here.

Since the bugs you identified are non-trivial,
I'd prefer to restore the chromium version of the test along with this patch so
we don't lose test coverage while the bug is being fixed.

I think the main goal now is to just to test a case where Client.type ==
'worker'. So waybe we should just add assertions to
clients-matchall-client-types.https.html instead? That way whether the worker is
controlled and its client ID should not matter.

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:73:
'Window client is undefined');
These assert_not_equals undefined seem redundant with the asserts for the
expectation.

Also, the assert description is usually written in the form of "should be",
e.g.,"Window client should not be undefined".

[leonhsl(Using Gerrit), 2017-05-26 03:35:48]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 03:36:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-05-26 03:44:17]

Thanks a lot for review!

Uploaded ps#3, which adds dedicated worker client related tests into
clients-matchall-client-types.https.html.

And, I suppose we're not losing test coverage compared with before?
clients-get-client-types.https.html in this CL has 4 assertions, the
expected.txt file indicates that the 3rd one failed, meaning that the first 2
assertions succeeded, which is the old test coverage.
'
FAIL Test Clients.get() with window and worker clients assert_not_equals: Worker
(Started by main frame) client should not be undefined got disallowed value
undefined
'

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:73:
'Window client is undefined');
On 2017/05/26 01:49:25, falken wrote:
> These assert_not_equals undefined seem redundant with the asserts for the
> expectation.
> 
> Also, the assert description is usually written in the form of "should be",
> e.g.,"Window client should not be undefined".

Adding assert_not_equals undefined here is because that if e.data[i] is
undefined, assert_array_equals just throws TypeError, without any useful output.
'
Unhandled rejection with value: object "TypeError: Cannot read property 'length'
of undefined"

[falken, 2017-05-26 04:24:58]

Thanks for adding the matchall test.

> And, I suppose we're not losing test coverage compared with before?
clients-get-client-types.https.html in this CL has 4 assertions, the
expected.txt file indicates that the 3rd one failed, meaning that the first 2
assertions succeeded, which is the old test coverage.

Yes, I was worried about if assertions get added after this later on, since I
don't think we'll get to this bug in a while.

But maybe that's unlikely since we probably won't be adding more types.

So I think this looks good. I'd still prefer we don't have assert not undefined.

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:73:
'Window client is undefined');
On 2017/05/26 03:44:17, leonhsl wrote:
> On 2017/05/26 01:49:25, falken wrote:
> > These assert_not_equals undefined seem redundant with the asserts for the
> > expectation.
> > 
> > Also, the assert description is usually written in the form of "should be",
> > e.g.,"Window client should not be undefined".
> 
> Adding assert_not_equals undefined here is because that if e.data[i] is
> undefined, assert_array_equals just throws TypeError, without any useful
output.
> '
> Unhandled rejection with value: object "TypeError: Cannot read property
'length'
> of undefined"

That error actually seems pretty useful: it basically says the array is
undefined.

There's a cost to test readability here. We should write tests mostly expecting
them to pass, and give enough information to allow debugging when they fail.
e.data[i] being undefined should be considered an uncommon case, despite
chrome's current implementation.

https://codereview.chromium.org/2884843004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:83:
assert_array_equals(e.data[3], expected[3]);
let's remove the asserts that it's not undefined and add a description to the
assert_array_eqquals calls, e.g.,

assert_array_equals(e.data[3], expected[3], 'Worker(Started by sub frame)
client');

[commit-bot: I haz the power, 2017-05-26 04:52:49]

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

[commit-bot: I haz the power, 2017-05-26 04:52:50]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-05-26 05:17:41]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 05:18:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-05-26 05:19:00]

Description was changed from

==========
[ServiceWorker] Add wpt tests to verify Client.type as 'worker'

Currently we have wpt tests verifying Clients.get() can return clients
with type 'window' or 'sharedworker' correctly, this CL adds support
for another type 'worker'.

BUG=705685
TEST=blink_tests
==========

to

==========
[ServiceWorker] Add wpt tests to verify Client.type as 'worker'

Currently we have wpt tests verifying Clients.{get,matchAll} can return
clients with type 'window' or 'sharedworker' correctly, this CL adds
support for another type 'worker'.

BUG=705685
TEST=blink_tests
==========

[leonhsl(Using Gerrit), 2017-05-26 05:27:32]

Uploaded ps#4, PTAnL, Thanks.

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:73:
'Window client is undefined');
On 2017/05/26 04:24:57, falken wrote:
> On 2017/05/26 03:44:17, leonhsl wrote:
> > On 2017/05/26 01:49:25, falken wrote:
> > > These assert_not_equals undefined seem redundant with the asserts for the
> > > expectation.
> > > 
> > > Also, the assert description is usually written in the form of "should
be",
> > > e.g.,"Window client should not be undefined".
> > 
> > Adding assert_not_equals undefined here is because that if e.data[i] is
> > undefined, assert_array_equals just throws TypeError, without any useful
> output.
> > '
> > Unhandled rejection with value: object "TypeError: Cannot read property
> 'length'
> > of undefined"
> 
> That error actually seems pretty useful: it basically says the array is
> undefined.
> 
> There's a cost to test readability here. We should write tests mostly
expecting
> them to pass, and give enough information to allow debugging when they fail.
> e.data[i] being undefined should be considered an uncommon case, despite
> chrome's current implementation.

I see, Thanks a lot for kindly explanations. Removed the assert_not_equals
undefined assertions. But after that I found that seems assert_array_equals does
not output assertion's description when throwing TypeError.

https://codereview.chromium.org/2884843004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
(right):

https://codereview.chromium.org/2884843004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:83:
assert_array_equals(e.data[3], expected[3]);
On 2017/05/26 04:24:58, falken wrote:
> let's remove the asserts that it's not undefined and add a description to the
> assert_array_eqquals calls, e.g.,
> 
> assert_array_equals(e.data[3], expected[3], 'Worker(Started by sub frame)
> client');

Done.

[leonhsl(Using Gerrit), 2017-05-26 05:31:15]

> Yes, I was worried about if assertions get added after this later on, since I
> don't think we'll get to this bug in a while.
> 

Actually I'm considering to start to dig in these 2 bugs now;-) Could I?

[falken, 2017-05-26 05:36:12]

On 2017/05/26 05:27:32, leonhsl wrote:
> Uploaded ps#4, PTAnL, Thanks.
> 
>
https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
> (right):
> 
>
https://codereview.chromium.org/2884843004/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:73:
> 'Window client is undefined');
> On 2017/05/26 04:24:57, falken wrote:
> > On 2017/05/26 03:44:17, leonhsl wrote:
> > > On 2017/05/26 01:49:25, falken wrote:
> > > > These assert_not_equals undefined seem redundant with the asserts for
the
> > > > expectation.
> > > > 
> > > > Also, the assert description is usually written in the form of "should
> be",
> > > > e.g.,"Window client should not be undefined".
> > > 
> > > Adding assert_not_equals undefined here is because that if e.data[i] is
> > > undefined, assert_array_equals just throws TypeError, without any useful
> > output.
> > > '
> > > Unhandled rejection with value: object "TypeError: Cannot read property
> > 'length'
> > > of undefined"
> > 
> > That error actually seems pretty useful: it basically says the array is
> > undefined.
> > 
> > There's a cost to test readability here. We should write tests mostly
> expecting
> > them to pass, and give enough information to allow debugging when they fail.
> > e.data[i] being undefined should be considered an uncommon case, despite
> > chrome's current implementation.
> 
> I see, Thanks a lot for kindly explanations. Removed the assert_not_equals
> undefined assertions. But after that I found that seems assert_array_equals
does
> not output assertion's description when throwing TypeError.

Ugh, sorry I didn't realize that would happen. Now we're definitely losing test
coverage since the expected file doesn't show which assert is failing.

Let's bring back the assert_not_equals then with a comment that
assert_array_equals doesn't print the error description when passed an undefined
value. Sorry about that.

With that change lgtm.

> 
>
https://codereview.chromium.org/2884843004/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html
> (right):
> 
>
https://codereview.chromium.org/2884843004/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-client-types.https.html:83:
> assert_array_equals(e.data[3], expected[3]);
> On 2017/05/26 04:24:58, falken wrote:
> > let's remove the asserts that it's not undefined and add a description to
the
> > assert_array_eqquals calls, e.g.,
> > 
> > assert_array_equals(e.data[3], expected[3], 'Worker(Started by sub frame)
> > client');
> 
> Done.

[leonhsl(Using Gerrit), 2017-05-26 05:52:51]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 05:53:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[leonhsl(Using Gerrit), 2017-05-26 05:58:50]

The CQ bit was unchecked by leon.han@intel.com

[leonhsl(Using Gerrit), 2017-05-26 05:59:55]

Uploaded ps#5, adding assert_not_equals together with explanation comments,
sending to CQ now. Thanks!

[leonhsl(Using Gerrit), 2017-05-26 06:00:08]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2017-05-26 06:00:09]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2884843004/#ps80001
(title: "Add assert_not_equals undefined")

[commit-bot: I haz the power, 2017-05-26 06:00:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 07:26:49]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495778408900640,
"parent_rev": "4c271c0deaf103d5612078b3d0d81847819c5d52", "commit_rev":
"2d88b05db6842a4cc1391dc30ed604588c147041"}

[commit-bot: I haz the power, 2017-05-26 07:27:00]

Description was changed from

==========
[ServiceWorker] Add wpt tests to verify Client.type as 'worker'

Currently we have wpt tests verifying Clients.{get,matchAll} can return
clients with type 'window' or 'sharedworker' correctly, this CL adds
support for another type 'worker'.

BUG=705685
TEST=blink_tests
==========

to

==========
[ServiceWorker] Add wpt tests to verify Client.type as 'worker'

Currently we have wpt tests verifying Clients.{get,matchAll} can return
clients with type 'window' or 'sharedworker' correctly, this CL adds
support for another type 'worker'.

BUG=705685
TEST=blink_tests

Review-Url: https://codereview.chromium.org/2884843004
Cr-Commit-Position: refs/heads/master@{#474954}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d88b05db6842a4cc1391dc30ed6...
==========

[commit-bot: I haz the power, 2017-05-26 07:27:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2d88b05db6842a4cc1391dc30ed6...

[falken, 2017-05-26 08:05:39]

On 2017/05/26 05:31:15, leonhsl wrote:
> > Yes, I was worried about if assertions get added after this later on, since
I
> > don't think we'll get to this bug in a while.
> > 
> 
> Actually I'm considering to start to dig in these 2 bugs now;-) Could I?

Thanks for looking into it :) As mentioned on
https://bugs.chromium.org/p/chromium/issues/detail?id=724371 I'm a bit worried
that intercepting the worker script will make the code more complex since we're
already starting to redesign how interception works in
https://bugs.chromium.org/p/chromium/issues/detail?id=715640. (There are a ton
of open bugs like SW should intercept X). So while digging into client id bug is
good, I'd rather we leave the worker script interception one alone for now
especially if the fix looks non-trivial.

[leonhsl(Using Gerrit), 2017-05-26 08:40:07]

On 2017/05/26 08:05:39, falken wrote:
> On 2017/05/26 05:31:15, leonhsl wrote:
> > > Yes, I was worried about if assertions get added after this later on,
since
> I
> > > don't think we'll get to this bug in a while.
> > > 
> > 
> > Actually I'm considering to start to dig in these 2 bugs now;-) Could I?
> 
> Thanks for looking into it :) As mentioned on
> https://bugs.chromium.org/p/chromium/issues/detail?id=724371 I'm a bit worried
> that intercepting the worker script will make the code more complex since
we're
> already starting to redesign how interception works in
> https://bugs.chromium.org/p/chromium/issues/detail?id=715640. (There are a ton
> of open bugs like SW should intercept X). So while digging into client id bug
is
> good, I'd rather we leave the worker script interception one alone for now
> especially if the fix looks non-trivial.

Yeah this afternoon after I tried to look into real details for these 2 bugs I
realized they are also relating closely with request interception, just like
issue 724371.
By reading related code, IIUC, currently all dedicated workers WON'T be a SW
client, because dedicated worker does not create its
ServiceWorkerNetworkProvider from beginning. Please correct me if I'm wrong..

And, about the client id bug, the root cause is also relating with that, because
the dedicated worker has no ServiceWorkerNetworkProvider for itself, it's
reusing the sub frame's ServiceWorkerNetworkProvider, meaning that fetch
requests originating from the dedicated worker will be intercepted just like
they're coming from the sub frame, so the returned client id will just be sub
frame's id...

Thanks a lot for kindly explanations again, and I'd like to revisit here
later;-) After issue 715640 solved and I can fully understand it.