Closed Bug 1409983 Opened 2 years ago Closed 2 years ago

[webvtt] Reorder the shouldCompute() function in vtt.jsm.

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

Details

Attachments

(1 file)

In WebVTT.processCues function,
1. we first remove all child nodes of MediaElement, then 
2. call shouldCompute() to see if we can reduce the computing.
It is better that we call shouldCompute() first, then remove all child nodes.

It blocks region implementation because I'm trying to add "regionNode" which is the parent of cue's displaystate, and the "regionNode" will be removed at step 1.
Comment on attachment 8920046 [details]
Bug 1409983 - Reorder the shouldCompute() funciton and "remove all child nodes of MediaElement" in vtt.jsm.

https://reviewboard.mozilla.org/r/191032/#review196684

Thanks!
Attachment #8920046 - Flags: review?(alwu) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e6296bec0ce1
Reorder the shouldCompute() funciton and "remove all child nodes of MediaElement" in vtt.jsm. r=alwu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6296bec0ce1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Should we uplift this patch to vtt.js[1]?

[1]https://github.com/mozilla/vtt.js/tree/master
Flags: needinfo?(bechen)
(In reply to Blake Wu [:bwu][:blakewu] from comment #5)
> Should we uplift this patch to vtt.js[1]?
> 
> [1]https://github.com/mozilla/vtt.js/tree/master

Maybe.
The reason that we should uplift is performance improvement.
On the other hand, it is just a small patch/change, not a new feature. We can uplift this one when we decide to add region feature into vtt.js.
Flags: needinfo?(bechen)
You need to log in before you can comment on or make changes to this bug.