Closed Bug 825771 Opened 12 years ago Closed 11 years ago

[css3-images] implement 'image-orientation' property

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
relnote-firefox --- 26+

People

(Reporter: dbaron, Assigned: seth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 22 obsolete files)

69.53 KB, patch
Details | Diff | Splinter Review
15.18 KB, patch
Details | Diff | Splinter Review
8.26 KB, patch
Details | Diff | Splinter Review
28.20 KB, patch
Details | Diff | Splinter Review
6.33 KB, patch
Details | Diff | Splinter Review
We should implement the image-orientation property in: http://dev.w3.org/csswg/css3-images/#image-orientation which allows authors to specify This property is in Candidate Recommendation: http://www.w3.org/TR/2012/CR-css3-images-20120417/#the-image-orientation We should additionally implement a from-image value, possibly prefixed, that addresses my comments in: http://lists.w3.org/Archives/Public/www-style/2011Dec/0001.html and allows authors to opt in to honoring EXIF orientation, which is particularly important for Thunderbird (and allows fixing bug 298619). (I really dislike the way this property is specified, but I lost that argument.) The Style System side of this is easy; I'm not sure how hard the ImageLib side is, but cc:ing some people who would know.
That third line should have been: which allows authors to specify a right-angle rotation that can be applied to the image data before any other processing.
I'm not sure that this is actually a useful thing to implement without the from-image value. It's in the spec because it was needed by the print industry, but is there a need in the Web industry (from-image aside)? > (I really dislike the way this property is specified, but I lost that argument.) Great to know, maybe you could elucidate on that point?
(In reply to fantasai from comment #2) > > (I really dislike the way this property is specified, but I lost that argument.) > > Great to know, maybe you could elucidate on that point? Just what's in the aforementioned: http://lists.w3.org/Archives/Public/www-style/2011Dec/0001.html
Joe may want to weigh in here too, but FWIW from my perspective this doesn't look too bad now that the refactoring we've done for media fragments is in place.
(from an imagelib perspective, that is)
Presuming that we just add it as another "transformation" (changing size, drawing differently) that your extra Image implementation will be adding, yeah, seems totally straightforward.
http://lists.w3.org/Archives/Public/www-style/2011Dec/0001.html http://lists.w3.org/Archives/Public/www-style/2011Dec/0003.html describe from-image. Tab's 'flip' proposal, which I think should go along, specifies both order (after the rotation) and axis (inline). Seth points out the order should probably match the textual order in the style sheet, as Tab does. I think we should implement that.
Depends on: 869723
This shouldn't be too hard if we can get bug 869723 taken care of.
(In reply to Seth Fowler [:seth] from comment #8) > This shouldn't be too hard if we can get bug 869723 taken care of. Sorry, I might be dense, but I don't see how bug 869723 blocks this one. From reading the image-orientation spec, there does not seem to be an opt-in to honoring EXIF data. While this CSS property could be used to implement EXIF-based rotation, this property defines orientation manually, and not from metadata. Right?
Looks like the "take the orientation from the image" value got pushed to css-images-4: http://dev.w3.org/csswg/css-images-4/#the-image-orientation
(In reply to Fred Wenzel [:wenzel] from comment #9) > (In reply to Seth Fowler [:seth] from comment #8) > > This shouldn't be too hard if we can get bug 869723 taken care of. > > Sorry, I might be dense, but I don't see how bug 869723 blocks this one. > From reading the image-orientation spec, there does not seem to be an opt-in > to honoring EXIF data. While this CSS property could be used to implement > EXIF-based rotation, this property defines orientation manually, and not > from metadata. Right? Yes, that's right, but we're not implementing the version in css-images-3. We're implementing Tab's proposal as mentioned in comment 7.
(Which is a superset of the version in css-images-3, to be clear.)
(In reply to Cameron McCormack (:heycam) from comment #10) > Looks like the "take the orientation from the image" value got pushed to > css-images-4: > > http://dev.w3.org/csswg/css-images-4/#the-image-orientation That's essentially what we're implementing except that we'll also implement the "flip" modifier mentioned in Issue 18.
Uploading an incomplete patch. This builds but it needs more work - in particular, because image-orientation is an inherited property, I don't see a good existing style struct to place it on. It may need a new one, which perhaps isn't so bad since there are some other properties (image-resolution, image-rendering) that would fit in on the same style struct logically.
Assignee: nobody → seth
After talking to dbaron we've agreed that this can go on the visibility or user interface style structs.
This (unfortunately fairly large) patch adds support for the image-orientation property to the CSS system. Later parts coming in the near future will add support for the property in the layout code and appropriate reftests.
Attachment #781495 - Flags: review?(dbaron)
Attachment #757450 - Attachment is obsolete: true
This patch adds image-orientation support to nsImageFrame (in the process modifying it to store a reference directly to the image container it displays, instead of always accessing it through the image request). Since nsImageFrame covers both image elements and generated content, this is enough to handle the important cases described in the spec. (The spec specifically forbids applying image-orientation to 'decoration' images like background-image.) Per discussion with dbaron, I need to ping the list about whether image-orientation should be supported for list-style-image and within SVGs. I'll create a followup bug for those cases shortly. Still to come: reftests.
Attachment #781962 - Flags: review?(dbaron)
Since I've changed some internal details of how nsImageFrame works, I went ahead and pushed a try job for the entire patch stack so far. I'll need another one once the reftests are ready, but I want some early warning if any issues have arisen. https://tbpl.mozilla.org/?tree=Try&rev=b53ca78b8ea5
Here are the first reftests. Not ready for review yet as more are needed; this only covers from-image for raster images. Tests still needed: * Explicit orientations for raster images. * Generated content. * Test that background-image and border-image don't work. * Non-axis-aligned angles. (To verify that they round appropriately.) * Tests for SVG-as-image in <img> tags. (Explicit orientations and a single from-image, since SVG images don't have EXIF.)
Interesting. So it looks like those changes to nsImageFrame make a previously intermittent failure happen 100% of the time. That might be good news, actually.
Fixed the reftest failure above. We needed to update nsImageFrame::mImage in NotifyNewCurrentRequest as well as in OnStartRequest, since we do not get the latter in a synchronous manner.
Attachment #782781 - Flags: review?(dbaron)
Attachment #781962 - Attachment is obsolete: true
Attachment #781962 - Flags: review?(dbaron)
Comment on attachment 781495 [details] [diff] [review] (Part 1) - Add CSS support for the image-orientation property. Note that nsStyleConsts.h moved from layout/base to layout/style a day or two ago. Patch should apply fine in the new location, but it needs manual fixup to get there. nsCSSParser: >+ * <angle> flip? | flip | from-image (and the corresponding code) Why do you accept 'flip' on its own? (And is it intended that you accept <angle> flip and not flip <angle>?) Though given that I'm not seeing 'flip' in the current spec draft, I wonder if we should accept it at all. It seems bad not to offer the same feature set that from-image offers, though. >+ if (ParseVariant(flip, VARIANT_KEYWORD | VARIANT_STRING, >+ nsCSSProps::kImageOrientationFlipKTable)) { This (and the next) should have VARIANT_KEYWORD only, not VARIANT_STRING. nsComputedDOMStyle: >+ nsCSSKeyword keyword = >+ nsCSSProps::ValueToKeywordEnum(NS_STYLE_IMAGE_ORIENTATION_FROM_IMAGE, >+ nsCSSProps::kImageOrientationKTable); >+ string.Append(NS_ConvertUTF8toUTF16(nsCSSKeywords::GetStringValue(keyword))); string.AppendLiteral("from-image"); >+ string.AppendLiteral(" "); >+ >+ nsCSSKeyword keyword = >+ nsCSSProps::ValueToKeywordEnum(NS_STYLE_IMAGE_ORIENTATION_FLIP, >+ nsCSSProps::kImageOrientationFlipKTable); >+ string.Append(NS_ConvertUTF8toUTF16(nsCSSKeywords::GetStr string.AppendLiteral(" from-image"); I'm inclined to say that nsStyleImageOrientation should live in nsStyleStruct.h rather than nsStyleCoord.h nsStyleStruct.cpp: >+ if ((mImageOrientation != aOther.mImageOrientation)) { >+ NS_UpdateHint(hint, nsChangeHint_NeedReflow); >+ } This should be nsChangeHint_AllReflowHints. It can change intrinsic widths, and it needs to reflow all descendants, and it needs to mark the frame dirty. nsStyleUtil::AppendAngleValue: This should take the nsStyleCoord by reference rather than by value. You should just use aResult.AppendFloat rather than constructing a temporary nsROCSSPrimitiveValue. r=dbaron with that fixed, although we might need some discussion about the flip questions
Attachment #781495 - Flags: review?(dbaron) → review+
Comment on attachment 782781 [details] [diff] [review] (Part 2) - Add support for image-orientation to nsImageFrame. Transferring this request to dholbert; sorry, should have done that sooner.
Attachment #782781 - Flags: review?(dbaron) → review?(dholbert)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #22) > Comment on attachment 781495 [details] [diff] [review] > Why do you accept 'flip' on its own? (And is it intended that you > accept <angle> flip and not flip <angle>?) Tab's proposed grammar was this: > image-orientation: from-image | [ <angle>? flip? ] It seemed to me that he was proposing to allow flip by itself. I have no problem with disallowing that, though; it certainly complicates the code a bit. > Though given that I'm not seeing 'flip' in the current spec draft, I > wonder if we should accept it at all. It seems bad not to offer the > same feature set that from-image offers, though. IMO we should go ahead and offer it. It seems only logical to reproduce the feature-set of the EXIF orientations. It wouldn't save anybody any code, anyway. Thanks for the review! Will go ahead and make the rest of the changes tomorrow.
Uploading more reftests. We now have reftests for generated content, for non-axis-aligned angles, and for explicit orientations. Still needed: * Test that background-image and border-image don't work. * Tests for SVG-as-image. (As described in comment 19.)
Attachment #782090 - Attachment is obsolete: true
Just a heads-up: I'm on PTO through the weekend (possibly for some of Mon & Tues) -- I'll be traveling to & from Michigan for a friend's wedding. Hoping to review this on the plane tomorrow, though.
Have fun at the wedding, Daniel! I've updated the patch slightly to better handle an unlikely failure case in NotifyNewCurrentRequest.
Attachment #784520 - Flags: review?(dholbert)
Attachment #782781 - Attachment is obsolete: true
Attachment #782781 - Flags: review?(dholbert)
This version has every needed reftest except for those related to SVG-as-image. I've decided to place those in a separate patch so that dholbert can review them, so this patch is now complete and ready for review.
Attachment #784550 - Flags: review?(dbaron)
Attachment #784195 - Attachment is obsolete: true
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #23) > Transferring this request to dholbert; sorry, should have done that sooner. This means I need to update the reviewer on the commit message (mentioning this mainly as a note to myself). I'll wait to do that until Daniel reviews the patch, though, and include that change with the other review changes.
Comment on attachment 784550 [details] [diff] [review] (Part 3) - Add image-orientation reftests for raster images. Review of attachment 784550 [details] [diff] [review]: ----------------------------------------------------------------- Switching this over to you, Daniel, per discussion with dbaron.
Attachment #784550 - Flags: review?(dbaron) → review?(dholbert)
All of the review comments above are addressed in this version of the patch except for the issue of whether we should accept 'flip' by itself. I think dbaron and I both don't feel strongly about this, so I'm going to ask on www-style (need to ask about list-style-image anyway) and we can make a final decision after we see if any other stakeholders have opinions on this.
Attachment #781495 - Attachment is obsolete: true
Blocks: 833511
Comment on attachment 784520 [details] [diff] [review] (Part 2) - Add support for image-orientation to nsImageFrame. So, the "nsImageFrame keeping a persistent handle to its imgIContainer" part of this patch is a bit of a change in code-responsibility. It seems reasonable to me, but I think it'd be good to have joe or an imagelib peer sign off on the idea, at least (maybe someone already has?), since there are surely (historical?) reasons that we go through the hoops we currently go through, everywhere in nsImageFrame where we need the imgIContainer. I'll proceed under the assumption that this has imagelib approval for now, anyway, and it looks like you're probably keeping mImage appropriately up-to-date. I also think it'd be worth splitting this patch into two atomic parts: - PART A: Refactor nsImageFrame to use "mImage". (i.e. the existing patch's nsImageFrame edits, minus the orientation stuff.) (This patch hopefully shouldn't have any functional effect.) - PART B: Add nsLayoutUtils::OrientImage(), and invoke it from nsImageFrame where appropriate. I'll review it all together for now - but I do think it'd be worth splitting up like that before you land, to facilitate regression-hunting and to improve changeset-understandability for future mozilla-central archeologists who are trying to grok what's changing here. >+ Angle angle; >+ >+ if (aOrientation.Is0Degrees()) angle = Angle::D0; >+ else if (aOrientation.Is90Degrees()) angle = Angle::D90; >+ else if (aOrientation.Is180Degrees()) angle = Angle::D180; >+ else if (aOrientation.Is270Degrees()) angle = Angle::D270; >+ else MOZ_ASSERT(false, "Invalid orientation"); As elegant as this block looks, it's not very debuggable/breakpointable, and it doesn't really match mozilla/local style. I'd marginally prefer the more mozilla-coding-style-conforming multi-line braced form -- but I do see you did something similar in the other patch here, and it looks like dbaron didn't object in that instance, so I won't object too strongly here, if you want to keep it. However: in the final "else" clause, you're definitely going to need to add curly-braces, because you need to add another line of code there. You need to either initialize |angle| there, or to do an early return. Otherwise, if we take that "else", we'll end up using |angle| without ever initializing it (which Clang will probably spam a compiler warning about, "-Wsometimes-initialized" or something like that). (Alternately, you could just give |angle| a default value up top - but that's probably hackier & a bit wasteful (double-setting it in all the normally-traversed cases)). >diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h >+ static already_AddRefed<imgIContainer> OrientImage(imgIContainer* aContainer, >+ const nsStyleImageOrientation& aOrientation); Add a newline before "OrientImage" so that these lines aren't so huge. (To match local style of e.g. AssertNoDuplicateContinuations, a bit further down, you'll want to align "OrientImage" right underneath "static" -- no need for bonus indentation.) >diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp > nsImageFrame::NotifyNewCurrentRequest(imgIRequest *aRequest, > nsresult aStatus) > { >+ nsCOMPtr<imgIContainer> image; >+ aRequest->GetImage(getter_AddRefs(image)); >+ NS_ASSERTION(mImage || NS_FAILED(aStatus), "Successful load with no container?"); >+ [...] >- if (NS_SUCCEEDED(aStatus)) { >+ if (NS_SUCCEEDED(aStatus) && mImage) { >+ mImage = nsLayoutUtils::OrientImage(image, StyleVisibility()->mImageOrientation); I'm not clear on why we need to add the |mImage| null-check here, when we're about to clear out its existing value anyway. Maybe add a comment to clarify that, if the check is needed? Or convert it to an assertion (if aStatus is success), if that makes sense? Also: move the |image| local-var (and the GetImage() call that populates it) down inside the "if"-guarded code, since that's the only place you use it, and also to match the old code's "check aStatus before you call GetImage" ordering. > nsImageFrame::EnsureIntrinsicSizeAndRatio(nsPresContext* aPresContext) > { >- // Jump through all the hoops to get the status of the request >- nsCOMPtr<imgIRequest> currentRequest; >- nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mContent); >- if (imageLoader) >- imageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, >- getter_AddRefs(currentRequest)); >- uint32_t status = 0; >- if (currentRequest) >- currentRequest->GetImageStatus(&status); >- >- // If we know the size, we can grab it and use it for an update >- if (status & imgIRequest::STATUS_SIZE_AVAILABLE) { >- nsCOMPtr<imgIContainer> imgCon; >- currentRequest->GetImage(getter_AddRefs(imgCon)); >- NS_ABORT_IF_FALSE(imgCon, "SIZE_AVAILABLE, but no imgContainer?"); >- UpdateIntrinsicSize(imgCon); >- UpdateIntrinsicRatio(imgCon); >+ if (mImage) { >+ UpdateIntrinsicSize(mImage); >+ UpdateIntrinsicRatio(mImage); Hmm, so this is removing a lot of status-checking. I tend to think we should preserve these checks, for the time being, in an #ifdef DEBUG block inside the "if (mImage)" clause. (with assertions replacing the old code's "if" checks). That'll help ensure we're not accidentally removing/violating any invariants that we had previously baked in. (e.g. it'll tell us if mImage gets set to something non-null by some odd code-path that doesn't satisfy the other checks that we have here.) Same goes for DisplayAltFeedback() and BuildDisplayList(), too - the patch removes a lot of status-checking there that could probably be converted into an assertion or two, to make sure we're honoring the existing assumptions. r=me with the above addressed.
Attachment #784520 - Flags: review?(dholbert) → review+
Comment on attachment 784550 [details] [diff] [review] (Part 3) - Add image-orientation reftests for raster images. >+# Tests for image-orientation used with 'from-image': >+fuzzy(2,5) == image-orientation-from-image.html?none image-orientation-ref.html?0 >+fuzzy(1,1) == image-orientation-from-image.html?0 image-orientation-ref.html?0 >+fuzzy(1,1) == image-orientation-from-image.html?90 image-orientation-ref.html?90 >+fuzzy(1,1) == image-orientation-from-image.html?180 image-orientation-ref.html?180 >+fuzzy(1,1) == image-orientation-from-image.html?270 image-orientation-ref.html?270 >+fuzzy(1,1) == image-orientation-from-image.html?0&flip image-orientation-ref.html?0&flip >+fuzzy(1,1) == image-orientation-from-image.html?90&flip image-orientation-ref.html?90&flip >+fuzzy(1,1) == image-orientation-from-image.html?180&flip image-orientation-ref.html?180&flip >+fuzzy(1,1) == image-orientation-from-image.html?270&flip image-orientation-ref.html?270&flip [etc] It's a bit unfortunate that all of these tests are fuzzy(1,1). Is that for all platforms, and is it always the same pixel that's fuzzy? It'd be worth adding a comment above this block of tests, regarding the fuzziness, or better filing a followup bug and noting it here.
Attachment #784550 - Flags: review?(dholbert) → review+
Blocks: 877520
(In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #33) > It's a bit unfortunate that all of these tests are fuzzy(1,1). Is that for > all platforms, and is it always the same pixel that's fuzzy? This is why I dislike doing reftests involving JPEG images. Despite being saved at the maximum quality I possibly could, the regions of solid color are not perfectly uniform. It is true for all platforms; it's not a rendering error. I'll add a comment.
Thanks for the review, Daniel! Great comments! Some responses to a few of them below: (In reply to Daniel Holbert [:dholbert] (lowered responsiveness through 8/6) from comment #32) > I also think it'd be worth splitting this patch into two atomic parts: Yes, that's a good idea! Will do. > As elegant as this block looks, it's not very debuggable/breakpointable, Remind me to rant about this at some point when we're together in person. =) > (Alternately, you could > just give |angle| a default value up top - but that's probably hackier & a > bit wasteful (double-setting it in all the normally-traversed cases)). This branch cannot possibly be taken - the branches of the |if| are exhaustive - but the compiler can't prove it. However, as a fan myself of establishing correctness statically through the type system, I suspect it might be better to replace the IsXXXDegrees functions with a single function returning an enum, and use a switch here. I originally wrote the code using the IsXXX functions rather than an enum in an attempt to solve a problem that no longer exists. Given that there's no need for this structure anymore, it'd be a quick change, so I'll make it. This way the compiler can prove that the switch cases are exhaustive. > I'm not clear on why we need to add the |mImage| null-check here, Good catch! Null-checking |mImage| doesn't make sense there, of course; it should be null-checking |image|. Same goes for the assertion above. This probably clarifies some of the other things that seemed to be wrong in that method. =) > Hmm, so this is removing a lot of status-checking. I'm pretty confident that the checks in EnsureIntrinsicSizeAndRatio do not add value. To the extent that those checks were useful, they duplicate checks that are happening inside e.g. UpdateIntrinsicSize and UpdateIntrinsicRatio. > Same goes for DisplayAltFeedback() and BuildDisplayList(), too - the patch > removes a lot of status-checking there that could probably be converted into > an assertion or two, to make sure we're honoring the existing assumptions. For these two I agree. I'll add some assertions there. I also slightly changed the behavior of DisplayAltFeedback intentionally, but I'm now having second thoughts about that, so I'll restore a check that the image is fully decoded (as opposed to partially decoded).
(In reply to Seth Fowler [:seth] from comment #35) > I also slightly > changed the behavior of DisplayAltFeedback intentionally, but I'm now having > second thoughts about that, so I'll restore a check that the image is fully > decoded (as opposed to partially decoded). No wait, now I remember why I changed this. The current behavior doesn't make sense because BuildDisplayList will display the image if the first frame is partially decoded, but if it wasn't available when BuildDisplayList was called so that we end up calling DisplayAltFeedback when painting instead, then DisplayAltFeedback discovers that the first frame is now partially decoded, it won't actually paint it unless the first frame is completely decoded. Whether the image gets displayed before the first frame is fully decoded is therefore determined by a race on the timing of the call to BuildDisplayList.
(What I should probably do is pull that change out into a separate bug which this one then depends on. I don't want to do it _after_ this bug since it increases the amount of code I have to uselessly write.)
Depends on: 901146
I pulled that change out into bug 901146.
Replace the boolean IsXXXAngle functions with a single getter that returns an Angle value.
Attachment #784697 - Attachment is obsolete: true
This is the portion of the old part 2 related to making nsImageFrame hold a reference to its image container.
This is the portion of the old part 2 that dealt with actually adding image-orientation support to nsImageFrame. Between these two new parts, the issues raised in the review comments should be resolved.
Attachment #784520 - Attachment is obsolete: true
(In reply to Seth Fowler [:seth] from comment #36) > No wait, now I remember why I changed this. The current behavior doesn't > make sense because BuildDisplayList will display the image if the first > frame is partially decoded, but if it wasn't available when BuildDisplayList > was called so that we end up calling DisplayAltFeedback when painting > instead, then DisplayAltFeedback discovers that the first frame is now > partially decoded, it won't actually paint it unless the first frame is > completely decoded. Whether the image gets displayed before the first frame > is fully decoded is therefore determined by a race on the timing of the call > to BuildDisplayList. I don't think display list are that durable; in general they're constructed right before they're painted. We keep them around to do invalidation for the next time. (At least, as long as there haven't been more changes than I thought with display-list-based invalidation.)
No longer depends on: 901146
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #42) > I don't think display list are that durable; in general they're constructed > right before they're painted. Regardless, it would not have made sense for the logic to be different in the two cases, in my view. However, Timothy pointed out quite rightly that that particular piece of code refers to the loading of the _icon_ which is displayed together with the alt text, and not to the image itself. I've removed that change from the patch; bug 901146 is now about updating the comment there to better reflect what's happening.
Updated part 2, removing the changes to DisplayAltFeedback. Timothy, could you look this over and let me know if you see any other issues? In general I've predicated some of the changes on the invariant that if mImage is non-null then we have a size, as enforced by OnStartContainer and NotifyNewCurrentRequest.
Attachment #785320 - Flags: review?(tnikkel)
Attachment #785290 - Attachment is obsolete: true
Updated commit message to reflect part 2 being split in two, and the new reviewer.
Attachment #784550 - Attachment is obsolete: true
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #42) > I don't think display list are that durable; in general they're constructed > right before they're painted. Correct, but even though we don't keep them around that long I think it is possible for image decoding to make progress during painting. For example StartDecoding is called during painting in some cases and it does some decoding, RequestDecode will "accept" decoding progress that has been made off main thread, and we also sync decode images during painting in some cases.
Comment on attachment 785320 [details] [diff] [review] (Part 2) - Make nsImageFrame hold a reference to its image container. > nsresult > nsImageFrame::OnStartContainer(imgIRequest *aRequest, imgIContainer *aImage) > { >- if (!aImage) return NS_ERROR_INVALID_ARG; >+ if (!aImage) { >+ mImage = nullptr; >+ return NS_ERROR_INVALID_ARG; >+ } >+ >+ mImage = aImage; Doesn't OnStartContainer get called with aImage different from our current request? In which case we need more conditions on assigning aImage to mImage.
(In reply to Timothy Nikkel (:tn) from comment #47) > Doesn't OnStartContainer get called with aImage different from our current > request? In which case we need more conditions on assigning aImage to mImage. Yes, this is a good point. It seems that if we get notifications for the current request, this is always the first request the nsImageFrame deals with (not 100% correct because if we change requests before we ever got OnStartContainer then we just treat the new request as current immediately, according to nsImageLoadingContent::PrepareNextRequest), while pending requests are subsequent loads. This design seems intended to ensure that we keep displaying the old image during the process of loading a new one, so we don't revert to a period of just displaying nothing. In the case where we're getting notifications for a pending request, it seems that OnStopRequest will call NotifyNewCurrentRequest, which already unconditionally updates mImage. I think, based on the inferred purpose of this design, that we do not want to update mImage in OnStartContainer unless we get past the IsPendingLoad check, which ensures that the notification came from the current request. I'll update a new version of the patch with this change made.
This version of the patch doesn't update mImage in OnStartContainer until we've established that the notification is for the current request, per comment #48.
Attachment #786470 - Flags: review?(tnikkel)
Attachment #785320 - Attachment is obsolete: true
Attachment #785320 - Flags: review?(tnikkel)
Attachment #786470 - Flags: review?(tnikkel) → review+
This is a rebase of part 3 against the updated part 2.
Attachment #785291 - Attachment is obsolete: true
Depends on: 902291
Rebase against current m-c.
Attachment #785289 - Attachment is obsolete: true
Addressed review comments and added one additional test that ensures that image-orientation doesn't apply to list-style-image, after discussion with fantasai to verify that that's correct.
Attachment #785321 - Attachment is obsolete: true
The long-awaited final part of this bug! =) This part adds reftests for SVG images.
Attachment #787918 - Flags: review?(dholbert)
Fixed a bug in the reftests for SVG images with no inherent size. I originally thought that the reftests showed a bug in the image-orientation code, which is why I uploaded them anyway, but after investigation it quickly became clear that the reftests themselves were at fault. Other than that bugfix this is the same patch as before.
Attachment #787930 - Flags: review?(dholbert)
Attachment #787918 - Attachment is obsolete: true
Attachment #787918 - Flags: review?(dholbert)
Comment on attachment 787930 [details] [diff] [review] (Part 5) - Add image-orientation reftests for SVG images. >+# Tests for image-orientation with a viewbox and an intrinsic size: >+== image-orientation-viewbox-and-size.html?0 image-orientation-ref.html?0 >+== image-orientation-viewbox-and-size.html?90 image-orientation-ref.html?90 >+== image-orientation-viewbox-and-size.html?180 image-orientation-ref.html?180 [etc] So this makes me think: I can totally envision someone making a mistake when patching this test & reference down the line, and accidentally breaking them such that they don't honor their URL parameters. This would make them continue to pass, but no longer test any interesting orientations. It'd probably worth adding a sanity-check chunk to reftest.list, so that we'll catch that if it happens. We can just test the reference, to be sure its various reference renderings you're relying on are actually different. Something like this should do it: # SANITY-CHECKS # Make sure the various rotation all look different from each other: != image-orientation-ref.html?0 image-orientation-ref.html?90 != image-orientation-ref.html?0 image-orientation-ref.html?180 != image-orientation-ref.html?0 image-orientation-ref.html?270 != image-orientation-ref.html?90 image-orientation-ref.html?180 != image-orientation-ref.html?90 image-orientation-ref.html?270 != image-orientation-ref.html?180 image-orientation-ref.html?270 # Make sure "flip" actually changes the rendering: != image-orientation-ref.html?0 image-orientation-ref.html?0&flip != image-orientation-ref.html?90 image-orientation-ref.html?90&flip != image-orientation-ref.html?180 image-orientation-ref.html?180&flip != image-orientation-ref.html?270 image-orientation-ref.html?270&flip
Comment on attachment 787930 [details] [diff] [review] (Part 5) - Add image-orientation reftests for SVG images. r=me with comment 55
Attachment #787930 - Flags: review?(dholbert) → review+
Thanks for the review, Daniel! I think that's a great suggestion. Here's an update of part 4 with sanity checks added.
Attachment #787911 - Attachment is obsolete: true
And here's part 5 with the sanity checks.
Attachment #787930 - Attachment is obsolete: true
This updated version of part 1: * Adds a pref to make it possible to disable image-orientation. Right now it defaults to enabled. * Adds tests for image-orientation to property_database.js.
Attachment #787265 - Attachment is obsolete: true
Try job for the entire patch stack here: https://tbpl.mozilla.org/?tree=Try&rev=6ebbf2516a0a
Reuploading due to reports of patch failures against m-c.
Attachment #786470 - Attachment is obsolete: true
Fix errors in property_database.js entry.
Attachment #789807 - Attachment is obsolete: true
Rebased to reflect that Flip::None got renamed to Flip::Unflipped because X.h #define's None. (Sigh.)
Attachment #786678 - Attachment is obsolete: true
Hopefully the build issues are dealt with. (They seem to be on my local box, at least.) Some silly test failures are also dealt with. Let's run this through try again. https://tbpl.mozilla.org/?tree=Try&rev=8cbb381209dc
I fixed a Windows linker issue in bug 869723 and added some debug output to the nsImageFrame assertion that's causing some oranges in the last try run. Time for another go. https://tbpl.mozilla.org/?tree=Try&rev=156c9c31f92e
This version of part 3 handles some corner cases that were causing oranges. At this point the patches in this bug look solid on try: https://tbpl.mozilla.org/?tree=Try&rev=92710b02abc2
Attachment #793289 - Attachment is obsolete: true
Blocks: 915909
Blocks: 916104
Release note could be something like "Firefox now supports CSS3's image orientation property."
Given that I'd like to stick to the "there is no such thing as CSS3" message, how about "CSS3's" -> "the CSS"?
And we didn't implement the version as in CSS Image Level 3 but the one in the draft of CSS Image Content Level 4 :-) [ http://dev.w3.org/csswg/css-images/ ] (EXIF support)
Depends on: 932102
Is this 26+? Testing in 26.0 (image-orientation: from-image) and not working. Pulled nightly build and it works just fine.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: