1344609 - [webvtt] refact the TimeMarchesOn() for :past/:future

Status: NEW

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

Description

[Benjamin Chen [:bechen]]

https://html.spec.whatwg.org/multipage/embedded-content.html#time-marches-on

The rendering for the cue is the last step of TimeMarchesOn. There are some early return during TimeMarchesOn especially the step 7. If we want to support :past :future, we need some modification at TimeMarchesOn.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: Bug 1344609 - Pseudo classes support.

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

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 8845298 [details]
Bug 1344609 - Pseudo classes support.

https://reviewboard.mozilla.org/r/118480/#review120676

General approach looks fine, but it looks like this is just a partial implementation? Actually setting the pseudo-classes is marked 'todo'. So I don't think this is ready to merge.

::: commit-message-9f246:2
(Diff revision 1)
> +Bug 1344609 - Pseudo classes support. r=rillian
> +

Please include the motivation for the change in the commit message. Even the text from the bug description would be good.

::: dom/media/webvtt/vtt.jsm:411
(Diff revision 1)
>            // Otherwise just ignore the end tag.
>            continue;
>          }
>          var ts = collectTimeStamp(t.substr(1, t.length - 2));
> +        // TODO: verify the ts is valid or not, compare to the cue.startTime endTime?
> +        // TODO: ensure the all ts are monotonic inscrease?

Please open a bug for these issues and give the bug number here if you want to include the TODO text.

::: dom/media/webvtt/vtt.jsm:1002
(Diff revision 1)
> +        nodeFilterConstants.SHOW_PROCESSING_INSTRUCTION, null, false);
> +      var targetNode;
> +      var lastNode = timestampWalker.nextNode();
> +      // The cue doesn't have timestamp node inside.
> +      if (!lastNode) {
> +        // TODO: Do we still need to apply :past or :future?

Same here, please.

::: dom/media/webvtt/vtt.jsm:1045
(Diff revision 1)
> +      while (textWalker.nextNode()) {
> +        var node = textWalker.currentNode;
> +        if (node.nodeType == nodeConstants.TEXT_NODE) {
> +          // TODO: apply :past
> +          dump("apply :past\n");
> +          dump(node.data + "\n");

Seem like this isn't ready to land.

Comment 3

[Benjamin Chen [:bechen]]

Comment on attachment 8845298 [details]
Bug 1344609 - Pseudo classes support.

https://reviewboard.mozilla.org/r/118480/#review120784

::: dom/media/webvtt/vtt.jsm:1036
(Diff revision 1)
> +      var textWalker = window.document.createTreeWalker(cueDiv,
> +        nodeFilterConstants.SHOW_TEXT | nodeFilterConstants.SHOW_PROCESSING_INSTRUCTION,

Since the TextNode is not domelement, I can not pply pseudo class on it. I need to use a domelement to wrap the textnode if the textnode doesn't have a domeelement parent.

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8845298 [details]
Bug 1344609 - Pseudo classes support.

https://reviewboard.mozilla.org/r/118480/#review121436

::: dom/media/webvtt/vtt.jsm:1002
(Diff revision 1)
> +        nodeFilterConstants.SHOW_PROCESSING_INSTRUCTION, null, false);
> +      var targetNode;
> +      var lastNode = timestampWalker.nextNode();
> +      // The cue doesn't have timestamp node inside.
> +      if (!lastNode) {
> +        // TODO: Do we still need to apply :past or :future?

https://w3c.github.io/webvtt/#the-past-and-future-pseudo-classes

spec says: "there exists a WebVTT Timestamp Object ....", so I think in this case, we don't need to apply :past or :future on the cue.