Last Comment Bug 766227 - More nsSVGIntegrationUtils cleanup to make it much easier to understand
: More nsSVGIntegrationUtils cleanup to make it much easier to understand
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on: 767535
Blocks: 614732
  Show dependency treegraph
 
Reported: 2012-06-19 11:33 PDT by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2012-06-30 17:44 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (24.57 KB, patch)
2012-06-19 11:34 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
longsonr: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-06-19 11:33:47 PDT
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] (Away Jun. 27 - Jul. 13) 2012-06-19 11:34:09 PDT
Created attachment 634512 [details] [diff] [review]
patch
Comment 2 Robert Longson 2012-06-20 01:19:33 PDT
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] (Away Jun. 27 - Jul. 13) 2012-06-21 03:25:06 PDT
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] 2012-06-21 13:04:35 PDT
https://hg.mozilla.org/mozilla-central/rev/e6875768cb49
Comment 5 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-06-22 03:47:19 PDT
(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] 2012-06-23 05:48:28 PDT
https://hg.mozilla.org/mozilla-central/rev/b0335d52d1b4

Note You need to log in before you can comment on or make changes to this bug.