Closed Bug 1275808 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Blocks: 1274884
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.
Blocks: 882717
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.
Attachment #8758131 - Flags: review?(giles) → review+
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/
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e059a7af074e
Move the UpdateCueDisplay() into TimeMarchesOn() as step 18. r=rillian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e059a7af074e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.