Bug 104593 - Add in-band text track cues only once
Summary: Add in-band text track cues only once
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:
 
Reported: 2012-12-10 13:47 PST by Eric Carlson
Modified: 2012-12-18 07:24 PST (History)
7 users (show)

See Also:

Attachments
Proposed patch (15.80 KB, patch)
2012-12-17 15:29 PST, Eric Carlson 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Skipped new tests on platforms that don't support it. (deleted)
2012-12-17 16:29 PST, Eric Carlson 		no flags Details | Formatted Diff | Diff
Fix style errors (19.68 KB, patch)
2012-12-17 16:41 PST, Eric Carlson 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2012-12-10 13:47:09 PST 
A media engine may deliver cues from in-band tracks every time the media data is parsed, but we should only add each cue to WebKit's TextTrack once.
Comment 1 Radar WebKit Bug Importer 2012-12-10 13:47:22 PST 
<rdar://problem/12849532>
Comment 2 Eric Carlson 2012-12-17 15:29:57 PST 
Created attachment 179813 [details]
Proposed patch
Comment 3 Build Bot 2012-12-17 16:14:30 PST 
Comment on attachment 179813 [details]
Proposed patch

Attachment 179813 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15363937

New failing tests:
media/track/track-in-band-cues-added-once.html
Comment 4 Eric Carlson 2012-12-17 16:29:14 PST 
Created attachment 179828 [details]
Skipped new tests on platforms that don't support it.
Comment 5 WebKit Review Bot 2012-12-17 16:31:44 PST 
Attachment 179828 [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/track/InbandTextTrack.cpp:139:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/html/track/InbandTextTrack.cpp:163:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Eric Carlson 2012-12-17 16:41:25 PST 
Created attachment 179832 [details]
Fix style errors
Comment 7 Dean Jackson 2012-12-17 17:15:09 PST 
Comment on attachment 179832 [details]
Fix style errors

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

> Source/WebCore/html/track/InbandTextTrack.cpp:154
> +            if (!cue)
> +                return false;
> +            if (cue->startTime() != startTime)
> +                return false;
> +            if (cue->endTime() != endTime)
> +                return false;
> +            if (cue->text() != content)
> +                return false;
> +            if (cue->cueSettings() != settings)
> +                return false;
> +            if (cue->id() != id)
> +                return false;

Is there any reason you chose this form over a single if statement? It is easier to read :)

> LayoutTests/media/track/track-in-band-cues-added-once.html:4
> +    <head>
> +        <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />

No title?
Comment 8 WebKit Review Bot 2012-12-18 07:24:35 PST 
Comment on attachment 179832 [details]
Fix style errors

Clearing flags on attachment: 179832

Committed r138017: <http://trac.webkit.org/changeset/138017>
Comment 9 WebKit Review Bot 2012-12-18 07:24:40 PST 
All reviewed patches have been landed.  Closing bug.