Bug 105473 – Implement :future pseudo class for the WebVTT ::cue pseudo element

• Component: Media
• Status: RESOLVED FIXED

[Dima Gorbik]

:future pseudoClass should match all WebVTT nodes that are in the future. http://dev.w3.org/html5/webvtt/#the-':past'-and-':future'-pseudo-classes

[Radar WebKit Bug Importer]

<rdar://problem/12913956>

[Dima Gorbik]

Created attachment 180244 [details]
Proposed fix 0.1

[Build Bot]

Comment on attachment 180244 [details]
Proposed fix 0.1

Attachment 180244 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15402887

[WebKit Review Bot]

Comment on attachment 180244 [details]
Proposed fix 0.1

Attachment 180244 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15400826

New failing tests:
media/track/track-cue-rendering-inner-timestamps.html

[WebKit Review Bot]

Comment on attachment 180244 [details]
Proposed fix 0.1

Attachment 180244 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15418794

New failing tests:
media/track/track-cue-rendering-inner-timestamps.html

[Dima Gorbik]

Created attachment 180465 [details]
Proposed fix 0.2

Looks like the compile assertion was failing for the windows build because values inside the bitfield were not aligned.
Test case needs more investigation.

[Dima Gorbik]

Created attachment 180470 [details]
Proposed fix 0.3

track-cue-rendering-inner-timestamps was deleted because the test was based on the older cue rendering behavior. The rendering tree of the cue is not changing now, only different styles are being applied.

[Dima Gorbik]

Created attachment 180558 [details]
Proposed fix 0.4

Rebased the patch, should be able to apply the patch on windows.

[Eric Carlson]

Comment on attachment 180558 [details]
Proposed fix 0.4

View in context: https://bugs.webkit.org/attachment.cgi?id=180558&action=review

> LayoutTests/ChangeLog:12
> +        * media/track/captions-webvtt/styling.vtt:
> +        * media/track/track-css-matching-expected.txt:
> +        * media/track/track-css-matching.html:

This does not list all of the changed and modified files.

Why was track-cue-rendering-inner-timestamps.html deleted?

> LayoutTests/media/track/track-css-matching.html:43
> +                consoleWrite("");
> +                consoleWrite("");
> +                consoleWrite("\n\n2. Test that cues are being matched properly by the ':future' pseudo class.");

Nit: consoleWrite("\n\n") doesn't introduce white-space. Use consoleWrite("<br><br>") instead.

> Source/WebCore/ChangeLog:18
> +        (WebCore::CSSSelector::pseudoId):
> +        (WebCore::nameToPseudoTypeMap):
> +        (WebCore::CSSSelector::extractPseudoType):

Please add per-method comments describing what you added/changed.

[Antti Koivisto]

Comment on attachment 180558 [details]
Proposed fix 0.4

View in context: https://bugs.webkit.org/attachment.cgi?id=180558&action=review

Looks good, r=me. Please fix the style issues.

> Source/WebCore/css/StyleResolver.cpp:1243
> +    if (element->isWebVTTNode() !=  m_element->isWebVTTNode())

extra space after !=

> Source/WebCore/dom/NodeRareData.h:276
>          , m_childrenAffectedByBackwardPositionalRules(false)
> +#if ENABLE(VIDEO_TRACK)
> +    , m_WebVTTNodeType(TextTrack::WebVTTNodeTypeNone)
> +#endif

intendation.

[Antti Koivisto]

Comment on attachment 180558 [details]
Proposed fix 0.4

View in context: https://bugs.webkit.org/attachment.cgi?id=180558&action=review

> Source/WebCore/html/track/TextTrackCue.cpp:667
> +void TextTrackCue::setNodeObjectFlags(Node *root, double previousTimestamp, double movieTime)
> +{

This function could use a more descriptive name. markFutureAndPastNodes or something.

This could be tightened to take ContainerNode instead of Node.

* belongs next to Node.

[Antti Koivisto]

Comment on attachment 180558 [details]
Proposed fix 0.4

View in context: https://bugs.webkit.org/attachment.cgi?id=180558&action=review

> Source/WebCore/html/track/TextTrackCue.h:138
> +    void setNodeObjectFlags(Node *, double, double);

Actually it can be tightened to take Element. Also * placement here.

> Source/WebCore/html/track/TextTrackCue.h:211
> -    RefPtr<HTMLDivElement> m_pastDocumentNodes;
> -    RefPtr<HTMLDivElement> m_futureDocumentNodes;
> +    RefPtr<HTMLDivElement> m_allDocumentNodes;

Name m_allDocumentNodes makes little sense except as a reference that there used to be two separate tree. At least drop "all" and preferably find a better name "m_displayTree"?

[Dima Gorbik]

Created attachment 181125 [details]
Proposed fix 0.5

[Dima Gorbik]

Renaming m_allDocumentNodes will cause renaming already existing m_displayTree so it's probably better to refactor that with a separate patch.

[Silvia Pfeiffer]

Why are you removing LayoutTests/media/track/captions-webvtt/captions-inner-timestamps.vtt and LayoutTests/media/track/track-cue-rendering-inner-timestamps.html ? That test is not checking CSS, but checking whether inner timestamps are handled correctly, including when there is seeking.

[Dima Gorbik]

(In reply to comment #15)
> Why are you removing LayoutTests/media/track/captions-webvtt/captions-inner-timestamps.vtt and LayoutTests/media/track/track-cue-rendering-inner-timestamps.html ? That test is not checking CSS, but checking whether inner timestamps are handled correctly, including when there is seeking.

They check wether the nodes are put property in the past and future containers. I've removed container, so the test is worthless since all nodes are put in one single container now.

[Silvia Pfeiffer]

(In reply to comment #16)
> (In reply to comment #15)
> > Why are you removing LayoutTests/media/track/captions-webvtt/captions-inner-timestamps.vtt and LayoutTests/media/track/track-cue-rendering-inner-timestamps.html ? That test is not checking CSS, but checking whether inner timestamps are handled correctly, including when there is seeking.
> 
> They check wether the nodes are put property in the past and future containers. I've removed container, so the test is worthless since all nodes are put in one single container now.

Thanks for the clarification.

[Victor Carbune]

Would be nice to have some more complexity in the added test,
especially since you have also fixed the node traversal with this patch (nice!).

I'm thinking particularly of some not very intuitive cases with nested nodes,
but probably filing a bug for now and adding more complicated cases later
would also be an option.

[Dima Gorbik]

Created attachment 181256 [details]
Proposed fix 0.6

[Dima Gorbik]

Created attachment 181257 [details]
Proposed fix 0.7

[Eric Seidel (no email)]

Comment on attachment 180558 [details]
Proposed fix 0.4

Cleared Antti Koivisto's review+ from obsolete attachment 180558 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Review Bot]

Comment on attachment 181257 [details]
Proposed fix 0.7

Clearing flags on attachment: 181257

Committed r138784: <http://trac.webkit.org/changeset/138784>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dima Gorbik]

Created attachment 181371 [details]
Proposed fix 0.8

[Ryosuke Niwa]

Comment on attachment 181371 [details]
Proposed fix 0.8

View in context: https://bugs.webkit.org/attachment.cgi?id=181371&action=review

> Source/WebCore/dom/NodeRareData.h:407
> -
> +#if ENABLE(VIDEO_TRACK)
> +    TextTrack::WebVTTNodeType m_WebVTTNodeType : 2;
> +#endif

We can’t do this. cl.exe (MSVC) will add variables of different types.