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

Issue 25155003: Fix cue rendering test and include support for left/right alignment. (Closed)

Created:
7 years, 2 months ago by vcarbune.chromium
Modified:
7 years, 2 months ago
CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org
Visibility:
Public.

Description

Fix cue rendering test and include support for left/right alignment. The test has been broken for a while somehow, but only because at some point the code for computing the width got close to the current version of the WebVTT specification and the alignment has an impact on the width of the text (so not specifying the alignment determines the default align:middle and in some cases the width gets zero, e.g. with position:100%) The specification also got changed in the sense that it also supports left and right values for cue alignment, which are now included in the test case and supported in the code. TEST=Update existing test case. BUG=299752 R=acolwell,scherkus Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159228

Patch Set 1 #

Total comments: 13

Patch Set 2 : Addressed comments #

Total comments: 8

Patch Set 3 : Static const variables #

Patch Set 4 : Addressed comments #

Patch Set 5 : Remove commented code #

Total comments: 4

Patch Set 6 : Added COMPILE_ASSERT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -306 lines) Patch
M LayoutTests/media/track/captions-webvtt/captions-snap-to-lines-not-set.vtt View 5 chunks +11 lines, -11 lines 0 comments Download
M LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set.html View 3 chunks +1 line, -11 lines 0 comments Download
A + LayoutTests/media/track/track-cue-rendering-snap-to-lines-not-set-expected.txt View 1 8 chunks +23 lines, -13 lines 0 comments Download
D LayoutTests/platform/mac/media/track/track-cue-rendering-snap-to-lines-not-set-expected.txt View 1 1 chunk +0 lines, -110 lines 0 comments Download
D LayoutTests/platform/win/media/track/track-cue-rendering-snap-to-lines-not-set-expected.txt View 1 1 chunk +0 lines, -110 lines 0 comments Download
M Source/core/html/track/TextTrackCue.h View 1 2 3 chunks +7 lines, -6 lines 0 comments Download
M Source/core/html/track/TextTrackCue.cpp View 1 2 3 4 5 11 chunks +105 lines, -45 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
vcarbune.chromium
Test fix and several code updates to better match the spec.
7 years, 2 months ago (2013-09-28 12:07:02 UTC) #1
nessy
On 2013/09/28 12:07:02, vcarbune.chromium wrote: > Test fix and several code updates to better match ...
7 years, 2 months ago (2013-09-29 05:25:22 UTC) #2
nessy
On 2013/09/29 05:25:22, nessy wrote: > https://dvcs.w3.org/hg/text-tracks/raw-file/default/webvtt/webvtt.html respec is broken - look at this instead: ...
7 years, 2 months ago (2013-09-29 08:06:40 UTC) #3
vcarbune.chromium
On 2013/09/29 08:06:40, nessy wrote: > On 2013/09/29 05:25:22, nessy wrote: > > https://dvcs.w3.org/hg/text-tracks/raw-file/default/webvtt/webvtt.html > ...
7 years, 2 months ago (2013-09-30 12:05:31 UTC) #4
acolwell GONE FROM CHROMIUM
philipj: Could you take a look at this as well. I'd like to get your ...
7 years, 2 months ago (2013-10-02 15:42:57 UTC) #5
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp#newcode679 Source/core/html/track/TextTrackCue.cpp:679: m_computedLinePosition = calculateComputedLinePosition(); Does this really need to be ...
7 years, 2 months ago (2013-10-02 16:19:36 UTC) #6
philipj_slow
On 2013/10/02 15:42:57, acolwell wrote: > philipj: Could you take a look at this as ...
7 years, 2 months ago (2013-10-03 09:02:04 UTC) #7
philipj_slow
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp#newcode168 Source/core/html/track/TextTrackCue.cpp:168: else if (m_cue->align() == middleKeyword()) The implementation of TextTrackCue::align() ...
7 years, 2 months ago (2013-10-03 09:04:43 UTC) #8
vcarbune.chromium
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp#newcode168 Source/core/html/track/TextTrackCue.cpp:168: else if (m_cue->align() == middleKeyword()) On 2013/10/03 09:04:43, philipj ...
7 years, 2 months ago (2013-10-04 13:30:26 UTC) #9
acolwell GONE FROM CHROMIUM
lgtm % nits https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/TextTrackCue.cpp#newcode744 Source/core/html/track/TextTrackCue.cpp:744: default: nit: You only need default ...
7 years, 2 months ago (2013-10-04 17:26:41 UTC) #10
philipj_slow
https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp#newcode754 Source/core/html/track/TextTrackCue.cpp:754: // 10.9 Determine the value of whichever of x-position ...
7 years, 2 months ago (2013-10-07 08:52:31 UTC) #11
philipj_slow
Something seems a bit strange with patch set 4, if I download the raw diff ...
7 years, 2 months ago (2013-10-07 08:54:09 UTC) #12
vcarbune.chromium
The commented code left there is my fault, sorry. I removed it now, PTAL. https://codereview.chromium.org/25155003/diff/1/Source/core/html/track/TextTrackCue.cpp ...
7 years, 2 months ago (2013-10-07 09:37:27 UTC) #13
philipj_slow
On 2013/10/07 09:37:27, vcarbune.chromium wrote: > The commented code left there is my fault, sorry. ...
7 years, 2 months ago (2013-10-07 09:41:45 UTC) #14
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/TextTrackCue.cpp#newcode744 Source/core/html/track/TextTrackCue.cpp:744: default: On 2013/10/07 08:52:32, philipj wrote: > On 2013/10/04 ...
7 years, 2 months ago (2013-10-07 15:32:19 UTC) #15
philipj_slow
https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/13001/Source/core/html/track/TextTrackCue.cpp#newcode744 Source/core/html/track/TextTrackCue.cpp:744: default: On 2013/10/07 15:32:19, acolwell wrote: > On 2013/10/07 ...
7 years, 2 months ago (2013-10-08 06:56:54 UTC) #16
vcarbune.chromium
Ping for an owner review (and maybe lgtm) from (auto) CCed owners.
7 years, 2 months ago (2013-10-08 15:40:39 UTC) #17
adamk
https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/TextTrackCue.cpp#newcode58 Source/core/html/track/TextTrackCue.cpp:58: static const CSSValueID displayWritingModeMap[TextTrackCue::NumberOfWritingDirections] = { Putting NumberOfWritingDirections inside ...
7 years, 2 months ago (2013-10-08 19:46:09 UTC) #18
vcarbune.chromium
https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): https://codereview.chromium.org/25155003/diff/33001/Source/core/html/track/TextTrackCue.cpp#newcode58 Source/core/html/track/TextTrackCue.cpp:58: static const CSSValueID displayWritingModeMap[TextTrackCue::NumberOfWritingDirections] = { On 2013/10/08 19:46:10, ...
7 years, 2 months ago (2013-10-08 22:20:31 UTC) #19
adamk
lgtm
7 years, 2 months ago (2013-10-08 22:24:30 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vcarbune@chromium.org/25155003/43001
7 years, 2 months ago (2013-10-09 07:14:28 UTC) #21
commit-bot: I haz the power
7 years, 2 months ago (2013-10-09 12:11:11 UTC) #22
Message was sent while issue was closed.
Change committed as 159228

Powered by Google App Engine
This is Rietveld 408576698