Closed
Bug 825720
Opened 12 years ago
Closed 11 years ago
Don't use GetRootLayoutFrame to calculate the size of SVG images
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: seth, Assigned: seth)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
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•12 years ago
|
||
Proposed patch. Try is closed right now so it has only been tested locally.
Assignee | ||
Comment 2•12 years ago
|
||
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=995bea5f3613
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
Attachment #696840 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 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•11 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•11 years ago
|
Attachment #697755 -
Flags: review?(joe)
Comment 6•11 years ago
|
||
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•11 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•11 years ago
|
||
Made the changes discussed above.
Assignee | ||
Updated•11 years ago
|
Attachment #697755 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 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•11 years ago
|
||
Whoops; forgot to change the UUID of the interface. _Now_ all of the changes discussed above are made. Canceled previous try job.
Assignee | ||
Updated•11 years ago
|
Attachment #700817 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=8db1b2f2a8e6
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8fe09ac1842
Keywords: checkin-needed
Comment 14•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Attachment #700825 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
Try looks good. I think this is ready to reland. Requesting checkin.
Keywords: checkin-needed
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d73b8ba1a1
Keywords: checkin-needed
Comment 18•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/42a50bccf86e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•