More nsSVGIntegrationUtils cleanup to make it much easier to understand

RESOLVED FIXED in mozilla16

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Following on from bug 766120, this is the rest of my cleanup to nsSVGIntegrationUtils from bug 614732 to make it easier to understand.
(Assignee)

Comment 1

5 years ago
Created attachment 634512 [details] [diff] [review]
patch
Attachment #634512 - Flags: review?(longsonr)

Comment 2

5 years ago
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+
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla16

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e6875768cb49
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

5 years ago
(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.

Updated

5 years ago
Depends on: 767535
https://hg.mozilla.org/mozilla-central/rev/b0335d52d1b4
(Assignee)

Updated

5 years ago
Blocks: 614732
You need to log in before you can comment on or make changes to this bug.