[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