Bug 226700 – Stop using legacy EventLoopDeferrableTask

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Chris Dumez]

Stop using legacy EventLoopDeferrableTask and use the EventLoop directly.

[Chris Dumez]

Created attachment 430698 [details]
Patch

[Chris Dumez]

Created attachment 430701 [details]
Patch

[Darin Adler]

Comment on attachment 430701 [details]
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.

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 430701 [details]
> 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.

Ok, will do.

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

I had this initially but I didn't want to make TaskWrapper public. I had trouble finding a way to make CancellableTaskHandle a public member class of CancellableTask without making TaskWrapper public, given that CancellableTaskHandle refers to TaskWrapper. I'll try again.

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

I'll look into this design and see if it indeed looks better.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Darin Adler from comment #3)
> > Comment on attachment 430701 [details]
> > 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.
> 
> Ok, will do.
> 
> > 
> > > 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.
> 
> I had this initially but I didn't want to make TaskWrapper public. I had
> trouble finding a way to make CancellableTaskHandle a public member class of
> CancellableTask without making TaskWrapper public, given that
> CancellableTaskHandle refers to TaskWrapper. I'll try again.
> 
> > 
> > > 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.
> 
> I'll look into this design and see if it indeed looks better.

In my current patch, if you call queueCancellableTaskKeepingObjectAlive() and ignore the return value, the task will still be scheduled (as one might expect). If destroying the value returned by the function cancels the task then it wouldn't. I guess we alleviate that using a WARN_UNUSED_RETURN. I also think I would need another name than "Handle" for that class if destroying it would cancel the task.

[Chris Dumez]

Maybe something like this?
```
class CancellableTask : public CanMakeWeakPtr<CancellableTask> {
public:
    explicit CancellableTask(Function<void()>&& task) : m_task(WTFMove(task)) { }

    bool isPending() const { return !!m_task; }

    class WeakCallable {
    public:
        explicit WeakCallable(CancellableTask& task) : m_task(makeWeakPtr(task)) { }
        void operator()() { if (std::exchange(m_task, nullptr)) m_task(); }
    private:
        WeakPtr<CancellableTask> m_task;
    };

    WeakCallable createWeakCallable() { return WeakCallable { *this }; }

private:
    Function<void()> m_task;
};
```

Then the user can create a CancellableTask, create a WeakCallable from it and schedule it on the event loop, and store the CancellableTask as a data member. To cancel the task, they'd just null out their data member.

I guess I'd rename queueCancellableTaskKeepingThisObjectAlive() to createAndQueueCancellableTaskKeepingThisObjectAlive() and use WARN_UNUSED_RETURN.

Do you think that would be better? Any better suggestion on either the pattern or naming?

[Darin Adler]

(In reply to Chris Dumez from comment #5)
> In my current patch, if you call queueCancellableTaskKeepingObjectAlive()
> and ignore the return value, the task will still be scheduled (as one might
> expect).

But you should have called the non-cancellable version in that case.

It's unusual, to get a handle for use for cancelling and then not store it anyway.

> I guess we alleviate that using a WARN_UNUSED_RETURN.

Yes, one way.

> I also think I would need another name than "Handle" for that class if
> destroying it would cancel the task.

Agreed.

The way we are using it, though, a handle that we have to call cancel on is not really the right design, because we’re always calling cancel when we destroy all of these. The same reason we made the handle in the first place is the reason we want to cancel.

[Darin Adler]

Comment on attachment 430701 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5626
>  void HTMLMediaElement::closeTaskQueues()

Maybe this can call cancelPendingTasks to cut down on the repetitive code.

[Darin Adler]

Honestly, I don’t think you should change it. Looking more carefully it is obviously OK for the two places you are currently using it and it’s probably over-engineering to work hard on the design for something we will not use that often.

I do think it’s annoying that you have to remember to use call cancel on one of these handles before you overwrite or destroy it. It seems it would always ve a programming mistake to not do that. So you could assert in the destructor and the assignment operator that isPending is false.

In HTMLMediaElement, the destroy case never happens because of the "keeping object alive" feature of the queue. And the overwrite case never happens because every client checks isPending before creating a new task.

In DocumentTimelinesController, the destroy case never happens because we call cancel in the destructor, and the overwrite case never happens because it checks isPending before creating a new task.

All that seems a little "manual", but it will work well enough.

If we were thinking of using this instead of capturing WeakPtr I might have a different view.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Maybe something like this?
> ```
> class CancellableTask : public CanMakeWeakPtr<CancellableTask> {
> public:
>     explicit CancellableTask(Function<void()>&& task) :
> m_task(WTFMove(task)) { }
> 
>     bool isPending() const { return !!m_task; }
> 
>     class WeakCallable {
>     public:
>         explicit WeakCallable(CancellableTask& task) :
> m_task(makeWeakPtr(task)) { }
>         void operator()() { if (std::exchange(m_task, nullptr)) m_task(); }
>     private:
>         WeakPtr<CancellableTask> m_task;
>     };
> 
>     WeakCallable createWeakCallable() { return WeakCallable { *this }; }
> 
> private:
>     Function<void()> m_task;
> };
> ```
> 
> Then the user can create a CancellableTask, create a WeakCallable from it
> and schedule it on the event loop, and store the CancellableTask as a data
> member. To cancel the task, they'd just null out their data member.
> 
> I guess I'd rename queueCancellableTaskKeepingThisObjectAlive() to
> createAndQueueCancellableTaskKeepingThisObjectAlive() and use
> WARN_UNUSED_RETURN.
> 
> Do you think that would be better? Any better suggestion on either the
> pattern or naming?

So you were not a fan of this particular proposal? It is actually less code and I have it mostly implemented.

[Chris Dumez]

Created attachment 430704 [details]
Alternative proposal

[Chris Dumez]

(In reply to Chris Dumez from comment #11)
> Created attachment 430704 [details]
> Alternative proposal

I uploaded the alternative proposal based on your suggestion. Let me know if you prefer this one. I think it is nice. One limitation of this design is that you can no longer have a cancellable task that outlives its scheduler. However, this is not a use-case we have at the moment.

[Chris Dumez]

Created attachment 430705 [details]
Reviewed patch

[Darin Adler]

I like both. Your newer proposal also seems really good.

[Chris Dumez]

Created attachment 430708 [details]
Patch

[EWS]

Committed r278543 (238541@main): <https://commits.webkit.org/238541@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430708 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78933487>