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

Issue 873483003: Move WebVTT snap-to-lines layout code to a helper class (Closed)

Created:
5 years, 11 months ago by fs
Modified:
5 years, 11 months ago
Reviewers:
philipj_slow
CC:
blink-reviews, blink-reviews-rendering, zoltan1, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, Dominik Röttsches, jchaffraix+rendering
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Move WebVTT snap-to-lines layout code to a helper class Move the bulk of the cue (post "box generation") layout code to a new helper SnapToLinesLayouter. This eliminates transient state from RenderVTTCue, and should make is easier to cache additional values (like the containing block and it's bounding box for example.) It could eventually also help breaking the dependency with the physical VTT cue structure, and make it possible to move the layout to the cue container. The code that tries to adjust for margin/border/padding is split out into a method. No functional changes intended. BUG=301580 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189004

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+110 lines, -83 lines) Patch
M Source/core/rendering/RenderVTTCue.h View 1 chunk +1 line, -12 lines 0 comments Download
M Source/core/rendering/RenderVTTCue.cpp View 11 chunks +109 lines, -71 lines 2 comments Download

Messages

Total messages: 10 (2 generated)
fs
5 years, 11 months ago (2015-01-26 17:12:21 UTC) #2
philipj_slow
lgtm https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/RenderVTTCue.cpp File Source/core/rendering/RenderVTTCue.cpp (right): https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/RenderVTTCue.cpp#newcode325 Source/core/rendering/RenderVTTCue.cpp:325: void RenderVTTCue::adjustForTopAndBottomMarginBorderAndPadding() Does this bit make sense to ...
5 years, 11 months ago (2015-01-27 05:06:59 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/873483003/1
5 years, 11 months ago (2015-01-27 05:07:24 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=189004
5 years, 11 months ago (2015-01-27 05:11:00 UTC) #6
fs
https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/RenderVTTCue.cpp File Source/core/rendering/RenderVTTCue.cpp (right): https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/RenderVTTCue.cpp#newcode325 Source/core/rendering/RenderVTTCue.cpp:325: void RenderVTTCue::adjustForTopAndBottomMarginBorderAndPadding() On 2015/01/27 05:06:59, philipj_UTC7 wrote: > Does ...
5 years, 11 months ago (2015-01-27 12:15:53 UTC) #7
philipj_slow
On 2015/01/27 12:15:53, fs wrote: > https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/RenderVTTCue.cpp > File Source/core/rendering/RenderVTTCue.cpp (right): > > https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/RenderVTTCue.cpp#newcode325 > ...
5 years, 11 months ago (2015-01-27 12:19:02 UTC) #8
philipj_slow
Oops. I'd be sympathetic to throwing this code out, perhaps trying it will reveal some ...
5 years, 11 months ago (2015-01-27 12:19:52 UTC) #9
fs
5 years, 11 months ago (2015-01-27 12:38:04 UTC) #10
Message was sent while issue was closed.
On 2015/01/27 12:19:52, philipj_UTC7 wrote:
> Oops. I'd be sympathetic to throwing this code out, perhaps trying it will
> reveal some tests where it does something useful?

Yes, hopefully. (Or maybe hopefully not...)

Powered by Google App Engine
This is Rietveld 408576698