Closed Bug 766227 Opened 9 years ago Closed 9 years ago

More nsSVGIntegrationUtils cleanup to make it much easier to understand

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

Following on from bug 766120, this is the rest of my cleanup to nsSVGIntegrationUtils from bug 614732 to make it easier to understand.
Attached patch patchSplinter Review
Attachment #634512 - Flags: review?(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?
Attachment #634512 - Flags: review?(longsonr) → review+
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.
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/e6875768cb49
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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.
Depends on: 767535
Blocks: 614732
You need to log in before you can comment on or make changes to this bug.