[webkit-reviews] review granted: [Bug 226700] Stop using legacy EventLoopDeferrableTask : [Attachment 430701] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 226700: Stop using legacy EventLoopDeferrableTask
https://bugs.webkit.org/show_bug.cgi?id=226700

Attachment 430701: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 430701
  --> https://bugs.webkit.org/attachment.cgi?id=430701
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430701&action=review

> Source/WTF/wtf/CancellableTask.h:36
> +class CancellableTask {

I’d find these new classes easier to read if the multi-line function bodies
were not included int he class definitions. Single-line work well, or
multi-line inlines after the class definitions. With this style, there’s no
interface summary since it’s interspersed with the code.

> Source/WTF/wtf/CancellableTask.h:72
> +class CancellableTaskHandle {

I suggest this just be a public member class of CancellableTask called Handle,
and not a separate namespace-level class.

> Source/WTF/wtf/CancellableTask.h:80
> +    void cancel()
> +    {
> +	   if (m_taskWrapper)
> +	       m_taskWrapper->task = nullptr;
> +    }

Seems like for most uses we want to call cancel in the destructor and the move
assignment operator when overwriting an old value. Did you consider that
design? It would get rid of the list of cancel function calls in all the
destructors of the classes that use this.

Could result in canceling by accident, but it seems every case you are using
this we want something like that. These are handles we hold onto for the
lifetime of a task.

One problem with that design idea is if you copy one of these instead of moving
it, because that would have a side effect of canceling. That could be fixed by
deleting the copy constructor and copy assignment operator. Should be no need
to copy these.