Closed Bug 294086 Opened 15 years ago Closed 12 years ago

<svg> should be a replaced element

Categories

(Core :: SVG, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: tor, Assigned: jwatt)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [reflow-refactor])

Attachments

(3 files, 4 obsolete files)

 
Whiteboard: [reflow-refactor]
Assignee: general → jwatt
No longer depends on: 294120
Note to self: I had to remove 294120 from the depends list because it was causing a circular dependency.
Status: NEW → ASSIGNED
In fact lets put it in the blocks list so we're at least linked.
Blocks: 294120
Blocks: 371428
No longer blocks: 275254
Blocks: 349541
No longer blocks: 368683
Attached patch latest patch (obsolete) — Splinter Review
So here's the current incarnation of my attempts to fix SVG sizing. It's incomplete, but worth getting out so interested parties can take a look. A few things to note:

 * nsLayoutUtils::ComputeSizeWithIntrinsicDimensions still needs a fair bit
   of work.
 * nsSVGOuterSVGFrame::GetMinWidth/GetPrefWidth - I'm not sure what to return
   when the intrinsic size is a percentage value.
 * nsSubDocumentFrame::ObtainIntrinsicSizeFrame is called more than I'd like
   during reflow. Maybe there's a way we can cache the result, but care would
   need to be taken to make sure nobody de-refs a dangling pointer.
 * Bunch of other smaller stuff to resolve.

On the plus side, this fixes all the testcases on this bugs deps.
> * nsSVGOuterSVGFrame::GetMinWidth/GetPrefWidth - I'm not sure what to return
>   when the intrinsic size is a percentage value.

What is the expected layout of such an intrinsic size when placed inside a <div style="float: left">?

The possibilities I see are to either have a min-size of 0 and unconstrained pref or to set them both to 0 or to try to do what a <div style="width: 100%"> would end up doing for IntrinsicForContainer.... but maybe we don't really need that.  In any case, this is a spec question.
CSS 2.1 defines the behavior doesn't it? In the case where the containing block's width relies on a replaced element child that has a percentage intrinsic width, the child is assumed to have no intrinsic width. It will then default to 300 px unless the height is specified and the element has an intrinsic ratio. GetMinWidth/GetPrefWidth can't know about whether the parent will rely on it though.
Sure they can.  If they're being called, the parent is relying on it.
Attached patch patch v2 (obsolete) — Splinter Review
This is virtually ready for review I think. I'm finished with the replaced element algorithm (nsLayoutUtils::ComputeSizeWithIntrinsicDimensions). One issue I'm unsure of is the GetPrevInFlow stuff I've added to nsSVGOuterSVGFrame::Reflow (commented out). I'm guessing I need it for something, but I can't quite tell what it's for, or why the versions in nsImageFrame and nsHTMLCanvasFrame are different.

I'm in the midsts of writing extra (ref)tests, and I'd like to resolve the issue above before requesting review. Having said that, I'll have very few cycles and limited access to my computer between now and the code freeze, so if someone wants to jump in early and start looking at this patch that'd be great. (Two blocking1.9+ and two blocking1.9? bugs depend on this.)
Attachment #278579 - Attachment is obsolete: true
You need the prev-in-flow thing if you can actually be paginated (that is, have part of the SVG outer frame on one page, and part on another).  Images can (because they return aStatus != NS_FRAME_COMPLETE as needed).  Your code can't as far as I can see.  The GetPrevInFlow() stuff in nsHTMLCanvasFrame::Reflow looks bogus to me, as does yours; it might be better to assert that there isn't one.
Thanks Boris, I'll do that.
Blocks: 387835
Blocks: 382325
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #278839 - Attachment is obsolete: true
Attachment #279083 - Flags: review?(bzbarsky)
Comment on attachment 279083 [details] [diff] [review]
patch v3

