[webkit-reviews] review granted: [Bug 177718] Add support for DOM aborting (https://dom.spec.whatwg.org/#aborting-ongoing-activities) : [Attachment 322325] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 1 13:40:18 PDT 2017


Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 177718: Add support for DOM aborting
(https://dom.spec.whatwg.org/#aborting-ongoing-activities)
https://bugs.webkit.org/show_bug.cgi?id=177718

Attachment 322325: Patch

https://bugs.webkit.org/attachment.cgi?id=322325&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 322325
  --> https://bugs.webkit.org/attachment.cgi?id=322325
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.


More information about the webkit-reviews mailing list