Bug 72555

Summary:

Make TextTrackCue more mutable

Product:

WebKit

Reporter:

Anna Cavender <annacc>

Component:

Media

Assignee:

Eric Carlson <eric.carlson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric.carlson, jknotten, ojan, vcarbune, webkit-bug-importer, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

43668

Attachments:

Description	Flags
Proposed patch	none
Pedantic update.	none
Updated patch - hopefully pedantic AND correct this time.	andersca: review+

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	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
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.