[webkit-reviews] review granted: [Bug 226692] Reduce use of legacy EventLoopEventQueue and EventLoopTaskQueue : [Attachment 430674] 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 226692: Reduce use of legacy EventLoopEventQueue and EventLoopTaskQueue
https://bugs.webkit.org/show_bug.cgi?id=226692

Attachment 430674: Patch

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




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