Make TextTrackCue more mutable

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

Several attributes of TextTrackCue should not be readonly, they should be settable.  Also, upon setting, some attributes should throw errors for improper values.

http://www.whatwg.org/specs/web-apps/current-work/#texttrackcue

[Radar WebKit Bug Importer]

<rdar://problem/10464483>

[Eric Carlson]

Link to TextTrackCue in the multi-page spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#texttrackcue

[Eric Carlson]

Also, getCueAsSource() has been replaced with a mutable 'text' attribute.

[Eric Carlson]

Created attachment 121317 [details]
Proposed patch

[Eric Carlson]

(In reply to comment #3)
> Also, getCueAsSource() has been replaced with a mutable 'text' attribute.

The patch was big enough that I wrote up https://bugs.webkit.org/show_bug.cgi?id=75646 for these changes.

[WebKit Review Bot]

Attachment 121317 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

Source/WebCore/html/TextTrackCue.h:72:  The parameter name "b" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/TextTrackCue.h:78:  The parameter name "b" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/TextTrackCue.cpp:274:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 121320 [details]
Pedantic update.

[Eric Carlson]

Created attachment 121321 [details]
Updated patch - hopefully pedantic AND correct this time.

[Anders Carlsson]

Comment on attachment 121321 [details]
Updated patch - hopefully pedantic AND correct this time.

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

> Source/WebCore/html/TextTrackCue.cpp:197
> +    if (direction != m_writingDirection) {

I'd make this an early return instead.

> Source/WebCore/html/TextTrackCue.cpp:304
> +    if (alignment != m_cueAlignment) {

Early return.

> Source/WebCore/html/TextTrackCue.h:63
> +    void setId(String);

This should take a const String&

[Eric Carlson]

http://trac.webkit.org/changeset/104327

[John Knottenbelt]

This patch seems to have broken test media/track/track-cues-seeking.html on chromium platform. This was diff in outputs:

@@ -13,19 +13,19 @@
 EVENT(seeked)
 
 EXPECTED (video.currentTime == '1') OK
-EXPECTED (activeCues.length == '1') OK
+EXPECTED (activeCues.length == '1'), OBSERVED '0' FAIL
 RUN(video.currentTime = 1.5)
 
 EVENT(seeked)
 
 EXPECTED (video.currentTime == '1.5') OK
-EXPECTED (activeCues.length == '2') OK
+EXPECTED (activeCues.length == '2'), OBSERVED '0' FAIL
 RUN(video.currentTime = 2)
 
 EVENT(seeked)
 
 EXPECTED (video.currentTime == '2') OK
-EXPECTED (activeCues.length == '3') OK
+EXPECTED (activeCues.length == '3'), OBSERVED '0' FAIL
 RUN(video.currentTime = 2.5)
 

So it seems that perhaps the activeCues haven't been loaded by the time the seeking takes place. Is this a fault with this patch, or does the test need to be updated in some way?