:future pseudoClass should match all WebVTT nodes that are in the future. http://dev.w3.org/html5/webvtt/#the-':past'-and-':future'-pseudo-classes
<rdar://problem/12913956>
Created attachment 180244 [details] Proposed fix 0.1
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
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
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
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.
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.
Created attachment 180558 [details] Proposed fix 0.4 Rebased the patch, should be able to apply the patch on windows.
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.
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.
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.
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"?
Created attachment 181125 [details] Proposed fix 0.5
Renaming m_allDocumentNodes will cause renaming already existing m_displayTree so it's probably better to refactor that with a separate patch.
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.
(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.
(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.
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.
Created attachment 181256 [details] Proposed fix 0.6
Created attachment 181257 [details] Proposed fix 0.7
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.
Comment on attachment 181257 [details] Proposed fix 0.7 Clearing flags on attachment: 181257 Committed r138784: <http://trac.webkit.org/changeset/138784>
All reviewed patches have been landed. Closing bug.
Created attachment 181371 [details] Proposed fix 0.8
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.