Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1665453003:
Add browser tests for FetchEvent.request.mode/credentials in the service worker (Closed)
DescriptionAdd browser tests for FetchEvent.request.mode/credentials in the service worker
This CL adds browser_tests to check the fetch request mode and
credentials mode in the service worker for the follwing requests.
- <embed src="test.pdf">
Fetched in MimeHandlerViewContainer::OnReady().
- <link rel="manifest" href="manifest.json">
Fetched in ManifestFetcher::Start().
- PPAPI's pp::URLLoader.
Fetched in PepperURLLoaderHost::InternalOnHostMsgOpen().
This CL depends on https://codereview.chromium.org/1665533003/.
BUG=576534, 543895
Committed: https://crrev.com/bcc7a2056bac1159aa480a8fb9355da98f84424c
Cr-Commit-Position: refs/heads/master@{#376704}
Patch Set 1 : #Patch Set 2 : rebase on 1665533003 #
Total comments: 20
Patch Set 3 : incorporated nhiroki's comment #Patch Set 4 : rebase on 1665533003 #
Total comments: 12
Patch Set 5 : incorporated jochen's comment #
Messages
Total messages: 39 (20 generated)
Description was changed from ========== [WIP]add browser tests for FetchEvent.request.mode/credentials in SW BUG= ========== to ========== [WIP]add browser tests for FetchEvent.request.mode/credentials in SW This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ==========
Description was changed from ========== [WIP]add browser tests for FetchEvent.request.mode/credentials in SW This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ========== to ========== [WIP]add browser tests for FetchEvent.request.mode/credentials in SW This CL adds browser_tests to check the FetchEvent.request.mode and FetchEvent.request.credentials which are set in - MimeHandlerViewContainer for <embed src="test.pdf"> - ManifestFetcher for <link rel="manifest" href="manifest.json"> - PepperURLLoaderHost for PPAPI's pp::URLLoader. This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ==========
Description was changed from ========== [WIP]add browser tests for FetchEvent.request.mode/credentials in SW This CL adds browser_tests to check the FetchEvent.request.mode and FetchEvent.request.credentials which are set in - MimeHandlerViewContainer for <embed src="test.pdf"> - ManifestFetcher for <link rel="manifest" href="manifest.json"> - PepperURLLoaderHost for PPAPI's pp::URLLoader. This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ========== to ========== [WIP]Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fethced in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fethced in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fethced in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [WIP]Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fethced in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fethced in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fethced in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ========== to ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fethced in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fethced in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fethced in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ==========
Patchset #1 (id:20001) has been deleted
horo@chromium.org changed reviewers: + nhiroki@chromium.org
nhiroki@ Could you please review this?
https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:137: std::string RequsestString(const std::string& url, s/Requsest/Request https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:152: " return self.clients.claim();" "return" is not necessary. event.waitUntil(self.clients.claim()); https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:180: "var fetchedRequests = [];" "fetchedRequests" sounds a bit strange to me because a target of a fetch operation is not a request but a response. How about "issuedRequests"? https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:249: if (cross_origin.length() != 0) { "!cross_origin.empty()" https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:281: Should we test 'crossOrigin="anonymous"'? https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:296: ditto: 'crossOrigin="anonymous"' https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:321: std::string GetRequsestStringForPNACL() const { s/Requsest/Request/ https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... File chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc (right): https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. "(c)" is not necessary: http://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc:20: virtual void HandleMessage(const pp::Var& var_message) { remove "virtual" and add "override" ? https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... File chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html (right): https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html:2: window.addEventListener("load", function() { nit: single-quotes (') are preferred to double-quotes (") https://google.github.io/styleguide/javascriptguide.xml?showone=Strings#Strings
Thank you! https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:137: std::string RequsestString(const std::string& url, On 2016/02/09 04:48:42, nhiroki (slow) wrote: > s/Requsest/Request Done. https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:152: " return self.clients.claim();" On 2016/02/09 04:48:42, nhiroki (slow) wrote: > "return" is not necessary. > > event.waitUntil(self.clients.claim()); Done. https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:180: "var fetchedRequests = [];" On 2016/02/09 04:48:42, nhiroki (slow) wrote: > "fetchedRequests" sounds a bit strange to me because a target of a fetch > operation is not a request but a response. How about "issuedRequests"? Done. https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:249: if (cross_origin.length() != 0) { On 2016/02/09 04:48:42, nhiroki (slow) wrote: > "!cross_origin.empty()" Done. https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:281: On 2016/02/09 04:48:42, nhiroki (slow) wrote: > Should we test 'crossOrigin="anonymous"'? ManifestFetcher supports only "use-credentials". WebDocument::manifestUseCredentials() https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:321: std::string GetRequsestStringForPNACL() const { On 2016/02/09 04:48:42, nhiroki (slow) wrote: > s/Requsest/Request/ Done. https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... File chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc (right): https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/02/09 04:48:42, nhiroki (slow) wrote: > "(c)" is not necessary: > http://www.chromium.org/developers/coding-style#TOC-File-headers Done. https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.cc:20: virtual void HandleMessage(const pp::Var& var_message) { On 2016/02/09 04:48:42, nhiroki (slow) wrote: > remove "virtual" and add "override" ? Done. https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... File chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html (right): https://codereview.chromium.org/1665453003/diff/60001/chrome/test/data/nacl/p... chrome/test/data/nacl/pnacl_url_loader/pnacl_url_loader.html:2: window.addEventListener("load", function() { On 2016/02/09 04:48:42, nhiroki (slow) wrote: > nit: single-quotes (') are preferred to double-quotes (") > > https://google.github.io/styleguide/javascriptguide.xml?showone=Strings#Strings Done.
lgtm https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/1665453003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:281: On 2016/02/10 06:09:22, horo wrote: > On 2016/02/09 04:48:42, nhiroki (slow) wrote: > > Should we test 'crossOrigin="anonymous"'? > > ManifestFetcher supports only "use-credentials". > > WebDocument::manifestUseCredentials() Acknowledged.
horo@chromium.org changed reviewers: + bradnelson@chromium.org
bradnelson@ Could you please review chrome/test/data/nacl/*?
Description was changed from ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fethced in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fethced in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fethced in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ========== to ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fetched in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fethced in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fethced in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ==========
Description was changed from ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fetched in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fethced in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fethced in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ========== to ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fetched in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fetched in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fetched in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ==========
bradnelson@google.com changed reviewers: + bradnelson@google.com
The CQ bit was checked by bradnelson@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665453003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665453003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
horo@chromium.org changed reviewers: + jochen@chromium.org
jochen@ Could you please review chrome/browser/chrome_service_worker_browsertest.cc?
lgtm https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:121: ChromeServiceWorkerFetchTest() {} please also add a virtual dtor https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:209: }; private: disallow copy/assign https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:234: ChromeServiceWorkerManifestFetchTest() {} also add dtor https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:274: }; disallow copy/assign https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:309: ChromeServiceWorkerFetchPPAPITest() {} same here https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:432: ChromeServiceWorkerFetchPPAPIPrivateTest() {} and here
Thank you! https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:121: ChromeServiceWorkerFetchTest() {} On 2016/02/17 11:01:04, jochen wrote: > please also add a virtual dtor Done. https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:209: }; On 2016/02/17 11:01:04, jochen wrote: > private: disallow copy/assign Done. https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:234: ChromeServiceWorkerManifestFetchTest() {} On 2016/02/17 11:01:04, jochen wrote: > also add dtor Done. https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:274: }; On 2016/02/17 11:01:04, jochen wrote: > disallow copy/assign Done. https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:309: ChromeServiceWorkerFetchPPAPITest() {} On 2016/02/17 11:01:04, jochen wrote: > same here Done. https://codereview.chromium.org/1665453003/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_service_worker_browsertest.cc:432: ChromeServiceWorkerFetchPPAPIPrivateTest() {} On 2016/02/17 11:01:04, jochen wrote: > and here Done.
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, bradnelson@google.com, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1665453003/#ps120001 (title: "incorporated jochen's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665453003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665453003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bradnelson@ Could you please send 'lgtm' with your chromium account?
The CQ bit was checked by bradnelson@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665453003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665453003/120001
Message was sent while issue was closed.
Description was changed from ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fetched in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fetched in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fetched in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ========== to ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fetched in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fetched in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fetched in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fetched in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fetched in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fetched in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 ========== to ========== Add browser tests for FetchEvent.request.mode/credentials in the service worker This CL adds browser_tests to check the fetch request mode and credentials mode in the service worker for the follwing requests. - <embed src="test.pdf"> Fetched in MimeHandlerViewContainer::OnReady(). - <link rel="manifest" href="manifest.json"> Fetched in ManifestFetcher::Start(). - PPAPI's pp::URLLoader. Fetched in PepperURLLoaderHost::InternalOnHostMsgOpen(). This CL depends on https://codereview.chromium.org/1665533003/. BUG=576534, 543895 Committed: https://crrev.com/bcc7a2056bac1159aa480a8fb9355da98f84424c Cr-Commit-Position: refs/heads/master@{#376704} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bcc7a2056bac1159aa480a8fb9355da98f84424c Cr-Commit-Position: refs/heads/master@{#376704}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/1739643002/ by horo@chromium.org. The reason for reverting is: https://codereview.chromium.org/1665533003/ introduced a security bug.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
