Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804533002:
Update Error Event inside a worker to provide the exact exception value (Closed)
|
Created:
3 years, 8 months ago by yiyix Modified:
3 years, 8 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, nhiroki, shimazu+worker_chromium.org, blink-reviews-bindings_chromium.org, kinuko+worker_chromium.org, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, tzik, blink-worker-reviews_chromium.org, varkha Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate Error Event inside a worker to provide the exact exception value
In WorkerOrWorkletScriptController.cpp, 'ErrorEvent' used to be created
without exception value, so the exception, |m_error|, is always set to
null. A new constructor of ErrorEvent is added in this patch which
takes exception as input and set |m_error| to the exception value.
TEST=error-script.js and error-worker-script.js
BUG=640724
Review-Url: https://codereview.chromium.org/2804533002
Cr-Commit-Position: refs/heads/master@{#462458}
Committed: https://chromium.googlesource.com/chromium/src/+/0662e1de8c6d9800c4baed479249c1fa5201c24c
Patch Set 1 #
Total comments: 28
Patch Set 2 : update tests #
Total comments: 6
Patch Set 3 : fix nits #
Total comments: 18
Patch Set 4 : address comments #
Total comments: 11
Patch Set 5 : rebase + comments #
Total comments: 11
Patch Set 6 : nits #
Total comments: 6
Patch Set 7 : nits #Messages
Total messages: 54 (30 generated)
Description was changed from ========== fix error description BUG= ========== to ========== Update Error Event inside a worker to provide the exact exception value BUG=640724 ==========
Description was changed from ========== Update Error Event inside a worker to provide the exact exception value BUG=640724 ========== to ========== Update Error Event inside a worker to provide the exact exception value In WorkerOrWorkletScriptController.cpp, 'ErrorEvent' used to be created without exception value, so the exception, |m_error|, is set to null. A new constructor of ErrorEvent is added in this patch which takes exception as input and set |m_error| to the exception. TEST=error-script.js and error-worker-script.js BUG=640724 ==========
Description was changed from ========== Update Error Event inside a worker to provide the exact exception value In WorkerOrWorkletScriptController.cpp, 'ErrorEvent' used to be created without exception value, so the exception, |m_error|, is set to null. A new constructor of ErrorEvent is added in this patch which takes exception as input and set |m_error| to the exception. TEST=error-script.js and error-worker-script.js BUG=640724 ========== to ========== Update Error Event inside a worker to provide the exact exception value In WorkerOrWorkletScriptController.cpp, 'ErrorEvent' used to be created without exception value, so the exception, |m_error|, is always set to null. A new constructor of ErrorEvent is added in this patch which takes exception as input and set |m_error| to the exception value. TEST=error-script.js and error-worker-script.js BUG=640724 ==========
yiyix@chromium.org changed reviewers: + falken@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by yiyix@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...
yiyix@chromium.org changed reviewers: + shimazu@chromium.org - falken@chromium.org
@shimazu, could you please review this patch? Thank you.
https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:9: test(function() { Could you use promise_test to write async test like the following instead of async_test? -- promise_test(() => { return new Promise(resolve => { var worker = new Worker(...); worker.onmessage = resolve; }) .then(e => { assert_true(...); ... }); }); https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:12: var data = e.data; You can use e.data directly for checking the parameter. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:13: assert_true(data.source == 'event listener'); asser_equals is preferable for comparison. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js:12: Could you remove these extra newlines? https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:15: return wait_for_state(t, registration.installing, 'activated'); I think you don't need to wait for activation. How about creating the promise to wait for a message and posting message here? https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:16: }) +2 indent to follow the coding style: https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style ex: something_returns_promise() .then(() => { /* something 1 */ }) .then(() => { /* something 2 */ }); https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:17: .then(function() { return with_iframe(scope); }) I think you don't need to trigger fetch request in this test. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:20: navigator.serviceWorker.onmessage = function(event) { 'navigator.serviceWorker.onmessage = resolve' would be simpler. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:27: assert_true(e.source == 'event listener'); You can use assert_equals for comparison. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/ErrorEvent.h (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/ErrorEvent.h:51: DOMWrapperWorld* world) { Just confirmation; do we still keep the old factory method?
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js:2: postMessage({ source: "event listener", value: e.error }); I'm not sure what we want to check by "source". Is it necessary? https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js:9: source.postMessage({ source: "event listener", value: e.error }); ditto(source) https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/ErrorEvent.h (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/ErrorEvent.h:102: DOMWrapperWorld*); How about changing this ctor as follows and then calling it from create(message, location, world), create(message, location, error, world) and createSanitizedError()? ErrorEvent(const String& message, std::unique_ptr<SourceLocation>, ScriptValue error, DOMWrapperWorld*);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yiyix@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...
@shimazu, @nihiroki, could you please review this new patch? Thank you very much. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:9: test(function() { On 2017/04/05 07:23:22, shimazu wrote: > Could you use promise_test to write async test like the following instead of > async_test? > > -- > promise_test(() => { > return new Promise(resolve => { > var worker = new Worker(...); > worker.onmessage = resolve; > }) > .then(e => { > assert_true(...); > ... > }); > }); Done. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:12: var data = e.data; On 2017/04/05 07:23:22, shimazu wrote: > You can use e.data directly for checking the parameter. Done. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:13: assert_true(data.source == 'event listener'); On 2017/04/05 07:23:22, shimazu wrote: > asser_equals is preferable for comparison. Done. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js:2: postMessage({ source: "event listener", value: e.error }); On 2017/04/05 07:38:38, nhiroki wrote: > I'm not sure what we want to check by "source". Is it necessary? I thought it is like a bonus to test the event source. It is not necessary to add source as an attribute in this message. I can remove "source" https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js:9: source.postMessage({ source: "event listener", value: e.error }); On 2017/04/05 07:38:38, nhiroki wrote: > ditto(source) Done. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js:12: On 2017/04/05 07:23:22, shimazu wrote: > Could you remove these extra newlines? Done. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:15: return wait_for_state(t, registration.installing, 'activated'); On 2017/04/05 07:23:22, shimazu wrote: > I think you don't need to wait for activation. How about creating the promise to > wait for a message and posting message here? Done. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:16: }) On 2017/04/05 07:23:22, shimazu wrote: > +2 indent to follow the coding style: > https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style > > ex: > something_returns_promise() > .then(() => { > /* something 1 */ > }) > .then(() => { > /* something 2 */ > }); Thank you so much. I thought the closing bracket should be aligned with the opening bracket like java style. I will read this article. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:17: .then(function() { return with_iframe(scope); }) On 2017/04/05 07:23:22, shimazu wrote: > I think you don't need to trigger fetch request in this test. removed. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:20: navigator.serviceWorker.onmessage = function(event) { On 2017/04/05 07:23:22, shimazu wrote: > 'navigator.serviceWorker.onmessage = resolve' would be simpler. is it similar approach as worker.onmessage = resolve; ? changed. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:27: assert_true(e.source == 'event listener'); On 2017/04/05 07:23:22, shimazu wrote: > You can use assert_equals for comparison. Done. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/ErrorEvent.h (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/ErrorEvent.h:51: DOMWrapperWorld* world) { On 2017/04/05 07:23:22, shimazu wrote: > Just confirmation; do we still keep the old factory method? I can add a default value to |error|, where error is set to null. So it doesn't break the existing functionalities. However, I noticed that the default value is not used in most of service worker code, I want to know what other viewer think about it. https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/ErrorEvent.h:102: DOMWrapperWorld*); On 2017/04/05 07:38:38, nhiroki wrote: > How about changing this ctor as follows and then calling it from create(message, > location, world), create(message, location, error, world) and > createSanitizedError()? > > ErrorEvent(const String& message, std::unique_ptr<SourceLocation>, ScriptValue > error, DOMWrapperWorld*); Done.
I know the style is difficult... https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:20: navigator.serviceWorker.onmessage = function(event) { On 2017/04/05 09:02:55, yiyix wrote: > On 2017/04/05 07:23:22, shimazu wrote: > > 'navigator.serviceWorker.onmessage = resolve' would be simpler. > > is it similar approach as worker.onmessage = resolve; ? changed. Yes, that's right! https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/ErrorEvent.h (right): https://codereview.chromium.org/2804533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/ErrorEvent.h:51: DOMWrapperWorld* world) { On 2017/04/05 09:02:56, yiyix wrote: > On 2017/04/05 07:23:22, shimazu wrote: > > Just confirmation; do we still keep the old factory method? > > I can add a default value to |error|, where error is set to null. So it doesn't > break the existing functionalities. However, I noticed that the default value is > not used in most of service worker code, I want to know what other viewer think > about it. Yes, as you mentioned default values are not preferable. I just wanted to confirm if there are other places using this method. Looks good as is. https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:13: .then(function(e) { Indentation will be like this: promise_test(() => { return new Promise(function(resolve) { var worker = ... }) .then(e => { }); }); Could you fix it? https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:13: .then(function(registration) { +2 indent; promise_test(() => { return service_worker_unregister_and_register(t, script, scope) .then(registration => { // Here is your code }) .then(() => }); }); https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:23: assert_equals(e.value, 'testError'); e.data.value
@shimazu and @nhiroki, Could you please review this new patch? Thank you. https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:13: .then(function(e) { On 2017/04/05 09:29:40, shimazu wrote: > Indentation will be like this: > > promise_test(() => { > return new Promise(function(resolve) { > var worker = ... > }) > .then(e => { > }); > }); > > Could you fix it? Done. https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:13: .then(function(registration) { On 2017/04/05 09:29:40, shimazu wrote: > +2 indent; > > promise_test(() => { > return service_worker_unregister_and_register(t, script, scope) > .then(registration => { > // Here is your code > }) > .then(() => > }); > }); > Done. https://codereview.chromium.org/2804533002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:23: assert_equals(e.value, 'testError'); On 2017/04/05 09:29:40, shimazu wrote: > e.data.value Done.
yiyix@chromium.org changed reviewers: + falken@chromium.org
https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:7: promise_test(function(t) { Can you add a comment like this? // TODO(yiyix): This test will be superseded by external/wpt/workers/Worker_ErrorEvent_error.htm once https://crbug.com/708857 is fixed. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:8: return new Promise(function(resolve) { Optional: Arrow function would be simpler: return new Promise(resolve => { // do something }) see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/A... https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:11: }) +2 indents for line 8-11 return new Promise(resolve => { var worker = new Worker('resources/error-script.js); worker.onmessage = resolve; }) .then(e => assert_equals(e.data.value, 'testError')); https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js:9: source.postMessage({ value: e.error }); How about testing other attributes of ErrorEvent if we don't have such a test for service workers? https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:1: <!DOCTYPE html> We could move this test into LayoutTests/external/service-workers/service-worker/ServiceWorkerGlobalScope so that the test will be automatically exported to the wpt's repository. (sorry for my misdirection) https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:8: var script = 'resources/error-worker-script.js'; We conventionally use "-worker.js" suffix for worker scripts, so can you remove "-script" part? https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:15: .then(function() { Lines 14-15 are not necessary: .then(registration => { serviceworker = registration.installing; return new Promise(resolve => { navigator.serviceWorker.onmessage = resolve; serviceworker.postMessage(''); }); }) .then(...) https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/ErrorEvent.h (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/ErrorEvent.h:58: ScriptValue error) { nit: It'd be better to align the argument order with class field declaration and the ctor, that is, (message, location, error, world).
Let me put an additional comment. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:1: <!DOCTYPE html> On 2017/04/06 01:50:35, nhiroki wrote: > We could move this test into > LayoutTests/external/service-workers/service-worker/ServiceWorkerGlobalScope so > that the test will be automatically exported to the wpt's repository. > > (sorry for my misdirection) Along with moving the test under wpt, you should change the suffix to ".https.html" to work with the test runner.
@shimazu, @nhiroki: could you please take another look at this patch? Thank you https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:7: promise_test(function(t) { On 2017/04/06 01:50:35, nhiroki wrote: > Can you add a comment like this? > > // TODO(yiyix): This test will be superseded by > external/wpt/workers/Worker_ErrorEvent_error.htm once https://crbug.com/708857 > is fixed. Done. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:8: return new Promise(function(resolve) { On 2017/04/06 01:50:35, nhiroki wrote: > Optional: Arrow function would be simpler: > > return new Promise(resolve => { > // do something > }) > > see > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/A... Done. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:11: }) On 2017/04/06 01:50:35, nhiroki wrote: > +2 indents for line 8-11 > > return new Promise(resolve => { > var worker = new Worker('resources/error-script.js); > worker.onmessage = resolve; > }) > .then(e => assert_equals(e.data.value, 'testError')); Done. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/error-worker-script.js:9: source.postMessage({ value: e.error }); On 2017/04/06 01:50:35, nhiroki wrote: > How about testing other attributes of ErrorEvent if we don't have such a test > for service workers? Done. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:1: <!DOCTYPE html> On 2017/04/06 01:50:35, nhiroki wrote: > We could move this test into > LayoutTests/external/service-workers/service-worker/ServiceWorkerGlobalScope so > that the test will be automatically exported to the wpt's repository. > > (sorry for my misdirection) i can't find this directory, do you mean LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope? https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:8: var script = 'resources/error-worker-script.js'; On 2017/04/06 01:50:35, nhiroki wrote: > We conventionally use "-worker.js" suffix for worker scripts, so can you remove > "-script" part? Done. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:15: .then(function() { On 2017/04/06 01:50:35, nhiroki wrote: > Lines 14-15 are not necessary: > > .then(registration => { > serviceworker = registration.installing; > return new Promise(resolve => { > navigator.serviceWorker.onmessage = resolve; > serviceworker.postMessage(''); > }); > }) > .then(...) Done. https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/ErrorEvent.h (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/ErrorEvent.h:58: ScriptValue error) { On 2017/04/06 01:50:35, nhiroki wrote: > nit: It'd be better to align the argument order with class field declaration and > the ctor, that is, (message, location, error, world). true, i forget to change the order in the previous change.
lgtm with nits // Please wait for nhiroki's comment. https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js:9: source.postMessage({ value: e.error, "message": e.message, "lineno": e.lineno, "colno": e.colno} ); Please wrap at 80 characters. https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:22: assert_greater_than(e.data.message.indexOf(errorname), -1, 'error message'); ditto
https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/service-worker-error-event.html:1: <!DOCTYPE html> On 2017/04/06 04:38:19, yiyix wrote: > On 2017/04/06 01:50:35, nhiroki wrote: > > We could move this test into > > LayoutTests/external/service-workers/service-worker/ServiceWorkerGlobalScope > so > > that the test will be automatically exported to the wpt's repository. > > > > (sorry for my misdirection) > > i can't find this directory, do you mean > LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope? Yes! https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js:9: source.postMessage({ value: e.error, "message": e.message, "lineno": e.lineno, "colno": e.colno} ); - s/value/error/ ? - Do we need to wrap |message|, |lineno| and |colno| with ""? - How about ErrorEvent.filename? https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:11: var serviceworker; You can move this variable declaration to line 14. https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:15: .then( e => assert_equals(e.data.value, 'testError')); nit: Can you remove an extra space before 'e'? https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js:2: postMessage({ source: "event listener", value: e.error }); "source:" is no longer necessary.
nhiroki@chromium.org changed reviewers: + haraken@chromium.org
+haraken, can you review bindings/core and core/?
LGTM
The CQ bit was checked by yiyix@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:120001) has been deleted
@nhiroki, could you please review this new patch? Thank you https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js:9: source.postMessage({ value: e.error, "message": e.message, "lineno": e.lineno, "colno": e.colno} ); On 2017/04/06 04:52:30, shimazu wrote: > Please wrap at 80 characters. Done.Good call, I will add filename to the test. https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:11: var serviceworker; On 2017/04/06 04:56:44, nhiroki wrote: > You can move this variable declaration to line 14. Done. true, i don't need serviceworker in the second then clause. https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:22: assert_greater_than(e.data.message.indexOf(errorname), -1, 'error message'); On 2017/04/06 04:52:30, shimazu wrote: > ditto Done. https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:15: .then( e => assert_equals(e.data.value, 'testError')); On 2017/04/06 04:56:44, nhiroki wrote: > nit: Can you remove an extra space before 'e'? Done. https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js (right): https://codereview.chromium.org/2804533002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/resources/error-script.js:2: postMessage({ source: "event listener", value: e.error }); On 2017/04/06 04:56:45, nhiroki wrote: > "source:" is no longer necessary. Done.
The CQ bit was checked by yiyix@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...
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by yiyix@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...
LGTM with nits. Thank you for working on this :) https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js (right): https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js:10: value: e.error, filename: e.filename, message: e.message, lineno: e.lineno, value: -> error:? https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html (right): https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:17: serviceworker.postMessage(''); optional: you could squash line 13-14 into this line: registration.installing.postMessage(''); https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:20: .then(function(e) { arrow function? https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:25: e.data.filename.indexOf('error-worker.js'), -1, 'filename'); How about replacing 'error-worker.js' with |script| to remove a fixed string? https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:29: }, 'Error handlers inside serviceworker should see the error value'); ...should see the attributes of ErrorEvent? https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:10: promise_test(function(t) { arrow function?
@shimazu and @nhiroki, Thank you so much for the detailed review. https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js (right): https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/resources/error-worker.js:10: value: e.error, filename: e.filename, message: e.message, lineno: e.lineno, On 2017/04/06 06:56:49, nhiroki wrote: > value: -> error:? Done. https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html (right): https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:17: serviceworker.postMessage(''); On 2017/04/06 06:56:50, nhiroki wrote: > optional: you could squash line 13-14 into this line: > > registration.installing.postMessage(''); Done. https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:20: .then(function(e) { On 2017/04/06 06:56:50, nhiroki wrote: > arrow function? Done. https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:25: e.data.filename.indexOf('error-worker.js'), -1, 'filename'); On 2017/04/06 06:56:50, nhiroki wrote: > How about replacing 'error-worker.js' with |script| to remove a fixed string? Done. https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html (right): https://codereview.chromium.org/2804533002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/workers/dedicated-worker-error-event.html:10: promise_test(function(t) { On 2017/04/06 06:56:50, nhiroki wrote: > arrow function? Done.
Still lgtm, thanks too! ٩(●˙▿˙●)۶
lgtm https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html (right): https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:10: var errorname = 'testError' nit: error_name https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:12: .then(registration => { nit: add_completion_callback(() => { registration.unregister(); }); To clean up the registration. https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:13: var serviceworker = registration.installing; nit: typically we just use "worker" in these tests
https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html (right): https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:10: var errorname = 'testError' On 2017/04/06 07:52:41, falken (ooo-ish until Apr 5) wrote: > nit: error_name Done. https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:12: .then(registration => { On 2017/04/06 07:52:41, falken (ooo-ish until Apr 5) wrote: > nit: add_completion_callback(() => { registration.unregister(); }); > > To clean up the registration. Yes, you are right! https://codereview.chromium.org/2804533002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/service-worker-error-event.https.html:13: var serviceworker = registration.installing; On 2017/04/06 07:52:41, falken (ooo-ish until Apr 5) wrote: > nit: typically we just use "worker" in these tests Done.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, nhiroki@chromium.org, falken@chromium.org, shimazu@chromium.org Link to the patchset: https://codereview.chromium.org/2804533002/#ps200001 (title: "nits")
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": 200001, "attempt_start_ts": 1491481691061320,
"parent_rev": "95d6e720c235f105aa3182e2a1b2f826cfaa483f", "commit_rev":
"0662e1de8c6d9800c4baed479249c1fa5201c24c"}
Message was sent while issue was closed.
Description was changed from ========== Update Error Event inside a worker to provide the exact exception value In WorkerOrWorkletScriptController.cpp, 'ErrorEvent' used to be created without exception value, so the exception, |m_error|, is always set to null. A new constructor of ErrorEvent is added in this patch which takes exception as input and set |m_error| to the exception value. TEST=error-script.js and error-worker-script.js BUG=640724 ========== to ========== Update Error Event inside a worker to provide the exact exception value In WorkerOrWorkletScriptController.cpp, 'ErrorEvent' used to be created without exception value, so the exception, |m_error|, is always set to null. A new constructor of ErrorEvent is added in this patch which takes exception as input and set |m_error| to the exception value. TEST=error-script.js and error-worker-script.js BUG=640724 Review-Url: https://codereview.chromium.org/2804533002 Cr-Commit-Position: refs/heads/master@{#462458} Committed: https://chromium.googlesource.com/chromium/src/+/0662e1de8c6d9800c4baed479249... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/0662e1de8c6d9800c4baed479249...
Message was sent while issue was closed.
On 2017/04/06 at 14:17:41, commit-bot wrote: > Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/0662e1de8c6d9800c4baed479249... A GitHub WPT PR was created for this: https://github.com/w3c/web-platform-tests/pull/5404 But there was a trailing whitespace lint error. I'll fix it on the GitHub branch to see if that goes through and update you here.
Message was sent while issue was closed.
On 2017/04/06 at 18:13:58, jeffcarp wrote: > On 2017/04/06 at 14:17:41, commit-bot wrote: > > Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/0662e1de8c6d9800c4baed479249... > > A GitHub WPT PR was created for this: https://github.com/w3c/web-platform-tests/pull/5404 > But there was a trailing whitespace lint error. I'll fix it on the GitHub branch to see if that goes through and update you here. Update: the PR ran into infra issues but was force merged. https://github.com/w3c/web-platform-tests/pull/5404 |
