Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1604893002:
ServiceWorker: Rewrite windowclient-navigate.html test. (Closed)
|
Created:
4 years, 11 months ago by zino Modified:
4 years, 10 months ago Reviewers:
nhiroki CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, falken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Rewrite windowclient-navigate.html test.
This CL splits the test into each test case to find a failure reason easily.
Also, adds a test case for in-scope-but-not-controlled to the test.
The test case comes from the spec dicussion[1] and the comment[2] in other CL.
[1] https://github.com/slightlyoff/ServiceWorker/issues/752
[2] https://codereview.chromium.org/1481753002/#msg8
BUG=567049
Committed: https://crrev.com/47a728652bd829f25edff357b5a640f9c78c328f
Cr-Commit-Position: refs/heads/master@{#375629}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rewrite test #Patch Set 3 : Fixed bot error #
Total comments: 1
Patch Set 4 : #
Total comments: 21
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Total comments: 6
Patch Set 7 : nit #
Messages
Total messages: 29 (8 generated)
jinho.bang@samsung.com changed reviewers: + nhiroki@chromium.org
PTAL. I wasn't sure whether it's better to separate the case from original tests or not. So, I just added the test case to original tests. What do you think? Thank you.
On 2016/01/19 13:00:42, zino wrote: > PTAL. > > I wasn't sure whether it's better to separate the case from original tests or > not. > So, I just added the test case to original tests. What do you think? > > Thank you. Either is ok. (Generally I'd prefer that each case has its own async/promise_test function so that we can easily add an edge case and investigate a failure reason. That said, all existing cases are squashed into one test function and making consistent with them would be also reasonable choice.)
https://codereview.chromium.org/1604893002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:9: in_scope_but_not_controlled = e.name.toString(); This might not work because there should be only an "out-of-scope" client (ie. "windowclient-navigate.html") on install phase. Maybe we need to create an in-scope client before the worker is activated and test it in the same way as other cases.
Description was changed from ========== ServiceWorker: Add a test case for in-scope-but-not-controlled-client. Add a test case for in-scope-but-not-controlled in windowclient-navigate.html. This CL comes from the spec discussion[1] and the review comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 ========== to ========== ServiceWorker: Rewrite windowclient-navigate.html test. This CL splits the test into each test case to find a failure reason easily. Also, adds a test case for in-scope-but-not-controlled to the test. The test case comes from the spec dicussion[1] and the comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 ==========
Description was changed from ========== ServiceWorker: Rewrite windowclient-navigate.html test. This CL splits the test into each test case to find a failure reason easily. Also, adds a test case for in-scope-but-not-controlled to the test. The test case comes from the spec dicussion[1] and the comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 ========== to ========== ServiceWorker: Rewrite windowclient-navigate.html test. This CL splits the test into each test case to find a failure reason easily. Also, adds a test case for in-scope-but-not-controlled to the test. The test case comes from the spec dicussion[1] and the comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 ==========
On 2016/01/20 03:56:22, nhiroki wrote: > (Generally I'd prefer that each case has its own async/promise_test function so > that we can easily add an edge case and investigate a failure reason. That said, > all existing cases are squashed into one test function and making consistent > with them would be also reasonable choice.) I agree with your former opinion. I thought that it's a kind of convension for service worker test. So, I rewrited the test. Thank you for input!
I uploaded a new patch set. Could you review again? Thank you! https://codereview.chromium.org/1604893002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:9: in_scope_but_not_controlled = e.name.toString(); On 2016/01/20 03:56:34, nhiroki wrote: > This might not work because there should be only an "out-of-scope" client (ie. > "windowclient-navigate.html") on install phase. > > Maybe we need to create an in-scope client before the worker is activated and > test it in the same way as other cases. Good catch!
Hi, Hiroki I fixed the trybot error. Could you review again?
https://codereview.chromium.org/1604893002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1604893002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:96: return navigator.serviceWorker.register(SCRIPT_URL, { scope: SCOPE }); On second thoughts, this seems strange. The iframe page is uncontrolled but there is no service worker in the frame. Sorry, I'll fixed this test again.
I've just uploaded a new patch set. Could you review it? Thank you!
https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js (right): https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js:2: importScripts('windowclient-navigate-worker.js'); I think importing non-library file (other worker file) would not be really maintainable and side-effect is likely to slip in. For example, get_message/client_navigate might be called twice at line 3 in this file and at line 1 in windowclient-navigate-worker.js. I guess this is not expected behavior (right?). Can we merge this event handler into "windowclient-navigate-worker.js" and execute it when an optional query parameter is specified? For example, if script url "windowclient-navigate-worker.js" has a query parameter "navigate-on-install", run client_navigate in the install event. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js:3: e.waitUntil(get_message().then(client_navigate)); Let me confirm... you mean to test the case where a non-active worker attempts to navigate an in-scope client. Is this right? Can we also test the case where an active worker attempts to navigate an in-scope-but-not-controlled-yet client? https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:1: get_message().then(client_navigate); This worker can be terminated before "client_navigate" is completed. Unfortunately, Chrome's MessageEvent is not extendable yet (http://crbug.com/543198), so we might need to run "client_navigate" in other extendable event like ActivateEvent until the issue is fixed. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:5: self.addEventListener('message', function(e) { "self.addEventListener('message', resolve)" https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:13: console.log(e); Can you remove this? https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:22: expected: FULL_SCOPE_URL + '?navigate', normalizeURL() defined in http/tests/serviceworker/resources/test-helpers.js might work? normalizeURL(SCRIPT_URL) + '?navigate' https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:36: script_url: 'resources/navigate-on-install-worker.js' SCRIPT_URL_ON_INSTALL https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:47: description : 'cross orgin url test', There is a whitespace before ':'. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:85: script_url : SCRIPT_URL ditto(whitespaces) https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:119: .catch(unreached_rejection(test)); Can you remove this 'cache' because 'promise_test' internally handles an exception?
PTAL. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js (right): https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js:2: importScripts('windowclient-navigate-worker.js'); On 2016/01/25 09:50:31, nhiroki wrote: > I think importing non-library file (other worker file) would not be really > maintainable and side-effect is likely to slip in. For example, > get_message/client_navigate might be called twice at line 3 in this file and at > line 1 in windowclient-navigate-worker.js. I guess this is not expected behavior > (right?). > > Can we merge this event handler into "windowclient-navigate-worker.js" and > execute it when an optional query parameter is specified? For example, if script > url "windowclient-navigate-worker.js" has a query parameter > "navigate-on-install", run client_navigate in the install event. Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js:3: e.waitUntil(get_message().then(client_navigate)); On 2016/01/25 09:50:31, nhiroki wrote: > Let me confirm... you mean to test the case where a non-active worker attempts > to navigate an in-scope client. Is this right? Can we also test the case where > an active worker attempts to navigate an in-scope-but-not-controlled-yet client? I tried it but I couldn't yet. I'm not sure but there is some bug when using e.waitUntil in activate event. I'll upload a minified test script to another CL soon. I need your help. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:1: get_message().then(client_navigate); On 2016/01/25 09:50:31, nhiroki wrote: > This worker can be terminated before "client_navigate" is completed. > > Unfortunately, Chrome's MessageEvent is not extendable yet > (http://crbug.com/543198), so we might need to run "client_navigate" in other > extendable event like ActivateEvent until the issue is fixed. Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:5: self.addEventListener('message', function(e) { On 2016/01/25 09:50:31, nhiroki wrote: > "self.addEventListener('message', resolve)" Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:13: console.log(e); On 2016/01/25 09:50:31, nhiroki wrote: > Can you remove this? Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:22: expected: FULL_SCOPE_URL + '?navigate', On 2016/01/25 09:50:31, nhiroki wrote: > normalizeURL() defined in http/tests/serviceworker/resources/test-helpers.js > might work? > > normalizeURL(SCRIPT_URL) + '?navigate' Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:36: script_url: 'resources/navigate-on-install-worker.js' On 2016/01/25 09:50:31, nhiroki wrote: > SCRIPT_URL_ON_INSTALL Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:47: description : 'cross orgin url test', On 2016/01/25 09:50:31, nhiroki wrote: > There is a whitespace before ':'. Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:85: script_url : SCRIPT_URL On 2016/01/25 09:50:31, nhiroki wrote: > ditto(whitespaces) Done. https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:119: .catch(unreached_rejection(test)); On 2016/01/25 09:50:31, nhiroki wrote: > Can you remove this 'cache' because 'promise_test' internally handles an > exception? Done.
https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js (right): https://codereview.chromium.org/1604893002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigate-on-install-worker.js:3: e.waitUntil(get_message().then(client_navigate)); On 2016/01/29 05:36:38, zino wrote: > On 2016/01/25 09:50:31, nhiroki wrote: > > Let me confirm... you mean to test the case where a non-active worker attempts > > to navigate an in-scope client. Is this right? Can we also test the case where > > an active worker attempts to navigate an in-scope-but-not-controlled-yet > client? > > I tried it but I couldn't yet. > I'm not sure but there is some bug when using e.waitUntil in activate event. > I'll upload a minified test script to another CL soon. I need your help. Commented in the patchset 5. https://codereview.chromium.org/1604893002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:49: get_test_data().then(navigate_test); Hmmm... there could be deadlock. e.waitUntil(get_test_data()) waits for a message from the test html and the 'activate' event does not finish until the message arrives. On the other hand, the test html waits for that the worker is activated, and it never happens because the worker cannot finish the 'activate' event. Possible solutions would be... (1) using FetchEvent to wait for the completion of navigate() instead of ActivateEvent, or (2) deferring implementing this test until ExtendableMessageEvent is implemented (http://crbug.com/543198)
https://codereview.chromium.org/1604893002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:49: get_test_data().then(navigate_test); On 2016/02/01 07:13:55, nhiroki wrote: > Hmmm... there could be deadlock. > > e.waitUntil(get_test_data()) waits for a message from the test html and the > 'activate' event does not finish until the message arrives. On the other hand, > the test html waits for that the worker is activated, and it never happens > because the worker cannot finish the 'activate' event. > > Possible solutions would be... > > (1) using FetchEvent to wait for the completion of navigate() instead of > ActivateEvent, or > (2) deferring implementing this test until ExtendableMessageEvent is implemented > (http://crbug.com/543198) I have some questions. (1) Can the fetch event be fired before worker's state is activated? (2) about deadlock, yes, but I don't know why it happen exactly. Is there a race condition between iframe loading and worker activating? While the iframe loading happens on main thread, I think the work might run on worker thread? I uploaded another test code. (https://codereview.chromium.org/1643823003/) If you run the test, you can see the 'console.log(state)' at developer console but you can't see the 'console.log(frame)'. I'm not sure but shouldn't the with_iframe(scope) has to resolve at least? (FYI, in windowclient-navigate.html, I used wait_state('activating').)
Sorry for the late reply. https://codereview.chromium.org/1604893002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:49: get_test_data().then(navigate_test); On 2016/02/02 15:58:31, zino wrote: > On 2016/02/01 07:13:55, nhiroki wrote: > > Hmmm... there could be deadlock. > > > > e.waitUntil(get_test_data()) waits for a message from the test html and the > > 'activate' event does not finish until the message arrives. On the other hand, > > the test html waits for that the worker is activated, and it never happens > > because the worker cannot finish the 'activate' event. > > > > Possible solutions would be... > > > > (1) using FetchEvent to wait for the completion of navigate() instead of > > ActivateEvent, or > > (2) deferring implementing this test until ExtendableMessageEvent is > implemented > > (http://crbug.com/543198) > > I have some questions. > > (1) Can the fetch event be fired before worker's state is activated? No. When the worker is in installed/activating state, the fetch event is queued up until the worker is activated. See the following links: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... If you'd like to use the fetch event to wait for the completion of navigating "in-scope-but-not-controlled-yet" client, you might need to create a frame before registering a service worker and then navigate the frame after the worker is activated. FYI: ExtendableMessageEvent will be available in LayoutTests very soon: http://crrev.com/1658073002/ > (2) about deadlock, yes, but I don't know why it happen exactly. > Is there a race condition between iframe loading and worker activating? > While the iframe loading happens on main thread, I think the work might run on > worker thread? > I uploaded another test code. (https://codereview.chromium.org/1643823003/) > If you run the test, you can see the 'console.log(state)' at developer console > but you can't see the 'console.log(frame)'. > I'm not sure but shouldn't the with_iframe(scope) has to resolve at least? > (FYI, in windowclient-navigate.html, I used wait_state('activating').) Sorry, in my previous reply, I assumed that 'activated' was passed to wait_for_state(). At any rate, as I commented on (1), iframe loading is blocked until the worker is activated and deadlock happens.
On 2016/02/08 04:37:15, nhiroki (slow) wrote: > No. When the worker is in installed/activating state, the fetch event is queued > up until the worker is activated. See the following links: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... > > If you'd like to use the fetch event to wait for the completion of navigating > "in-scope-but-not-controlled-yet" client, you might need to create a frame > before registering a service worker and then navigate the frame after the worker > is activated. > > FYI: ExtendableMessageEvent will be available in LayoutTests very soon: > http://crrev.com/1658073002/ > Thank you for explanation. I'll wait to land your CL. Thank you.
On 2016/02/11 03:37:53, zino wrote: > On 2016/02/08 04:37:15, nhiroki (slow) wrote: > > No. When the worker is in installed/activating state, the fetch event is > queued > > up until the worker is activated. See the following links: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... > > > > If you'd like to use the fetch event to wait for the completion of navigating > > "in-scope-but-not-controlled-yet" client, you might need to create a frame > > before registering a service worker and then navigate the frame after the > worker > > is activated. > > > > FYI: ExtendableMessageEvent will be available in LayoutTests very soon: > > http://crrev.com/1658073002/ > > > > Thank you for explanation. > > I'll wait to land your CL. > > Thank you. fyi: The CL was landed.
PTAL. Thank you!
lgtm with nits. Thank you for patiently working on this! https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:39: console.log(self.registration.installing); Can you remove this logging? https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:55: description: 'invalid url(http://[eaxmple.com]) test', s/eaxmple/example/ https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:118: return Promise.resolve(client_frame); You can just return the frame. "return client_frame;"
Thank you for your review. :) https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js (right): https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/windowclient-navigate-worker.js:39: console.log(self.registration.installing); On 2016/02/16 07:33:22, nhiroki (slow) wrote: > Can you remove this logging? Done. https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html (right): https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:55: description: 'invalid url(http://[eaxmple.com]) test', On 2016/02/16 07:33:22, nhiroki (slow) wrote: > s/eaxmple/example/ Done. https://codereview.chromium.org/1604893002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html:118: return Promise.resolve(client_frame); On 2016/02/16 07:33:22, nhiroki (slow) wrote: > You can just return the frame. > > "return client_frame;" Done.
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1604893002/#ps120001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1604893002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604893002/120001
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Rewrite windowclient-navigate.html test. This CL splits the test into each test case to find a failure reason easily. Also, adds a test case for in-scope-but-not-controlled to the test. The test case comes from the spec dicussion[1] and the comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 ========== to ========== ServiceWorker: Rewrite windowclient-navigate.html test. This CL splits the test into each test case to find a failure reason easily. Also, adds a test case for in-scope-but-not-controlled to the test. The test case comes from the spec dicussion[1] and the comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Rewrite windowclient-navigate.html test. This CL splits the test into each test case to find a failure reason easily. Also, adds a test case for in-scope-but-not-controlled to the test. The test case comes from the spec dicussion[1] and the comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 ========== to ========== ServiceWorker: Rewrite windowclient-navigate.html test. This CL splits the test into each test case to find a failure reason easily. Also, adds a test case for in-scope-but-not-controlled to the test. The test case comes from the spec dicussion[1] and the comment[2] in other CL. [1] https://github.com/slightlyoff/ServiceWorker/issues/752 [2] https://codereview.chromium.org/1481753002/#msg8 BUG=567049 Committed: https://crrev.com/47a728652bd829f25edff357b5a640f9c78c328f Cr-Commit-Position: refs/heads/master@{#375629} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/47a728652bd829f25edff357b5a640f9c78c328f Cr-Commit-Position: refs/heads/master@{#375629}
Message was sent while issue was closed.
Patchset #8 (id:140001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
