Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691903005:
ServiceWorker: Change base URL for parsing script URL and scope URL (Closed)
|
Created:
3 years, 10 months ago by jungkees Modified:
3 years, 10 months ago CC:
blink-reviews, chromium-reviews, falken+watch_chromium.org, horo+watch_chromium.org, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Change base URL for parsing script URL and scope URL
According to the spec change, this changes to use the relevant settings object
instead of the entry settings object to parse the script URL and the scope URL
given to ServiceWorkerContainer.register() and
ServiceWorkerContainer.getRegistration().
Before this CL, register() and getRegistration() used entered execution context
to parse the URLs. After this CL, the methods use the execution context
retrieved from ScriptState::getExecutionContext().
WPT: external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html
Spec issue: https://github.com/w3c/ServiceWorker/issues/922
Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581a8f2f23
BUG=691008
Review-Url: https://codereview.chromium.org/2691903005
Cr-Commit-Position: refs/heads/master@{#453033}
Committed: https://chromium.googlesource.com/chromium/src/+/255f7943ef67812af69da392fc5f62cb6f74cf96
Patch Set 1 #Patch Set 2 : Remove enteredExecutionContext and remove layout tests in favor of WPT #Patch Set 3 : Fix tests #
Total comments: 13
Patch Set 4 : Address comments on tests #Messages
Total messages: 54 (26 generated)
Description was changed from ========== Use relevant global for register() and getRegistration() According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). The layout tests are taken from WPT. Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 ========== to ========== Use relevant global for register() and getRegistration() According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). The layout tests are taken from WPT. Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 ==========
jungkee.song@samsung.com changed reviewers: + falken@chromium.org, jinho.bang@samsung.com, nhiroki@chromium.org
On 2017/02/14 11:50:16, jungkees wrote: > mailto:jungkee.song@samsung.com changed reviewers: > + mailto:falken@chromium.org, mailto:jinho.bang@samsung.com, mailto:nhiroki@chromium.org PTAL. Thanks.
shimazu@chromium.org changed reviewers: + shimazu@chromium.org
drive-by: The WPT has already been imported and will be enabled soon, so it's not necessary to include that in this patch. Imported: https://crrev.com/2697453005 Enabling: https://crrev.com/2692123003
And SWContainer seems the last user of enteredExecutionContext. Could it be removed (I'm not familiar with the bindings though)? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
Nit for the CL description: add "service worker: " or something similar so the oneline summary has better context. Can we now remove enteredExecutionContext(v8::Isolate*) completely? These were the only callsites. Yes, we shouldn't land the tests in this patch. The tests in exported/wpt should cover this. Let's wait for the WPT roll or do it manually: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat.... And then land the patch.
On 2017/02/15 00:55:59, falken wrote: > Nit for the CL description: add "service worker: " or something similar so the > oneline summary has better context. > > Can we now remove enteredExecutionContext(v8::Isolate*) completely? These were > the only callsites. Ah, shimazu@ beat me to this :) > > Yes, we shouldn't land the tests in this patch. The tests in exported/wpt should > cover this. Let's wait for the WPT roll or do it manually: > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat.... > And then land the patch.
Description was changed from ========== Use relevant global for register() and getRegistration() According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). The layout tests are taken from WPT. Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 ========== to ========== ServiceWorker: Change base URL for parsing script URL and scope URL According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). The layout tests are taken from WPT. Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 ==========
> The WPT has already been imported and will be enabled soon The imported test (https://codereview.chromium.org/2697453005/patch/10001/4978399672008704) has an issue. The patch (https://github.com/w3c/web-platform-tests/commit/d3fc8663759d7ab82795e2a1ceca...) for that issue just landed today. I don't think I'm allowed to fix the imported test manually. Should we wait for the next sync? FYI, the latest WPT upstream has exactly the same code (except the format) as the layout test in this CL currently contains. Also, the imported WPT test (third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html) should not be run as-is. Could you provide a guidance what I'm expected to do for it when the enabling CL lands? > And SWContainer seems the last user of enteredExecutionContext. Could it be removed I think we should retain that. That function matches entry settings concept in HTML spec, and HTML still uses it. That's where the spec and the implementation aren't exactly agree at the moment I think. But I guess we need it until entry settings isn't used in the spec, if it happens at all.
On 2017/02/15 01:58:41, jungkees wrote: > > The WPT has already been imported and will be enabled soon > > The imported test > (https://codereview.chromium.org/2697453005/patch/10001/4978399672008704) has an > issue. The patch > (https://github.com/w3c/web-platform-tests/commit/d3fc8663759d7ab82795e2a1ceca...) > for that issue just landed today. I don't think I'm allowed to fix the imported > test manually. Should we wait for the next sync? FYI, the latest WPT upstream > has exactly the same code (except the format) as the layout test in this CL > currently contains. > > Also, the imported WPT test > (third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html) > should not be run as-is. Could you provide a guidance what I'm expected to do > for it when the enabling CL lands? I would have suggested importing the test manually but it's broken for me: https://bugs.chromium.org/p/chromium/issues/detail?id=692335 I'd recommend waiting for the roll or trying it yourself. Once the correct test is in the repository, you can land this patch along with the change to the expectations files (LayoutTests/TestExpectations and/or url-parsing.https-expected.txt) that will verify that Chrome runs the tests and they pass. > > > And SWContainer seems the last user of enteredExecutionContext. Could it be > removed > > I think we should retain that. That function matches entry settings concept in > HTML spec, and HTML still uses it. That's where the spec and the implementation > aren't exactly agree at the moment I think. But I guess we need it until entry > settings isn't used in the spec, if it happens at all. I don't think we should keep code that is never used just because it matches something in the spec. We can also find it in source version control if we need to later. Let's remove dead code to simplify the codebase and shrink the binary size.
> Once the correct test is in the repository, you can land this patch along with the change to the expectations files Okay. Will do it that way. > We can also find it in source version control if we need to later. Let's remove dead code to simplify the codebase and shrink the binary size. Agreed. FYI. A pointer to reduce the use of entered execution context: https://github.com/whatwg/html/issues/1431
I just rolled wpt successfully. Am I supposed to include all the tests rolled into this CL or include only the tests relevant to this issue?
On 2017/02/15 08:38:30, jungkees wrote: > I just rolled wpt successfully. Am I supposed to include all the tests rolled > into this CL or include only the tests relevant to this issue? the roll should be a separate CL
On 2017/02/15 08:38:30, jungkees wrote: > I just rolled wpt successfully. Am I supposed to include all the tests rolled > into this CL or include only the tests relevant to this issue? the roll should be a separate CL
On 2017/02/15 08:42:58, falken wrote: > On 2017/02/15 08:38:30, jungkees wrote: > > I just rolled wpt successfully. Am I supposed to include all the tests rolled > > into this CL or include only the tests relevant to this issue? > > the roll should be a separate CL Okay. I'll create a separate CL for that.
I had a proxy issue while uploading a CL for the roll. Now I'm trying to roll wpt at home but encountering another error: "AssertionError: Expected SHA-1 hash, got usage: git rev-list [OPTION] <commit-id>... [ -- paths... ]" Can anyone try to run wpt-import to see if the same error occurs?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Description was changed from ========== ServiceWorker: Change base URL for parsing script URL and scope URL According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). The layout tests are taken from WPT. Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 ========== to ========== ServiceWorker: Change base URL for parsing script URL and scope URL According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). WPT: external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 ==========
I uploaded a new snapshot having addressed the comments. PTAL.
The CQ bit was checked by jungkee.song@samsung.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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/02/21 14:58:46, jungkees wrote: > I uploaded a new snapshot having addressed the comments. PTAL. Thanks. Some tests are red on the trybots: virtual/mojo-loading/http/tests/background_sync/oneshot.html http/tests/serviceworker/registration-iframe.html http/tests/serviceworker/controller-on-reload.html external/wpt/service-workers/service-worker/registration-iframe.https.html virtual/service-worker-navigation-preload/http/tests/serviceworker/controller-on-reload.html external/wpt/service-workers/service-worker/controller-on-load.https.html http/tests/background_sync/oneshot.html virtual/mojo-loading/http/tests/serviceworker/controller-on-reload.html virtual/mojo-loading/http/tests/serviceworker/registration-iframe.html external/wpt/service-workers/service-worker/multiple-register.https.html virtual/service-worker-navigation-preload/http/tests/serviceworker/registration-iframe.html external/wpt/service-workers/service-worker/controller-on-reload.https.html I suspect you need to undo the changes to the tests made here, along with some tests added after that change: https://codereview.chromium.org/835673006 So I guess this patch will need to additionally: - Fix external/wpt/service-workers/service-worker/registration-iframe.https.html - Fix http/tests/serviceworker/registration-iframe.html - Alternatively, remove http/tests/serviceworker/registration-iframe.html if it's identical to external/wpt. - Do the same for controller-on-reload.html - Fix external/wpt/service-workers/service-worker/controller-on-load.https.html - Fix external/wpt/service-workers/service-worker/multiple-register.https.html - Fix http/tests/background_sync/oneshot.html It's probably easiest if you can build manually and then run the tests: - Build blink_tests - Run third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default --no-retry-failures http/tests/serviceworker/ external/wpt/service-workers
On 2017/02/22 02:13:05, falken wrote: > On 2017/02/21 14:58:46, jungkees wrote: > > I uploaded a new snapshot having addressed the comments. PTAL. > > Thanks. Some tests are red on the trybots: > virtual/mojo-loading/http/tests/background_sync/oneshot.html > http/tests/serviceworker/registration-iframe.html > http/tests/serviceworker/controller-on-reload.html > external/wpt/service-workers/service-worker/registration-iframe.https.html > virtual/service-worker-navigation-preload/http/tests/serviceworker/controller-on-reload.html > external/wpt/service-workers/service-worker/controller-on-load.https.html > http/tests/background_sync/oneshot.html > virtual/mojo-loading/http/tests/serviceworker/controller-on-reload.html > virtual/mojo-loading/http/tests/serviceworker/registration-iframe.html > external/wpt/service-workers/service-worker/multiple-register.https.html > virtual/service-worker-navigation-preload/http/tests/serviceworker/registration-iframe.html > external/wpt/service-workers/service-worker/controller-on-reload.https.html > > I suspect you need to undo the changes to the tests made here, along with some > tests added after that change: > https://codereview.chromium.org/835673006 > > So I guess this patch will need to additionally: > - Fix external/wpt/service-workers/service-worker/registration-iframe.https.html > - Fix http/tests/serviceworker/registration-iframe.html > - Alternatively, remove http/tests/serviceworker/registration-iframe.html if > it's identical to external/wpt. > - Do the same for controller-on-reload.html > - Fix external/wpt/service-workers/service-worker/controller-on-load.https.html > - Fix external/wpt/service-workers/service-worker/multiple-register.https.html > - Fix http/tests/background_sync/oneshot.html > > It's probably easiest if you can build manually and then run the tests: > - Build blink_tests > - Run third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default > --no-retry-failures http/tests/serviceworker/ external/wpt/service-workers Thanks for the comment. We've been around a long way to relevant settings: relevant to incumbent to entry to relevant. :) I'll fix those tests. BTW, what's the usual way to fix the WPT tests? Fixing only here and exporting to WPT or fixing them in both projects (Chromium and WPT directly)?
On 2017/02/22 05:42:22, jungkees wrote: > On 2017/02/22 02:13:05, falken wrote: > > On 2017/02/21 14:58:46, jungkees wrote: > > > I uploaded a new snapshot having addressed the comments. PTAL. > > > > Thanks. Some tests are red on the trybots: > > virtual/mojo-loading/http/tests/background_sync/oneshot.html > > http/tests/serviceworker/registration-iframe.html > > http/tests/serviceworker/controller-on-reload.html > > external/wpt/service-workers/service-worker/registration-iframe.https.html > > > virtual/service-worker-navigation-preload/http/tests/serviceworker/controller-on-reload.html > > external/wpt/service-workers/service-worker/controller-on-load.https.html > > http/tests/background_sync/oneshot.html > > virtual/mojo-loading/http/tests/serviceworker/controller-on-reload.html > > virtual/mojo-loading/http/tests/serviceworker/registration-iframe.html > > external/wpt/service-workers/service-worker/multiple-register.https.html > > > virtual/service-worker-navigation-preload/http/tests/serviceworker/registration-iframe.html > > external/wpt/service-workers/service-worker/controller-on-reload.https.html > > > > I suspect you need to undo the changes to the tests made here, along with some > > tests added after that change: > > https://codereview.chromium.org/835673006 > > > > So I guess this patch will need to additionally: > > - Fix > external/wpt/service-workers/service-worker/registration-iframe.https.html > > - Fix http/tests/serviceworker/registration-iframe.html > > - Alternatively, remove http/tests/serviceworker/registration-iframe.html if > > it's identical to external/wpt. > > - Do the same for controller-on-reload.html > > - Fix > external/wpt/service-workers/service-worker/controller-on-load.https.html > > - Fix external/wpt/service-workers/service-worker/multiple-register.https.html > > - Fix http/tests/background_sync/oneshot.html > > > > It's probably easiest if you can build manually and then run the tests: > > - Build blink_tests > > - Run third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Default > > --no-retry-failures http/tests/serviceworker/ external/wpt/service-workers > > Thanks for the comment. We've been around a long way to relevant settings: > relevant to incumbent to entry to relevant. :) I'll fix those tests. BTW, what's > the usual way to fix the WPT tests? Fixing only here and exporting to WPT or > fixing them in both projects (Chromium and WPT directly)? We can commit to external/wpt directly in the Chromium repository, and it will be automatically synced with WPT repository.
> We can commit to external/wpt directly in the Chromium repository, and it will > be automatically synced with WPT repository. Okay. Great.
The CQ bit was checked by jungkee.song@samsung.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...
> So I guess this patch will need to additionally: > - Fix external/wpt/service-workers/service-worker/registration-iframe.https.html > - Fix http/tests/serviceworker/registration-iframe.html > - Alternatively, remove http/tests/serviceworker/registration-iframe.html if > it's identical to external/wpt. Removed http/tests/serviceworker/registration-iframe.html as it's identical to wpt except a word in comment. Corrected the word in wpt file. > - Do the same for controller-on-reload.html > - Fix external/wpt/service-workers/service-worker/controller-on-load.https.html > - Fix external/wpt/service-workers/service-worker/multiple-register.https.html > - Fix http/tests/background_sync/oneshot.html I uploaded a new snapshot having addressed the above points. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, looking good. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:2146: crbug.com/626703 external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html [ Pass ] Remove this line, [ Pass ] expectations are the same as no expectation. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/controller-on-reload.https.html (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/controller-on-reload.https.html:9: var scope = 'resources/blank.html'; Could we avoid repeating 'blank.html' three times in this file? Maybe use: const iframe_scope = 'blank.html'; const scope = 'resources/' + iframe_scope; https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html (left): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:10: // document's url. I actually liked these comments explaining each test. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:42: 'scope - normal case'); maybe s/relevant global/the "relevant global object"/ to be more clear that this is a precise spec concept and not just the everyday usage of "relevant". https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:72: 'scope - error case'); ditto https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html:30: return w.navigator.serviceWorker.getRegistration('oneshot.html'); I prefer to not repeat the 'oneshot.html' string. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (left): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:784: Thanks for deleting this.
Thanks for the comments. I uploaded a new snapshot having addressed them. PTAL. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:2146: crbug.com/626703 external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html [ Pass ] On 2017/02/24 06:49:27, falken wrote: > Remove this line, [ Pass ] expectations are the same as no expectation. Done. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/controller-on-reload.https.html (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/controller-on-reload.https.html:9: var scope = 'resources/blank.html'; On 2017/02/24 06:49:27, falken wrote: > Could we avoid repeating 'blank.html' three times in this file? Maybe use: > > const iframe_scope = 'blank.html'; > const scope = 'resources/' + iframe_scope; Yes. Addressed. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html (left): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:10: // document's url. On 2017/02/24 06:49:27, falken wrote: > I actually liked these comments explaining each test. Okay. I thought it was a bit redundant to the test description. I added it back with the necessary correction. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:42: 'scope - normal case'); On 2017/02/24 06:49:27, falken wrote: > maybe s/relevant global/the "relevant global object"/ to be more clear that this > is a precise spec concept and not just the everyday usage of "relevant". Yes. Addressed. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:72: 'scope - error case'); On 2017/02/24 06:49:27, falken wrote: > ditto Done. https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html (right): https://codereview.chromium.org/2691903005/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html:30: return w.navigator.serviceWorker.getRegistration('oneshot.html'); On 2017/02/24 06:49:27, falken wrote: > I prefer to not repeat the 'oneshot.html' string. Done.
The CQ bit was checked by jungkee.song@samsung.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! thanks.
jungkee.song@samsung.com changed reviewers: + haraken@chromium.org, jochen@chromium.org
On 2017/02/24 13:46:22, falken wrote: > lgtm! thanks. Thanks for the review, falken@! jochen@, haraken@, Could you review V8Binding.*?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
LGTM
On 2017/02/25 00:11:15, haraken wrote: > LGTM Thanks for the review!
The CQ bit was checked by jungkee.song@samsung.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": 160001, "attempt_start_ts": 1487984102720120,
"parent_rev": "f40832ec30f1e19f28ca4dfab9e24d608fcefa28", "commit_rev":
"255f7943ef67812af69da392fc5f62cb6f74cf96"}
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Change base URL for parsing script URL and scope URL According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). WPT: external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 ========== to ========== ServiceWorker: Change base URL for parsing script URL and scope URL According to the spec change, this changes to use the relevant settings object instead of the entry settings object to parse the script URL and the scope URL given to ServiceWorkerContainer.register() and ServiceWorkerContainer.getRegistration(). Before this CL, register() and getRegistration() used entered execution context to parse the URLs. After this CL, the methods use the execution context retrieved from ScriptState::getExecutionContext(). WPT: external/wpt/service-workers/service-worker/multi-globals/url-parsing.https.html Spec issue: https://github.com/w3c/ServiceWorker/issues/922 Spec change: https://github.com/w3c/ServiceWorker/commit/ec1aac220646a5ea15583e22da1373581... BUG=691008 Review-Url: https://codereview.chromium.org/2691903005 Cr-Commit-Position: refs/heads/master@{#453033} Committed: https://chromium.googlesource.com/chromium/src/+/255f7943ef67812af69da392fc5f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/chromium/src/+/255f7943ef67812af69da392fc5f... |
