Bug 92330 – [CSS] Pass an image orientation data to drawImage()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[David Barr]

In support of ongoing css3-images image-orientation implementation, extend the platform graphics interfaces to accept an explicit orientation.

[David Barr]

Created attachment 154534 [details]
Patch

[Early Warning System Bot]

Comment on attachment 154534 [details]
Patch

Attachment 154534 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13361323

[Early Warning System Bot]

Comment on attachment 154534 [details]
Patch

Attachment 154534 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13344531

[Gyuyoung Kim]

Comment on attachment 154534 [details]
Patch

Attachment 154534 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13353492

[David Barr]

Created attachment 154536 [details]
Patch

[Tony Chang]

Comment on attachment 154536 [details]
Patch

Simon would be a good reviewer for this patch.

It feels like we should somehow merge ImageOrientationEnum and RespectImageOrientationEnum instead of passing them both around.  Maybe a struct or maybe we can add RespectImageOrientationEnum to the ImageOrientation class.

[Simon Fraser (smfr)]

Comment on attachment 154536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154536&action=review

> Source/WebCore/page/Frame.cpp:1109
> +    ImageOrientationEnum orientation = OriginTopLeft;

OriginTopLeft is so ambiguous here given the various things that can affect rotation (encoded orientation, EXIF, context transform etc).

[Tony Chang]

Comment on attachment 154536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154536&action=review

>> Source/WebCore/page/Frame.cpp:1109
>> +    ImageOrientationEnum orientation = OriginTopLeft;
> 
> OriginTopLeft is so ambiguous here given the various things that can affect rotation (encoded orientation, EXIF, context transform etc).

Would it be better to use DefaultImageOrientation here?  I'm not sure if that's any better.  Would it help to change the parameter to be named something like orientationFromCSS?

[Gyuyoung Kim]

If David doesn't continue to work on this bug anymore, I'd like to take this bug over. David, please give your comment.

[David Barr]

I am limited to working on this in my leisure time,
as much as I'd like to see this complete.

I welcome anyone who has time to push it forward.

[Gyuyoung Kim]

