If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Replace non-hit-testing-related SetupCairoStrokeHitGeometry calls with SetupCairoStrokeGeometry

RESOLVED INVALID

Status

()

Core
SVG
RESOLVED INVALID
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

4 years ago
We have several calls to nsSVGUtils::SetupCairoStrokeHitGeometry for functions that aren't hit-testing-specific.

nsSVGUtils::SetupCairoStrokeHitGeometry is itself just a thin wrapper for nsSVGUtils::SetupCairoStrokeGeometry, with a "SetDash" call added (presumably so that hit-testing on the between-dash region works correctly).

So: filing this bug on doing s/SetupCairoStrokeHitGeometry/SetupCairoStrokeGeometry/ where there no hit-testing going on.

Comment 1

4 years ago
nsSVGUtils::SetupCairoStrokeGeometry is only called in nsSVGPathGeometryFrame::GetBBoxContribution it skips the SetDash there as whether dash styles can't affect the bounding box, at least not with the approximation based code we currently have. 

We could just have the one call and always call SetDash though which would be simpler and only slightly less efficient. Alternatively s/SetupCairoStrokeGeometry/SetupCairoStrokeBBoxGeometry/ and s/SetupCairoStrokeHitGeometry/SetupCairoStrokeGeometry/ perhaps.
(Assignee)

Comment 2

4 years ago
(I was particularly eying the calls to SetupCairoStrokeHitGeometry in nsSVGGlyphFrame.cpp and nsSVGTextFrame2.cpp, which don't appear to need to know about dash geometry -- at least, if I do s/SetupCairoStrokeHitGeometry/SetupCairoStrokeGeometry/ there, SVG reftests still pass, modulo some minor machine-specific fuzziness.)
(Assignee)

Comment 3

4 years ago
Exploratory Try run: https://tbpl.mozilla.org/?tree=Try&rev=0dd6c25cb647
(Assignee)

Comment 4

4 years ago
Try seemed to like this, but it does actually break a reftest -- though it breaks the reftest and the reference case in the same way, so it still passes. :-/

It breaks the rendering of:
 http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/stroke-dasharray-and-text-01.svg
and
 http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/stroke-dasharray-and-text-01-ref.svg
to no longer have dashes.

So it looks like we don't have any reftests or mochitests that test whether we successfully apply stroke-dasharray to text. We should add some. (Probably easiest as "!=" reftests, with dashed text != normal text.)

And this bug here is INVALID, since we need SetupCairoStrokeHitGeometry's SetDash call in these places after all.

I may spin off a different bug to do the renaming that Robert suggested in comment 1, to make this clearer.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID
(Assignee)

Comment 5

4 years ago
(I filed bug 886611 for the renaming.)
You need to log in before you can comment on or make changes to this bug.