New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 627233 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Bounds too large in SVGInlineTextBoxPainter::boundsForDrawingRecorder

Project Member Reported by pdr@chromium.org, Jul 11 2016

Issue description

While investigating another bug, Walter and I found SVGInlineTextBoxPainter::boundsForDrawingRecorder incorrectly offsets the local selection rect by the paint offset, effectively double-applying paint offset. This is hit on several tests, svg/text/text-selection-align-06-b.svg is one example.

P3 to clean this up. The actual code has no effect because the bounds are always large enough before includeSelectionRect is applied. includeSelectionRect is also poorly named and should be something like includeSelectionRectForMarkers.
 

Comment 1 by pdr@chromium.org, Jul 12 2016

Owner: wkorman@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca

commit 3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca
Author: wkorman <wkorman@chromium.org>
Date: Fri Jul 15 02:08:45 2016

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 

Review-Url: https://codereview.chromium.org/2137753002
Cr-Commit-Position: refs/heads/master@{#405675}

[modify] https://crrev.com/3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca/third_party/WebKit/Source/core/editing/DOMSelection.h
[modify] https://crrev.com/3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
[add] https://crrev.com/3f0f1c6a0edb31e2ec3ce4dbb21acc83301982ca/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp

Status: Fixed (was: Assigned)
I'm not sure re: renaming includeSelectionRect, it looks to me like it could be true either re: marker highlights *or* just for selected text:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp?q=includeSelectionRect&sq=package:chromium&l=83&dr=C

Marking fixed but let me know if you still think we should rename and I'll send change.
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0b85266865004e7a708a7a5e7e272a0b8784ef2

commit b0b85266865004e7a708a7a5e7e272a0b8784ef2
Author: henrika <henrika@chromium.org>
Date: Fri Jul 15 08:21:30 2016

Revert of Calculate correct cull rect for SVG inline text boxes. (patchset #3 id:40001 of https://codereview.chromium.org/2137753002/ )

Reason for revert:
Most likely breaks:

SVGInlineTextBoxPainterTest.TextCullRect_DefaultWritingMode
SVGInlineTextBoxPainterTest.TextCullRect_WritingModeTopToBottom

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29/builds/50365

Original issue's 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}

TBR=pdr@chromium.org,wkorman@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 627233 

Review-Url: https://codereview.chromium.org/2151203002
Cr-Commit-Position: refs/heads/master@{#405722}

[modify] https://crrev.com/b0b85266865004e7a708a7a5e7e272a0b8784ef2/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/b0b85266865004e7a708a7a5e7e272a0b8784ef2/third_party/WebKit/Source/core/editing/DOMSelection.h
[modify] https://crrev.com/b0b85266865004e7a708a7a5e7e272a0b8784ef2/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
[delete] https://crrev.com/5a484af29ce16842a03e9a7cfcb9f273722ac5ec/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/344d1d48097756a754766c75b9c49c756f85d20e

commit 344d1d48097756a754766c75b9c49c756f85d20e
Author: wkorman <wkorman@chromium.org>
Date: Sat Jul 16 02:07:42 2016

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.

Reland of http://crrev.com/2137753002 with updated test to
approximate expected rects to allow for minor font metrics differences
across platforms. Notably, previous patch failed on Android Nexus 4
due to 1 - 2 pixel deltas.

BUG= 627233 
TBR=pdr

Review-Url: https://codereview.chromium.org/2151323004
Cr-Commit-Position: refs/heads/master@{#405920}

[modify] https://crrev.com/344d1d48097756a754766c75b9c49c756f85d20e/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/344d1d48097756a754766c75b9c49c756f85d20e/third_party/WebKit/Source/core/editing/DOMSelection.h
[modify] https://crrev.com/344d1d48097756a754766c75b9c49c756f85d20e/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
[add] https://crrev.com/344d1d48097756a754766c75b9c49c756f85d20e/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f33d0154201310ea6eeb515da696d2daab06791f

commit f33d0154201310ea6eeb515da696d2daab06791f
Author: lizeb <lizeb@chromium.org>
Date: Mon Jul 18 11:41:12 2016

Revert of Calculate correct cull rect for SVG inline text boxes. (patchset #4 id:60001 of https://codereview.chromium.org/2151323004/ )

Reason for revert:
Broke WebKit Android (Nexus4).

BUG= 629023 , 627233 

Original issue's 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.
>
> Reland of http://crrev.com/2137753002 with updated test to
> approximate expected rects to allow for minor font metrics differences
> across platforms. Notably, previous patch failed on Android Nexus 4
> due to 1 - 2 pixel deltas.
>
> BUG= 627233 
> TBR=pdr
>
> Committed: https://crrev.com/344d1d48097756a754766c75b9c49c756f85d20e
> Cr-Commit-Position: refs/heads/master@{#405920}

TBR=pdr@google.com,wkorman@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.

Review-Url: https://codereview.chromium.org/2161573002
Cr-Commit-Position: refs/heads/master@{#405977}

[modify] https://crrev.com/f33d0154201310ea6eeb515da696d2daab06791f/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/f33d0154201310ea6eeb515da696d2daab06791f/third_party/WebKit/Source/core/editing/DOMSelection.h
[modify] https://crrev.com/f33d0154201310ea6eeb515da696d2daab06791f/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
[delete] https://crrev.com/dd11513c44ebc8ed0d5da23b9a1b96e17050deea/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp

Status: Started (was: Fixed)
From the failure in  http://crbug.com/629023  it reads as if the latest test expectations were just in need of expansion by a couple more pixels. Won't commit again until have tested on Android manually.
I added a method to SkPictureRecorder or whatever it's called to allow the specification of a cull rect _after_ the picture was finished recording. The goal was to use bounds from text blobs to get accurate text bounds. Does that help you now?
Interesting, send me sample usage? In this case there was an actual bug w.r.t. ordering of applying paintOffset, and uniting the selection bounds with the text bounds. The challenge has purely been in writing a unit test with explicit expected rect bounds across the various platforms. My latest change allows for essentially epsilon-type variance, but I didn't make the variable amount quite big enough. Open to alternate approaches.

To use the SkPictureBuilder/Recorder thing I think it would introduce some add'l perf overhead through having to wrap in another picture, and also would not fix the cull rect of the individual text items?
Only example I have is in the Skia unit test:
https://chromium.googlesource.com/skia/+/eeff8bb8ffdd6e77823a23bad3d23e4f15057681/tests/PictureTest.cpp#987

I don't think it addresses your problem here. I assumed it was an issue with the text bounds themselves, not offsets and the like. I should have looked more carefully. Sorry.

You're right that post-setting cull rects would require wrapping in another picture for any case where a method paints into something it did not start the recording for. The only other solution that comes to mind is to maintain a cull rect as everything is painted, then set the final rect when the picture is done. That's a big change, however.
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fcc8306cb32357373c4154f510a48fb869de6ec5

commit fcc8306cb32357373c4154f510a48fb869de6ec5
Author: wkorman <wkorman@chromium.org>
Date: Tue Jul 19 21:38:11 2016

Calculate correct cull rect for SVG inline text boxes.

Reland of http://crrev.com/2151323004, now updated to allow for 8
pixels delta and using outsets fixing bug where we didn't move the
expected bounding rects after expanding/contracting.

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 
TBR=pdr

Review-Url: https://codereview.chromium.org/2154143005
Cr-Commit-Position: refs/heads/master@{#406386}

[modify] https://crrev.com/fcc8306cb32357373c4154f510a48fb869de6ec5/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/fcc8306cb32357373c4154f510a48fb869de6ec5/third_party/WebKit/Source/core/editing/DOMSelection.h
[modify] https://crrev.com/fcc8306cb32357373c4154f510a48fb869de6ec5/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
[add] https://crrev.com/fcc8306cb32357373c4154f510a48fb869de6ec5/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainterTest.cpp

Status: Fixed (was: Started)

Sign in to add a comment