Bug 105473

Summary: Implement :future pseudo class for the WebVTT ::cue pseudo element
Product: WebKit Reporter: Dima Gorbik <dgorbik>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, ddkilzer, dglazkov, eric.carlson, feature-media-reviews, koivisto, macpherson, menard, ojan.autocc, silviapf, vcarbune, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 43668    
Attachments:
Description Flags
Proposed fix 0.1
buildbot: commit-queue-
Proposed fix 0.2
none
Proposed fix 0.3
none
Proposed fix 0.4
none
Proposed fix 0.5
none
Proposed fix 0.6
none
Proposed fix 0.7
none
Proposed fix 0.8 dgorbik: review-, dgorbik: commit-queue-

Description Dima Gorbik 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
Comment 1 Radar WebKit Bug Importer 2012-12-19 15:44:23 PST
<rdar://problem/12913956>
Comment 2 Dima Gorbik 2012-12-19 16:35:21 PST
Created attachment 180244 [details]
Proposed fix 0.1
Comment 3 Build Bot 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
Comment 4 WebKit Review Bot 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
Comment 5 WebKit Review Bot 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
Comment 6 Dima Gorbik 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.
Comment 7 Dima Gorbik 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.
Comment 8 Dima Gorbik 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.
Comment 9 Eric Carlson 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.
Comment 10 Antti Koivisto 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.
Comment 11 Antti Koivisto 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.
Comment 12 Antti Koivisto 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"?
Comment 13 Dima Gorbik 2013-01-02 17:53:18 PST
Created attachment 181125 [details]
Proposed fix 0.5
Comment 14 Dima Gorbik 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.
Comment 15 Silvia Pfeiffer 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.
Comment 16 Dima Gorbik 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.
Comment 17 Silvia Pfeiffer 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.
Comment 18 Victor Carbune 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.
Comment 19 Dima Gorbik 2013-01-03 17:45:39 PST
Created attachment 181256 [details]
Proposed fix 0.6
Comment 20 Dima Gorbik 2013-01-03 17:59:37 PST
Created attachment 181257 [details]
Proposed fix 0.7
Comment 21 Eric Seidel (no email) 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.
Comment 22 WebKit Review Bot 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>
Comment 23 WebKit Review Bot 2013-01-04 01:17:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Dima Gorbik 2013-01-04 13:54:43 PST
Created attachment 181371 [details]
Proposed fix 0.8
Comment 25 Ryosuke Niwa 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.