Bug 177718 – Add support for DOM aborting (https://dom.spec.whatwg.org/#aborting-ongoing-activities)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Add support for AbortController / AbortSignal as a first step to supporting abort for fetch().

[Sam Weinig]

Created attachment 322325 [details]
Patch

[Darin Adler]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

(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.

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

Committed r222692: <http://trac.webkit.org/changeset/222692>

[Radar WebKit Bug Importer]

<rdar://problem/34761538>

[Sam Weinig]

Update TestExpectations for new flakey test that this triggered (the test itself is flakey when failing) with r222694.

[Keith Miller]

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?

[Sam Weinig]

(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!