Bug 91566

Summary: Integrate css3-images image-orientation with existing EXIF support
Product: WebKit Reporter: David Barr <davidbarr>
Component: CSSAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, buildbot, commit-queue, d-r, eflews.bot, eric.carlson, eric, esprehn+autocc, fmalita, glenn, gns, gtk-ews, gyuyoung.kim, gyuyoung.kim, jackalmage, japhet, jeremya, jer.noble, kondapallykalyan, menard, mifenton, mikelawther, mrobinson, noam, noel.gordon, pdr, philn, rakuco, rego+ews, rniwa, schenney, simon.fraser, tabatkins, thakis, thorton, tony, webkit.review.bot, xan.lopez
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.w3.org/TR/2012/CR-css3-images-20120417/#the-image-orientation
Bug Depends on: 89624, 91961, 92330    
Bug Blocks: 89052, 93422    
Attachments:
Description Flags
Draft patch
none
Draft patch
none
Draft patch
none
Draft patch
none
Updated patch
none
Patch for ews
none
Patch for ews
none
Patch for ews
none
Patch for ews
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch for mac port regression
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Rebased patch
none
Patch
none
Patch
none
Patch according to Beth's comments
none
Patch according to Beth's comments
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Patch
none
Patch
none
Patch none

Description David Barr 2012-07-17 17:40:12 PDT
Add image-orientation support to WebKit.
http://www.w3.org/TR/2012/CR-css3-images-20120417/#the-image-orientation

The css3-images module is at candidate recommendation.
I propose to introduce the property initially behind a compile flag.

There is existing code that allows a setting to determine whether embedded image orientations are respected.
I propose that to integrate this new feature with the existing one, that if the setting is enabled and an embedded orientation is present that the embedded orientation take precedence over any out-of-band orientation.
Comment 1 Simon Fraser (smfr) 2012-07-17 17:52:21 PDT
> I propose that to integrate this new feature with the existing one, that if the setting is enabled and an embedded orientation is present that the embedded orientation take precedence over any out-of-band orientation.

It's not clear to me that this is the correct behavior. css3-images dodges the issue of EXIF, but it seems that the spec implies that image-orientation would win.
Comment 2 David Barr 2012-07-17 18:43:55 PDT
From the spec:
Note that some devices will "tag" an image with some metadata indicating its correct orientation, so image viewing software can do the necessary transformation themselves. Due to legacy compatibility restraints, Web browsers are required to ignore this data by default. A future level of this specification is expected to have a value that applies the metadata-specified transformation automatically.

From the somewhat-relate image-resolution section:
from-image: The image's intrinsic resolution is taken as that specified by the image format. If the image does not specify its own resolution, the explicitly specified resolution is used (if given), else it defaults to '1ddpx'.

My interpretation:
The existing setting is considered non-conformant if enabled by default. How embedded image orientation and out-of-band orientation ought to be resolved is unspecified and left for a future level of the spec.

However, if the future level follows the same pattern set forth for image-resolution, it would read:
from-image: The image's intrinsic orientation is taken as that specified by the image format. If the image does not specify its own orientation, the explicitly specified orientation is used (if given), else it defaults to '0deg'.

With this projection, I would consider the existing setting to be equivalent to:
* { image-orientation: from-image; }

In that case, the following ought to ignore embedded orientation:
img { image-orientation: 90deg; }

