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

RESOLVED FIXED 105473

Implement :future pseudo class for the WebVTT ::cue pseudo element

https://bugs.webkit.org/show_bug.cgi?id=105473

Summary Implement :future pseudo class for the WebVTT ::cue pseudo element

Dima Gorbik

Reported 2012-12-19 15:38:36 PST

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

Attachments
Proposed fix 0.1 (26.46 KB, patch) 2012-12-19 16:35 PST, Dima Gorbik	buildbot: commit-queue-	Details Formatted Diff Diff
Proposed fix 0.2 (27.13 KB, patch) 2012-12-20 20:08 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
Proposed fix 0.3 (36.12 KB, patch) 2012-12-20 20:36 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
Proposed fix 0.4 (36.12 KB, patch) 2012-12-21 14:45 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
Proposed fix 0.5 (36.93 KB, patch) 2013-01-02 17:53 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
Proposed fix 0.6 (37.71 KB, patch) 2013-01-03 17:45 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
Proposed fix 0.7 (37.71 KB, patch) 2013-01-03 17:59 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
Proposed fix 0.8 (37.70 KB, patch) 2013-01-04 13:54 PST, Dima Gorbik	dgorbik: review- dgorbik: commit-queue-	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2012-12-19 15:44:23 PST

<rdar://problem/12913956>

Dima Gorbik

Comment 2 2012-12-19 16:35:21 PST

Created attachment 180244 [details] Proposed fix 0.1

Build Bot

Comment 3 2012-12-19 17:12:04 PST

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 4 2012-12-19 19:59:14 PST

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 5 2012-12-19 20:46:21 PST

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

Comment 6 2012-12-20 20:08:59 PST

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

Comment 7 2012-12-20 20:36:09 PST

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

Comment 8 2012-12-21 14:45:37 PST

Created attachment 180558 [details] Proposed fix 0.4 Rebased the patch, should be able to apply the patch on windows.

Eric Carlson

Comment 9 2012-12-23 15:58:49 PST

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 10 2013-01-02 14:40:43 PST

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 11 2013-01-02 14:52:43 PST

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 12 2013-01-02 15:00:30 PST

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

Comment 13 2013-01-02 17:53:18 PST

Created attachment 181125 [details] Proposed fix 0.5

Dima Gorbik

Comment 14 2013-01-02 17:54:35 PST

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

Silvia Pfeiffer

Comment 15 2013-01-02 20:54:37 PST

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

Comment 16 2013-01-03 14:39:48 PST

(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

Comment 17 2013-01-03 16:30:29 PST

(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

Comment 18 2013-01-03 16:44:22 PST

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

Comment 19 2013-01-03 17:45:39 PST

Created attachment 181256 [details] Proposed fix 0.6

Dima Gorbik

Comment 20 2013-01-03 17:59:37 PST

Created attachment 181257 [details] Proposed fix 0.7

Eric Seidel (no email)

Comment 21 2013-01-04 00:42:34 PST

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 22 2013-01-04 01:17:15 PST

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

Comment 23 2013-01-04 01:17:22 PST

All reviewed patches have been landed. Closing bug.

Dima Gorbik

Comment 24 2013-01-04 13:54:43 PST

Created attachment 181371 [details] Proposed fix 0.8

Ryosuke Niwa

Comment 25 2013-01-04 17:36:48 PST

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.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Media

Assignee

Nobody

Reported

2012-12-19 15:38 PST

Modified

2013-01-04 17:36 PST History

CC List

14 users Show

URL

Keywords InRadar, WebExposed

Depends on

Blocks

43668

Dependencies

tree graph