Bug 226692 – Reduce use of legacy EventLoopEventQueue and EventLoopTaskQueue

• Component: Media
• Status: RESOLVED FIXED

[Chris Dumez]

Reduce use of legacy EventLoopEventQueue and EventLoopTaskQueue, and use the HTML event loop directly instead.

[Chris Dumez]

Created attachment 430669 [details]
Patch

[Chris Dumez]

Created attachment 430670 [details]
Patch

[Chris Dumez]

Created attachment 430672 [details]
Patch

[Chris Dumez]

Created attachment 430674 [details]
Patch

[Darin Adler]

Comment on attachment 430674 [details]
Patch

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

> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:293
> +    queueTaskToDispatchEvent(*this, TaskSource::MediaElement, event.releaseNonNull());

The old code would silently do nothing if m_state had a bad value. I think this one will crash. Not that a bad value is really possible, but it is a change in behavior.

Also, if we are going to refactor to share more code like this, why not have the switch statement just put the event name into an AtomString or const AtomString*, and share the call to Event::create too?

> Source/WebCore/dom/FullscreenManager.cpp:68
> +                weakThis->dispatchFullscreenChangeEvents();

I’d think that generally we’d want to do something more like:

    makeRef(*weakThis)->dispatchFullscreenChangeEvents();

I will guess that there is no practical real issue in this case, but our object lifetime strategy is normally to say the caller should be holding a reference to objects before calling functions on them. I’d like these rules to be easier to understand and less situational, but this is what I’ve gleaned from talking to others about our lifetime strategy.

[Chris Dumez]

Comment on attachment 430674 [details]
Patch

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

>> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:293
>> +    queueTaskToDispatchEvent(*this, TaskSource::MediaElement, event.releaseNonNull());
> 
> The old code would silently do nothing if m_state had a bad value. I think this one will crash. Not that a bad value is really possible, but it is a change in behavior.
> 
> Also, if we are going to refactor to share more code like this, why not have the switch statement just put the event name into an AtomString or const AtomString*, and share the call to Event::create too?

I did not really see the harm since the switch case was handling all enum values. That said, I like your suggestion of extracting the event from the switch better. I’ll do that before landing.

>> Source/WebCore/dom/FullscreenManager.cpp:68
>> +                weakThis->dispatchFullscreenChangeEvents();
> 
> I’d think that generally we’d want to do something more like:
> 
>     makeRef(*weakThis)->dispatchFullscreenChangeEvents();
> 
> I will guess that there is no practical real issue in this case, but our object lifetime strategy is normally to say the caller should be holding a reference to objects before calling functions on them. I’d like these rules to be easier to understand and less situational, but this is what I’ve gleaned from talking to others about our lifetime strategy.

Not a new issue as the previous code was not protecting this either, but I will make the change since it is safer.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 430674 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430674&action=review
> 
> >> Source/WebCore/Modules/remoteplayback/RemotePlayback.cpp:293
> >> +    queueTaskToDispatchEvent(*this, TaskSource::MediaElement, event.releaseNonNull());
> > 
> > The old code would silently do nothing if m_state had a bad value. I think this one will crash. Not that a bad value is really possible, but it is a change in behavior.
> > 
> > Also, if we are going to refactor to share more code like this, why not have the switch statement just put the event name into an AtomString or const AtomString*, and share the call to Event::create too?
> 
> I did not really see the harm since the switch case was handling all enum
> values. That said, I like your suggestion of extracting the event from the
> switch better. I’ll do that before landing.
> 
> >> Source/WebCore/dom/FullscreenManager.cpp:68
> >> +                weakThis->dispatchFullscreenChangeEvents();
> > 
> > I’d think that generally we’d want to do something more like:
> > 
> >     makeRef(*weakThis)->dispatchFullscreenChangeEvents();
> > 
> > I will guess that there is no practical real issue in this case, but our object lifetime strategy is normally to say the caller should be holding a reference to objects before calling functions on them. I’d like these rules to be easier to understand and less situational, but this is what I’ve gleaned from talking to others about our lifetime strategy.
> 
> Not a new issue as the previous code was not protecting this either, but I
> will make the change since it is safer.

Same issue as with the last patch you suggested this. FullscreenManager is not RefCounted.

[Chris Dumez]

Created attachment 430687 [details]
Patch

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #7)
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 430674 [details]
> > Patch
> > >> Source/WebCore/dom/FullscreenManager.cpp:68
> > >> +                weakThis->dispatchFullscreenChangeEvents();
> > > 
> > > I’d think that generally we’d want to do something more like:
> > > 
> > >     makeRef(*weakThis)->dispatchFullscreenChangeEvents();
> > > 
> > > I will guess that there is no practical real issue in this case, but our object lifetime strategy is normally to say the caller should be holding a reference to objects before calling functions on them. I’d like these rules to be easier to understand and less situational, but this is what I’ve gleaned from talking to others about our lifetime strategy.
> > 
> > Not a new issue as the previous code was not protecting this either, but I
> > will make the change since it is safer.
> 
> Same issue as with the last patch you suggested this. FullscreenManager is
> not RefCounted.

We should CheckedPtr in these instances.

[EWS]

Committed r278538 (238536@main): <https://commits.webkit.org/238536@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78926233>