Closed Bug 988808 Opened 6 years ago Closed 6 years ago

Convert SVG hit-testing to use Moz2D instead of Thebes gfxASurface

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We should convert SVG hit-testing to use Moz2D instead of Thebes gfxASurface.
Blocks: 703159, 933019
Blocks: 987190
Assignee: nobody → jwatt
Depends on: 1034115
Attached patch patch (obsolete) — Splinter Review
Attachment #8450306 - Flags: review?(matt.woodrow)
The AutoStrokeOptions class is primarily to get around the issue of the lifetime of mDathPattern. Avoiding allocating in the common case is a side benefit.
Comment on attachment 8450306 [details] [diff] [review]
patch

We need to be able to generate the path using a different fill rule to the current computed style value, so I need to supply the fill rule to the nsSVGPathGeometryElement::BuildPath overrides. Actually, to be more flexible (I'm pretty sure I'm going to need this) I'll change these overrides so that a PathBuilder object can be supplied.
Attachment #8450306 - Flags: review?(matt.woodrow)
Comment on attachment 8450522 [details] [diff] [review]
Allow a PathBuilder object to be supplied to nsSVGPathGeometryElement::BuildPath() and its overrides

Review of attachment 8450522 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/SVGPathData.cpp
@@ +301,5 @@
>      }                                                                         \
>    } while(0)
>  
>  TemporaryRef<Path>
> +SVGPathData::BuildPath(PathBuilder* builder,

nit: Please just rename the arg to our conventions and change the rest of the function ;).

::: content/svg/content/src/SVGPathElement.cpp
@@ +404,5 @@
>    }
>  
> +  RefPtr<PathBuilder> builder;
> +  if (aBuilder) {
> +    builder = aBuilder;

nit: Might be good to assert aBuilder->GetFillRule() == GetFillRule()
Attachment #8450522 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8450523 [details] [diff] [review]
Convert SVG hit-testing to use Moz2D instead of Thebes backed gfxContext

Review of attachment 8450523 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGPathGeometryElement.cpp
@@ +222,5 @@
> +  aStroke->mMiterLimit = Float(style->mStrokeMiterlimit);
> +
> +  GetStrokeDashData(aStroke, aFrame);
> +
> +  switch (style->mStrokeLinejoin) {

nit: Can you move these to helper functions if you think we'll ever need these again? :).

::: content/svg/content/src/nsSVGPathGeometryElement.h
@@ +116,5 @@
> +        mDashPattern = mSmallArray;
> +        return mSmallArray;
> +      }
> +      static const mozilla::fallible_t fallible = mozilla::fallible_t();
> +      Float* nonConstTmp = new (fallible) Float[mDashLength];

nit: Just pass fallible_t() or std::nothrow directly? No need to have the stack var.

::: layout/svg/nsSVGPathGeometryFrame.cpp
@@ +233,5 @@
>  }
>  
>  nsIFrame*
>  nsSVGPathGeometryFrame::GetFrameForPoint(const nsPoint &aPoint)
>  {

nit: This function could use some vertical whitespace for readability.

@@ +264,2 @@
>  
> +  // Using ScreenReferenceDrawTarget() opens us to Moz2D backend specific hit-

I don't think so, first of all we want to kill Cairo, second of all if something has a bug that displays something 10 pixels to the right, I'd rather have hittesting move along with it :-).
Attachment #8450523 - Flags: review?(matt.woodrow) → review+
I had to change content/svg/content/test/test_pointer-events-3.xhtml to not test right on the boundary of the circle. I also moved the style getting stuff to SVGContentUtils rather than having it on nsSVGPathGeometryElement since I realized we'll actually need that for text later.
https://hg.mozilla.org/mozilla-central/rev/15767e979335
https://hg.mozilla.org/mozilla-central/rev/60d0ee6ffa30
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.