Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
491203002:
[ServiceWorker] Add test for FetchEvent's body with a local file. (Closed)
Description[ServiceWorker] Add test for FetchEvent's body with a local file.
This test uses eventSender.beginDragWithFiles() in JS to upload the local file.
So we have to locate the files under LayoutTests/http/tests/local/.
BUG=402387
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180945
Patch Set 1 : #
Total comments: 16
Patch Set 2 : incorporated nhiroki's comment #
Total comments: 18
Patch Set 3 : incorporated nhiroki's comment #Patch Set 4 : wait_for_update wait_for_activated #Patch Set 5 : remove test-helpers.js from iframe #
Messages
Total messages: 14 (0 generated)
nhiroki@ Could you please review?
I just looked over this superficially. Will take a deeper look later. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... File LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html (right): https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:13: .then(t.step_func(function(frame) { You don't have to wrap with "step_func" in a promise chain. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:29: .then(t.step_func(function(frame) { ditto (step_func) https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:59: .then(t.step_func(function() { ditto (step_func) https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:97: .then(t.step_func(function(frame) { ditto (step_func) https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html (right): https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:26: }); How about factoring out this to 'wait_for_update()' as we are doing in the http/tests/sw/resources/test-helpers.js ? https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:35: }); ditto. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js (right): https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js:6: }); indent-nit: event.request.headers.forEach(function(value, key) { headers.push([key, value]); }); https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js:13: }))); +2-indents?
https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... File LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html (right): https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:13: .then(t.step_func(function(frame) { On 2014/08/25 04:29:36, nhiroki wrote: > You don't have to wrap with "step_func" in a promise chain. Done. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:29: .then(t.step_func(function(frame) { On 2014/08/25 04:29:37, nhiroki wrote: > ditto (step_func) Done. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:59: .then(t.step_func(function() { On 2014/08/25 04:29:36, nhiroki wrote: > ditto (step_func) Done. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:97: .then(t.step_func(function(frame) { On 2014/08/25 04:29:37, nhiroki wrote: > ditto (step_func) Done. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html (right): https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:26: }); On 2014/08/25 04:29:37, nhiroki wrote: > How about factoring out this to 'wait_for_update()' as we are doing in the > http/tests/sw/resources/test-helpers.js ? Done. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:35: }); On 2014/08/25 04:29:37, nhiroki wrote: > ditto. Done. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js (right): https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js:6: }); On 2014/08/25 04:29:37, nhiroki wrote: > indent-nit: > > event.request.headers.forEach(function(value, key) { > headers.push([key, value]); > }); Done. https://codereview.chromium.org/491203002/diff/60001/LayoutTests/http/tests/l... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-worker.js:13: }))); On 2014/08/25 04:29:37, nhiroki wrote: > +2-indents? Done.
https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html (right): https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:8: var SCOPE = 'http://127.0.0.1:8000/local/serviceworker/resources/fetch-request-body-file-test'; Why do we need to specify the origin part, that is, "http://127.0.0.1:8000"? (I might be missing something??) https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:18: document.body.removeChild(frame); unload_iframe()? https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:36: document.body.removeChild(frame); ditto. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:62: register() register() wrapped with async_test() indirectly calls async_test() in fetch-request-body-file-iframe.html... this nest seems strange. (please see my comments in fetch-request-body-file-iframe.html) https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:103: document.body.removeChild(frame); ditto. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html (right): https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:15: async_test(function(t) { Would it be possible to remove async_test() from here? https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:16: service_worker_unregister_and_register(t, script, scope) Question: Why don't you call register()/unregister() in fetch-request-body-file.html? Does this file exist in a different origin? https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:28: async_test(function(t) { ditto (removing async_test).
https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html (right): https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:8: var SCOPE = 'http://127.0.0.1:8000/local/serviceworker/resources/fetch-request-body-file-test'; On 2014/08/27 03:52:34, nhiroki wrote: > Why do we need to specify the origin part, that is, "http://127.0.0.1:8000"? (I > might be missing something??) This test is executed as a local file because it is in http/tests/local/ directory. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:18: document.body.removeChild(frame); On 2014/08/27 03:52:34, nhiroki wrote: > unload_iframe()? Done. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:36: document.body.removeChild(frame); On 2014/08/27 03:52:34, nhiroki wrote: > ditto. Done. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:103: document.body.removeChild(frame); On 2014/08/27 03:52:34, nhiroki wrote: > ditto. Done. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html (right): https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:15: async_test(function(t) { On 2014/08/27 03:52:34, nhiroki wrote: > Would it be possible to remove async_test() from here? I introduced this in order to use wait_for_update as your comment in patch set 1. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:16: service_worker_unregister_and_register(t, script, scope) On 2014/08/27 03:52:34, nhiroki wrote: > Question: Why don't you call register()/unregister() in > fetch-request-body-file.html? Does this file exist in a different origin? In layouttest, fetch-request-body-file.html is opened as a local file. So to register a ServiceWorker we have to open this file on HTTP server.
nits: - s/lcate/locate/ (in the CL description) - Can you update the TEST= line? https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html (right): https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/fetch-request-body-file.html:8: var SCOPE = 'http://127.0.0.1:8000/local/serviceworker/resources/fetch-request-body-file-test'; On 2014/08/27 05:05:45, horo wrote: > On 2014/08/27 03:52:34, nhiroki wrote: > > Why do we need to specify the origin part, that is, "http://127.0.0.1:8000"? > (I > > might be missing something??) > > This test is executed as a local file because it is in http/tests/local/ > directory. That makes sense. Thanks for your explanation. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html (right): https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:15: async_test(function(t) { On 2014/08/27 05:05:45, horo wrote: > On 2014/08/27 03:52:34, nhiroki wrote: > > Would it be possible to remove async_test() from here? > > I introduced this in order to use wait_for_update as your comment in patch set > 1. I see... I didn't mean to reuse the wait_for_update() in test_helper.js. The comment wanted to say "how about making a separate function like wait_for_update() in this file?". Sorry for my ambiguous comment. https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:16: service_worker_unregister_and_register(t, script, scope) On 2014/08/27 05:05:45, horo wrote: > On 2014/08/27 03:52:34, nhiroki wrote: > > Question: Why don't you call register()/unregister() in > > fetch-request-body-file.html? Does this file exist in a different origin? > > In layouttest, fetch-request-body-file.html is opened as a local file. > So to register a ServiceWorker we have to open this file on HTTP server. Acknowledged.
https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html (right): https://codereview.chromium.org/491203002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/local/serviceworker/resources/fetch-request-body-file-iframe.html:15: async_test(function(t) { On 2014/08/27 05:23:21, nhiroki wrote: > On 2014/08/27 05:05:45, horo wrote: > > On 2014/08/27 03:52:34, nhiroki wrote: > > > Would it be possible to remove async_test() from here? > > > > I introduced this in order to use wait_for_update as your comment in patch set > > 1. > > I see... I didn't mean to reuse the wait_for_update() in test_helper.js. The > comment wanted to say "how about making a separate function like > wait_for_update() in this file?". Sorry for my ambiguous comment. Done.
lgtm
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/491203002/140001
The CQ bit was unchecked by horo@chromium.org
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/491203002/160001
Message was sent while issue was closed.
Committed patchset #5 (160001) as 180945 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
