Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2685543002:
Support data URLs in worker constructors (Closed)
|
Created:
3 years, 10 months ago by andypaicu2 Modified:
3 years, 10 months ago CC:
blink-reviews, blink-worker-reviews_chromium.org, chromium-reviews, falken+watch_chromium.org, horo+watch_chromium.org, kinuko+worker_chromium.org, shimazu+worker_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport data URLs in worker constructors to be inline with spec.
Changes made:
- Allow Cross Origin Requests when trying to retrieve the URL resources if it's a
data URL (for workers only)
- Enhanced data-url and data-url-shared external wpt tests and removed the failure
test expectations for these tests
BUG=270979, 655458, 508730
Review-Url: https://codereview.chromium.org/2685543002
Cr-Commit-Position: refs/heads/master@{#450358}
Committed: https://chromium.googlesource.com/chromium/src/+/9801daec162285a1f2c6695682ace5d508ea9b7f
Patch Set 1 #Patch Set 2 : Changes to allow data URL workers #
Total comments: 6
Patch Set 3 : Implemented Pass 2 code review suggestions. Removed external/wpt/workers/data-url.html from the lis… #
Total comments: 12
Patch Set 4 : Made SharedWorkers work with 'data:' URLs. Implemented suggested code review change #Patch Set 5 : Added external tests for data: SharedWorkers #
Total comments: 2
Patch Set 6 : Added additional test for invalid javascript data: worker #
Total comments: 3
Messages
Total messages: 50 (35 generated)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by mkwst@chromium.org
Thanks! Here's a first pass! Please also update the CL description with some details about what you're changing and why. That will be invaluable if we need to come back to this patch in a year to figure out why something broke. :) https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html (right): https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html:1: <!doctype html> Please update the test in //LayoutTests/external/wpt/workers/data-url.html rather than creating a new test. It's not obvious from the layout, I know, but those tests are synced with https://github.com/w3c/web-platform-tests, which means everyone can use them, while the tests in other directories are Blink-only. https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html:6: async_test(function(t) { I think we can cut the repetition here by extracting some common code. Something like: ``` function assert_worker_sends_pass(type, code) { async_test(t => { var w = new Worker(`data:${type},${code}`); w.onmessage = t.step_func_done(e => { assert_equals(e.data, 'PASS'); }); }); } function assert_worker_throws(code) { assert_worker_sends_pass("", `try { ${code}; self.postMessage('FAIL'); } catch (e) { self.postMessage('PASS'); }`); } // We'll load any MIME type. assert_worker_sends_pass("application/javascript", "self.postMessage('pass');"); assert_worker_sends_pass("text/plain", "self.postMessage('pass');"); assert_worker_sends_pass("", "self.postMessage('pass');"); // `data:` Workers are cross-origin. assert_worker_sends_pass("", "fetch('/').then(_ => self.postMessage('FAIL'), _ => self.postMessage('PASS'))"); assert_worker_throws("self.localStorage['a'] = 'a';"); ... ``` https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right): https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:56: } I'd prefer to spell this with a ternary-if since it's just setting a single variable. That is: ``` CrossOriginRequestPolicy policy = scriptURL.protocolIsData() ? AllowCrossOriginRequests : DenyCrossOriginRequests; ``` Also, for the future, the general style in Blink (and the rest of Chromium) is to drop braces for single-line if statements.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:2077: crbug.com/508730 external/wpt/workers/data-url.html [ Failure ] Will you be dealing with `SharedWorker` in a future patch? https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html (right): https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:8: if (!protocol) protocol = 'data'; I don't actually think this adds much. That is, it's testing URL parsing, not whether `data` URLs are usable as workers. I'd just leave it up to the URL tests to verify that behavior. https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:22: //Any MIME type Nit: Space between "//" and the comment, here and elsewhere. https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:28: assert_worker_sends_pass('communication is both ways', 'application/javascript', 'onmessage = function(e) { self.postMessage("PASS"); }'); Nit: "is bidirectional", or "goes both ways". https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:34: assert_worker_throws('indexDB inaccessible', 'self.indexedDB.open("bug270979")'); 1. Nit: "indexedDB", not "indexDB". 2. Why `bug270979`? That's a very strange name for a database, given that this is a generic test repository that doesn't have anything to do with the Chromium project. :) https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/AbstractWorker.cpp (right): https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/AbstractWorker.cpp:60: if (!scriptURL.protocolIsData() && This is going to allow `SharedWorker`, right? Or is it blocked in some other way? Let's make sure we're still failing the `SharedWorker` test we're failing today until you get `data:` working for `SharedWorker` as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
jochen@chromium.org changed reviewers: + jochen@chromium.org
can you also add a test where we pass a data: URL with an invalid script to the worker constructor? https://codereview.chromium.org/2685543002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2685543002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:183: ((KURL)m_url).protocolIsData() ? AllowCrossOriginRequests please use c++ style cases (static_cast<KURL>) - see https://google.github.io/styleguide/cppguide.html#Casting
The CQ bit was checked by andypaicu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [WIP] Support data URLs in worker constructors BUG=270979 ========== to ========== [WIP] Support data URLs in worker constructors BUG=270979,655458,508730 ==========
Description was changed from ========== [WIP] Support data URLs in worker constructors BUG=270979,655458,508730 ========== to ========== [WIP] Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - BUG=270979,655458,508730 ==========
Description was changed from ========== [WIP] Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - BUG=270979,655458,508730 ========== to ========== [WIP] Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - Enhanced data-url and data-url-shared external wpt tests and removed the failure test expectations for these tests BUG=270979,655458,508730 ==========
The CQ bit was checked by andypaicu@google.com
The CQ bit was unchecked by andypaicu@google.com
On 2017/02/08 at 13:15:44, mkwst wrote: > Thanks! Here's a first pass! > > Please also update the CL description with some details about what you're changing and why. That will be invaluable if we need to come back to this patch in a year to figure out why something broke. :) > > https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html (right): > > https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html:1: <!doctype html> > Please update the test in //LayoutTests/external/wpt/workers/data-url.html rather than creating a new test. It's not obvious from the layout, I know, but those tests are synced with https://github.com/w3c/web-platform-tests, which means everyone can use them, while the tests in other directories are Blink-only. > > https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html:6: async_test(function(t) { > I think we can cut the repetition here by extracting some common code. Something like: > > ``` > function assert_worker_sends_pass(type, code) { > async_test(t => { > var w = new Worker(`data:${type},${code}`); > w.onmessage = t.step_func_done(e => { > assert_equals(e.data, 'PASS'); > }); > }); > } > > function assert_worker_throws(code) { > assert_worker_sends_pass("", `try { ${code}; self.postMessage('FAIL'); } catch (e) { self.postMessage('PASS'); }`); > } > > // We'll load any MIME type. > assert_worker_sends_pass("application/javascript", "self.postMessage('pass');"); > assert_worker_sends_pass("text/plain", "self.postMessage('pass');"); > assert_worker_sends_pass("", "self.postMessage('pass');"); > > // `data:` Workers are cross-origin. > assert_worker_sends_pass("", "fetch('/').then(_ => self.postMessage('FAIL'), _ => self.postMessage('PASS'))"); > assert_worker_throws("self.localStorage['a'] = 'a';"); > ... > ``` > > https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right): > > https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:56: } > I'd prefer to spell this with a ternary-if since it's just setting a single variable. That is: > > ``` > CrossOriginRequestPolicy policy = scriptURL.protocolIsData() ? > AllowCrossOriginRequests : DenyCrossOriginRequests; > ``` > > Also, for the future, the general style in Blink (and the rest of Chromium) is to drop braces for single-line if statements. Updated CL description
https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html (right): https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html:1: <!doctype html> On 2017/02/08 at 13:15:44, Mike West (sloooooow) wrote: > Please update the test in //LayoutTests/external/wpt/workers/data-url.html rather than creating a new test. It's not obvious from the layout, I know, but those tests are synced with https://github.com/w3c/web-platform-tests, which means everyone can use them, while the tests in other directories are Blink-only. Done https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/workers/worker-allow-data-url.html:6: async_test(function(t) { On 2017/02/08 at 13:15:44, Mike West (sloooooow) wrote: > I think we can cut the repetition here by extracting some common code. Something like: > > ``` > function assert_worker_sends_pass(type, code) { > async_test(t => { > var w = new Worker(`data:${type},${code}`); > w.onmessage = t.step_func_done(e => { > assert_equals(e.data, 'PASS'); > }); > }); > } > > function assert_worker_throws(code) { > assert_worker_sends_pass("", `try { ${code}; self.postMessage('FAIL'); } catch (e) { self.postMessage('PASS'); }`); > } > > // We'll load any MIME type. > assert_worker_sends_pass("application/javascript", "self.postMessage('pass');"); > assert_worker_sends_pass("text/plain", "self.postMessage('pass');"); > assert_worker_sends_pass("", "self.postMessage('pass');"); > > // `data:` Workers are cross-origin. > assert_worker_sends_pass("", "fetch('/').then(_ => self.postMessage('FAIL'), _ => self.postMessage('PASS'))"); > assert_worker_throws("self.localStorage['a'] = 'a';"); > ... > ``` Done https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp (right): https://codereview.chromium.org/2685543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp:56: } On 2017/02/08 at 13:15:44, Mike West (sloooooow) wrote: > I'd prefer to spell this with a ternary-if since it's just setting a single variable. That is: > > ``` > CrossOriginRequestPolicy policy = scriptURL.protocolIsData() ? > AllowCrossOriginRequests : DenyCrossOriginRequests; > ``` > > Also, for the future, the general style in Blink (and the rest of Chromium) is to drop braces for single-line if statements. Done https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:2077: crbug.com/508730 external/wpt/workers/data-url.html [ Failure ] On 2017/02/08 at 15:18:53, Mike West (sloooooow) wrote: > Will you be dealing with `SharedWorker` in a future patch? Yes, SharedWorker is coming in patch 4 https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html (right): https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:8: if (!protocol) protocol = 'data'; On 2017/02/08 at 15:18:53, Mike West (sloooooow) wrote: > I don't actually think this adds much. That is, it's testing URL parsing, not whether `data` URLs are usable as workers. I'd just leave it up to the URL tests to verify that behavior. Removed https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:22: //Any MIME type On 2017/02/08 at 15:18:53, Mike West (sloooooow) wrote: > Nit: Space between "//" and the comment, here and elsewhere. Done https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:28: assert_worker_sends_pass('communication is both ways', 'application/javascript', 'onmessage = function(e) { self.postMessage("PASS"); }'); On 2017/02/08 at 15:18:53, Mike West (sloooooow) wrote: > Nit: "is bidirectional", or "goes both ways". Done https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:34: assert_worker_throws('indexDB inaccessible', 'self.indexedDB.open("bug270979")'); On 2017/02/08 at 15:18:53, Mike West (sloooooow) wrote: > 1. Nit: "indexedDB", not "indexDB". > 2. Why `bug270979`? That's a very strange name for a database, given that this is a generic test repository that doesn't have anything to do with the Chromium project. :) Done https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/AbstractWorker.cpp (right): https://codereview.chromium.org/2685543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/AbstractWorker.cpp:60: if (!scriptURL.protocolIsData() && On 2017/02/08 at 15:18:53, Mike West (sloooooow) wrote: > This is going to allow `SharedWorker`, right? Or is it blocked in some other way? > > Let's make sure we're still failing the `SharedWorker` test we're failing today until you get `data:` working for `SharedWorker` as well. SharedWorker tests were still failing because (similar to Worker) they could not make the Cross Origin Request to retrieve the resource from the URL. Fixed in patch 4.
On 2017/02/10 at 09:57:37, jochen wrote: > can you also add a test where we pass a data: URL with an invalid script to the worker constructor? > > https://codereview.chromium.org/2685543002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): > > https://codereview.chromium.org/2685543002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:183: ((KURL)m_url).protocolIsData() ? AllowCrossOriginRequests > please use c++ style cases (static_cast<KURL>) - see https://google.github.io/styleguide/cppguide.html#Casting Added test for invalid script
Description was changed from ========== [WIP] Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - Enhanced data-url and data-url-shared external wpt tests and removed the failure test expectations for these tests BUG=270979,655458,508730 ========== to ========== Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - Enhanced data-url and data-url-shared external wpt tests and removed the failure test expectations for these tests BUG=270979,655458,508730 ==========
The CQ bit was checked by andypaicu@google.com
Added patch with changes. https://codereview.chromium.org/2685543002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right): https://codereview.chromium.org/2685543002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:183: ((KURL)m_url).protocolIsData() ? AllowCrossOriginRequests On 2017/02/10 at 09:57:37, jochen wrote: > please use c++ style cases (static_cast<KURL>) - see https://google.github.io/styleguide/cppguide.html#Casting Done
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by andypaicu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - Enhanced data-url and data-url-shared external wpt tests and removed the failure test expectations for these tests BUG=270979,655458,508730 ========== to ========== Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - Enhanced data-url and data-url-shared external wpt tests and removed the failure test expectations for these tests BUG=270979,655458,508730 ==========
lgtm once the tests pass
The CQ bit was checked by andypaicu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by andypaicu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487085763160720,
"parent_rev": "4cdadb3a5457e6e59bb13be30c2584e81feac9b4", "commit_rev":
"9801daec162285a1f2c6695682ace5d508ea9b7f"}
Message was sent while issue was closed.
Description was changed from ========== Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - Enhanced data-url and data-url-shared external wpt tests and removed the failure test expectations for these tests BUG=270979,655458,508730 ========== to ========== Support data URLs in worker constructors to be inline with spec. Changes made: - Allow Cross Origin Requests when trying to retrieve the URL resources if it's a data URL (for workers only) - Enhanced data-url and data-url-shared external wpt tests and removed the failure test expectations for these tests BUG=270979,655458,508730 Review-Url: https://codereview.chromium.org/2685543002 Cr-Commit-Position: refs/heads/master@{#450358} Committed: https://chromium.googlesource.com/chromium/src/+/9801daec162285a1f2c6695682ac... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9801daec162285a1f2c6695682ac...
Message was sent while issue was closed.
mek@chromium.org changed reviewers: + mek@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html (right): https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:45: assert_worker_throws('indexedDB inaccessible', 'self.indexedDB.open("someDBName")'); I'm not convinced that this is actually spec compliant (I don't see anywhere in the IDB spec where it is supposed to throw for trying to open a db from an opaque origin). https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:46: assert_worker_throws('localStorage inaccessible', 'self.localStorage.testItem'); This test makes no sense. Workers don't have localStorage under any circumstance, so I'm not sure what this is trying to test?
Message was sent while issue was closed.
https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html (right): https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:45: assert_worker_throws('indexedDB inaccessible', 'self.indexedDB.open("someDBName")'); On 2017/02/14 at 18:38:10, Marijn Kruisselbrink wrote: > I'm not convinced that this is actually spec compliant (I don't see anywhere in the IDB spec where it is supposed to throw for trying to open a db from an opaque origin). interesting. I filed https://github.com/w3c/IndexedDB/issues/148 about this
Message was sent while issue was closed.
On 2017/02/14 at 18:38:10, mek wrote: > https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html (right): > > https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:45: assert_worker_throws('indexedDB inaccessible', 'self.indexedDB.open("someDBName")'); > I'm not convinced that this is actually spec compliant (I don't see anywhere in the IDB spec where it is supposed to throw for trying to open a db from an opaque origin). > > https://codereview.chromium.org/2685543002/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/external/wpt/workers/data-url.html:46: assert_worker_throws('localStorage inaccessible', 'self.localStorage.testItem'); > This test makes no sense. Workers don't have localStorage under any circumstance, so I'm not sure what this is trying to test? Removed localStorage test. And added a new websql test. New codereview created for this: https://codereview.chromium.org/2697003002. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
