Bug 92330 - [CSS] Pass an image orientation data to drawImage()
Summary: [CSS] Pass an image orientation data to drawImage()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on: 119418
Blocks: 91566
  Show dependency treegraph
 
Reported: 2012-07-25 21:11 PDT by David Barr
Modified: 2015-02-27 10:12 PST (History)
34 users (show)

See Also:


Attachments
Patch (40.67 KB, patch)
2012-07-25 21:36 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (42.75 KB, patch)
2012-07-25 21:53 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch for EWS (40.41 KB, patch)
2013-04-10 02:23 PDT, Gyuyoung Kim
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch for EWS (40.72 KB, patch)
2013-04-11 00:15 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (41.23 KB, patch)
2013-04-24 22:42 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (604.04 KB, application/zip)
2013-04-25 01:29 PDT, Build Bot
no flags Details
Patch (41.26 KB, patch)
2013-07-13 05:27 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Updated patch based on Bug 118454 (42.10 KB, patch)
2013-07-18 01:38 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (13.93 KB, patch)
2013-08-16 02:52 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (13.95 KB, patch)
2013-08-16 08:19 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2013-08-19 21:32 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (485.49 KB, application/zip)
2013-08-20 01:28 PDT, Build Bot
no flags Details
Patch (14.08 KB, patch)
2013-08-20 01:35 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Barr 2012-07-25 21:11:47 PDT
In support of ongoing css3-images image-orientation implementation, extend the platform graphics interfaces to accept an explicit orientation.
Comment 1 David Barr 2012-07-25 21:36:17 PDT
Created attachment 154534 [details]
Patch
Comment 2 Early Warning System Bot 2012-07-25 21:47:58 PDT
Comment on attachment 154534 [details]
Patch

Attachment 154534 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13361323
Comment 3 Early Warning System Bot 2012-07-25 21:51:38 PDT
Comment on attachment 154534 [details]
Patch

Attachment 154534 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13344531
Comment 4 Gyuyoung Kim 2012-07-25 21:52:37 PDT
Comment on attachment 154534 [details]
Patch

