Bug 72555 - Make TextTrackCue more mutable
Summary: Make TextTrackCue more mutable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2011-11-16 14:59 PST by Anna Cavender
Modified: 2012-01-09 04:34 PST (History)
7 users (show)

See Also:


Attachments
Proposed patch (30.12 KB, patch)
2012-01-05 13:03 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Pedantic update. (30.48 KB, patch)
2012-01-05 13:21 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch - hopefully pedantic AND correct this time. (30.01 KB, patch)
2012-01-05 13:23 PST, Eric Carlson
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 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
Comment 1 Radar WebKit Bug Importer 2011-11-17 12:40:08 PST
<rdar://problem/10464483>
Comment 2 Eric Carlson 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
Comment 3 Eric Carlson 2011-11-18 09:43:05 PST
Also, getCueAsSource() has been replaced with a mutable 'text' attribute.
Comment 4 Eric Carlson 2012-01-05 13:03:28 PST
Created attachment 121317 [details]
Proposed patch
Comment 5 Eric Carlson 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.
Comment 6 WebKit Review Bot 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.
Comment 7 Eric Carlson 2012-01-05 13:21:47 PST
Created attachment 121320 [details]
Pedantic update.
Comment 8 Eric Carlson 2012-01-05 13:23:09 PST
Created attachment 121321 [details]
Updated patch - hopefully pedantic AND correct this time.
Comment 9 Anders Carlsson 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&
Comment 10 Eric Carlson 2012-01-06 13:13:30 PST
http://trac.webkit.org/changeset/104327
Comment 11 John Knottenbelt 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?