766227 - More nsSVGIntegrationUtils cleanup to make it much easier to understand

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Following on from bug 766120, this is the rest of my cleanup to nsSVGIntegrationUtils from bug 614732 to make it easier to understand.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

Comment on attachment 634512 [details] [diff] [review]
patch

>+  virtual void AddBox(nsIFrame* aFrame) {
>+    nsRect overflow;
>+    if (aFrame == mCurrentFrame) {
>+      overflow = mCurrentFrameOverflowArea;
>+    } else {
>+      nsRect* r = static_cast<nsRect*>
>+        (aFrame->Properties().Get(nsIFrame::PreEffectsBBoxProperty()));
>+      // XXXjwatt GetVisualOverflowRect() won't return what we want if the
>+      // frame has CSS transforms!

Please raise a followup bug to track this unless we already have something.

>+/**
>+ * Gets the union of the pre-effects visual overflow rects of all of a frame's
>+ * continuations, relative to "user space".

what does 'relative to "user space"' mean? relative to the frame's origin in "user space" presumably, or just 'in "user space"'

>+                                              aCurrentFramePreEffectsOverflow);
>+  // Compute union of all overflow areas relative to aFirstContinuation:
>+  nsLayoutUtils::GetAllInFlowBoxes(aFirstContinuation, &collector);
>+  // Return the result in users space:

s/users space/user space/

>+  //
>+  // XXXjwatt It seems like the only reason we pass an override bbox to
>+  // GetPostFilterBounds instead of just letting the filter code call into our
>+  // GetSVGBBoxForNonSVGFrame method is as a slight optimization so
>+  // GetPreEffectsVisualOverflowUnion won't have to look up the
>+  // aPreEffectsOverflowRect that we have to hand. The bbox we provide is
>+  // the same as the bbox it would otherwise get - well, almost. We do round it
>+  // out to pixel boundaries, but does that matter? If so, it would have been
>+  // nice to have a comment here explaining why (and should
>+  // GetSVGBBoxForNonSVGFrame also round out?). If not, it doesn't look like
>+  // the extra code complexity is worth it to me.

Ask roc about this perhaps?

Comment 3

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/e6875768cb49

(In reply to Robert Longson from comment #2)
> Please raise a followup bug to track this unless we already have something.

Actually, there is no bug. I added a comment and some assertions.

> what does 'relative to "user space"' mean? relative to the frame's origin in
> "user space" presumably, or just 'in "user space"'

Uh, yeah. I meant *in* user space. Fixed.

> Ask roc about this perhaps?

Will do, but pushed for now due to time pressure.

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e6875768cb49

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #2)
> Ask roc about this perhaps?

Done, and pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b0335d52d1b4 as a follow-up.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0335d52d1b4