nsImageFrame only considers mImageOrientation in NotifyNewCurrentRequest and OnStartContainer. That means that dynamic changes in the property value are ignored until a new load happens. That seems pretty wrong.

Is this the right approach? (the reftest pass locally on Linux)

Comment on attachment 8409861 [details] [diff] [review] wip You shouldn't need the invalidate in Reflow; just change the hint for that property in nsStyleStruct to include repainting. Apart from that, the main problem here is that this is updating the intrinsic size when we get to Reflow(), which may be too late. We might already have computed an intrinsic size for our parent using our incorrect intrinsic size. So we should be doing similar things in GetPrefWidth()/GetMinWidth(), no? And perhaps in GetIntrinsicRatio() and ComputeSize().

Mats, if it's alright with you I'm going to jump in here and take this one.

This patch updates the appropriate size information if the value of image-orientation changes, using the DidSetStyleContext hook.

This is Boris's reftest from comment 1. I noticed it had never been reviewed, so let's do that now.

The new imgIContainer::Unwrap function added in Part 1 is needed to remove any existing OrientedImage wrapper. Otherwise we'd end up layer the new orientation on top of the previous one, so if we had an original rotation of 90 degrees and we set a new value of 180 degrees, we'd end up with a 270 degree rotation if we didn't use imgIContainer::Unwrap.

Comment on attachment 8442633 [details] [diff] [review] (Part 1) - Support dynamic changes to the image-orientation property Looks fine, though if you add nostdcall to the idl you won't need the NS_IMETHODIMP_ bits either.

(In reply to Boris Zbarsky [:bz] from comment #8) > Looks fine, though if you add nostdcall to the idl you won't need the > NS_IMETHODIMP_ bits either. Thanks for the quick review! That sounds like a good change; I'll make it in the final version of the patch.

Comment on attachment 8442633 [details] [diff] [review] (Part 1) - Support dynamic changes to the image-orientation property Review of attachment 8442633 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsImageFrame.cpp @@ +230,5 @@ > + > + bool shouldUpdateOrientation = > + !aContext || > + aContext->StyleVisibility()->mImageOrientation != newOrientation; > + Argh, just noticed this whitespace (and the other chunk below). This will be removed in the final version of the patch.

Comment on attachment 8442633 [details] [diff] [review] (Part 1) - Support dynamic changes to the image-orientation property > void >+nsImageFrame::DidSetStyleContext(nsStyleContext* aContext) Please call this aOldStyleContext rather than aContext. This is consistent with other implementations of this method, and makes the code make a lot more sense. >+{ >+ ImageFrameSuper::DidSetStyleContext(aContext); >+ >+ if (!mImage) { >+ // We'll pick this change up whenever we do get an image. >+ return; >+ } >+ >+ nsStyleImageOrientation newOrientation = StyleVisibility()->mImageOrientation; >+ >+ bool shouldUpdateOrientation = >+ !aContext || >+ aContext->StyleVisibility()->mImageOrientation != newOrientation; No need no null-check aContext. >+ >+ if (shouldUpdateOrientation) { >+ nsCOMPtr<imgIContainer> image(mImage->Unwrap()); >+ mImage = nsLayoutUtils::OrientImage(image, StyleVisibility()->mImageOrientation); Reuse newOrientation here, please. (Or, if you don't, don't bother with the newOrientation variable at all. But it's probably better to reuse it.) >+ virtual void DidSetStyleContext(nsStyleContext* aContext) MOZ_OVERRIDE; aOldStyleContext here too. r=dbaron with that

Thanks for the review, David! I'll make those changes. (Though as we discussed in person, we do need to null-check aContext. I'll add a comment about why.)

I've addressed the review comments in this version of the patch. If try is green then this is ready to go.

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

Try looks good, but unfortunately bug 997604 needs more work. The dependency between these two bugs is fairly artificial, so I could rebase these patches to avoid the issue, but I'll hold off a bit to see if bug 997604 gets ready soon.

I've rearranged the dependencies between this bugs so these patches can go ahead and land.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4efae1bab3 https://hg.mozilla.org/integration/mozilla-inbound/rev/13ac876b0439

https://hg.mozilla.org/mozilla-central/rev/6c4efae1bab3 https://hg.mozilla.org/mozilla-central/rev/13ac876b0439