Add support for AbortController / AbortSignal as a first step to supporting abort for fetch().
Created attachment 322325 [details] Patch
Comment on attachment 322325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322325&action=review Looks like the tests aren’t passing yet. > Source/WebCore/dom/AbortController.h:46 > + AbortController(ScriptExecutionContext&); I think explicit would be good. > Source/WebCore/dom/AbortSignal.cpp:53 > + // 1. If signalâs aborted flag is set, then return. Straighten the quotes perhaps? Not sure how we feel about UTF-8 in our sources. > Source/WebCore/dom/AbortSignal.h:37 > +class AbortSignal final : public RefCounted<AbortSignal>, public EventTargetWithInlineData, public ContextDestructionObserver { Can we derive privately from ContextDestructionObserver? > Source/WebCore/dom/AbortSignal.h:40 > + ~AbortSignal(); I think we can just leave this out and let it get generated. > Source/WebCore/dom/AbortSignal.h:47 > + using RefCounted<AbortSignal>::ref; > + using RefCounted<AbortSignal>::deref; Here inside the class you can write this without repeating the template argument: using RefCounted::ref; using RefCounted::deref; > Source/WebCore/dom/AbortSignal.h:50 > + AbortSignal(ScriptExecutionContext&); I suggest explicit here. > Source/WebCore/dom/AbortSignal.h:52 > + // EventTargetWithInlineData. Not personally a huge fan of comments citing which class we are implementing virtual function overrides for. > Source/WebCore/dom/AbortSignal.h:56 > + EventTargetInterface eventTargetInterface() const override { return AbortSignalEventTargetInterfaceType; } > + ScriptExecutionContext* scriptExecutionContext() const override { return ContextDestructionObserver::scriptExecutionContext(); } > + void refEventTarget() override { ref(); } > + void derefEventTarget() override { deref(); } I suggest final rather than override.
Comment on attachment 322325 [details] Patch Attachment 322325 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4716999 New failing tests: imported/w3c/web-platform-tests/fetch/api/abort/general.any.html imported/w3c/web-platform-tests/dom/interfaces.html js/dom/global-constructors-attributes-dedicated-worker.html imported/w3c/web-platform-tests/fetch/api/abort/general.any.worker.html
Created attachment 322329 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 322325 [details] Patch Attachment 322325 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4717005 New failing tests: imported/w3c/web-platform-tests/fetch/api/abort/general.any.html imported/w3c/web-platform-tests/dom/interfaces.html imported/w3c/web-platform-tests/payment-request/interfaces.https.html js/dom/global-constructors-attributes-dedicated-worker.html imported/w3c/web-platform-tests/fetch/api/abort/general.any.worker.html
Created attachment 322330 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 322325 [details] Patch Attachment 322325 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4717011 New failing tests: imported/w3c/web-platform-tests/fetch/api/abort/general.any.html imported/w3c/web-platform-tests/fetch/api/abort/general.any.worker.html js/dom/global-constructors-attributes-dedicated-worker.html
Created attachment 322332 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Darin Adler from comment #2) > Comment on attachment 322325 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322325&action=review > > Looks like the tests aren’t passing yet. Yup, got more to rebase. > > > Source/WebCore/dom/AbortController.h:46 > > + AbortController(ScriptExecutionContext&); > > I think explicit would be good. Will change. > > > Source/WebCore/dom/AbortSignal.cpp:53 > > + // 1. If signalâs aborted flag is set, then return. > > Straighten the quotes perhaps? Not sure how we feel about UTF-8 in our > sources. I'm personally fine with it. I'm not sure what good reasons there are to limit our selves. > > > Source/WebCore/dom/AbortSignal.h:37 > > +class AbortSignal final : public RefCounted<AbortSignal>, public EventTargetWithInlineData, public ContextDestructionObserver { > > Can we derive privately from ContextDestructionObserver? Probably. Will find out. > > > Source/WebCore/dom/AbortSignal.h:40 > > + ~AbortSignal(); > > I think we can just leave this out and let it get generated. Yup. At one point I had a member I didn't want to #include the header of, but it's not there anymore. > > > Source/WebCore/dom/AbortSignal.h:47 > > + using RefCounted<AbortSignal>::ref; > > + using RefCounted<AbortSignal>::deref; > > Here inside the class you can write this without repeating the template > argument: > > using RefCounted::ref; > using RefCounted::deref; > Neat. > > Source/WebCore/dom/AbortSignal.h:50 > > + AbortSignal(ScriptExecutionContext&); > > I suggest explicit here. Will do. > > > Source/WebCore/dom/AbortSignal.h:52 > > + // EventTargetWithInlineData. > > Not personally a huge fan of comments citing which class we are implementing > virtual function overrides for. I'll remove it. I was cargo-culting my way through adding an EventTarget. > > > Source/WebCore/dom/AbortSignal.h:56 > > + EventTargetInterface eventTargetInterface() const override { return AbortSignalEventTargetInterfaceType; } > > + ScriptExecutionContext* scriptExecutionContext() const override { return ContextDestructionObserver::scriptExecutionContext(); } > > + void refEventTarget() override { ref(); } > > + void derefEventTarget() override { deref(); } > > I suggest final rather than override. Will do.
Comment on attachment 322325 [details] Patch Attachment 322325 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4717035 New failing tests: imported/w3c/web-platform-tests/fetch/api/abort/general.any.html imported/w3c/web-platform-tests/fetch/api/abort/general.any.worker.html imported/w3c/web-platform-tests/payment-request/interfaces.https.html js/dom/global-constructors-attributes-dedicated-worker.html imported/w3c/web-platform-tests/dom/interfaces.html
Created attachment 322333 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Committed r222692: <http://trac.webkit.org/changeset/222692>
<rdar://problem/34761538>
Update TestExpectations for new flakey test that this triggered (the test itself is flakey when failing) with r222694.
Comment on attachment 322325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322325&action=review > Source/WebCore/DerivedSources.make:411 > + $(WebCore)/dom/AbortController.idl \ > + $(WebCore)/dom/AbortController.idl \ Was there a reason you added this idl twice? Can I remove the duplicate?
(In reply to Keith Miller from comment #15) > Comment on attachment 322325 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322325&action=review > > > Source/WebCore/DerivedSources.make:411 > > + $(WebCore)/dom/AbortController.idl \ > > + $(WebCore)/dom/AbortController.idl \ > > Was there a reason you added this idl twice? Can I remove the duplicate? It was a mistake. You can remove it!