(In reply to comment #10)
> I am limited to working on this in my leisure time,
> as much as I'd like to see this complete.
> 
> I welcome anyone who has time to push it forward.

Thanks, I will submit a rebased patch with previous reviewer's comments. Please take a look it when you are available.

[Gyuyoung Kim]

Created attachment 197226 [details]
Patch for EWS

David, if you don't mind to add my name as second, please let me know. I will remove myself then. :)

[Gyuyoung Kim]

(In reply to comment #12)

don't mind -> you mind.

[Build Bot]

Comment on attachment 197226 [details]
Patch for EWS

Attachment 197226 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17693012

[Build Bot]

Comment on attachment 197226 [details]
Patch for EWS

Attachment 197226 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17744005

[Build Bot]

Comment on attachment 197226 [details]
Patch for EWS

Attachment 197226 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17723049

[Gyuyoung Kim]

Created attachment 197514 [details]
Patch for EWS

[Gyuyoung Kim]

Comment on attachment 154536 [details]
Patch

Clearing r? because of out of date.

[Gyuyoung Kim]

(In reply to comment #7)
> (From update of attachment 154536 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154536&action=review
> 
> > Source/WebCore/page/Frame.cpp:1109
> > +    ImageOrientationEnum orientation = OriginTopLeft;
> 
> OriginTopLeft is so ambiguous here given the various things that can affect rotation (encoded orientation, EXIF, context transform etc).

Simon, could you take a look latest patch again ? I change OriginTopLeft with DefaultImageOrientation.

[Gyuyoung Kim]

Created attachment 199619 [details]
Patch

[Gyuyoung Kim]

Rebased.

[Build Bot]

Comment on attachment 199619 [details]
Patch

Attachment 199619 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/113579

New failing tests:
editing/pasteboard/4641033.html

[Build Bot]

Created attachment 199633 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Gyuyoung Kim]

Created attachment 206602 [details]
Patch

[Gyuyoung Kim]

I succeed to pass image-orientation layout test using a patch on Bug 118454. But, the patch only can pass on efl and gtk ports yet. I think I'm able to support mac and qt port soon. To enable css-image-orientation, this patch needs to be landed first. 

Simon, could you take a look last patch ? 

Below is to test if css-image-orientation can work for efl and gtk.
https://bugs.webkit.org/attachment.cgi?id=206593&action=review

[Gyuyoung Kim]

Created attachment 206964 [details]
Updated patch based on Bug 118454

[Gyuyoung Kim]

I'd like to work detail work on Bug 91566. So, this patch should be landed first. Please review.

[Gyuyoung Kim]

CC'ing Beth, would you please review this patch ?

[Beth Dakin]

Comment on attachment 206964 [details]
Updated patch based on Bug 118454

View in context: https://bugs.webkit.org/attachment.cgi?id=206964&action=review

> Source/WebCore/ChangeLog:12
> +        No new tests; interface change only - no change in behavior.

If there is no behavior change, what is the point of this patch? Please edit the ChangeLog to explain the point of this patch. Is it laying groundwork for a behavior change?

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 154536 [details])
> Simon would be a good reviewer for this patch.
> 
> It feels like we should somehow merge ImageOrientationEnum and RespectImageOrientationEnum instead of passing them both around.  Maybe a struct or maybe we can add RespectImageOrientationEnum to the ImageOrientation class.

I agree with this comment as well.

[Gyuyoung Kim]

(In reply to comment #30)
> (In reply to comment #6)
> > (From update of attachment 154536 [details] [details])
> > Simon would be a good reviewer for this patch.
> > 
> > It feels like we should somehow merge ImageOrientationEnum and RespectImageOrientationEnum instead of passing them both around.  Maybe a struct or maybe we can add RespectImageOrientationEnum to the ImageOrientation class.
> 
> I agree with this comment as well.

Ok, I will update patch according to this comment. Thank you for your review.

[Gyuyoung Kim]

(In reply to comment #29)
> (From update of attachment 206964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206964&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests; interface change only - no change in behavior.
> 
> If there is no behavior change, what is the point of this patch? Please edit the ChangeLog to explain the point of this patch. Is it laying groundwork for a behavior change?

Yes, this patch is one of groundwork in order to let draw layer know image orientation value. I will update ChangeLog in next patch. Thanks.

[Gyuyoung Kim]

Beth, I think we can handle it on new bug. So, I file a bug 119418. I will submit a patch to there soon.

[Gyuyoung Kim]

Created attachment 208899 [details]
Patch

[Gyuyoung Kim]

Bug 119418 covered the patch of original this bug as well. So, role of this bug needs to be changed. Latest patch is to pass information of image orientation to drawImage() function so that the function knows what is desired orientation.

[kov's GTK+ EWS bot]

Comment on attachment 208899 [details]
Patch

Attachment 208899 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1471439

[Gyuyoung Kim]

Created attachment 208926 [details]
Patch

[Beth Dakin]

This patch looks good. Quick question before I r+ though, is there any was to test this?

[Tim Horton]

Comment on attachment 208926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208926&action=review

> Source/WebCore/rendering/RenderImage.cpp:359
> +                orientationDescription.setImageOrientationEnum(style()->imageOrientation());

Why don't we orientationDescription.setRespectImageOrientation here like we do down below?

[Gyuyoung Kim]

(In reply to comment #38)
> This patch looks good. Quick question before I r+ though, is there any was to test this?

There is a test for image-orientation in below path.
LayoutTests/fast/css/image-orientation/image-orientation.html

Because this patch is one of steps to support it, this patch doesn't mention the test yet. Bug 91566 will supports the test after landing this patch.

[Gyuyoung Kim]

Created attachment 209156 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #39)
> (From update of attachment 208926 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208926&action=review
> 
> > Source/WebCore/rendering/RenderImage.cpp:359
> > +                orientationDescription.setImageOrientationEnum(style()->imageOrientation());
> 
> Why don't we orientationDescription.setRespectImageOrientation here like we do down below?

Ok, fixed it.

[Build Bot]

Comment on attachment 209156 [details]
Patch

Attachment 209156 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1503789

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 209165 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Gyuyoung Kim]

Created attachment 209167 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #43)
> (From update of attachment 209156 [details])
> Attachment 209156 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/1503789
> 
> New failing tests:
> fast/workers/termination-with-port-messages.html

It looks this failing is unrelated with this patch. I upload this patch again.

[WebKit Commit Bot]

Comment on attachment 209167 [details]
Patch

Clearing flags on attachment: 209167

Committed r154375: <http://trac.webkit.org/changeset/154375>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Comment on attachment 209167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209167&action=review

> Source/WebCore/ChangeLog:1
> [phone]  David Barr <[e-mail]>, Gyuyoung Kim  <[e-mail]>

Wrong format.  We never list two people on the first line like this.

Instead, mention the original author in the long description.

[Gyuyoung Kim]

(In reply to comment #49)
> (From update of attachment 209167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209167&action=review
> 
> > Source/WebCore/ChangeLog:1
> > [phone]  David Barr <[e-mail]>, Gyuyoung Kim  <[e-mail]>
> 
> Wrong format.  We never list two people on the first line like this.
> Instead, mention the original author in the long description.

IIRC, I saw similar line in ChangeLog before. Anyway, I will do that in next patch. Thank you for notifying it to me.

[Antonio Gomes]

(In reply to comment #50)
> (In reply to comment #49)
> > (From update of attachment 209167 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=209167&action=review
> > 
> > > Source/WebCore/ChangeLog:1
> > > [phone]  David Barr <[e-mail]>, Gyuyoung Kim  <[e-mail]>
> > 
> > Wrong format.  We never list two people on the first line like this.
> > Instead, mention the original author in the long description.
> 
> IIRC, I saw similar line in ChangeLog before. Anyway, I will do that in next patch. Thank you for notifying it to me.

I have seen and used it as well. Should be ok ..

[Tim Horton]

This caused https://bugs.webkit.org/show_bug.cgi?id=123831