Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(130)

Issue 850363005: VTTCue: Support 'auto' for line (Closed)

Created:
5 years, 11 months ago by fs
Modified:
5 years, 11 months ago
Reviewers:
philipj_slow
CC:
blink-reviews, nessy, arv+blink, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews-html_chromium.org, Inactive, vcarbune.chromium
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

VTTCue: Support 'auto' for line Since the setter is no longer limitting the value to [0, 100] we need to store an additional flag for the 'auto' value. Minor updates to VTTCue::calculateComputedLinePosition to match updated spec. (this is now where the line value is checked against the above range.) BUG=448000 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188723

Patch Set 1 #

Total comments: 12

Patch Set 2 : Minor fixups. Add bracers for non-finite values... #

Patch Set 3 : NaN-as-auto #

Patch Set 4 : Fix Windows compilation. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -94 lines) Patch
M LayoutTests/media/track/opera/interfaces/VTTCue/line.html View 1 2 chunks +16 lines, -10 lines 0 comments Download
D LayoutTests/media/track/opera/interfaces/VTTCue/line-expected.txt View 1 chunk +0 lines, -5 lines 0 comments Download
M LayoutTests/media/track/opera/track/webvtt/parsing/001.html View 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/media/track/track-add-remove-cue.html View 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/media/track/track-add-remove-cue-expected.txt View 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/media/track/track-cue-mutable.html View 2 chunks +10 lines, -6 lines 0 comments Download
M LayoutTests/media/track/track-cue-mutable-expected.txt View 2 chunks +10 lines, -6 lines 0 comments Download
M LayoutTests/media/track/track-webvtt-tc013-settings.html View 3 chunks +3 lines, -3 lines 0 comments Download
M LayoutTests/media/track/track-webvtt-tc013-settings-expected.txt View 2 chunks +4 lines, -4 lines 0 comments Download
M LayoutTests/media/track/track-webvtt-tc017-line-position.html View 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/media/track/track-webvtt-tc017-line-position-expected.txt View 1 chunk +6 lines, -6 lines 0 comments Download
M LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html View 2 chunks +2 lines, -2 lines 0 comments Download
M LayoutTests/media/track/track-webvtt-tc018-align-text-line-position-expected.txt View 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/media/track/vtt-cue-exceptions.html View 1 chunk +0 lines, -3 lines 0 comments Download
M LayoutTests/media/track/vtt-cue-exceptions-expected.txt View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/html/track/vtt/VTTCue.h View 1 2 3 chunks +4 lines, -2 lines 0 comments Download
M Source/core/html/track/vtt/VTTCue.cpp View 1 2 3 10 chunks +80 lines, -34 lines 0 comments Download
M Source/core/html/track/vtt/VTTCue.idl View 1 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 10 (2 generated)
fs
5 years, 11 months ago (2015-01-20 13:25:08 UTC) #2
philipj_slow
I can take another look, but LGTM if I'm too slow.
5 years, 11 months ago (2015-01-20 14:52:22 UTC) #3
philipj_slow
https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left): https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html#oldcode27 LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27: assert_equals(c2.line, -2); I see that this -2 test used ...
5 years, 11 months ago (2015-01-20 14:52:31 UTC) #4
fs
https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left): https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html#oldcode27 LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27: assert_equals(c2.line, -2); On 2015/01/20 14:52:31, philipj wrote: > I ...
5 years, 11 months ago (2015-01-20 16:35:17 UTC) #5
philipj_slow
PS4 viewed with great pleasure, LGTM. https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left): https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html#oldcode27 LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27: assert_equals(c2.line, -2); On ...
5 years, 11 months ago (2015-01-20 20:53:36 UTC) #6
fs
https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left): https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/opera/interfaces/VTTCue/line.html#oldcode27 LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27: assert_equals(c2.line, -2); On 2015/01/20 20:53:36, philipj wrote: > On ...
5 years, 11 months ago (2015-01-21 09:00:12 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/850363005/60001
5 years, 11 months ago (2015-01-21 09:01:18 UTC) #9
commit-bot: I haz the power
5 years, 11 months ago (2015-01-21 09:04:35 UTC) #10
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188723

Powered by Google App Engine
This is Rietveld 408576698