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

RESOLVED FIXED in mozilla22

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 696840 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Blocks: 790640
(Assignee)

Updated

5 years ago
Blocks: 826093
(Assignee)

Comment 3

5 years ago
Created attachment 697755 [details] [diff] [review]
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
(Assignee)

Updated

5 years ago
Attachment #696840 - Attachment is obsolete: true
(Assignee)

Comment 4

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

Comment 5

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

Updated

5 years ago
Attachment #697755 - Flags: review?(joe)
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?
Attachment #697755 - Flags: review?(joe) → review+
(Assignee)

Comment 7

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

Comment 8

5 years ago
Created attachment 700817 [details] [diff] [review]
Don't use GetRootLayoutFrame to calculate the size of SVG images.

Made the changes discussed above.
(Assignee)

Updated

5 years ago
Attachment #697755 - Attachment is obsolete: true
(Assignee)

Comment 9

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

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

Comment 10

5 years ago
Created attachment 700825 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Attachment #700817 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Looks good to go. Requesting checkin.
Keywords: checkin-needed
Sorry, but this was backed out for making bug 772443 much more frequent.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de9ec989b34
(Assignee)

Comment 15

5 years ago
Created attachment 714665 [details] [diff] [review]
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
(Assignee)

Updated

5 years ago
Attachment #700825 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 842850
(Assignee)

Comment 16

5 years ago
Try looks good. I think this is ready to reland. Requesting checkin.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/42a50bccf86e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.