RESOLVED FIXED 92330
[CSS] Pass an image orientation data to drawImage()
https://bugs.webkit.org/show_bug.cgi?id=92330
Summary [CSS] Pass an image orientation data to drawImage()
David Barr
Reported 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.
Attachments
Patch (40.67 KB, patch)
2012-07-25 21:36 PDT, David Barr
no flags
Patch (42.75 KB, patch)
2012-07-25 21:53 PDT, David Barr
no flags
Patch for EWS (40.41 KB, patch)
2013-04-10 02:23 PDT, Gyuyoung Kim
buildbot: commit-queue-
Patch for EWS (40.72 KB, patch)
2013-04-11 00:15 PDT, Gyuyoung Kim
no flags
Patch (41.23 KB, patch)
2013-04-24 22:42 PDT, Gyuyoung Kim
no flags
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
Patch (41.26 KB, patch)
2013-07-13 05:27 PDT, Gyuyoung Kim
no flags
Updated patch based on Bug 118454 (42.10 KB, patch)
2013-07-18 01:38 PDT, Gyuyoung Kim
no flags
Patch (13.93 KB, patch)
2013-08-16 02:52 PDT, Gyuyoung Kim
no flags
Patch (13.95 KB, patch)
2013-08-16 08:19 PDT, Gyuyoung Kim
no flags
Patch (14.08 KB, patch)
2013-08-19 21:32 PDT, Gyuyoung Kim
no flags
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
Patch (14.08 KB, patch)
2013-08-20 01:35 PDT, Gyuyoung Kim
no flags
David Barr
Comment 1 2012-07-25 21:36:17 PDT
Early Warning System Bot
Comment 2 2012-07-25 21:47:58 PDT
Early Warning System Bot
Comment 3 2012-07-25 21:51:38 PDT
Gyuyoung Kim
Comment 4 2012-07-25 21:52:37 PDT
David Barr
Comment 5 2012-07-25 21:53:48 PDT
Tony Chang
Comment 6 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.
Simon Fraser (smfr)
Comment 7 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).
Tony Chang
Comment 8 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?
Gyuyoung Kim
Comment 9 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.
David Barr
Comment 10 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.
Gyuyoung Kim
Comment 11 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.
Gyuyoung Kim
Comment 12 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. :)
Gyuyoung Kim
Comment 13 2013-04-10 02:27:59 PDT
(In reply to comment #12) don't mind -> you mind.
Build Bot
Comment 14 2013-04-10 02:54:50 PDT
Build Bot
Comment 15 2013-04-10 03:15:04 PDT
Build Bot
Comment 16 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
Gyuyoung Kim
Comment 17 2013-04-11 00:15:18 PDT
Created attachment 197514 [details] Patch for EWS
Gyuyoung Kim
Comment 18 2013-04-11 04:23:45 PDT
Comment on attachment 154536 [details] Patch Clearing r? because of out of date.
Gyuyoung Kim
Comment 19 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.
Gyuyoung Kim
Comment 20 2013-04-24 22:42:51 PDT
Gyuyoung Kim
Comment 21 2013-04-24 22:43:28 PDT
Rebased.
Build Bot
Comment 22 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
Build Bot
Comment 23 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
Gyuyoung Kim
Comment 24 2013-07-13 05:27:14 PDT
Gyuyoung Kim
Comment 25 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
Gyuyoung Kim
Comment 26 2013-07-18 01:38:07 PDT
Created attachment 206964 [details] Updated patch based on Bug 118454
Gyuyoung Kim
Comment 27 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.
Gyuyoung Kim
Comment 28 2013-07-27 23:37:15 PDT
CC'ing Beth, would you please review this patch ?
Beth Dakin
Comment 29 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?
Beth Dakin
Comment 30 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.
Gyuyoung Kim
Comment 31 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.
Gyuyoung Kim
Comment 32 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.
Gyuyoung Kim
Comment 33 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.
Gyuyoung Kim
Comment 34 2013-08-16 02:52:40 PDT
Gyuyoung Kim
Comment 35 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.
kov's GTK+ EWS bot
Comment 36 2013-08-16 03:26:04 PDT
Gyuyoung Kim
Comment 37 2013-08-16 08:19:09 PDT
Beth Dakin
Comment 38 2013-08-19 11:55:18 PDT
This patch looks good. Quick question before I r+ though, is there any was to test this?
Tim Horton
Comment 39 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?
Gyuyoung Kim
Comment 40 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.
Gyuyoung Kim
Comment 41 2013-08-19 21:32:15 PDT
Gyuyoung Kim
Comment 42 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.
Build Bot
Comment 43 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
Build Bot
Comment 44 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
Gyuyoung Kim
Comment 45 2013-08-20 01:35:04 PDT
Gyuyoung Kim
Comment 46 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.
WebKit Commit Bot
Comment 47 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>
WebKit Commit Bot
Comment 48 2013-08-20 16:50:11 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 49 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.
Gyuyoung Kim
Comment 50 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.
Antonio Gomes
Comment 51 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 ..
Tim Horton
Comment 52 2013-11-05 14:46:36 PST
Note You need to log in before you can comment on or make changes to this bug.