[webkit-reviews] review granted: [Bug 191004] [GTK] Layout test media/track/track-cue-missing.html is failing : [Attachment 428626] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 14 11:18:46 PDT 2021
Alicia Boya García <aboya at igalia.com> has granted Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 191004: [GTK] Layout test media/track/track-cue-missing.html is failing
https://bugs.webkit.org/show_bug.cgi?id=191004
Attachment 428626: Patch
https://bugs.webkit.org/attachment.cgi?id=428626&action=review
--- Comment #17 from Alicia Boya García <aboya at igalia.com> ---
Comment on attachment 428626
--> https://bugs.webkit.org/attachment.cgi?id=428626
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=428626&action=review
LGTM, r+ with the rename nits.
>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2868
> + m_playbackRate >= 0 ? TaskAtMediaTimeScheduler::LessOrEqual :
TaskAtMediaTimeScheduler::GreaterOrEqual);
Shouldn't this be reversed? If m_playbackRate >= 0 we want to check when time
is greater or equal to the target. The code being implemented with early return
(and therefore negative condition) should not affect this.
>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2871
> + if (taskToSchedule.hasValue())
Add a explanatory comment: // Dispatch the task synchronously if the time is
already reached.
> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:405
> + Optional<Function<void()>> scheduleIfTimeReached(const MediaTime&
currentTime)
The name of the function is now confusing if the scheduling is not done inside.
We could have a more descriptive name. What about `checkTaskForScheduling()`?
More information about the webkit-reviews
mailing list