[webkit-reviews] review granted: [Bug 208080] Support in-band generic cues when loading media in the GPU Process : [Attachment 391851] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 208080: Support in-band generic cues when loading media in the GPU Process
https://bugs.webkit.org/show_bug.cgi?id=208080

Attachment 391851: Patch

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




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 391851
  --> https://bugs.webkit.org/attachment.cgi?id=391851
Patch

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

> Source/WebCore/html/track/InbandGenericTextTrack.cpp:49
> +TextTrackCueGeneric* GenericTextTrackCueMap::find(InbandGenericCue&
inbandCue)

We could pass for all these methods the cue ID instead of InbandGenericCue
object.

> Source/WebCore/html/track/InbandGenericTextTrack.h:46
> +    using CueToDataMap = HashMap<RefPtr<TextTrackCue>, uint64_t>;

Can we use ObjectIdentifier?

> Source/WebCore/html/track/InbandGenericTextTrack.h:47
> +    using CueDataToCueMap = HashMap<uint64_t, RefPtr<TextTrackCueGeneric>>;

Ditto.

Also, do we need to keep ownership of the keys?
Should it be TextTrackCue*?

> Source/WebCore/platform/graphics/InbandGenericCue.cpp:79
> +	   switch (m_cueData.m_align) {

Can this switch be useful elsewhere?
Should it be made as a helper function?

> Source/WebCore/platform/graphics/InbandGenericCue.cpp:111
> +    if (!m_cueData.m_fontName.isEmpty())

empty or null?

> Source/WebCore/platform/graphics/InbandGenericCue.cpp:144
> +    return true;

Would be more compact as
return m_relativeFontSize == other.m_relativeFontSize
    && m_.... == ...

> Source/WebCore/platform/graphics/InbandGenericCue.h:93
> +	   return WTF::nullopt;

Should we serialise/deserialize invalid IDs?

> Source/WebCore/platform/graphics/InbandGenericCue.h:228
> +    explicit InbandGenericCue();

No need for explicit.
Constructor as private would be better.

> Source/WebCore/platform/graphics/InbandGenericCue.h:284
> +    static uint64_t s_nextUniqueId;

Needed? ObjectIdentifier as well

>
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:87
> +void InbandTextTrackPrivateAVF::processCueAttributes(CFAttributedStringRef
attributedString, InbandGenericCue& cueData)

It seems processCueAttributes is called in one place where cureData is directly
created.
It seems we could then change it to something like
Ref<InbandGenericCue>
InbandTextTrackPrivateAVF::processCueAttributes(CFAttributedStringRef)
Or maybe RefPtr< InbandGenericCue > if there are error cases.

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:99
> +    Vector<RefPtr<InbandGenericCue>> m_cues;

Vector<Ref<>>?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:531
> +    if (const auto& track = m_textTracks.get(identifier))

s/const//
Seems strange that a const auto& track could be calling addGenericCue.
 Ditto below.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:532
> +	  
track->addGenericCue(WTFMove(InbandGenericCue::create(WTFMove(cueData)).get()))
;

Maybe addGenericCue should take a Ref<>.
WTFMove not needed probably.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:540
> +	  
track->updateGenericCue(WTFMove(InbandGenericCue::create(WTFMove(cueData)).get(
)));

WTFMove not needed probably.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:548
> +	  
track->removeGenericCue(WTFMove(InbandGenericCue::create(WTFMove(cueData)).get(
)));

WTFMove not needed probably.

> Source/WebKit/WebProcess/GPU/media/TextTrackPrivateRemote.cpp:91
> +void TextTrackPrivateRemote::addGenericCue(InbandGenericCue&& cue)

Do we really needed a &&? Since it is refcounted, I would expect to either pass
a Ref<>&& or a InbandGenericCue&.
Ditto for the others.

> LayoutTests/gpu-process/TestExpectations:240
> +media/track/track-in-band-metadata-display-order.html [ Pass ]

Nice!