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

Status: RESOLVED FIXED

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

Description

[Benjamin Chen [:bechen]]

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 1

[Benjamin Chen [:bechen]]

Attachment: Bug 1409983 - Reorder the shouldCompute() funciton and "remove all child nodes of MediaElement" in vtt.jsm.

We should call shouldCompute() function first, then remove all children of MediaElement.

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

Comment 2

[Alastor Wu [:alwu][PTO 11/1-11/9]]

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!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6296bec0ce1
Reorder the shouldCompute() funciton and "remove all child nodes of MediaElement" in vtt.jsm. r=alwu

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 5

[Blake Wu [:bwu][:blakewu]]

Should we uplift this patch to vtt.js[1]?

[1]https://github.com/mozilla/vtt.js/tree/master

Comment 6

[Benjamin Chen [: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.