1275808 - [webvtt] Merge the TextTrackManager::TimeMarchesOn() and UpdateCueDisplay() function

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Benjamin Chen [:bechen]]

Since the TimeMarchesOn() is called at the end of UpdateCueDisplay(), we should merge them into a function. Also the DispatchTimeMarchesOn() should dispatch a task that runs UpdateCueDisplay()+TimeMarchesOn().

In addition, since we have precise time of every cues, we can invoke the new function more precisely instead every timeupdate, so that we may improve the performance without drawing the same cue at the dom tree through WebVTTParserWrapper.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1275808 - Move the UpdateCueDisplay() into TimeMarchesOn() as step 18. r=rillian

Review commit: https://reviewboard.mozilla.org/r/56500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56500/

Comment 2

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/56498/#review53060

Invoke the UpdateCueDisplay() at the end of TimeMarchesOn() as step 18, and we expose the TimeMarchesOn() and DispatchTimeMarchesOn() as sync/async interface for usage.
The sync function TimeMarchesOn() will fix some bugs if we call it at the right time, ex: change subtitle when MediaElement is paused, the cue will be shown immediately if we call TimeMarchesOn(). And the async function DispatchTimeMarchesOn() is called when the scritp add/remove texttrack/texttrackcue.

And another benefit of calling UpdateCueDisplay() in TimeMarchesOn() is that there are some early return during TimeMarchesOn() so that we won't redraw the same cue at the dom tree.

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8758131 [details]
MozReview Request: Bug 1275808 - Move the UpdateCueDisplay() into TimeMarchesOn() as step 18. r=rillian

https://reviewboard.mozilla.org/r/56500/#review53374

I didn't see an obvious cause for the windows debug mda crashes. Otherwise looks good.

::: dom/html/TextTrackManager.h:105
(Diff revision 1)
>  
>  private:
> -  void TimeMarchesOn();
> +  /**
> +   * Converts the TextTrackCue's cuetext into a tree of DOM objects and attaches
> +   * it to a div on it's owning TrackElement's MediaElement's caption overlay.
> +   */

s/it's/its/ while we're here.

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8758131 [details]
MozReview Request: Bug 1275808 - Move the UpdateCueDisplay() into TimeMarchesOn() as step 18. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56500/diff/1-2/

Comment 5

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c51e53905a5

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e059a7af074e
Move the UpdateCueDisplay() into TimeMarchesOn() as step 18. r=rillian

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e059a7af074e