>+  svgElem->mViewportWidth =
>+    nsPresContext::AppUnitsToFloatCSSPixels(aReflowMetrics.width);
>+  svgElem->mViewportHeight =
>+    nsPresContext::AppUnitsToFloatCSSPixels(aReflowMetrics.height);

I should get the computed values from aReflowState there. Borders are still not being painted into the space that's made available for them though for some reason. I don't think that's a blocker for this, but it would be nice to have that working too.
Attachment #279083 - Attachment is obsolete: true
Attachment #279131 - Flags: review?(bzbarsky)
Attachment #279083 - Flags: review?(bzbarsky)
I don't think I can get to this in time for the M8 freeze.  I should be able to review towards the end of Sept, though.
Comment on attachment 279131 [details] [diff] [review]
patch v4 - now supporting borders and padding

I'm working on testing this to death and I have a good grasp of the changes I'm making. However, I think this is still high enough risk that we want to get it in for M8 if at all possible and get other people testing their SVG on it. I'd rather not end up having to land it later if at all possible, so I'll move the review request to dbaron for now. Thanks bz.
Attachment #279131 - Flags: review?(bzbarsky) → review?(dbaron)
Keep in mind that M8 freeze is on Tuesday and this is a holiday weekend.  I suspect getting reviews will be difficult in general....  :(
I thought it was Thursday for some reason, and I didn't realize it was a holiday weekend. :-( We'll see I guess.
+      // Get tentitive values - CSS 2.1 sections 10.3.2 and 10.6.2:

It's tentative

Thanks, I'll fix that in the next patch.
Flags: blocking1.9?
Blocks: 384046
Here are the comments I have so far.  I still need to look at:
 * the nsSVGOuterSVGFrame changes
 * CSS2.1 section 10.3.2
 * handling of intrinsic widths for things with specified height and intrinsic ratio
and perhaps some other things.  But I want a backup of my comments so far:

In nsSVGSVGElement, the removal of SetCoordCtxRect means you'll never
initialize mViewportWidth and mViewportHeight to anything nonzero.  But
they're still used in a bunch of places, so that seems like a problem

Seems like you should rename ObtainIntrinsicSizeFrame to
GetIntrinsicSizeFrame, unless there's a good reason to use Obtain rather
than Get.

I'm not entirely comfortable with calling the percentages an intrinsic
size rather than a specified size, but I suppose it works, and we
probably do need it at this level.

How do you handle when the document inside such an <object> changes,
e.g., due to link navigation?

nsSubDocumentFrame::ObtainIntrinsicSizeFrame seems way too complicated.
You can just get the document from the pres shell and the root element
from the document, no?

Why are you checking GetIntrinsicRatio in IsPercentageAware in
nsLineLayout?  That seems like it will include a good bit of stuff that
shouldn't be included.

Why are you adding display:inline to svg.css?  display:inline is the
default.

In nsLayoutUtils.cpp:

Where you set hasIntrinsicWidth and hasIntrinsicHeight, could you make
the width and the height if/else the same way around rather than the
opposite of each other?

I'm a little skeptical that condensing the intrinsic ratio's height and
width both being nonzero into hasIntrinsicRatio -- it's defined such
that if one side is nonzero than there's a ratio, although I'm not sure
whether that can actually happen.  Maybe that's what the spec says to do
-- but at least add a comment that it's possible to handle 0xN and Nx0
better in some cases.  Though there are actually a bunch of cases that
you're changing from the better handling to the worse handling, although
maybe it doesn't really matter.  What was the motivation for the change?

I also don't like the reuse of the |width| and |height| variables for
two different things.  Why not call your new one |intrinsicWidth| and
|intrinsicHeight| or |intrinsicWidthCoord| and |intrinsicHeightCoord|
and keep them separate?  (You'd need to readd the very last clause in
the if/else chain, where you'd do width=intrinsicWidth and
height=intrinsicHeight.)

Have you tested fantasai's tests for the min/max-height handling rules
on images?  They should really be part of reftests if they're not
already.

+        } else if (height < minHeight) {
+          width = maxWidth;
+          height = minHeight;

+        } else if (height > maxHeight) {
           width = minWidth;
           height = maxHeight;

These two three-line insertions aren't needed, but you might want to
instead add a comment around the else following each of them that that
else covers two rows of the table.  We know the extras aren't needed
since if (w < min-width) and (h > max-height), then we know that:
  * max(max-height * w/h, min-width) is equal to min-width since the two
    sides of the max() are the product of the two smaller terms in the
    inequalities (w and max-height) divided by h, and the product of the
    two larger terms in the inequalities (h and min-width) divided by h.
  * min(min-width * h/w, max-height) is equal to max-height since the
    two sides of the min() are the product of the two larger terms in
    the inequalities (min-width and h) divided by w, and the product of
    the two smaller terms in the inequalities (w and max-height) divided
    by w.
Thus the last two rows of the table really aren't needed -- they're just
clarifying that in that case the two out of the first four rows of the
table that apply both yield the same result -- so we can just include
the code for whichever of those two rows is easier.
  It's worth a comment to explain this, though.

I don't like the unindenting (removal of else{}) surrounding the height
conditions around the end of that if/else chain.  It was nice to have
all the width conditions at one level of indentation and all the height
conditions at the next.
I'm having trouble making sense of some of the code in nsSVGOuterFrame because I'm not sure what svg->mLengthAttributes[nsSVGSVGElement::WIDTH] gives in the case where nothing is specified.  The 1.1 spec suggests 100% (which I think gives bad results, since it avoids a bunch of the nice things in CSS2.1, although that may have been changed in errata and/or 1.2), but it looks like our code gives 0, unless I'm missing something.
Anyway, may as well post the next chunk of review comments, although I'm expecting to have some pretty substantial comments on the GetPrefWidth / GetIntrinsicRatio / GetIntrinsicSize / Reflow code in nsSVGOuterFrame once I understand it:

I realized that some of the nsLayoutUtils.cpp changes are wrong if
box-sizing has a non-default value.  Your added calls to
ComputeWidthValue and ComputeHeightDependentValue when you're passing in
aIntrinsicSize.width and aIntrinsicSize.height need to be computed using
bogus box sizing values -- you need to replace (when and around making
those calls) boxSizingAdjust.width and .height with 0 and
boxSizingToMarginEdgeWidth with (boxSizingToMarginEdgeWidth +
boxSizingAdjust.width) since the intrinsic sizes always use content-box
widths and heights.

And the start of my nsSVGOuterFrame.cpp comments:

In nsSVGOuterSVGFrame::AttributeChanged:
+    if (embeddingFrame && !HasFixedSize(embeddingFrame)) {
...
+    } else if (!embeddingFrame) {
This pattern is probably better written as:
  if (embeddingFrame) {
    if (!HasFixedSize(embeddingFrame)) {
      ...
    }
  } else {
    ...
  }
or maybe better as:
  if (!embeddingFrame) {
    ...
  } else if (!HasFixedSize(embeddingFrame)) {
    ...
  }

In nsSVGOuterSVGFrame::GetPrefWidth, I think using 300 is wrong when you
have an intrinsic ratio -- at least for min-width, if not for pref-width
as well.  I think min-width should be 0, and probably pref-width too,
since the SVG will fit in its container.

You have some occurrences of NS_STATIC_CAST that need to be static_cast
now.
Blocks: 310326
(In reply to comment #19)
> In nsSVGSVGElement, the removal of SetCoordCtxRect means you'll never
> initialize mViewportWidth and mViewportHeight to anything nonzero.  But
> they're still used in a bunch of places, so that seems like a problem

The only caller of SetCoordCtxRect used to be nsSVGOuterSVGFrame::Reflow, and now I just set mViewportWidth and mViewportHeight directly in that method.

> Seems like you should rename ObtainIntrinsicSizeFrame to
> GetIntrinsicSizeFrame, unless there's a good reason to use Obtain rather
> than Get.

My rational was that nsSubDocumentFrame::ObtainIntrinsicSizeFrame calls nsSubDocumentFrame::GetDocShell, and nsSubDocumentFrame::GetDocShell has the following comment:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrameFrame.cpp&rev=3.317&mark=642,643#641

> How do you handle when the document inside such an <object> changes,
> e.g., due to link navigation?

Ah, I covered the case where the document inside an <object> changes from a document without intrinsic size to SVG with intrinsic size, but I hadn't considered the converse. I'll look into that.

> nsSubDocumentFrame::ObtainIntrinsicSizeFrame seems way too complicated.
> You can just get the document from the pres shell and the root element
> from the document, no?

It's the frame for the root element I want though. I can perhaps change the code to:

    nsCOMPtr<nsIDocShell> docShell;
    GetDocShell(getter_AddRefs(docShell));
    if (docShell) {
      nsCOMPtr<nsIPresShell> presShell;
      docShell->GetPresShell(getter_AddRefs(presShell));
      if (presShell) {
        nsIDocument* subDoc = presShell->GetDocument();
        if (subDoc) {
          nsIContent* rootContent = subdoc->GetRootContent();
          if (rootContent) {
            subDocRoot = presShell->GetPrimaryFrameFor(rootContent);
          }
        }
      }
    }

if you like, but I was trying to avoid the GetPrimaryFrameFor overhead. What do you think?

> Why are you checking GetIntrinsicRatio in IsPercentageAware in
> nsLineLayout?  That seems like it will include a good bit of stuff that
> shouldn't be included.

I really can't recall now, but it was something to do with bug 369850 comment 16. From my tests it doesn't appear to be necessary any more though, so I can remove it.

> Why are you adding display:inline to svg.css?  display:inline is the
> default.

Just to make it explicit that the SVG spec requires it to default to inline.
(In reply to comment #19)
> I also don't like the reuse of the |width| and |height| variables for
> two different things.  Why not call your new one |intrinsicWidth| and
> |intrinsicHeight| or |intrinsicWidthCoord| and |intrinsicHeightCoord|
> and keep them separate?  (You'd need to readd the very last clause in
> the if/else chain, where you'd do width=intrinsicWidth and
> height=intrinsicHeight.)

I don't really think "intrinsic" should be in the name. The intrinsic values are fixed values (during the lifetime of the function call), which are then modified according to the section 10 algorithm, on their way to becoming the used values. I'll add two variables tmpWidth and tmpHeight to indicate they contain intermediary values.

> Have you tested fantasai's tests for the min/max-height handling rules
> on images?  They should really be part of reftests if they're not
> already.

I've been running our entire reftest suite, and all 19 tests on

  http://fantasai.inkedblade.net/style/specs/css2.1/tests/min-max-replaced

pass. I also have the ~180 reftests I wrote to cover this bug. I'll upload them to my site and include the reftest.list for them in my next patch.

> These two three-line insertions aren't needed, but you might want to
> instead add a comment...

Ah, I see, will do.

> I don't like the unindenting (removal of else{}) surrounding the height
> conditions around the end of that if/else chain.  It was nice to have
> all the width conditions at one level of indentation and all the height
> conditions at the next.

The problem I had was that it really confused me the first time I was trying to correlate the code with the table in the spec. I found my changes made it a lot easier for me, but if you want I can revert that part.
(In reply to comment #20)
> I'm having trouble making sense of some of the code in nsSVGOuterFrame because
> I'm not sure what svg->mLengthAttributes[nsSVGSVGElement::WIDTH] gives in the
> case where nothing is specified.

Yeah, our code to get the defaults is very "clever" in how it saves space, but it sure isn't easy to figure out. So what happens in the case of no width/height attributes is that when we end up here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGElement.cpp&rev=1.140&mark=112-117#102

the lengthInfo.mLengthInfo is this static object here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGSVGElement.cpp&rev=1.105&mark=67-73#66

As you can see that gives the default for width and height as 100%.
(In reply to comment #21)
> I realized that some of the nsLayoutUtils.cpp changes are wrong if
> box-sizing has a non-default value...since the intrinsic sizes always use
> content-box widths and heights.

Actually I did that intentionally since I thought it was best for box-sizing to be taken into account for intrinsic dimensions. I was thinking in the context of SVG at the time, specifically inline SVG with width/height attributes. In retrospect those changes don't make as much sense for other image types, so I'll fix that up as you suggest and perhaps revisit for SVG in future.

> In nsSVGOuterSVGFrame::AttributeChanged:
> +    if (embeddingFrame && !HasFixedSize(embeddingFrame)) {
> ...
> +    } else if (!embeddingFrame) {
> This pattern is probably better written as:
>   if (embeddingFrame) {
>     if (!HasFixedSize(embeddingFrame)) {
>       ...
>     }
>   } else {
>     ...
>   }

Then we'd never request any reflow at all if there was an embeddingFrame which doesn't rely on our intrinsic size. Actually come to think of it, in that case the SVG's width and height is overridden, so that's exactly what we want. I'll fix that in the next patch.

> In nsSVGOuterSVGFrame::GetPrefWidth, I think using 300 is wrong when you
> have an intrinsic ratio -- at least for min-width, if not for pref-width
> as well.  I think min-width should be 0, and probably pref-width too,
> since the SVG will fit in its container.

Okay.
(In reply to comment #25)
> Then we'd never request any reflow at all if there was an embeddingFrame which
> doesn't rely on our intrinsic size. Actually come to think of it, in that case

I wasn't proposing any logic change -- just not checking embeddingFrame twice.
(In reply to comment #22)
> The only caller of SetCoordCtxRect used to be nsSVGOuterSVGFrame::Reflow, and
> now I just set mViewportWidth and mViewportHeight directly in that method.

Could you use a setter and make the members private?  It seems highly confusing to have member variables set in another *module*.
Glad to hear you have so many reftests.

I'm actually a bit hesitant about supporting borders and padding on the
svg element given that 100% default, since it means the borders and
padding will overflow by default.  Do you know if other browsers support
them?

I notice you're removing SuspendRedraw/UnsuspendRedraw from
nsSVGOuterSVGFrame::Reflow -- why were they there, and why aren't they
needed anymore?

In nsSVGOuterSVGFrame::Reflow, could you leave the nsHTMLReflowMetrics
parameter called aDesiredSize, for consistency with other reflow
methods?

+  aReflowMetrics.width  = aReflowState.ComputedWidth();
+  aReflowMetrics.height = aReflowState.ComputedHeight();
+
+  // add borders and padding
+  aReflowMetrics.width  += aReflowState.mComputedBorderPadding.LeftRight();
+  aReflowMetrics.height += aReflowState.mComputedBorderPadding.TopBottom();

Seems like you should just assign the sums straight away rather than
using two statements.

+      // XXX what if we have CSS width/height? How can we tell the embedding
+      // element about changes to those properties?

You should probably implement DidSetStyleContext, although it doesn't
currently have access to the old style data (but it would be very easy
to give it access, although that would probably be best as a separate
patch because it'll have a lot of boring signature changes).  Given
that, I'm ok if you do that as a later patch, but I think it should
happen for 1.9.

(The CSS width/height affect the SVG's mLengthAttributes, right?)

In nsLayoutUtils::ComputeSizeWithIntrinsicDimensions, can't you assert
that aCBSize.width != NS_UNCONSTRAINEDSIZE rather than checking it?
(You need to check the height, though.)

r+sr=dbaron with all these comments addressed, although I think I'd like
to look at the patch that addresses them to check some of them.
> Could you use a setter and make the members private?  It seems highly
> confusing to have member variables set in another *module*.

Sure. My problem was that I needed something like an nsSize that has float members, not nscoord or double members. I'll add that I guess.

> I'm actually a bit hesitant about supporting borders and padding on the
> svg element given that 100% default, since it means the borders and
> padding will overflow by default.  Do you know if other browsers support
> them?

Opera and Safari both support border, padding and margin on SVG. The 100% is stupid in this case of course, which is partly why I wanted intrinsic sizing to take box-sizing into account on SVG. Anyway, users can specify the width and height, so I think it's still useful.

> I notice you're removing SuspendRedraw/UnsuspendRedraw from
> nsSVGOuterSVGFrame::Reflow -- why were they there, and why aren't they
> needed anymore?

They were there to batch together the changes made by various calls. Actually, they're no longer needed even in the current code, since the only thing that might require them is the NotifyViewportChange() call, and it has it's own SuspendRedraw/UnsuspendRedraw calls. They should have been removed already.

> In nsSVGOuterSVGFrame::Reflow, could you leave the nsHTMLReflowMetrics
> parameter called aDesiredSize, for consistency with other reflow
> methods?

If I must. I hate that name though (the word "metrics" is a lot less misleading about what it contains IMHO).

> +      // XXX what if we have CSS width/height? How can we tell the embedding
> +      // element about changes to those properties?
> 
> You should probably implement DidSetStyleContext, although it doesn't
> currently have access to the old style data (but it would be very easy
> to give it access, although that would probably be best as a separate
> patch because it'll have a lot of boring signature changes).  Given
> that, I'm ok if you do that as a later patch, but I think it should
> happen for 1.9.

Actually I wrote that comment when I was thinking that embedding elements like <object> should use the CSS width/height on the embedded document's root as its intrinsic dimensions if specified. I.e. CSS should be taken over the attributes when asking embedded SVG documents for their intrinsic dimensions. Thinking about that got me into a whole world of pain though - although it would make some sense, it's not part of a spec., and it has implementation complications -  so I concluded that for the purposes of intrinsic sizing we should ignore CSS on embedded SVG for now. If someone specifies CSS and the attributes then embeds the SVG by reference they'll get what they get. It's a much bigger issue that should be revisited later I think.

> (The CSS width/height affect the SVG's mLengthAttributes, right?)

No, mLengthAttributes only contains the values parsed from the attributes. It wouldn't be much use as the source of the intrinsic dimensions for CSS sizing if CSS width/height were mapped to them.
(In reply to comment #29)
> > (The CSS width/height affect the SVG's mLengthAttributes, right?)
> 
> No, mLengthAttributes only contains the values parsed from the attributes. It
> wouldn't be much use as the source of the intrinsic dimensions for CSS sizing
> if CSS width/height were mapped to them.

Hrm.  In that case, it seems like there are a lot of tests that aren't right, although maybe if they're consistently wrong it's not so horrible.  I'll try to look at that more later...
I would really like for this patch to go in for M9 if possible since it fixes so much, and I'd like it to get exposure so I can know about any problems as soon as possible. If you're happy to give it r+sr can you also mark approval?

Concerning the resizing after unloading SVG in <object>, it isn't entirely straightforward to fix since although nsObjectLoadingContent inherits from nsImageLoadingContent, the only observers that can currently be registered are imgIDecoderObserver observers. The unload resizing is a minor problem compared to all the other issues this fixes, but I'll certainly work to fix it (and I added a reftest for this case).

Oh, and note that the IsPercentageAware change is required for inline SVG to reflow correctly when it has a percentage width attribute and its containing block is resized.

(In reply to comment #30)
> Hrm.  In that case, it seems like there are a lot of tests that aren't right,
> although maybe if they're consistently wrong it's not so horrible.  I'll try
> to look at that more later...

Not quite sure what you mean there.
Attachment #279131 - Attachment is obsolete: true
Attachment #285739 - Flags: superreview?(dbaron)
Attachment #285739 - Flags: review?(dbaron)
Attachment #279131 - Flags: review?(dbaron)
Did you mean to just attach the reftests names or did you mean to attach their contents?
That was deliberate so as not to make the patch too large to attach (adding the file reftest.list has bumped up the size enough as it is). You can find the actual reftests at http://jwatt.org/moz/tmp/sizing/
Flags: blocking1.9? → blocking1.9+
Blocks: 401144
Just to remind myself, for <svg width="50" style="width: 25px" /> (with some contents), the width attribute is supposed to win in standalone SVG (since the 'width' CSS property doesn't apply in SVG), but the CSS property wins when the svg element is a child element of an HTML element, since the width CSS property applies at that boundary case.  So it's correct that IntrinsicSize doesn't consider the CSS 'width' property in any cases.  So I think that part of the patch is ok.

Although maybe it should be that the 'width' property shouldn't apply at all to svg:svg.
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M10
Comment on attachment 285739 [details] [diff] [review]
patch addressing comments

In nsLayoutUtils -- can you depend on both sides of the intrinsic ratio being nonnegative, rather than checking them?  It seems like it would be better if we guaranteed that intrinsic ratios were always composed of nonnegative numbers.


In nsLayoutUtils::ComputeSizeWithIntrinsicDimensions I'd think you could assert that aCBSize.width is not NS_UNCONSTRAINEDSIZE independently of any other conditions.

Please get followup bugs filed on:
 * implementing DidSetStyleContext
 * any remaining issues about navigation between documents inside the <object>
 * a better fix for the box-sizing issues for SVG with padding/border/margin

And don't forget to land all of the reftests along with the patch!

r+sr=dbaron

Sorry to take so long to get to the second pass of review here... and thanks for fixing all of this -- this looks like it makes a big improvement.
Attachment #285739 - Flags: superreview?(dbaron)
Attachment #285739 - Flags: superreview+
Attachment #285739 - Flags: review?(dbaron)
Attachment #285739 - Flags: review+
(In reply to comment #36)
> Please get followup bugs filed on:
>  * implementing DidSetStyleContext
>  * any remaining issues about navigation between documents inside the <object>
>  * a better fix for the box-sizing issues for SVG with padding/border/margin

... and anything else I missed.
Checked in.

Thanks for the review David. I'll be filing follow up bugs shortly. I'm going to leave the dependent bugs as they are for 24 hours or so until I'm sure the checkin will stick.
I can't recompile trunk after this checkin - http://pastebin.mozilla.org/244826 (including the bustage fix from JWatt)
The second attempt to fix the bustage did the trick.

dbaron - the fix was to change nsLayoutUtils::ComputeSizeWithIntrinsicDimensions to have aIntrinsicSize be pass by value instead of pass by reference. I'm not sure why that's necessary on Mac and Linux, but I'm assuming it's okay to make that change.
jag filled me in on temporaries returned from functions being rvalues, meaning that to be passed to a function directly, the function parameter must be const. I've checked that fix in, so here's the code (minus reftests) that landed as a single patch.
The reftest counts on tinderbox didn't go up.  Looks like you need to add an include of the reftest.list to the reftest.list in the parent directory?
Oops. I've turned them on now. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Depends on: 404511
Depends on: 404636
Depends on: 404677
Depends on: 404491
Blocks: 290770
Blocks: 401775
(In reply to comment #36)
> Please get followup bugs filed on:
>  * implementing DidSetStyleContext

bug 404998

>  * any remaining issues about navigation between documents inside the <object>

bug 404999

>  * a better fix for the box-sizing issues for SVG with padding/border/margin

bug 400155
Blocks: 405382
Blocks: 315862
Blocks: 317474
Blocks: 336801
Blocks: 367204
Blocks: 382528
Blocks: 321531
Blocks: 70978
Depends on: 441927
Depends on: 509274
Depends on: 596378
You need to log in before you can comment on or make changes to this bug.