RESOLVED FIXED 72555
Make TextTrackCue more mutable 
https://bugs.webkit.org/show_bug.cgi?id=72555
Summary Make TextTrackCue more mutable
Anna Cavender
Reported 2011-11-16 14:59:56 PST
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
Attachments
Proposed patch (30.12 KB, patch)
2012-01-05 13:03 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Pedantic update. (30.48 KB, patch)
2012-01-05 13:21 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Updated patch - hopefully pedantic AND correct this time. (30.01 KB, patch)
2012-01-05 13:23 PST, Eric Carlson 		andersca: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2011-11-17 12:40:08 PST
<rdar://problem/10464483>
Eric Carlson
Comment 2 2011-11-18 09:42:27 PST
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
Comment 3 2011-11-18 09:43:05 PST
Also, getCueAsSource() has been replaced with a mutable 'text' attribute.
Eric Carlson
Comment 4 2012-01-05 13:03:28 PST
Created attachment 121317 [details] Proposed patch
Eric Carlson
Comment 5 2012-01-05 13:07:34 PST
(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
Comment 6 2012-01-05 13:11:45 PST
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
Comment 7 2012-01-05 13:21:47 PST
Created attachment 121320 [details] Pedantic update.
Eric Carlson
Comment 8 2012-01-05 13:23:09 PST
Created attachment 121321 [details] Updated patch - hopefully pedantic AND correct this time.
Anders Carlsson
Comment 9 2012-01-06 12:32:53 PST
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
Comment 10 2012-01-06 13:13:30 PST
http://trac.webkit.org/changeset/104327
John Knottenbelt
Comment 11 2012-01-09 04:34:30 PST
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?
Note You need to log in before you can comment on or make changes to this bug.