Attachment 154534 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13353492
Comment 5 David Barr 2012-07-25 21:53:48 PDT
Created attachment 154536 [details]
Patch
Comment 6 Tony Chang 2012-07-26 14:21:18 PDT
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.
Comment 7 Simon Fraser (smfr) 2012-07-26 14:24:39 PDT
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).
Comment 8 Tony Chang 2012-09-14 13:56:03 PDT
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?
Comment 9 Gyuyoung Kim 2013-04-10 01:17:25 PDT
If David doesn't continue to work on this bug anymore, I'd like to take this bug over. David, please give your comment.
Comment 10 David Barr 2013-04-10 01:25:13 PDT
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.
Comment 11 Gyuyoung Kim 2013-04-10 01:27:31 PDT
(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.
Comment 12 Gyuyoung Kim 2013-04-10 02:23:02 PDT
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. :)
Comment 13 Gyuyoung Kim 2013-04-10 02:27:59 PDT
(In reply to comment #12)

don't mind -> you mind.
Comment 14 Build Bot 2013-04-10 02:54:50 PDT
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
Comment 15 Build Bot 2013-04-10 03:15:04 PDT
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
Comment 16 Build Bot 2013-04-10 03:19:22 PDT
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
Comment 17 Gyuyoung Kim 2013-04-11 00:15:18 PDT
Created attachment 197514 [details]
Patch for EWS
Comment 18 Gyuyoung Kim 2013-04-11 04:23:45 PDT
Comment on attachment 154536 [details]
Patch

Clearing r? because of out of date.
Comment 19 Gyuyoung Kim 2013-04-11 16:57:10 PDT
(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.
Comment 20 Gyuyoung Kim 2013-04-24 22:42:51 PDT
Created attachment 199619 [details]
Patch
Comment 21 Gyuyoung Kim 2013-04-24 22:43:28 PDT
Rebased.
Comment 22 Build Bot 2013-04-25 01:29:16 PDT
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
Comment 23 Build Bot 2013-04-25 01:29:21 PDT
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
Comment 24 Gyuyoung Kim 2013-07-13 05:27:14 PDT
Created attachment 206602 [details]
Patch
Comment 25 Gyuyoung Kim 2013-07-13 07:40:53 PDT
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
Comment 26 Gyuyoung Kim 2013-07-18 01:38:07 PDT
Created attachment 206964 [details]
Updated patch based on Bug 118454
Comment 27 Gyuyoung Kim 2013-07-18 01:39:18 PDT
I'd like to work detail work on Bug 91566. So, this patch should be landed first. Please review.
Comment 28 Gyuyoung Kim 2013-07-27 23:37:15 PDT
CC'ing Beth, would you please review this patch ?
Comment 29 Beth Dakin 2013-07-29 10:25:11 PDT
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?
Comment 30 Beth Dakin 2013-07-29 10:27:15 PDT
(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.
Comment 31 Gyuyoung Kim 2013-07-31 19:28:11 PDT
(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.
Comment 32 Gyuyoung Kim 2013-07-31 19:31:36 PDT
(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.
Comment 33 Gyuyoung Kim 2013-08-01 17:43:14 PDT
Beth, I think we can handle it on new bug. So, I file a bug 119418. I will submit a patch to there soon.
Comment 34 Gyuyoung Kim 2013-08-16 02:52:40 PDT
Created attachment 208899 [details]
Patch
Comment 35 Gyuyoung Kim 2013-08-16 02:57:31 PDT
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.
Comment 36 kov's GTK+ EWS bot 2013-08-16 03:26:04 PDT
Comment on attachment 208899 [details]
Patch

Attachment 208899 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1471439
Comment 37 Gyuyoung Kim 2013-08-16 08:19:09 PDT
Created attachment 208926 [details]
Patch
Comment 38 Beth Dakin 2013-08-19 11:55:18 PDT
This patch looks good. Quick question before I r+ though, is there any was to test this?
Comment 39 Tim Horton 2013-08-19 11:59:08 PDT
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?
Comment 40 Gyuyoung Kim 2013-08-19 20:50:17 PDT
(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.
Comment 41 Gyuyoung Kim 2013-08-19 21:32:15 PDT
Created attachment 209156 [details]
Patch
Comment 42 Gyuyoung Kim 2013-08-19 21:35:58 PDT
(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.
Comment 43 Build Bot 2013-08-20 01:28:36 PDT
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
Comment 44 Build Bot 2013-08-20 01:28:41 PDT
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
Comment 45 Gyuyoung Kim 2013-08-20 01:35:04 PDT
Created attachment 209167 [details]
Patch
Comment 46 Gyuyoung Kim 2013-08-20 01:39:39 PDT
(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.
Comment 47 WebKit Commit Bot 2013-08-20 16:50:03 PDT
Comment on attachment 209167 [details]
Patch

Clearing flags on attachment: 209167

Committed r154375: <http://trac.webkit.org/changeset/154375>
Comment 48 WebKit Commit Bot 2013-08-20 16:50:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 49 Ryosuke Niwa 2013-08-21 13:33:58 PDT
Comment on attachment 209167 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2013-08-19  David Barr <davidbarr@chromium.org>, Gyuyoung Kim  <gyuyoung.kim@samsung.com>

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

Instead, mention the original author in the long description.
Comment 50 Gyuyoung Kim 2013-08-21 18:26:30 PDT
(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
> > +2013-08-19  David Barr <davidbarr@chromium.org>, Gyuyoung Kim  <gyuyoung.kim@samsung.com>
> 
> 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.
Comment 51 Antonio Gomes 2013-08-22 08:28:56 PDT
(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
> > > +2013-08-19  David Barr <davidbarr@chromium.org>, Gyuyoung Kim  <gyuyoung.kim@samsung.com>
> > 
> > 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 ..
Comment 52 Tim Horton 2013-11-05 14:46:36 PST
This caused https://bugs.webkit.org/show_bug.cgi?id=123831