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

Issue 1866703002: Use characters (not code units) when computing value list positions (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@svg-metrics-cleanup-14
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use characters (not code units) when computing value list positions The value list position is updated by one for each character, and not at all when spaces are skipped (collapsed). When assigning value list positions, we are currently counting surrogates as two (on for each code unit.) Use the text metrics data to count the number of (non-collapsed) characters instead. BUG=597312, 594058 Committed: https://crrev.com/9331211c22aee7b499131aa1c58ce515deceeb45 Cr-Commit-Position: refs/heads/master@{#386305}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Drop offset* access in test #

Patch Set 3 : Simplify the test some more. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -39 lines) Patch
A third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions-expected.html View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributesBuilder.h View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/svg/SVGTextLayoutAttributesBuilder.cpp View 6 chunks +21 lines, -37 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 12 (3 generated)
fs
After some shaving of large furry animal, the time has come to revisit this issue.
4 years, 8 months ago (2016-04-06 18:00:50 UTC) #2
pdr.
LGTM with some optional grumbles about our font loading. https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html File third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html (right): https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html#newcode11 third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html:11: ...
4 years, 8 months ago (2016-04-06 18:10:57 UTC) #3
fs
https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html File third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html (right): https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html#newcode11 third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html:11: document.body.offsetLeft; // Kick off loading @font-face On 2016/04/06 at ...
4 years, 8 months ago (2016-04-06 18:21:14 UTC) #4
pdr.
On 2016/04/06 at 18:21:14, fs wrote: > https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html > File third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html (right): > > https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html#newcode11 ...
4 years, 8 months ago (2016-04-06 22:47:03 UTC) #5
fs
On 2016/04/06 at 22:47:03, pdr wrote: > On 2016/04/06 at 18:21:14, fs wrote: > > ...
4 years, 8 months ago (2016-04-07 08:45:21 UTC) #6
pdr.
On 2016/04/07 at 08:45:21, fs wrote: > On 2016/04/06 at 22:47:03, pdr wrote: > > ...
4 years, 8 months ago (2016-04-07 18:29:57 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1866703002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1866703002/40001
4 years, 8 months ago (2016-04-09 18:12:36 UTC) #9
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 8 months ago (2016-04-09 19:13:53 UTC) #10
commit-bot: I haz the power
4 years, 8 months ago (2016-04-09 19:15:39 UTC) #12
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/9331211c22aee7b499131aa1c58ce515deceeb45
Cr-Commit-Position: refs/heads/master@{#386305}

Powered by Google App Engine
This is Rietveld 408576698