Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 9 months ago by fs Modified:
4 years, 9 months ago Reviewers:
pdr., dgrogan, eae CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd spacingDisabled() check to ShapeResultSpacing
SVGTextLayoutEngine applies letter-spacing and word-spacing itself, so
without this we'd apply the spacing properties twice.
This is essentially a bandaid work-around, until we can figure out how
to handle this in a better way.
BUG=583298
Committed: https://crrev.com/c991adfd9a653ed4d3c57af3736d16b2b21c9aa5
Cr-Commit-Position: refs/heads/master@{#383078}
Patch Set 1 #
Total comments: 2
Created: 4 years, 9 months ago
Messages
Total messages: 15 (4 generated)
fs@opera.com changed reviewers: + eae@chromium.org, pdr@chromium.org
Reinstating this hack to fix the letter/word-spacing regression...
On 2016/03/24 at 15:55:57, fs wrote: > Reinstating this hack to fix the letter/word-spacing regression... Random example from new expectations: https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
LGTM assuming this only applies to SVG text. https://codereview.chromium.org/1827083002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.cpp (right): https://codereview.chromium.org/1827083002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.cpp:27: if (m_textRun.spacingDisabled()) This only applies to SVG, right?
https://codereview.chromium.org/1827083002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.cpp (right): https://codereview.chromium.org/1827083002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.cpp:27: if (m_textRun.spacingDisabled()) On 2016/03/24 at 16:37:47, eae wrote: > This only applies to SVG, right? Yepp: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
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/1827083002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827083002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Add spacingDisabled() check to ShapeResultSpacing SVGTextLayoutEngine applies letter-spacing and word-spacing itself, so without this we'd apply the spacing properties twice. This is essentially a bandaid work-around, until we can figure out how to handle this in a better way. BUG=583298 ========== to ========== Add spacingDisabled() check to ShapeResultSpacing SVGTextLayoutEngine applies letter-spacing and word-spacing itself, so without this we'd apply the spacing properties twice. This is essentially a bandaid work-around, until we can figure out how to handle this in a better way. BUG=583298 Committed: https://crrev.com/c991adfd9a653ed4d3c57af3736d16b2b21c9aa5 Cr-Commit-Position: refs/heads/master@{#383078} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c991adfd9a653ed4d3c57af3736d16b2b21c9aa5 Cr-Commit-Position: refs/heads/master@{#383078}
Message was sent while issue was closed.
dgrogan@chromium.org changed reviewers: + dgrogan@chromium.org
Message was sent while issue was closed.
Some svg tests are broken on Win7 after the rebaseline (https://codereview.chromium.org/1827233002) for this patch landed. svg/W3C-SVG-1.1/text-spacing-01-b.svg svg/text/text-selection-spacing-01-b.svg svg/custom/text-letter-spacing.svg svg/batik/text/textOnPathSpaces.svg svg/batik/text/textLayout.svg svg/batik/text/textPosition2.svg svg/batik/text/textPosition.svg The test results from one of the broken runs can be found here: https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/4076... https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/4076...
Message was sent while issue was closed.
On 2016/03/25 at 00:14:46, dgrogan wrote: > Some svg tests are broken on Win7 after the rebaseline (https://codereview.chromium.org/1827233002) for this patch landed. > > svg/W3C-SVG-1.1/text-spacing-01-b.svg > svg/text/text-selection-spacing-01-b.svg > svg/custom/text-letter-spacing.svg > svg/batik/text/textOnPathSpaces.svg > svg/batik/text/textLayout.svg > svg/batik/text/textPosition2.svg > svg/batik/text/textPosition.svg > > The test results from one of the broken runs can be found here: > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/4076... > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7/4076... The new results ("actual") are the correct ones. Looks like the rebaseline wasn't entirely proper, so needs to be re-done for that bot (and possibly others...?)
Message was sent while issue was closed.
I've never used the rebaseline bot so I don't know what to tell you. Perhaps try marking the failing tests as needing a rebaseline again?
Message was sent while issue was closed.
On 2016/03/25 at 00:30:23, dgrogan wrote: > I've never used the rebaseline bot so I don't know what to tell you. Perhaps try marking the failing tests as needing a rebaseline again? I uploaded https://codereview.chromium.org/1827103004 - hope that works out, because I'm going to bed now. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
