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

Issue 880873002: WebVTT: Implement 'best position' from the rendering section (Closed)

Created:
5 years, 11 months ago by fs
Modified:
5 years, 4 months ago
Reviewers:
philipj_slow
CC:
blink-reviews, nessy, zoltan1, eae+blinkwatch, gasubic, eric.carlson_apple.com, leviw+renderwatch, Dominik Röttsches, feature-media-reviews_chromium.org, blink-reviews-rendering, jchaffraix+rendering, pdr+renderingwatchlist_chromium.org, vcarbune.chromium
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

WebVTT: Implement 'best position' from the rendering section This adds the spec. notion of 'best position', which brings the code in line with the snap-to-lines part of the current draft. BUG=301580

Patch Set 1 #

Patch Set 2 : Fix compilation w/ asserts enabled. #

Total comments: 5

Patch Set 3 : Make computePositionScore impl. more clear. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -40 lines) Patch
A + LayoutTests/media/track/track-cue-rendering-best-position.html View 2 chunks +6 lines, -19 lines 0 comments Download
A + LayoutTests/media/track/track-cue-rendering-best-position-expected.html View 2 chunks +1 line, -15 lines 0 comments Download
M Source/core/rendering/RenderVTTCue.cpp View 1 2 8 chunks +42 lines, -6 lines 0 comments Download

Messages

Total messages: 27 (1 generated)
fs
https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/track-cue-rendering-best-position.html File LayoutTests/media/track/track-cue-rendering-best-position.html (right): https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/track-cue-rendering-best-position.html#newcode1 LayoutTests/media/track/track-cue-rendering-best-position.html:1: <!DOCTYPE html> If you know of any more "interesting" ...
5 years, 11 months ago (2015-01-27 10:14:52 UTC) #2
philipj_slow
Some thoughts on the code, running the tests locally to see what I think of ...
5 years, 11 months ago (2015-01-27 10:36:10 UTC) #3
philipj_slow
On 2015/01/27 10:14:52, fs wrote: > https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/track-cue-rendering-best-position.html > File LayoutTests/media/track/track-cue-rendering-best-position.html (right): > > https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/track-cue-rendering-best-position.html#newcode1 > ...
5 years, 11 months ago (2015-01-27 10:36:57 UTC) #4
philipj_slow
Comparing the test results with and without this change I see what you mean. Now ...
5 years, 11 months ago (2015-01-27 11:52:11 UTC) #5
fs
On 2015/01/27 10:36:57, philipj_UTC7 wrote: > On 2015/01/27 10:14:52, fs wrote: > > > https://codereview.chromium.org/880873002/diff/20001/LayoutTests/media/track/track-cue-rendering-best-position.html ...
5 years, 11 months ago (2015-01-27 11:54:53 UTC) #6
fs
On 2015/01/27 11:52:11, philipj_UTC7 wrote: > Comparing the test results with and without this change ...
5 years, 11 months ago (2015-01-27 12:08:03 UTC) #7
philipj_slow
I agree that simplicity is probably better than trying to do some marginally better thing ...
5 years, 11 months ago (2015-01-27 12:12:50 UTC) #8
fs
On 2015/01/27 12:12:50, philipj_UTC7 wrote: > I agree that simplicity is probably better than trying ...
5 years, 11 months ago (2015-01-27 12:32:33 UTC) #9
philipj_slow
On 2015/01/27 12:32:33, fs wrote: > On 2015/01/27 12:12:50, philipj_UTC7 wrote: > > I agree ...
5 years, 11 months ago (2015-01-27 15:51:44 UTC) #10
fs
On 2015/01/27 15:51:44, philipj_UTC7 wrote: > On 2015/01/27 12:32:33, fs wrote: > > On 2015/01/27 ...
5 years, 11 months ago (2015-01-27 16:21:43 UTC) #11
philipj_slow
Here's a simple revert: https://github.com/foolip/webvtt/compare/revert-best-position
5 years, 10 months ago (2015-01-28 11:00:51 UTC) #12
philipj_slow
That doesn't solve the problem "if the cue did not fit at all, it's moved ...
5 years, 10 months ago (2015-01-28 11:25:57 UTC) #13
fs
On 2015/01/28 11:25:57, philipj_UTC7 wrote: > That doesn't solve the problem "if the cue did ...
5 years, 10 months ago (2015-01-28 11:38:48 UTC) #14
philipj_slow
I see that Mozilla have already implemented the "best position" bits: https://github.com/mozilla/vtt.js/blob/master/lib/vtt.js Perhaps a conservative ...
5 years, 10 months ago (2015-01-28 11:51:59 UTC) #15
philipj_slow
On 2015/01/28 11:38:48, fs wrote: > On 2015/01/28 11:25:57, philipj_UTC7 wrote: > > That doesn't ...
5 years, 10 months ago (2015-01-28 11:53:10 UTC) #16
fs
On 2015/01/28 11:51:59, philipj_UTC7 wrote: > I see that Mozilla have already implemented the "best ...
5 years, 10 months ago (2015-01-28 12:14:20 UTC) #17
philipj_slow
On 2015/01/28 12:14:20, fs wrote: > On 2015/01/28 11:51:59, philipj_UTC7 wrote: > > I see ...
5 years, 10 months ago (2015-01-28 16:50:58 UTC) #18
fs
On 2015/01/28 16:50:58, philipj_UTC7 wrote: > On 2015/01/28 12:14:20, fs wrote: > > On 2015/01/28 ...
5 years, 10 months ago (2015-01-28 17:25:02 UTC) #19
philipj_slow
On 2015/01/28 17:25:02, fs wrote: > On 2015/01/28 16:50:58, philipj_UTC7 wrote: > > On 2015/01/28 ...
5 years, 10 months ago (2015-01-28 18:04:25 UTC) #20
fs
On 2015/01/28 18:04:25, philipj_UTC7 wrote: > On 2015/01/28 17:25:02, fs wrote: > > On 2015/01/28 ...
5 years, 10 months ago (2015-01-29 09:37:15 UTC) #21
philipj_slow
I realize now that I'd need to change "If there are any line boxes in ...
5 years, 10 months ago (2015-01-29 10:15:42 UTC) #22
fs
On 2015/01/29 10:15:42, philipj_UTC7 wrote: > I realize now that I'd need to change "If ...
5 years, 10 months ago (2015-01-29 11:33:20 UTC) #23
philipj_slow
On 2015/01/29 11:33:20, fs wrote: > On 2015/01/29 10:15:42, philipj_UTC7 wrote: > > I realize ...
5 years, 10 months ago (2015-01-29 15:49:24 UTC) #24
fs
On 2015/01/29 15:49:24, philipj_UTC7 wrote: > On 2015/01/29 11:33:20, fs wrote: > > On 2015/01/29 ...
5 years, 10 months ago (2015-01-29 16:56:47 UTC) #25
philipj_slow
OK, I've reopened the spec bug and opened a pull request: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c25 https://github.com/w3c/webvtt/pull/171
5 years, 10 months ago (2015-01-29 17:15:09 UTC) #26
philipj_slow
5 years, 10 months ago (2015-02-23 02:44:01 UTC) #27
On 2015/01/29 17:15:09, philipj_UTC7 wrote:
> OK, I've reopened the spec bug and opened a pull request:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=17483#c25
> https://github.com/w3c/webvtt/pull/171

This has now been merged, the 'best position' concept is no more.

Powered by Google App Engine
This is Rietveld 408576698