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

Issue 1162653002: Drop SVGTextChunk::m_boxes (Closed)

Created:
5 years, 6 months ago by fs
Modified:
5 years, 6 months ago
Reviewers:
pdr., f(malita)
CC:
blink-reviews, blink-reviews-rendering, krit, eae+blinkwatch, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Drop SVGTextChunk::m_boxes Since text chunks are now processed as they are formed, the m_boxes in SVGTextChunk is of little use, and just means an additional copy is needed. Drop it and use the range passed into handleTextChunk directly instead. This requires moving SVGTextChunk::calculateLength to SVGTextChunkBuilder.cpp, so do that and put it in a helper class. Also changing from passing <vector,startIndex,endIndex> to just passing two (vector) iterators describing the start and end in the vector. In SVGTextChunkBuilder::handleTextChunk, drop a redundant test (!boxCount), and move the code that recalculates the chunk extent after applying spacing adjustments for 'textLength' up into the relevant branch. BUG=486669 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196229

Patch Set 1 #

Total comments: 8

Patch Set 2 : Pass reference. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -152 lines) Patch
M Source/core/layout/svg/SVGTextChunk.h View 2 chunks +6 lines, -13 lines 0 comments Download
M Source/core/layout/svg/SVGTextChunk.cpp View 1 chunk +0 lines, -48 lines 0 comments Download
M Source/core/layout/svg/SVGTextChunkBuilder.h View 2 chunks +4 lines, -2 lines 0 comments Download
M Source/core/layout/svg/SVGTextChunkBuilder.cpp View 1 5 chunks +131 lines, -89 lines 0 comments Download

Messages

Total messages: 14 (4 generated)
fs
One more nail in the SVGTextChunk coffin...
5 years, 6 months ago (2015-05-28 21:32:38 UTC) #2
pdr.
I like where this is going! LGTM, just minor stuff below. https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGTextChunkBuilder.cpp File Source/core/layout/svg/SVGTextChunkBuilder.cpp (right): ...
5 years, 6 months ago (2015-05-28 22:00:03 UTC) #3
fs
https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGTextChunkBuilder.cpp File Source/core/layout/svg/SVGTextChunkBuilder.cpp (right): https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGTextChunkBuilder.cpp#newcode31 Source/core/layout/svg/SVGTextChunkBuilder.cpp:31: namespace { On 2015/05/28 22:00:03, pdr wrote: > Is ...
5 years, 6 months ago (2015-05-29 09:26:36 UTC) #4
pdr.
(still LGTM) On 2015/05/29 at 09:26:36, fs wrote: > https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGTextChunkBuilder.cpp > File Source/core/layout/svg/SVGTextChunkBuilder.cpp (right): > ...
5 years, 6 months ago (2015-05-29 18:37:09 UTC) #5
f(malita)
lgtm++ On 2015/05/29 18:37:09, pdr wrote: > I don't yet understand the empty namespace, but ...
5 years, 6 months ago (2015-05-29 18:51:11 UTC) #6
fs
On 2015/05/29 18:51:11, f(malita) wrote: > lgtm++ > > On 2015/05/29 18:37:09, pdr wrote: > ...
5 years, 6 months ago (2015-06-01 11:15:15 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162653002/20001
5 years, 6 months ago (2015-06-01 11:15:28 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64568)
5 years, 6 months ago (2015-06-01 13:53:40 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162653002/20001
5 years, 6 months ago (2015-06-01 13:59:08 UTC) #13
commit-bot: I haz the power
5 years, 6 months ago (2015-06-01 14:59:08 UTC) #14
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196229

Powered by Google App Engine
This is Rietveld 408576698