Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845963002:
[ServiceWorker] Add type attribute into Client object (Closed)
|
Created:
3 years, 8 months ago by leonhsl(Using Gerrit) Modified:
3 years, 7 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, nhiroki, blink-reviews, kinuko+serviceworker, haraken, horo+watch_chromium.org, falken+watch_chromium.org, tzik, xiaofengzhang Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ServiceWorker] Add type attribute for Client interface
Specification: https://w3c.github.io/ServiceWorker/#client
For Clients.{get,matchAll,openWindow}, currently the C++ impl has
already been able to get the correct client type
{"window","worker","sharedworker"} but does not export it to Js Client
object. This CL adds this support and revises related layout tests.
BUG=705685
TEST=blink_tests
Review-Url: https://codereview.chromium.org/2845963002
Cr-Commit-Position: refs/heads/master@{#470480}
Committed: https://chromium.googlesource.com/chromium/src/+/2ffd4b3b6bc3e571d2fee28955ba9bae10289a28
Patch Set 1 #Patch Set 2 : layout tests #
Total comments: 2
Patch Set 3 : Address comments #Patch Set 4 : Fix layout tests #
Total comments: 6
Patch Set 5 : Address comments from foolip@ #Messages
Total messages: 57 (39 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceWorker] Add type attribute into Client object Specification: https://w3c.github.io/ServiceWorker/#client For Clients.{get,matchAll}, currently the C++ impl has already been able to get the correct client type {window, worker, sharedworker} but does not export it to Js Client object. This CL just adds the export. BUG=705685 TEST=blink_tests ========== to ========== [ServiceWorker] Add type attribute into Client object Specification: https://w3c.github.io/ServiceWorker/#client For Clients.{get,matchAll,openWindow}, currently the C++ impl has already been able to get the correct client type {"window","worker","sharedworker"} but does not export it to Js Client object. This CL adds this support. BUG=705685 TEST=blink_tests ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceWorker] Add type attribute into Client object Specification: https://w3c.github.io/ServiceWorker/#client For Clients.{get,matchAll,openWindow}, currently the C++ impl has already been able to get the correct client type {"window","worker","sharedworker"} but does not export it to Js Client object. This CL adds this support. BUG=705685 TEST=blink_tests ========== to ========== [ServiceWorker] Add type attribute for Client interface Specification: https://w3c.github.io/ServiceWorker/#client For Clients.{get,matchAll,openWindow}, currently the C++ impl has already been able to get the correct client type {"window","worker","sharedworker"} but does not export it to Js Client object. This CL adds this support and revises related layout tests. BUG=705685 TEST=blink_tests ==========
leon.han@intel.com changed reviewers: + shimazu@chromium.org
PTAL, Thanks! Although I've revised global-interface-listing-service-worker-expected.txt file, there're still 2 failures. I have no idea about what's the 'virtual/*' tests, so would you please also help to take a look there? Thanks! ' Unexpected Failures: * virtual/service-worker-navigation-preload-disabled/http/tests/serviceworker/webexposed/global-interface-listing-service-worker.html * virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker.html '
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, added a comment. On 2017/04/28 03:51:46, leonhsl wrote: > PTAL, Thanks! > > Although I've revised global-interface-listing-service-worker-expected.txt file, > there're still 2 failures. I have no idea about what's the 'virtual/*' tests, so > would you please also help to take a look there? Thanks! > ' > Unexpected Failures: > * > virtual/service-worker-navigation-preload-disabled/http/tests/serviceworker/webexposed/global-interface-listing-service-worker.html > * > virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker.html > ' VirtualTests are existing in //src/third_party/WebKit/LayoutTests/virtual/. Each directory in virtual/ has each setting in //src/third_party/WebKit/LayoutTests/VirtualTestSuites to set custom flags for launching the content_shell to run the test. For example, you can see //src/third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt, so tests will be green after modifying these expected files. https://codereview.chromium.org/2845963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp (right): https://codereview.chromium.org/2845963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:61: case kWebServiceWorkerClientTypeLast: I prefer kWebServiceWorkerClientTypeAll to Last to clarify what is prohibited. Just removing this case looks also good.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot for review&hints! Uploaded ps#3, PTAnL. https://codereview.chromium.org/2845963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp (right): https://codereview.chromium.org/2845963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerClient.cpp:61: case kWebServiceWorkerClientTypeLast: On 2017/05/01 06:40:35, shimazu (ooo May3-7) wrote: > I prefer kWebServiceWorkerClientTypeAll to Last to clarify what is prohibited. > Just removing this case looks also good. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm:)
leon.han@intel.com changed reviewers: + foolip@chromium.org
+foolip@ for OWNER approval of third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt, Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This CL LGTM % nits. This is a rather small addition, but can you send an Intent to Implement and Ship before landing? https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/Client.idl (right): https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/Client.idl:5: // https://w3c.github.io/ServiceWorker/#service-worker-client-interface Can you update this link to https://w3c.github.io/ServiceWorker/#client-interface? I tried following it but it's stale. https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/Client.idl:11: readonly attribute ClientType type; Please match the order of attributes in the spec. https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/Client.idl:12: readonly attribute ContextFrameType frameType; If frameType isn't in the spec, please file a bug for it and put it in a "Non-standard" section at the bottom of the interface.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot for review! I've sent out an Intent to Implement&Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/cy-jDCgnlsk Uploaded ps#5 to address comments. https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/Client.idl (right): https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/Client.idl:5: // https://w3c.github.io/ServiceWorker/#service-worker-client-interface On 2017/05/03 15:18:10, foolip wrote: > Can you update this link to > https://w3c.github.io/ServiceWorker/#client-interface? I tried following it but > it's stale. Done. https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/Client.idl:11: readonly attribute ClientType type; On 2017/05/03 15:18:10, foolip wrote: > Please match the order of attributes in the spec. Done. https://codereview.chromium.org/2845963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/Client.idl:12: readonly attribute ContextFrameType frameType; On 2017/05/03 15:18:10, foolip wrote: > If frameType isn't in the spec, please file a bug for it and put it in a > "Non-standard" section at the bottom of the interface. Done. Put frameType into a "Non-standard" section at the bottom. There is an existing bug https://crbug.com/697110 tracking the removal of frameType.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2845963002/#ps80001 (title: "Address comments from foolip@")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494392663543350,
"parent_rev": "a942138fe716b963ce8247cdcbd3082e841fd19c", "commit_rev":
"2ffd4b3b6bc3e571d2fee28955ba9bae10289a28"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Add type attribute for Client interface Specification: https://w3c.github.io/ServiceWorker/#client For Clients.{get,matchAll,openWindow}, currently the C++ impl has already been able to get the correct client type {"window","worker","sharedworker"} but does not export it to Js Client object. This CL adds this support and revises related layout tests. BUG=705685 TEST=blink_tests ========== to ========== [ServiceWorker] Add type attribute for Client interface Specification: https://w3c.github.io/ServiceWorker/#client For Clients.{get,matchAll,openWindow}, currently the C++ impl has already been able to get the correct client type {"window","worker","sharedworker"} but does not export it to Js Client object. This CL adds this support and revises related layout tests. BUG=705685 TEST=blink_tests Review-Url: https://codereview.chromium.org/2845963002 Cr-Commit-Position: refs/heads/master@{#470480} Committed: https://chromium.googlesource.com/chromium/src/+/2ffd4b3b6bc3e571d2fee28955ba... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2ffd4b3b6bc3e571d2fee28955ba...
Message was sent while issue was closed.
On 2017/05/10 05:35:52, commit-bot: I haz the power wrote: > Committed patchset #5 (id:80001) as > https://chromium.googlesource.com/chromium/src/+/2ffd4b3b6bc3e571d2fee28955ba... The test coverage seems a bit small here, it seems to only test when Client.type is "window". Can you add more tests for when the Client.type is the other values? I think you should be able to do this by augmenting existing tests. Also, can https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... now be removed?
Message was sent while issue was closed.
On 2017/05/10 06:05:57, falken wrote: > On 2017/05/10 05:35:52, commit-bot: I haz the power wrote: > > Committed patchset #5 (id:80001) as > > > https://chromium.googlesource.com/chromium/src/+/2ffd4b3b6bc3e571d2fee28955ba... > > The test coverage seems a bit small here, it seems to only test when Client.type > is "window". Can you add more tests for when the Client.type is the other > values? I think you should be able to do this by augmenting existing tests. > > Also, can > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > now be removed? chromium/clients-matchall.html chromium/clients-get-client-types.html chromium/clients-matchall-include-uncontrolled.html reference the linked bug. Do the equivalent WPT tests have test coverage for Client.type and do we pass them?
Message was sent while issue was closed.
On 2017/05/10 06:11:19, falken1 wrote: > On 2017/05/10 06:05:57, falken wrote: > > On 2017/05/10 05:35:52, commit-bot: I haz the power wrote: > > > Committed patchset #5 (id:80001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/2ffd4b3b6bc3e571d2fee28955ba... > > > > The test coverage seems a bit small here, it seems to only test when > Client.type > > is "window". Can you add more tests for when the Client.type is the other > > values? I think you should be able to do this by augmenting existing tests. > > > > Also, can > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > > now be removed? > > chromium/clients-matchall.html > chromium/clients-get-client-types.html > chromium/clients-matchall-include-uncontrolled.html > > reference the linked bug. Do the equivalent WPT tests have test coverage for > Client.type and do we pass them? Hi, falken@, could you help to clarify when will tests in LayoutTests/http/tests/serviceworker/chromium/* be executed? If there has no any bots are running them, why we're still keeping them? Thanks! And, IIUC, we'd better port the following test coverage to WPT tests and make them PASS there? If yes, I'd like to start to do so. > chromium/clients-matchall.html > chromium/clients-get-client-types.html > chromium/clients-matchall-include-uncontrolled.html
Message was sent while issue was closed.
On 2017/05/11 04:01:39, leonhsl wrote: > On 2017/05/10 06:11:19, falken1 wrote: > > On 2017/05/10 06:05:57, falken wrote: > > > On 2017/05/10 05:35:52, commit-bot: I haz the power wrote: > > > > Committed patchset #5 (id:80001) as > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/2ffd4b3b6bc3e571d2fee28955ba... > > > > > > The test coverage seems a bit small here, it seems to only test when > > Client.type > > > is "window". Can you add more tests for when the Client.type is the other > > > values? I think you should be able to do this by augmenting existing tests. > > > > > > Also, can > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > > > now be removed? > > > > chromium/clients-matchall.html > > chromium/clients-get-client-types.html > > chromium/clients-matchall-include-uncontrolled.html > > > > reference the linked bug. Do the equivalent WPT tests have test coverage for > > Client.type and do we pass them? > > Hi, falken@, could you help to clarify when will tests in > LayoutTests/http/tests/serviceworker/chromium/* be executed? > If there has no any bots are running them, why we're still keeping them? Thanks! > > And, IIUC, we'd better port the following test coverage to WPT tests and make > them PASS there? If yes, I'd like to start to do so. > > chromium/clients-matchall.html > > chromium/clients-get-client-types.html > > chromium/clients-matchall-include-uncontrolled.html Ah sure, I should have explained more. chromium/* tests are executed on all bots like normal tests. We're in the process of moving (upstreaming) tests from LayoutTests/http/tests/serviceworker to LayoutTests/external/wpt/service-workers. Sometimes while doing so we change the test to make it conform more closely to the specification text. When possible, we simply delete the test from http/tests/serviceworker and add it to external/wpt/service-workers. In some cases, the test was changed to conform more closely to the spec, and Chrome is currently failing the revised test. In these cases, we add the test to external/wpt, but also keep a version in http/tests/serviceworker/chromium/ that Chrome can pass. This was we don't lose test coverage. The tests in chromium/ should link to a crbug and explain why Chrome is failing the WPT test. In this case, the three files above are linking to the bug about Client.type. So in theory, when we implement Client.type, Chrome should start passing the corresponding WPT tests and these chromium/ tests can be deleted. However it looks like the corresponding WPT tests are not all testing Client.type thoroughly. Otherwise, I'd expect this CL to include more expected file changes for those tests. So I think what we need to do here is: - Add Client.type testing to all three WPT tests that don't have it. - Verify that the WPT test is a superset of the chromium/ tests. - Delete the chromium/ tests
Message was sent while issue was closed.
I see. Thanks a lot for the sharing and I'll start this work soon. |
