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

Issue 2137753002: Calculate correct cull rect for SVG inline text boxes. (Closed)

Created:
4 years, 5 months ago by wkorman
Modified:
4 years, 5 months ago
Reviewers:
pdr.
CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Calculate correct cull rect for SVG inline text boxes. Cull rects were larger than needed when text was selected. Identified while investigating test failure of: svg/text/text-selection-align-06-b.svg as part of http://crrev.com/2073563002 for crbug.com/616600, though fixing these did not actually fix any issues for that patch or bug. BUG=627233 Committed: https://crrev.com/3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca Cr-Commit-Position: refs/heads/master@{#405675}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Exploring a test. #

Total comments: 2

Patch Set 3 : Rework test to search list for painting rather than via explicit index. #

Patch Set 4 : Approximate expected rects. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -3 lines) Patch
M third_party/WebKit/Source/core/core.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/DOMSelection.h View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp View 1 chunk +1 line, -2 lines 0 comments Download
A third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp View 1 2 3 1 chunk +134 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (9 generated)
wkorman
Minimal test case we were looking at was: <?xml version="1.0" encoding="UTF-8"?> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="400px" ...
4 years, 5 months ago (2016-07-09 00:11:33 UTC) #3
pdr.
https://codereview.chromium.org/2137753002/diff/1/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/2137753002/diff/1/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp#newcode50 third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:50: if (includeSelectionRect) { I looked into this and it ...
4 years, 5 months ago (2016-07-11 18:41:09 UTC) #4
wkorman
PTAL, I took this as an opportunity to write a trivial RenderingTest. Passes on both ...
4 years, 5 months ago (2016-07-12 22:38:05 UTC) #5
pdr.
Looks great! LGTM Please add BUG=627233 https://codereview.chromium.org/2137753002/diff/20001/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp (right): https://codereview.chromium.org/2137753002/diff/20001/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp#newcode101 third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp:101: // The selection ...
4 years, 5 months ago (2016-07-12 22:51:41 UTC) #6
wkorman
Apologies, didn't mean to swipe your bug! Hope you had not started work on it ...
4 years, 5 months ago (2016-07-12 23:33:20 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2137753002/20001
4 years, 5 months ago (2016-07-12 23:39:33 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/254555)
4 years, 5 months ago (2016-07-13 01:16:26 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2137753002/40001
4 years, 5 months ago (2016-07-15 00:52:31 UTC) #16
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 5 months ago (2016-07-15 02:09:00 UTC) #17
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-15 02:09:05 UTC) #18
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca Cr-Commit-Position: refs/heads/master@{#405675}
4 years, 5 months ago (2016-07-15 02:11:48 UTC) #20
henrika (OOO until Aug 14)
4 years, 5 months ago (2016-07-15 08:20:47 UTC) #21
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2151203002/ by henrika@chromium.org.

The reason for reverting is: Most likely breaks:

SVGInlineTextBoxPainterTest.TextCullRect_DefaultWritingMode
SVGInlineTextBoxPainterTest.TextCullRect_WritingModeTopToBottom

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Andr....

Powered by Google App Engine
This is Rietveld 408576698