Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1883553004:
Refactor the current text position update in SVGTextLayoutEngine (Closed)
|
Created:
4 years, 8 months ago by fs Modified:
4 years, 8 months ago Reviewers:
pdr. CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, krit, eae+blinkwatch, f(malita), 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor the current text position update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
BUG=486669
Committed: https://crrev.com/143115b480cad3f35087b24eeb155c4aa2f579e4
Cr-Commit-Position: refs/heads/master@{#387694}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Touchups #Patch Set 3 : One more TE. #Patch Set 4 : Restore old behavior wrt text-on-a-path. #
Messages
Total messages: 16 (6 generated)
Description was changed from
==========
Refactor the CTP update in SVGTextLayoutEngine
Also makes CTP-handling more correct in the <textPath> case.
BUG=486669
==========
to
==========
Refactor the CTP update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
This also makes CTP-handling more correct in the <textPath> case
by maintaining/updating the CTP even across text-on-a-path. This
change is reflected in the two updated test expectations.
BUG=486669
==========
fs@opera.com changed reviewers: + pdr@chromium.org
Had this lying around, gathering dust, for some time. Maybe now the time is right. (Not entirely easy to review I imagine though.)
https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/TestExpectations:110: crbug.com/486669 svg/batik/text/textOnPath2.svg [ NeedsRebaseline ] Can you double-check that this isn't a regression? It's hard to say what should happen here, but the new behavior differs from the batik expectations. https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp (right): https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:391: // When we've advanced to the box start offset, determine using the original x/y values, This code is sandwiched between setCurrentTextPositionIfNeeded and applyRelativePositionAdjustmentsIfNeeded but neither function affects it. WDYT of joining setCurrentTextPositionIfNeeded and applyRelativePositionAdjustmentsIfNeeded into a single function, or moving the calls next to each other? https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h (right): https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h:61: bool setCurrentTextPositionIfNeeded(const SVGCharacterData&); Optional bikeshed: -isNeeded seems to have fallen out of favor lately. I have mixed feelings about using -isNeeded for expensive operations since it makes performance issues clear at callsites, but in this case we could remove it without harm. https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h:88: FloatPoint m_ctp; +1 to merging these into a FloatPoint, but ctp isn't a very common acronym. WDYT of "m_textPosition"?
https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/TestExpectations:110: crbug.com/486669 svg/batik/text/textOnPath2.svg [ NeedsRebaseline ] On 2016/04/14 at 01:05:51, pdr wrote: > Can you double-check that this isn't a regression? It's hard to say what should happen here, but the new behavior differs from the batik expectations. I have, and I think this is a progression. (In my reading of the spec, text-on-a-path layout should not have any effect on the current text position. The new rendering/behavior matches Gecko [and Presto - so, yes, I'm biased...]) https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp (right): https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:391: // When we've advanced to the box start offset, determine using the original x/y values, On 2016/04/14 at 01:05:51, pdr wrote: > This code is sandwiched between setCurrentTextPositionIfNeeded and applyRelativePositionAdjustmentsIfNeeded but neither function affects it. WDYT of joining setCurrentTextPositionIfNeeded and applyRelativePositionAdjustmentsIfNeeded into a single function, or moving the calls next to each other? As I tried to note in the TODO above, I intend for the result of setCurrentTextPosition to feed into this - so the sandwich structure is intentional. (I'm also pondering if this and the above can be hoisted out of the loop.) https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h (right): https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h:61: bool setCurrentTextPositionIfNeeded(const SVGCharacterData&); On 2016/04/14 at 01:05:52, pdr wrote: > Optional bikeshed: -isNeeded seems to have fallen out of favor lately. I have mixed feelings about using -isNeeded for expensive operations since it makes performance issues clear at callsites, but in this case we could remove it without harm. Dropped suffix. https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.h:88: FloatPoint m_ctp; On 2016/04/14 at 01:05:52, pdr wrote: > +1 to merging these into a FloatPoint, but ctp isn't a very common acronym. WDYT of "m_textPosition"? For me it's a very common acronym =D [1]. I guess I don't count though, so changed. [1] https://www.w3.org/TR/SVG11/text.html#CurrentTextPosition
https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/TestExpectations:110: crbug.com/486669 svg/batik/text/textOnPath2.svg [ NeedsRebaseline ] On 2016/04/14 at 08:56:42, fs wrote: > On 2016/04/14 at 01:05:51, pdr wrote: > > Can you double-check that this isn't a regression? It's hard to say what should happen here, but the new behavior differs from the batik expectations. > > I have, and I think this is a progression. (In my reading of the spec, text-on-a-path layout should not have any effect on the current text position. The new rendering/behavior matches Gecko [and Presto - so, yes, I'm biased...]) (I should probably also mention the "ulterior motives", which is to avoid fragmentation for text-paths [or at least do it as a deferred pass] to be able to better support BiDi - and possibly even let Skia do the path-mapping [which would allow us to support 'stretch' somewhat efficiently.])
On 2016/04/14 at 09:04:43, fs wrote: > https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/TestExpectations (right): > > https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/TestExpectations:110: crbug.com/486669 svg/batik/text/textOnPath2.svg [ NeedsRebaseline ] > On 2016/04/14 at 08:56:42, fs wrote: > > On 2016/04/14 at 01:05:51, pdr wrote: > > > Can you double-check that this isn't a regression? It's hard to say what should happen here, but the new behavior differs from the batik expectations. > > > > I have, and I think this is a progression. (In my reading of the spec, text-on-a-path layout should not have any effect on the current text position. The new rendering/behavior matches Gecko [and Presto - so, yes, I'm biased...]) > > (I should probably also mention the "ulterior motives", which is to avoid fragmentation for text-paths [or at least do it as a deferred pass] to be able to better support BiDi - and possibly even let Skia do the path-mapping [which would allow us to support 'stretch' somewhat efficiently.]) I'm all on board with this change except for the text path part. I think it actually differs from all other engines. I made a simpler example to show the change: http://jsbin.com/gugoje I think your approach makes sense, but I'd like to get it specified that way in the spec (or at least clarified by heycam et al.). I've filed https://github.com/w3c/svgwg/issues/104 to do that. If I've forgotten any info in the github issue, could you add it?
Description was changed from
==========
Refactor the CTP update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
This also makes CTP-handling more correct in the <textPath> case
by maintaining/updating the CTP even across text-on-a-path. This
change is reflected in the two updated test expectations.
BUG=486669
==========
to
==========
Refactor the CTP update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
BUG=486669
==========
On 2016/04/14 at 20:47:16, pdr wrote: > On 2016/04/14 at 09:04:43, fs wrote: > > https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/TestExpectations (right): > > > > https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/TestExpectations:110: crbug.com/486669 svg/batik/text/textOnPath2.svg [ NeedsRebaseline ] > > On 2016/04/14 at 08:56:42, fs wrote: > > > On 2016/04/14 at 01:05:51, pdr wrote: > > > > Can you double-check that this isn't a regression? It's hard to say what should happen here, but the new behavior differs from the batik expectations. > > > > > > I have, and I think this is a progression. (In my reading of the spec, text-on-a-path layout should not have any effect on the current text position. The new rendering/behavior matches Gecko [and Presto - so, yes, I'm biased...]) > > > > (I should probably also mention the "ulterior motives", which is to avoid fragmentation for text-paths [or at least do it as a deferred pass] to be able to better support BiDi - and possibly even let Skia do the path-mapping [which would allow us to support 'stretch' somewhat efficiently.]) > > I'm all on board with this change except for the text path part. I think it actually differs from all other engines. I made a simpler example to show the change: > http://jsbin.com/gugoje > > I think your approach makes sense, but I'd like to get it specified that way in the spec (or at least clarified by heycam et al.). I've filed https://github.com/w3c/svgwg/issues/104 to do that. If I've forgotten any info in the github issue, could you add it? I've restored behavior in PS4 pending further feedback.
Description was changed from
==========
Refactor the CTP update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
BUG=486669
==========
to
==========
Refactor the current text position update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
BUG=486669
==========
On 2016/04/15 at 08:59:44, fs wrote: > On 2016/04/14 at 20:47:16, pdr wrote: > > On 2016/04/14 at 09:04:43, fs wrote: > > > https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... > > > File third_party/WebKit/LayoutTests/TestExpectations (right): > > > > > > https://codereview.chromium.org/1883553004/diff/1/third_party/WebKit/LayoutTe... > > > third_party/WebKit/LayoutTests/TestExpectations:110: crbug.com/486669 svg/batik/text/textOnPath2.svg [ NeedsRebaseline ] > > > On 2016/04/14 at 08:56:42, fs wrote: > > > > On 2016/04/14 at 01:05:51, pdr wrote: > > > > > Can you double-check that this isn't a regression? It's hard to say what should happen here, but the new behavior differs from the batik expectations. > > > > > > > > I have, and I think this is a progression. (In my reading of the spec, text-on-a-path layout should not have any effect on the current text position. The new rendering/behavior matches Gecko [and Presto - so, yes, I'm biased...]) > > > > > > (I should probably also mention the "ulterior motives", which is to avoid fragmentation for text-paths [or at least do it as a deferred pass] to be able to better support BiDi - and possibly even let Skia do the path-mapping [which would allow us to support 'stretch' somewhat efficiently.]) > > > > I'm all on board with this change except for the text path part. I think it actually differs from all other engines. I made a simpler example to show the change: > > http://jsbin.com/gugoje > > > > I think your approach makes sense, but I'd like to get it specified that way in the spec (or at least clarified by heycam et al.). I've filed https://github.com/w3c/svgwg/issues/104 to do that. If I've forgotten any info in the github issue, could you add it? > > I've restored behavior in PS4 pending further feedback. LGTM!
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883553004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883553004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Refactor the current text position update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
BUG=486669
==========
to
==========
Refactor the current text position update in SVGTextLayoutEngine
The main change is around the handling of "delta" adjustments
('dx' / 'dy'), that is changed to not require keeping state.
Additionally text-on-a-path layout is changed to track the
displacement from the path (the accumulated delta adjustments
in the perpendicular direction.) Baseline adjustments are
consolidated between code-paths and part of the "fragmentation"
condition is hoisted out of the per-"glyph" loop.
BUG=486669
Committed: https://crrev.com/143115b480cad3f35087b24eeb155c4aa2f579e4
Cr-Commit-Position: refs/heads/master@{#387694}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/143115b480cad3f35087b24eeb155c4aa2f579e4 Cr-Commit-Position: refs/heads/master@{#387694} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
