825720 - Don't use GetRootLayoutFrame to calculate the size of SVG images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now when layout code needs the intrinsic size and aspect ratio of an SVG image, it calls imgIContainer::GetRootLayoutFrame and then inspects the returned nsIFrame directly for size and ratio information. This causes breaks ExtractFrame, media fragments, etc. because no matter what sort of clipping is applied to the image, the layout code only sees the intrinsic size of the underlying SVG document.

The solution is to add methods for computing the intrinsic size and intrinsic ratio of an image directly to the imgIContainer interface, encapsulating the logic involved.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't use GetRootLayoutFrame to calculate the size of SVG images.

Proposed patch. Try is closed right now so it has only been tested locally.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=995bea5f3613

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't use GetRootLayoutFrame to calculate the size of SVG images.

It seems it's not same to use nsIFrame::IntrinsicSize in public imagelib interfaces. Switched it out for nsSize. New try job here: https://tbpl.mozilla.org/?tree=Try&rev=d1e4ebe1ce4b

Comment 4

[Seth Fowler [:seth] [:s2h]]

Argh. There's an SVG-related failure in the previous try run, and I'm pretty sure it's not caused by this patch (it's a known intermittent failure with an existing bug, and it only happened on one run) but just to be sure I'm running another try run: https://tbpl.mozilla.org/?tree=Try&rev=2db10a62b5ab

Comment 5

[Seth Fowler [:seth] [:s2h]]

Try seems OK, I suppose. I'm hitting that failure even on patches that have nothing to do with this one, so I guess it's just a fairly common intermittent failure. (That I'd love to get fixed.) Going ahead and requesting review.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 697755 [details] [diff] [review]
Don't use GetRootLayoutFrame to calculate the size of SVG images.

Review of attachment 697755 [details] [diff] [review]:
-----------------------------------------------------------------

In general looks fine, though I'm not a layout peer. I do have some questions/things that need addressing though.

::: image/public/imgIContainer.idl
@@ +36,1 @@
>  [ptr] native gfxASurface(gfxASurface);

You have to change the UUID of this interface.

@@ +71,5 @@
>     */
>    readonly attribute int32_t height;
>  
>    /**
> +   * The intrinsic size of this image. If the image has no intrinsic size in

It needs to be documented what units this is stored in.

::: image/src/RasterImage.cpp
@@ +765,5 @@
>  
>    *aHeight = mSize.height;
>    return NS_OK;
>  }
> + 

whitespace

@@ +785,5 @@
> +NS_IMETHODIMP
> +RasterImage::GetIntrinsicRatio(nsSize* aRatio)
> +{
> +  if (mError) {
> +    *aRatio = nsSize(0, 0);

In the error case you shouldn't touch the outparam.

::: layout/generic/nsImageFrame.cpp
@@ +293,5 @@
> +  if (NS_SUCCEEDED(aImage->GetIntrinsicSize(&intrinsicSize))) {
> +    if (intrinsicSize.width != -1)
> +      mIntrinsicSize.width.SetCoordValue(intrinsicSize.width);
> +    if (intrinsicSize.height != -1)
> +      mIntrinsicSize.height.SetCoordValue(intrinsicSize.height);

What should happen to the width and height's coord value if there is no intrinsic size? Should it be set to 0 then, as in the failure case?

Comment 7

[Seth Fowler [:seth] [:s2h]]

Agreed on the first four comments; will update the patch.

> What should happen to the width and height's coord value if there is no intrinsic
> size? Should it be set to 0 then, as in the failure case?

This code needs a comment, which I'll add in the updated version of the patch. That case is handled implicitly because mIntrinsicSize is first initialized with nsIFrame::IntrinsicSize's default constructor, which sets the unit for both width and height to eStyleUnit_None, which is exactly what we want in this case. (This is distinct from giving the width and height a unit of eStyleUnit_Coord and setting them to 0, which is what SetCoordValue does.) I agree that this isn't obvious though; classic case of "seems obvious while writing the code; quite difficult to follow later".

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't use GetRootLayoutFrame to calculate the size of SVG images.

Made the changes discussed above.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=a5b316a583d7

If this looks good then we're ready for checkin.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't use GetRootLayoutFrame to calculate the size of SVG images.

Whoops; forgot to change the UUID of the interface. _Now_ all of the changes discussed above are made. Canceled previous try job.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=8db1b2f2a8e6

Comment 12

[Seth Fowler [:seth] [:s2h]]

Looks good to go. Requesting checkin.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8fe09ac1842

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Sorry, but this was backed out for making bug 772443 much more frequent.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de9ec989b34

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't use GetRootLayoutFrame to calculate the size of SVG images.

Rebase on top of bug 704059, which should eliminate the bug that was causing the random oranges that led to this patch being backed out. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=ed7dc4057029

Comment 16

[Seth Fowler [:seth] [:s2h]]

Try looks good. I think this is ready to reland. Requesting checkin.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d73b8ba1a1

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This was backed out and re-landed while trying to track down intermittent Android Armv6 R2 timeouts. New changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a50bccf86e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42a50bccf86e