Bounds too large in SVGInlineTextBoxPainter::boundsForDrawingRecorder |
||||
Issue descriptionWhile 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.
,
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
,
Jul 15 2016
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.
,
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
,
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
,
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
,
Jul 18 2016
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.
,
Jul 18 2016
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?
,
Jul 18 2016
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?
,
Jul 18 2016
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.
,
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
,
Jul 19 2016
|
||||
►
Sign in to add a comment |
||||
Comment 1 by pdr@chromium.org
, Jul 12 2016