Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
835673006:
Use caller's document url to resolve scriptURL/patternURL in registerServiceWorker/getRegistration (Closed)
|
Created:
5 years, 11 months ago by jungkees Modified:
5 years, 9 months ago CC:
blink-reviews, horo+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionService Worker: Use the caller's context to resolve URLs passed to register().
The scriptURL and patternURL passed to
ServiceWorkerContainer.registerServiceWorker/getRegistration should be resolved
relative to the caller's document url rather than the ServiceWorkerContainer's.
Corresponding AUTHORS change: https://codereview.chromium.org/851103006/
BUG=449422
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191059
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use calling frame's document's url to parse script url and scope url; Add tests #Patch Set 3 : Remove unnecessary forward class declaration - class DOMWindow; in NavigatorServiceWorker.h #
Total comments: 10
Patch Set 4 : Always use calling document's URL as a base URL for parsing script URL and scope URL. #Patch Set 5 : Use callingExecutionContext(scriptState->isolate()) instead of callingDOMWindow(scriptState->isolat… #Patch Set 6 : Fixed two related tests which are using iframe #
Total comments: 6
Patch Set 7 : Address comments for registration-iframe.html and rebase #
Total comments: 4
Patch Set 8 : Address comments in registration-iframe.html #
Messages
Total messages: 38 (6 generated)
jungkee.song@samsung.com changed reviewers: + dominicc@chromium.org, slightlyoff@chromium.org
Please review my first Service Worker patch! :-)
Hi Jungkees, thanks for your contributions on the spec side to date; it's exciting to see you contribute to the implementation of Service Worker in Chromium. Welcome! Please add a test in LayoutTests/http/tests/serviceworker and include it in this patch. You can probably test this by creating in iframe which has the same origin but a different path to the outer iframe, and check some conditions that depend on the path. For now this not LGTM, see my comments inline. https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... File Source/modules/serviceworkers/NavigatorServiceWorker.h (right): https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... Source/modules/serviceworkers/NavigatorServiceWorker.h:40: RefPtrWillBeMember<DOMWindow> m_domWindow; Could this cause a leak by creating a cycle including the window? I think the correct thing to do here is access the context through frame(). https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:120: // FIXME: This should use the container's execution context, not This FIXME is essentially what you're fixing here, so delete this. Same comment below. https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... File Source/modules/serviceworkers/ServiceWorkerContainer.h (right): https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... Source/modules/serviceworkers/ServiceWorkerContainer.h:100: RawPtrWillBeMember<ExecutionContext> m_executionContext; Likewise, does this potentially cause a cycle? Because ServiceWorkerContainer is a ContextLifecycleObserver, can it simply retrieve the execution context with executionContext()? ContextLifecycleObserver does not create leaks because executionContext() is reset to null when the ExecutionContext is dying.
On 2015/01/19 03:19:08, dominicc wrote: > Hi Jungkees, thanks for your contributions on the spec side to date; it's > exciting to see you contribute to the implementation of Service Worker in > Chromium. Welcome! > > Please add a test in LayoutTests/http/tests/serviceworker and include it in this > patch. You can probably test this by creating in iframe which has the same > origin but a different path to the outer iframe, and check some conditions that > depend on the path. > > For now this not LGTM, see my comments inline. > > https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... > File Source/modules/serviceworkers/NavigatorServiceWorker.h (right): > > https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... > Source/modules/serviceworkers/NavigatorServiceWorker.h:40: > RefPtrWillBeMember<DOMWindow> m_domWindow; > Could this cause a leak by creating a cycle including the window? I think the > correct thing to do here is access the context through frame(). > > https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): > > https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:120: // FIXME: This > should use the container's execution context, not > This FIXME is essentially what you're fixing here, so delete this. Same comment > below. > > https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... > File Source/modules/serviceworkers/ServiceWorkerContainer.h (right): > > https://codereview.chromium.org/835673006/diff/1/Source/modules/serviceworker... > Source/modules/serviceworkers/ServiceWorkerContainer.h:100: > RawPtrWillBeMember<ExecutionContext> m_executionContext; > Likewise, does this potentially cause a cycle? Because ServiceWorkerContainer is > a ContextLifecycleObserver, can it simply retrieve the execution context with > executionContext()? > > ContextLifecycleObserver does not create leaks because executionContext() is > reset to null when the ExecutionContext is dying. Thanks Dominic. It gets much simpler with your comment. The only change from the original code would be to use the result of executionContext() of the container which is already associated with the navigator object it's gotten from. After writing the related tests, I'll upload the new patch. FYI, https://codereview.chromium.org/851103006/ depends on this issue.
> After writing the related tests, I'll upload the new patch. Thanks! > FYI, https://codereview.chromium.org/851103006/ depends on this issue. Noted.
On 2015/01/20 04:04:20, dominicc wrote: > > After writing the related tests, I'll upload the new patch. > > Thanks! > > > FYI, https://codereview.chromium.org/851103006/ depends on this issue. > > Noted. - Additionally noticed that register/getRegistration methods had been using container's document's url to parse the scriptURL and scope. So, I changed them to use calling frame's document's url for parsing them. - Added relevant tests * For the security origin check, I double-checked it uses container's execution context rather than caller's execution context. For the cross-origin iframe case, I also checked it out using related logs on chrome.exe with --disable-web-security option.
On 2015/01/21 13:02:06, jungkees wrote: > On 2015/01/20 04:04:20, dominicc wrote: > > > After writing the related tests, I'll upload the new patch. > > > > Thanks! > > > > > FYI, https://codereview.chromium.org/851103006/ depends on this issue. > > > > Noted. > > - Additionally noticed that register/getRegistration methods had been using > container's document's url to parse the scriptURL and scope. So, I changed them > to use calling frame's document's url for parsing them. > - Added relevant tests > > * For the security origin check, I double-checked it uses container's execution > context rather than caller's execution context. For the cross-origin iframe > case, I also checked it out using related logs on chrome.exe with > --disable-web-security option. Friendly ping.
Sorry for the slow reply; I was travelling. Back now. https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-iframe.html:78: // Set scope url such that it does not start with script url This seems imprecise; the scope URL has to be a prefix of the script URL up to the last slash, etc.; this comment makes it sound as though the script URL is a prefix of the scope URL. https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:120: ExecutionContext* executionContext = scriptState->executionContext(); I'm still not sure this is the right context. Which context is this? For example if in frame A you do frameB.contentWindow.ServiceWorkerContainer.prototype.register.call(frameC.navigator.serviceWorker, ...) which one is used? (I think that the one we want is "C", BTW.) https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:136: KURL patternURL = callingDocument ? callingDocument->completeURL(options.scope()) : executionContext->completeURL(options.scope()); I don't think this in right; I think if from frame A we do frameB.contentWindow.navigator.serviceWorker.register then the URL should be resolved against frame B's location. For example look at how LocalDOMWindow::open (which corresponds to Window.open) uses the entered document to complete the URL, not the calling document. I'd be surprised if register worked differently, but maybe you can point me to specs? Am I missing something?
On 2015/02/09 07:31:48, dominicc wrote: > Sorry for the slow reply; I was travelling. Back now. > > https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... > File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): > > https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... > LayoutTests/http/tests/serviceworker/registration-iframe.html:78: // Set scope > url such that it does not start with script url > This seems imprecise; the scope URL has to be a prefix of the script URL up to > the last slash, etc.; this comment makes it sound as though the script URL is a > prefix of the scope URL. > > https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): > > https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:120: ExecutionContext* > executionContext = scriptState->executionContext(); > I'm still not sure this is the right context. Which context is this? For example > if in frame A you do > > frameB.contentWindow.ServiceWorkerContainer.prototype.register.call(frameC.navigator.serviceWorker, > ...) > > which one is used? (I think that the one we want is "C", BTW.) > > https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:136: KURL patternURL = > callingDocument ? callingDocument->completeURL(options.scope()) : > executionContext->completeURL(options.scope()); > I don't think this in right; I think if from frame A we do > > frameB.contentWindow.navigator.serviceWorker.register > > then the URL should be resolved against frame B's location. For example look at > how LocalDOMWindow::open (which corresponds to Window.open) uses the entered > document to complete the URL, not the calling document. I'd be surprised if > register worked differently, but maybe you can point me to specs? Am I missing > something? Oh I see. Thanks for the comments. I'll follow up on them.
https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-iframe.html:78: // Set scope url such that it does not start with script url On 2015/02/09 07:31:48, dominicc wrote: > This seems imprecise; the scope URL has to be a prefix of the script URL up to > the last slash, etc.; this comment makes it sound as though the script URL is a > prefix of the scope URL. It seems the script URL (up to the last slash) being a prefix of the scope URL is right. E.g.: - script: "/resources/sw.js" - scope: "/resources/foo/" This is a valid setting where we can say "The scope URL starts with the script URL (up to the last slash)". Am I confused? https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:120: ExecutionContext* executionContext = scriptState->executionContext(); On 2015/02/09 07:31:48, dominicc wrote: > I'm still not sure this is the right context. Which context is this? For example > if in frame A you do > > frameB.contentWindow.ServiceWorkerContainer.prototype.register.call(frameC.navigator.serviceWorker, > ...) > > which one is used? (I think that the one we want is "C", BTW.) Having double-checked it, it's basically the container's context (not the caller's context). I.e. for frameB.contentWindow.navigator.serviceWorker.register(), frameB's context is being used. For the JS function's call method case as in your example, frameB's context is also used, though. The ScriptState object (related to the context) is set to the isolate's current context in the auto generated V8 binding code like the following: ScriptState* scriptState = ScriptState::current(info.GetIsolate()); ScriptPromise result = impl->registerServiceWorker(scriptState, url, options); If frameC's context is expected, do you think we better file a separate issue for this? https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:136: KURL patternURL = callingDocument ? callingDocument->completeURL(options.scope()) : executionContext->completeURL(options.scope()); On 2015/02/09 07:31:48, dominicc wrote: > I don't think this in right; I think if from frame A we do > > frameB.contentWindow.navigator.serviceWorker.register > > then the URL should be resolved against frame B's location. For example look at > how LocalDOMWindow::open (which corresponds to Window.open) uses the entered > document to complete the URL, not the calling document. I'd be surprised if > register worked differently, but maybe you can point me to specs? Am I missing > something? This is not register specific, but HTML specifies this. I think this pointer explains: https://html.spec.whatwg.org/multipage/webappapis.html#entry-settings-object. See the example snippet just below the definition of entry settings object and incumbent settings objects.
https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-iframe.html:78: // Set scope url such that it does not start with script url On 2015/02/13 at 08:12:37, jungkees wrote: > Am I confused? No, but read the comment. It says: "Set scope url such that it does not start with script url" In fact, no *valid* scope URL starts with a script URL. So setting the scope URL to something that does not start with the script URL doesn't necessarily make in invalid. https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:136: KURL patternURL = callingDocument ? callingDocument->completeURL(options.scope()) : executionContext->completeURL(options.scope()); On 2015/02/13 at 08:12:38, jungkees wrote: > This is not register specific, but HTML specifies this. I think this pointer explains: https://html.spec.whatwg.org/multipage/webappapis.html#entry-settings-object. See the example snippet just below the definition of entry settings object and incumbent settings objects. Thanks for the link, that does look like the definitive thing. Where is the spec for using the current execution context when the calling document is null? What's the precedent for that?
https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/registration-iframe.html:78: // Set scope url such that it does not start with script url How about "Set the scope URL to a non-subdirectory of the script URL."? Does it make sense? https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... Source/modules/serviceworkers/ServiceWorkerContainer.cpp:136: KURL patternURL = callingDocument ? callingDocument->completeURL(options.scope()) : executionContext->completeURL(options.scope()); On 2015/02/16 06:54:45, dominicc wrote: > Where is the spec for using the current execution context when the calling > document is null? What's the precedent for that? My intention with checking whether the callingDocument be null was just to set a guard to not dereference the value when null is returned. So did I assume the callingDocument in this code refers to the iframe's document when it exists. If a document doesn't have any iframe and the method is called withing the context, shouldn't it use the current execution context by default? Please let me know if I'm missing or overlooking any basic stuff here.
On 2015/02/16 at 09:06:11, jungkee.song wrote: > https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... > File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): > > https://codereview.chromium.org/835673006/diff/40001/LayoutTests/http/tests/s... > LayoutTests/http/tests/serviceworker/registration-iframe.html:78: // Set scope url such that it does not start with script url > How about "Set the scope URL to a non-subdirectory of the script URL."? Does it make sense? > > https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right): > > https://codereview.chromium.org/835673006/diff/40001/Source/modules/servicewo... > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:136: KURL patternURL = callingDocument ? callingDocument->completeURL(options.scope()) : executionContext->completeURL(options.scope()); > On 2015/02/16 06:54:45, dominicc wrote: > > Where is the spec for using the current execution context when the calling > > document is null? What's the precedent for that? > > My intention with checking whether the callingDocument be null was just to set a guard to not dereference the value when null is returned. So did I assume the callingDocument in this code refers to the iframe's document when it exists. If a document doesn't have any iframe and the method is called withing the context, shouldn't it use the current execution context by default? Please let me know if I'm missing or overlooking any basic stuff here. Dereferencing null and crashing is wrong, so it is good to not do that. But I'm not sure if you should use the current execution context instead. Does anything else work that way? Does a spec say to do that?
On 2015/02/17 01:21:58, dominicc wrote: > Dereferencing null and crashing is wrong, so it is good to not do that. Fixed it as commented. > But I'm > not sure if you should use the current execution context instead. Does anything > else work that way? Does a spec say to do that? I had lost the conversation context indeed. Having double-checked the result of the execution, using a calling document's URL as a base URL always return the correct result for parsing scope URL and the script URL.
In order to consider worker clients, it'd make more sense to use callingExecutionContext(scriptState->isolate()) instead of using callingDOMWindow(scriptState->isolate())->document().
On 2015/02/17 08:57:32, jungkees wrote: > In order to consider worker clients, it'd make more sense to use > callingExecutionContext(scriptState->isolate()) instead of using > callingDOMWindow(scriptState->isolate())->document(). I built and fully tested this change through the Layout test file.
Two regression errors are found from the tests that are using iframe. Fixed and included the files in the patch set as such.
On 2015/02/17 at 09:34:46, jungkee.song wrote: > Two regression errors are found from the tests that are using iframe. Fixed and included the files in the patch set as such. LGTM
The CQ bit was checked by dominicc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/835673006/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...) android_blink_compile_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
falken@chromium.org changed reviewers: + falken@chromium.org
the CL description seems out of date, can you update it for the patch you're landing? https://codereview.chromium.org/835673006/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): https://codereview.chromium.org/835673006/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:8: // Set script url and scope url relative to the calling frame's document's url nit: please end sentences with '.' https://codereview.chromium.org/835673006/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:12: var step = t.step_func.bind(t); I believe the promise-chain means we don't need to step() everything. Other SW tests don't use step. https://codereview.chromium.org/835673006/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:68: .catch(step(function(e) { I don't think this works-- if register succeeds, the test will still pass. Use the two-arg then(p, q) and in q test the exception rather than a catch. https://codereview.chromium.org/835673006/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:84: var parsed_scope = normalizeURL(scope); nit: unused variable https://codereview.chromium.org/835673006/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:86: var parsed_script = normalizeURL(script); unused variable https://codereview.chromium.org/835673006/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:99: assert_equals(e.name, 'SecurityError'); Test this in the two-arg then(p, q) also.
On 2015/02/26 07:16:17, falken wrote: > the CL description seems out of date, can you update it for the patch you're > landing? > Updated. Thanks for comments. I'll upload a new patch set when done with the build and the test.
Also regarding the CLA, can you let me know about when you signed it? I see your name on an old list that I'm not sure is still valid (asking internally now). Regardless, can you put the link to https://codereview.chromium.org/851103006/ in the CL description of this issue, and vice versa?
On 2015/02/27 01:51:57, falken wrote: > Also regarding the CLA, can you let me know about when you signed it? I see your > name on an old list that I'm not sure is still valid (asking internally now). I've confirmed now that you've signed. Thanks! > Regardless, can you put the link to https://codereview.chromium.org/851103006/ > in the CL description of this issue, and vice versa? ^^ please still do this, thanks.
On 2015/02/27 02:14:02, falken wrote: > On 2015/02/27 01:51:57, falken wrote: > > > Regardless, can you put the link to https://codereview.chromium.org/851103006/ > > in the CL description of this issue, and vice versa? > > ^^ please still do this, thanks. Done it. And upload a new patch set. :-)
On 2015/02/27 02:21:00, jungkees wrote: > On 2015/02/27 02:14:02, falken wrote: > > On 2015/02/27 01:51:57, falken wrote: > > > > > Regardless, can you put the link to > https://codereview.chromium.org/851103006/ > > > in the CL description of this issue, and vice versa? > > > > ^^ please still do this, thanks. > > Done it. And upload a new patch set. :-) @falken, I'm not accustomed to the process and the rule yet. Should I go ahead with the commit based on @dominicc's LGTM? Or should I wait until the reviewer does it after reviewing the patch set for the additional comments?
On 2015/02/27 05:52:04, jungkees wrote: > On 2015/02/27 02:21:00, jungkees wrote: > > On 2015/02/27 02:14:02, falken wrote: > > > On 2015/02/27 01:51:57, falken wrote: > > > > > > > Regardless, can you put the link to > > https://codereview.chromium.org/851103006/ > > > > in the CL description of this issue, and vice versa? > > > > > > ^^ please still do this, thanks. > > > > Done it. And upload a new patch set. :-) > > @falken, I'm not accustomed to the process and the rule yet. Should I go ahead > with the commit based on @dominicc's LGTM? Or should I wait until the reviewer > does it after reviewing the patch set for the additional comments? Sorry for the delay. Please let me or dominicc take another look before committing. I plan to look later today.
On 2015/02/27 05:58:53, falken wrote: > On 2015/02/27 05:52:04, jungkees wrote: > > On 2015/02/27 02:21:00, jungkees wrote: > > > On 2015/02/27 02:14:02, falken wrote: > > > > On 2015/02/27 01:51:57, falken wrote: > > > > > > > > > Regardless, can you put the link to > > > https://codereview.chromium.org/851103006/ > > > > > in the CL description of this issue, and vice versa? > > > > > > > > ^^ please still do this, thanks. > > > > > > Done it. And upload a new patch set. :-) > > > > @falken, I'm not accustomed to the process and the rule yet. Should I go ahead > > with the commit based on @dominicc's LGTM? Or should I wait until the reviewer > > does it after reviewing the patch set for the additional comments? > > Sorry for the delay. Please let me or dominicc take another look before > committing. I plan to look later today. Great. Thanks!
lgtm with nits It's a bit more orthodox to move the "related issue" (which is a bit vague) above the BUG= line and wrap CL description lines to around 80 characters (yet in a way that the first line is a summary of the change). So something like: Service Worker: Use the caller's context to resolve URLs passed to register(). The scriptURL and patternURL passed to ServiceWorkerContainer.registerServiceWorker/getRegistration should be resolved relative to the caller's document url rather than the ServiceWorkerContainer's. Corresponding AUTHORS change: https://codereview.chromium.org/851103006/ BUG=449422 https://codereview.chromium.org/835673006/diff/120001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/registration-iframe.html (right): https://codereview.chromium.org/835673006/diff/120001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:46: 'document\'s url as a base url for parsing its script url and scope url - ' + nit: this ' should be lined up with the ' above (and throughout this file) https://codereview.chromium.org/835673006/diff/120001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:66: .then(unreached_rejection(t), We want to assert the opposite. This should be assert_unreached with a message like 'register() should reject.' https://codereview.chromium.org/835673006/diff/120001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:67: function(e) { See http/tests/serviceworker/register-default-scope.html for an example of how to format the two-arg then(). There's also an example at http://www.chromium.org/blink/serviceworker/testing https://codereview.chromium.org/835673006/diff/120001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/registration-iframe.html:94: .then(unreached_rejection(t), Same here this should be assert_unreached.
On 2015/02/27 12:30:03, falken wrote: > lgtm with nits > Thank you so much for the review in detail! Uploaded a new patch set.
still lgtm, thanks
The CQ bit was checked by falken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominicc@chromium.org Link to the patchset: https://codereview.chromium.org/835673006/#ps140001 (title: "Address comments in registration-iframe.html")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/835673006/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191059 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