So I'm inclined to agree with Simon.
Comment 3 Mike Lawther 2012-07-17 20:08:35 PDT
I concur. If the page author has specified a rotation, that would win.
Comment 4 Tab Atkins Jr. 2012-07-17 23:55:27 PDT
I also agree.  The setting that makes images respect EXIF can be recast as a user-agent-level rule that applies "image-orientation: from-image" to all image elements.  The author's explicit declarations will then automatically win.
Comment 5 Tab Atkins Jr. 2012-07-17 23:56:52 PDT
Oh, and just to put your fear to rest, David, Images 4 (which I'll write up Any Day Now) will indeed have the "image-orientation: from-image" value, and it will work exactly like image-resolution, as you expect.
Comment 6 David Barr 2012-07-22 20:58:00 PDT
Created attachment 153729 [details]
Draft patch

Draft patch outlining scope of changes. Still navigating build and logical errors.
Comment 7 David Barr 2012-07-22 22:07:54 PDT
Comment on attachment 153729 [details]
Draft patch

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

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:215
> +    if (styleOrientation != OriginTopLeft && shouldRespectOrientation == RespectImageOrientation)

Oops, the condition should be: (styleOrientation == OriginTopLeft && shouldRespectOrientation == RespectImageOrientation).
The basis for this condition is that we cannot distinguish between explicit 'image-orientation: 0deg' and the initial value.
Comment 8 David Barr 2012-07-24 23:49:45 PDT
Created attachment 154275 [details]
Draft patch

Largely complete, need to make sure it applies cleany to all ports and possibly split into smaller patches.
Comment 9 Early Warning System Bot 2012-07-25 00:16:40 PDT
Comment on attachment 154275 [details]
Draft patch

Attachment 154275 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13353123
Comment 10 Gyuyoung Kim 2012-07-25 00:17:49 PDT
Comment on attachment 154275 [details]
Draft patch

Attachment 154275 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13339161
Comment 11 Early Warning System Bot 2012-07-25 00:24:01 PDT
Comment on attachment 154275 [details]
Draft patch

Attachment 154275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13347141
Comment 12 David Barr 2012-07-25 22:47:36 PDT
Created attachment 154546 [details]
Draft patch

Rebased against bug 92330.
Comment 13 David Barr 2012-07-25 23:13:01 PDT
Created attachment 154550 [details]
Draft patch

Missed a single instance of a variable rename.
Comment 14 Jeremy Apthorp 2012-11-03 20:02:11 PDT
David is no longer working on Chromium -- Mike, can you reassign?
Comment 15 Gyuyoung Kim 2013-07-19 03:56:07 PDT
Created attachment 207077 [details]
Updated patch
Comment 16 Gyuyoung Kim 2013-07-19 03:56:57 PDT
Assigned to me.
Comment 17 Early Warning System Bot 2013-07-19 04:01:14 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1105789
Comment 18 Early Warning System Bot 2013-07-19 04:02:52 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1098900
Comment 19 EFL EWS Bot 2013-07-19 04:03:41 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1099814
Comment 20 kov's GTK+ EWS bot 2013-07-19 04:04:12 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1132097
Comment 21 EFL EWS Bot 2013-07-19 04:11:10 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1106832
Comment 22 Build Bot 2013-07-19 04:15:59 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1103803
Comment 23 Build Bot 2013-07-19 04:33:44 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1095774
Comment 24 Build Bot 2013-07-19 05:08:50 PDT
Comment on attachment 207077 [details]
Updated patch

Attachment 207077 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1131178
Comment 25 Gyuyoung Kim 2013-08-21 22:17:55 PDT
Created attachment 209324 [details]
Patch for ews
Comment 26 Early Warning System Bot 2013-08-21 22:25:32 PDT
Comment on attachment 209324 [details]
Patch for ews

Attachment 209324 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1523613
Comment 27 Early Warning System Bot 2013-08-21 22:26:26 PDT
Comment on attachment 209324 [details]
Patch for ews

Attachment 209324 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1532048
Comment 28 Build Bot 2013-08-21 22:28:26 PDT
Comment on attachment 209324 [details]
Patch for ews

Attachment 209324 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1494354
Comment 29 Build Bot 2013-08-21 22:58:18 PDT
Comment on attachment 209324 [details]
Patch for ews

Attachment 209324 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1524053
Comment 30 Gyuyoung Kim 2013-08-21 23:47:10 PDT
Created attachment 209328 [details]
Patch for ews
Comment 31 Gyuyoung Kim 2013-08-22 06:27:19 PDT
Created attachment 209360 [details]
Patch for ews
Comment 32 EFL EWS Bot 2013-08-22 06:44:49 PDT
Comment on attachment 209360 [details]
Patch for ews

Attachment 209360 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1538090
Comment 33 kov's GTK+ EWS bot 2013-08-22 06:51:20 PDT
Comment on attachment 209360 [details]
Patch for ews

Attachment 209360 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1380553
Comment 34 Build Bot 2013-08-22 07:00:41 PDT
Comment on attachment 209360 [details]
Patch for ews

Attachment 209360 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1488096
Comment 35 Build Bot 2013-08-22 07:11:03 PDT
Comment on attachment 209360 [details]
Patch for ews

Attachment 209360 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1536098
Comment 36 EFL EWS Bot 2013-08-22 07:13:59 PDT
Comment on attachment 209360 [details]
Patch for ews

Attachment 209360 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1393148
Comment 37 Gyuyoung Kim 2013-08-26 01:04:44 PDT
Created attachment 209624 [details]
Patch for ews
Comment 38 Gyuyoung Kim 2013-08-26 01:56:06 PDT
Created attachment 209628 [details]
Patch
Comment 39 Build Bot 2013-08-26 03:05:29 PDT
Comment on attachment 209628 [details]
Patch

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

New failing tests:
fast/canvas/webgl/premultiplyalpha-test.html
fast/canvas/pattern-with-transform.html
fast/canvas/canvas-pattern-modify.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgb565.html
fast/canvas/webgl/tex-sub-image-2d-bad-args.html
canvas/philip/tests/initial.reset.pattern.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgba4444.html
canvas/philip/tests/2d.pattern.modify.canvas2.html
fast/canvas/webgl/texture-active-bind.html
fast/canvas/webgl/tex-image-webgl.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgba5551.html
fast/canvas/webgl/texture-npot.html
fast/canvas/webgl/canvas-2d-webgl-texture.html
canvas/philip/tests/2d.pattern.paint.orientation.canvas.html
fast/canvas/canvas-pattern-behaviour.html
canvas/philip/tests/2d.pattern.modify.canvas1.html
canvas/philip/tests/2d.pattern.basic.canvas.html
http/tests/inspector/inspect-element.html
Comment 40 Build Bot 2013-08-26 03:05:37 PDT
Created attachment 209633 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 41 Gyuyoung Kim 2013-08-26 03:53:41 PDT
Comment on attachment 209628 [details]
Patch

Need to fix some tests as well as exif-orientation tests on mac port.
Comment 42 Gyuyoung Kim 2013-08-28 18:34:57 PDT
Created attachment 209941 [details]
Patch for mac port regression
Comment 43 kov's GTK+ EWS bot 2013-08-28 19:11:52 PDT
Comment on attachment 209941 [details]
Patch for mac port regression

Attachment 209941 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1624444
Comment 44 Build Bot 2013-08-28 20:03:38 PDT
Comment on attachment 209941 [details]
Patch for mac port regression

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

New failing tests:
fast/images/exif-orientation-image-document.html
fast/images/exif-orientation.html
fast/images/exif-orientation-composited.html
Comment 45 Build Bot 2013-08-28 20:03:47 PDT
Created attachment 209948 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 46 Build Bot 2013-08-28 20:30:10 PDT
Comment on attachment 209941 [details]
Patch for mac port regression

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

New failing tests:
fast/images/exif-orientation-image-document.html
fast/images/exif-orientation.html
fast/images/exif-orientation-composited.html
Comment 47 Build Bot 2013-08-28 20:30:18 PDT
Created attachment 209949 [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 48 Build Bot 2013-08-28 20:59:56 PDT
Comment on attachment 209941 [details]
Patch for mac port regression

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

New failing tests:
fast/images/exif-orientation-image-document.html
fast/images/exif-orientation.html
fast/images/exif-orientation-composited.html
Comment 49 Build Bot 2013-08-28 21:00:04 PDT
Created attachment 209950 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 50 Gyuyoung Kim 2013-08-29 06:57:01 PDT
Created attachment 209972 [details]
Patch
Comment 51 Build Bot 2013-08-29 08:40:53 PDT
Comment on attachment 209972 [details]
Patch

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

New failing tests:
http/tests/security/canvas-cors-with-two-hosts.html
fast/canvas/canvas-blending-image-over-gradient.html
http/tests/security/canvas-remote-read-data-url-image.html
fast/canvas/DrawImageSinglePixelStretch.html
fast/images/paint-subrect-grid.html
fast/canvas/canvas-blending-gradient-over-image.html
http/tests/inspector/inspect-element.html
fast/canvas/canvas-blending-image-over-image.html
canvas/philip/tests/toDataURL.png.complexcolours.html
fast/images/paint-subrect.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba5551.html
canvas/philip/tests/toDataURL.jpeg.quality.basic.html
fast/canvas/canvas-blending-color-over-image.html
fast/dom/image-object.html
http/tests/canvas/webgl/origin-clean-conformance.html
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
http/tests/security/canvas-remote-read-remote-image-allowed-with-credentials.html
fast/canvas/canvas-blending-pattern-over-image.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html
canvas/philip/tests/toDataURL.png.primarycolours.html
fast/canvas/canvas-blending-image-over-pattern.html
canvas/philip/tests/security.dataURI.html
fast/images/cmyk-jpeg-with-color-profile.html
fast/canvas/canvas-incremental-repaint.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html
fast/canvas/canvas-scale-drawImage-shadow.html
fast/canvas/canvas-blending-image-over-color.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
fast/canvas/canvas-drawImage-shadow.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba4444.html
Comment 52 Build Bot 2013-08-29 08:41:02 PDT
Created attachment 209985 [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.4
Comment 53 Build Bot 2013-08-29 15:33:27 PDT
Comment on attachment 209972 [details]
Patch

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

New failing tests:
http/tests/security/canvas-cors-with-two-hosts.html
fast/canvas/canvas-blending-image-over-gradient.html
http/tests/security/canvas-remote-read-data-url-image.html
fast/canvas/DrawImageSinglePixelStretch.html
fast/images/paint-subrect-grid.html
fast/canvas/canvas-blending-gradient-over-image.html
http/tests/inspector/inspect-element.html
fast/canvas/canvas-blending-image-over-image.html
canvas/philip/tests/toDataURL.png.complexcolours.html
fast/images/paint-subrect.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba5551.html
canvas/philip/tests/toDataURL.jpeg.quality.basic.html
fast/canvas/canvas-blending-color-over-image.html
fast/dom/image-object.html
http/tests/canvas/webgl/origin-clean-conformance.html
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
http/tests/security/canvas-remote-read-remote-image-allowed-with-credentials.html
fast/canvas/canvas-blending-pattern-over-image.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html
fast/canvas/canvas-blending-image-over-pattern.html
platform/mac/fast/canvas/canvas-draw-xbm-image.html
canvas/philip/tests/security.dataURI.html
fast/images/cmyk-jpeg-with-color-profile.html
platform/mac/editing/deleting/deletionUI-single-instance.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html
fast/canvas/canvas-blending-image-over-color.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
canvas/philip/tests/toDataURL.png.primarycolours.html
fast/replaced/width-and-height-of-positioned-replaced-elements.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba4444.html
Comment 54 Build Bot 2013-08-29 15:33:36 PDT
Created attachment 210038 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 55 Gyuyoung Kim 2013-08-31 21:26:58 PDT
Created attachment 210224 [details]
Patch
Comment 56 Early Warning System Bot 2013-08-31 21:35:49 PDT
Comment on attachment 210224 [details]
Patch

Attachment 210224 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1657496
Comment 57 Early Warning System Bot 2013-08-31 21:37:38 PDT
Comment on attachment 210224 [details]
Patch

Attachment 210224 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1658539
Comment 58 EFL EWS Bot 2013-08-31 22:08:54 PDT
Comment on attachment 210224 [details]
Patch

Attachment 210224 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1661540
Comment 59 EFL EWS Bot 2013-08-31 22:11:33 PDT
Comment on attachment 210224 [details]
Patch

Attachment 210224 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1680059
Comment 60 kov's GTK+ EWS bot 2013-08-31 23:21:27 PDT
Comment on attachment 210224 [details]
Patch

Attachment 210224 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1603511
Comment 61 kov's GTK+ EWS bot 2013-08-31 23:28:51 PDT
Comment on attachment 210224 [details]
Patch

Attachment 210224 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1659500
Comment 62 Gyuyoung Kim 2013-09-01 17:12:46 PDT
Created attachment 210257 [details]
Patch
Comment 63 Gyuyoung Kim 2013-09-01 23:27:29 PDT
Created attachment 210262 [details]
Patch
Comment 64 Gyuyoung Kim 2013-09-02 03:06:04 PDT
CC'ing Beth.
Comment 65 Gyuyoung Kim 2013-09-12 19:10:14 PDT
Created attachment 211496 [details]
Rebased patch
Comment 66 Sergio Villar Senin 2013-09-13 02:48:38 PDT
Comment on attachment 211496 [details]
Rebased patch

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

LGTM

> Source/WebCore/loader/cache/CachedImage.cpp:260
> +    ImageOrientationDescription orientationDescirption(renderer->shouldRespectImageOrientation());

nit orientationDescription
Comment 67 Gyuyoung Kim 2013-09-13 02:58:39 PDT
Created attachment 211527 [details]
Patch
Comment 68 Gyuyoung Kim 2013-09-13 02:59:29 PDT
(In reply to comment #66)
> (From update of attachment 211496 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211496&action=review
> 
> LGTM
> 
> > Source/WebCore/loader/cache/CachedImage.cpp:260
> > +    ImageOrientationDescription orientationDescirption(renderer->shouldRespectImageOrientation());
> 
> nit orientationDescription

Thank you for your review. Fixed it.
Comment 69 kov's GTK+ EWS bot 2013-09-13 03:12:06 PDT
Comment on attachment 211527 [details]
Patch

Attachment 211527 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1884153
Comment 70 Gyuyoung Kim 2013-09-13 03:32:03 PDT
Created attachment 211530 [details]
Patch
Comment 71 Gyuyoung Kim 2013-09-15 21:19:40 PDT
Beth or others, could anyone review this patch ? I really want to land this patch.
Comment 72 Beth Dakin 2013-09-17 21:38:06 PDT
Comment on attachment 211530 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.cpp:261
> +    if (renderer && renderer->style())

It doesn't make any sense to null-check renderer here. If it is ever null, then you will crash on the previous line since you access it without a null check there. Figure out if it can ever be null, and if so, null-check it before you use it at all. If it cannot be null, then don't null check it.

> Source/WebCore/page/DragController.cpp:889
> +    if (element->renderer() && element->renderer()->style())

Same comment about null-checking applies here.

> Source/WebCore/page/Frame.cpp:1040
> +    if (renderer->style()) 

Is it necessary to null-check style?

> Source/WebCore/platform/graphics/Image.h:-206
> -    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator, BlendMode) = 0;

Why is this function being removed?

> Source/WebCore/rendering/RenderImage.cpp:150
> +    if (diff == StyleDifferenceLayout

There's no reason to break this if-condition onto two lines.

> LayoutTests/fast/css/image-orientation/image-orientation.html:8
> +description('Apply image-orientation property and check computed style.');

You should add an explanation to the LayoutTest change log to explain your changes to this test. Was this description always inaccurate and you are just correcting it? Please explain.
Comment 73 Gyuyoung Kim 2013-09-19 03:27:56 PDT
Comment on attachment 211530 [details]
Patch

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

Beth, Thank you for your review :)

>> Source/WebCore/loader/cache/CachedImage.cpp:261
>> +    if (renderer && renderer->style())
> 
> It doesn't make any sense to null-check renderer here. If it is ever null, then you will crash on the previous line since you access it without a null check there. Figure out if it can ever be null, and if so, null-check it before you use it at all. If it cannot be null, then don't null check it.

It looks we need to check if *renderer* is null. However, we don't need to check if style() is null. Renderer should have a style.

>> Source/WebCore/platform/graphics/Image.h:-206
>> -    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator, BlendMode) = 0;
> 
> Why is this function being removed?

I thought that we don't need to keep this function anymore, because all child classes derived from Image class should implement this draw() which doesn't have *ImageOrientationDescription* argument additionally. But, I think now I don't have a clear reason to remove this function. So, I keep this function in next patch.

>> Source/WebCore/rendering/RenderImage.cpp:150
>> +    if (diff == StyleDifferenceLayout
> 
> There's no reason to break this if-condition onto two lines.

Done.

>> LayoutTests/fast/css/image-orientation/image-orientation.html:8
>> +description('Apply image-orientation property and check computed style.');
> 
> You should add an explanation to the LayoutTest change log to explain your changes to this test. Was this description always inaccurate and you are just correcting it? Please explain.

This line has wrong description for this test. This test is for testing image orientation, not image resolution. Fixed it.

> LayoutTests/fast/css/image-orientation/image-orientation.html:22
> +        shouldBe('p.style.cssText', '"image-orientation: ' + test + ';"');

I think this test doesn't need to add a space at the end of line.
Comment 74 Gyuyoung Kim 2013-09-19 03:37:14 PDT
Created attachment 212048 [details]
Patch according to Beth's comments
Comment 75 Gyuyoung Kim 2013-09-19 08:42:08 PDT
Created attachment 212070 [details]
Patch according to Beth's comments
Comment 76 Build Bot 2013-09-19 09:45:43 PDT
Comment on attachment 212070 [details]
Patch according to Beth's comments

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

New failing tests:
http/tests/security/canvas-cors-with-two-hosts.html
fast/canvas/canvas-blending-image-over-pattern.html
fast/canvas/canvas-blending-image-over-gradient.html
http/tests/security/canvas-remote-read-data-url-image.html
webgl/conformance/extensions/oes-texture-float-with-image.html
fast/canvas/drawImage-with-invalid-args.html
fast/canvas/canvas-blending-gradient-over-image.html
fast/canvas/canvas-blending-image-over-image.html
canvas/philip/tests/toDataURL.png.complexcolours.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba5551.html
canvas/philip/tests/toDataURL.jpeg.quality.basic.html
fast/canvas/canvas-blending-color-over-image.html
fast/dom/image-object.html
http/tests/canvas/webgl/origin-clean-conformance.html
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
http/tests/security/canvas-remote-read-remote-image-allowed-with-credentials.html
fast/canvas/canvas-blending-pattern-over-image.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html
canvas/philip/tests/toDataURL.png.primarycolours.html
svg/as-image/svg-canvas-xhtml-tainted.html
canvas/philip/tests/security.dataURI.html
svg/as-image/svg-canvas-link-not-colored.html
fast/images/cmyk-jpeg-with-color-profile.html
fast/canvas/canvas-incremental-repaint.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html
fast/canvas/canvas-scale-drawImage-shadow.html
fast/canvas/canvas-blending-image-over-color.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
fast/canvas/canvas-drawImage-shadow.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba4444.html
Comment 77 Build Bot 2013-09-19 09:45:53 PDT
Created attachment 212077 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 78 Build Bot 2013-09-19 12:31:27 PDT
Comment on attachment 212070 [details]
Patch according to Beth's comments

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

New failing tests:
http/tests/security/canvas-cors-with-two-hosts.html
webgl/conformance/canvas/to-data-url-test.html
fast/canvas/canvas-blending-image-over-pattern.html
fast/canvas/canvas-blending-image-over-gradient.html
http/tests/security/canvas-remote-read-data-url-image.html
webgl/conformance/extensions/oes-texture-float-with-image.html
fast/canvas/drawImage-with-invalid-args.html
fast/canvas/canvas-blending-gradient-over-image.html
fast/canvas/canvas-blending-image-over-image.html
canvas/philip/tests/toDataURL.png.complexcolours.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba5551.html
canvas/philip/tests/toDataURL.jpeg.quality.basic.html
fast/canvas/canvas-blending-color-over-image.html
fast/dom/image-object.html
http/tests/canvas/webgl/origin-clean-conformance.html
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
fast/canvas/canvas-blending-pattern-over-image.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html
canvas/philip/tests/toDataURL.png.primarycolours.html
svg/as-image/svg-canvas-xhtml-tainted.html
canvas/philip/tests/security.dataURI.html
svg/as-image/svg-canvas-link-not-colored.html
fast/images/cmyk-jpeg-with-color-profile.html
fast/canvas/canvas-incremental-repaint.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html
fast/canvas/canvas-scale-drawImage-shadow.html
fast/canvas/canvas-blending-image-over-color.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
fast/canvas/canvas-drawImage-shadow.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba4444.html
Comment 79 Build Bot 2013-09-19 12:31:36 PDT
Created attachment 212089 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 80 Gyuyoung Kim 2013-09-20 22:21:34 PDT
Created attachment 212254 [details]
Patch
Comment 81 kov's GTK+ EWS bot 2013-09-20 23:50:03 PDT
Comment on attachment 212254 [details]
Patch

Attachment 212254 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1883503
Comment 82 Build Bot 2013-09-21 00:18:03 PDT
Comment on attachment 212254 [details]
Patch

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

New failing tests:
http/tests/local/drag-over-remote-content.html
editing/pasteboard/4947130.html
platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html
editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
http/tests/security/drag-over-remote-content-iframe.html
editing/pasteboard/drag-and-drop-image-contenteditable.html
editing/pasteboard/drag-selected-image-to-contenteditable.html
editing/pasteboard/files-during-page-drags.html
editing/selection/drag-to-contenteditable-iframe.html
editing/pasteboard/drag-image-in-about-blank-frame.html
fast/events/standalone-image-drag-to-editable.html
http/tests/security/drag-drop-same-unique-origin.html
http/tests/misc/bubble-drag-events.html
Comment 83 Build Bot 2013-09-21 00:18:12 PDT
Created attachment 212259 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 84 Build Bot 2013-09-21 01:47:06 PDT
Comment on attachment 212254 [details]
Patch

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

New failing tests:
http/tests/local/drag-over-remote-content.html
editing/pasteboard/4947130.html
platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html
editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
http/tests/security/drag-over-remote-content-iframe.html
editing/pasteboard/drag-and-drop-image-contenteditable.html
editing/pasteboard/drag-selected-image-to-contenteditable.html
editing/pasteboard/files-during-page-drags.html
editing/selection/drag-to-contenteditable-iframe.html
editing/pasteboard/drag-image-in-about-blank-frame.html
fast/events/standalone-image-drag-to-editable.html
http/tests/security/drag-drop-same-unique-origin.html
http/tests/misc/bubble-drag-events.html
Comment 85 Build Bot 2013-09-21 01:47:14 PDT
Created attachment 212267 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 86 Gyuyoung Kim 2013-10-06 06:16:29 PDT
Created attachment 213510 [details]
Patch
Comment 87 Build Bot 2013-10-06 15:46:21 PDT
Comment on attachment 213510 [details]
Patch

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

New failing tests:
http/tests/local/drag-over-remote-content.html
editing/pasteboard/4947130.html
platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html
editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
http/tests/security/drag-over-remote-content-iframe.html
editing/pasteboard/drag-and-drop-image-contenteditable.html
editing/pasteboard/drag-selected-image-to-contenteditable.html
editing/pasteboard/files-during-page-drags.html
editing/selection/drag-to-contenteditable-iframe.html
editing/pasteboard/drag-image-in-about-blank-frame.html
fast/events/standalone-image-drag-to-editable.html
http/tests/security/drag-drop-same-unique-origin.html
http/tests/misc/bubble-drag-events.html
Comment 88 Build Bot 2013-10-06 15:46:29 PDT
Created attachment 213532 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 89 Build Bot 2013-10-06 16:27:28 PDT
Comment on attachment 213510 [details]
Patch

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

New failing tests:
http/tests/local/drag-over-remote-content.html
editing/pasteboard/4947130.html
platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html
editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
http/tests/security/drag-over-remote-content-iframe.html
editing/pasteboard/drag-and-drop-image-contenteditable.html
editing/pasteboard/drag-selected-image-to-contenteditable.html
editing/pasteboard/files-during-page-drags.html
editing/selection/drag-to-contenteditable-iframe.html
editing/pasteboard/drag-image-in-about-blank-frame.html
fast/events/standalone-image-drag-to-editable.html
http/tests/security/drag-drop-same-unique-origin.html
http/tests/misc/bubble-drag-events.html
Comment 90 Build Bot 2013-10-06 16:27:35 PDT
Created attachment 213537 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 91 Build Bot 2013-10-06 17:32:28 PDT
Comment on attachment 213510 [details]
Patch

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

New failing tests:
http/tests/local/drag-over-remote-content.html
editing/pasteboard/4947130.html
platform/mac/editing/pasteboard/drag-selections-to-contenteditable.html
editing/pasteboard/drag-image-to-contenteditable-in-iframe.html
http/tests/security/drag-over-remote-content-iframe.html
editing/pasteboard/drag-and-drop-image-contenteditable.html
editing/pasteboard/drag-selected-image-to-contenteditable.html
editing/pasteboard/files-during-page-drags.html
editing/selection/drag-to-contenteditable-iframe.html
editing/pasteboard/drag-image-in-about-blank-frame.html
fast/events/standalone-image-drag-to-editable.html
http/tests/security/drag-drop-same-unique-origin.html
http/tests/misc/bubble-drag-events.html
Comment 92 Build Bot 2013-10-06 17:32:35 PDT
Created attachment 213543 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 93 Build Bot 2013-10-06 17:37:48 PDT
Comment on attachment 213510 [details]
Patch

Attachment 213510 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3493122
Comment 94 Gyuyoung Kim 2013-10-16 04:54:24 PDT
Created attachment 214355 [details]
Patch
Comment 95 Build Bot 2013-10-16 06:30:32 PDT
Comment on attachment 214355 [details]
Patch

Attachment 214355 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/4111259
Comment 96 Gyuyoung Kim 2013-10-16 17:17:06 PDT
Created attachment 214408 [details]
Patch
Comment 97 Build Bot 2013-10-16 18:03:10 PDT
Comment on attachment 214408 [details]
Patch

Attachment 214408 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3494625
Comment 98 Gyuyoung Kim 2013-10-16 18:37:39 PDT
Created attachment 214413 [details]
Patch
Comment 99 Gyuyoung Kim 2013-10-20 18:19:26 PDT
(In reply to comment #72)

> > Source/WebCore/platform/graphics/Image.h:-206
> > -    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator, BlendMode) = 0;
> 
> Why is this function being removed?


Hi Beth, first of all, sorry for too late update. I fixed other stuffs according to your comments except for this draw() removal. In my humble opinion, this draw() has just called other draw() with an imageOrientationDescription argument. So, I don't think we don't need to keep it anymore.

For instance, please look at below function.

void BitmapImage::draw(GraphicsContext* ctx, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator op, BlendMode blendMode)
{
    draw(ctx, dstRect, srcRect, styleColorSpace, op, blendMode, ImageOrientationDescription());
}
Comment 100 WebKit Commit Bot 2013-10-23 20:38:29 PDT
Comment on attachment 214413 [details]
Patch

Clearing flags on attachment: 214413

Committed r157909: <http://trac.webkit.org/changeset/157909>
Comment 101 WebKit Commit Bot 2013-10-23 20:38:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 102 Gyuyoung Kim 2013-10-23 22:38:03 PDT
30 regressions occur after landing this patch on GTK port only. Let me fix it. Please do not revert this patch.