RESOLVED FIXED Bug 91566
Integrate css3-images image-orientation with existing EXIF support
https://bugs.webkit.org/show_bug.cgi?id=91566
Summary Integrate css3-images image-orientation with existing EXIF support
David Barr
Reported 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.
Attachments
Draft patch (23.64 KB, patch)
2012-07-22 20:58 PDT, David Barr
no flags
Draft patch (41.54 KB, patch)
2012-07-24 23:49 PDT, David Barr
no flags
Draft patch (17.11 KB, patch)
2012-07-25 22:47 PDT, David Barr
no flags
Draft patch (17.38 KB, patch)
2012-07-25 23:13 PDT, David Barr
no flags
Updated patch (42.29 KB, patch)
2013-07-19 03:56 PDT, Gyuyoung Kim
no flags
Patch for ews (35.15 KB, patch)
2013-08-21 22:17 PDT, Gyuyoung Kim
no flags
Patch for ews (36.85 KB, patch)
2013-08-21 23:47 PDT, Gyuyoung Kim
no flags
Patch for ews (39.48 KB, patch)
2013-08-22 06:27 PDT, Gyuyoung Kim
no flags
Patch for ews (37.56 KB, patch)
2013-08-26 01:04 PDT, Gyuyoung Kim
no flags
Patch (37.55 KB, patch)
2013-08-26 01:56 PDT, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (270.06 KB, application/zip)
2013-08-26 03:05 PDT, Build Bot
no flags
Patch for mac port regression (35.45 KB, patch)
2013-08-28 18:34 PDT, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (822.79 KB, application/zip)
2013-08-28 20:03 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (754.13 KB, application/zip)
2013-08-28 20:30 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (800.33 KB, application/zip)
2013-08-28 21:00 PDT, Build Bot
no flags
Patch (36.38 KB, patch)
2013-08-29 06:57 PDT, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (682.56 KB, application/zip)
2013-08-29 08:41 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (1.69 MB, application/zip)
2013-08-29 15:33 PDT, Build Bot
no flags
Patch (37.20 KB, patch)
2013-08-31 21:26 PDT, Gyuyoung Kim
no flags
Patch (37.29 KB, patch)
2013-09-01 17:12 PDT, Gyuyoung Kim
no flags
Patch (37.26 KB, patch)
2013-09-01 23:27 PDT, Gyuyoung Kim
no flags
Rebased patch (37.32 KB, patch)
2013-09-12 19:10 PDT, Gyuyoung Kim
no flags
Patch (37.32 KB, patch)
2013-09-13 02:58 PDT, Gyuyoung Kim
no flags
Patch (37.32 KB, patch)
2013-09-13 03:32 PDT, Gyuyoung Kim
no flags
Patch according to Beth's comments (37.48 KB, patch)
2013-09-19 03:37 PDT, Gyuyoung Kim
no flags
Patch according to Beth's comments (38.00 KB, patch)
2013-09-19 08:42 PDT, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (534.64 KB, application/zip)
2013-09-19 09:45 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (529.88 KB, application/zip)
2013-09-19 12:31 PDT, Build Bot
no flags
Patch (44.64 KB, patch)
2013-09-20 22:21 PDT, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (863.52 KB, application/zip)
2013-09-21 00:18 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (884.02 KB, application/zip)
2013-09-21 01:47 PDT, Build Bot
no flags
Patch (42.72 KB, patch)
2013-10-06 06:16 PDT, Gyuyoung Kim
no flags
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (928.35 KB, application/zip)
2013-10-06 15:46 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (885.87 KB, application/zip)
2013-10-06 16:27 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (893.79 KB, application/zip)
2013-10-06 17:32 PDT, Build Bot
no flags
Patch (42.63 KB, patch)
2013-10-16 04:54 PDT, Gyuyoung Kim
no flags
Patch (44.84 KB, patch)
2013-10-16 17:17 PDT, Gyuyoung Kim
no flags
Patch (47.90 KB, patch)
2013-10-16 18:37 PDT, Gyuyoung Kim
no flags
Simon Fraser (smfr)
Comment 1 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.
David Barr
Comment 2 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.
Mike Lawther
Comment 3 2012-07-17 20:08:35 PDT
I concur. If the page author has specified a rotation, that would win.
Tab Atkins Jr.
Comment 4 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.
Tab Atkins Jr.
Comment 5 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.
David Barr
Comment 6 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.
David Barr
Comment 7 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.
David Barr
Comment 8 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.
Early Warning System Bot
Comment 9 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
Gyuyoung Kim
Comment 10 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
Early Warning System Bot
Comment 11 2012-07-25 00:24:01 PDT
David Barr
Comment 12 2012-07-25 22:47:36 PDT
Created attachment 154546 [details] Draft patch Rebased against bug 92330.
David Barr
Comment 13 2012-07-25 23:13:01 PDT
Created attachment 154550 [details] Draft patch Missed a single instance of a variable rename.
Jeremy Apthorp
Comment 14 2012-11-03 20:02:11 PDT
David is no longer working on Chromium -- Mike, can you reassign?
Gyuyoung Kim
Comment 15 2013-07-19 03:56:07 PDT
Created attachment 207077 [details] Updated patch
Gyuyoung Kim
Comment 16 2013-07-19 03:56:57 PDT
Assigned to me.
Early Warning System Bot
Comment 17 2013-07-19 04:01:14 PDT
Early Warning System Bot
Comment 18 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
EFL EWS Bot
Comment 19 2013-07-19 04:03:41 PDT
kov's GTK+ EWS bot
Comment 20 2013-07-19 04:04:12 PDT
EFL EWS Bot
Comment 21 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
Build Bot
Comment 22 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
Build Bot
Comment 23 2013-07-19 04:33:44 PDT
Build Bot
Comment 24 2013-07-19 05:08:50 PDT
Gyuyoung Kim
Comment 25 2013-08-21 22:17:55 PDT
Created attachment 209324 [details] Patch for ews
Early Warning System Bot
Comment 26 2013-08-21 22:25:32 PDT
Early Warning System Bot
Comment 27 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
Build Bot
Comment 28 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
Build Bot
Comment 29 2013-08-21 22:58:18 PDT
Gyuyoung Kim
Comment 30 2013-08-21 23:47:10 PDT
Created attachment 209328 [details] Patch for ews
Gyuyoung Kim
Comment 31 2013-08-22 06:27:19 PDT
Created attachment 209360 [details] Patch for ews
EFL EWS Bot
Comment 32 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
kov's GTK+ EWS bot
Comment 33 2013-08-22 06:51:20 PDT
Build Bot
Comment 34 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
Build Bot
Comment 35 2013-08-22 07:11:03 PDT
EFL EWS Bot
Comment 36 2013-08-22 07:13:59 PDT
Gyuyoung Kim
Comment 37 2013-08-26 01:04:44 PDT
Created attachment 209624 [details] Patch for ews
Gyuyoung Kim
Comment 38 2013-08-26 01:56:06 PDT
Build Bot
Comment 39 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
Build Bot
Comment 40 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
Gyuyoung Kim
Comment 41 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.
Gyuyoung Kim
Comment 42 2013-08-28 18:34:57 PDT
Created attachment 209941 [details] Patch for mac port regression
kov's GTK+ EWS bot
Comment 43 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
Build Bot
Comment 44 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
Build Bot
Comment 45 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
Build Bot
Comment 46 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
Build Bot
Comment 47 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
Build Bot
Comment 48 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
Build Bot
Comment 49 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
Gyuyoung Kim
Comment 50 2013-08-29 06:57:01 PDT
Build Bot
Comment 51 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
Build Bot
Comment 52 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
Build Bot
Comment 53 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
Build Bot
Comment 54 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
Gyuyoung Kim
Comment 55 2013-08-31 21:26:58 PDT
Early Warning System Bot
Comment 56 2013-08-31 21:35:49 PDT
Early Warning System Bot
Comment 57 2013-08-31 21:37:38 PDT
EFL EWS Bot
Comment 58 2013-08-31 22:08:54 PDT
EFL EWS Bot
Comment 59 2013-08-31 22:11:33 PDT
kov's GTK+ EWS bot
Comment 60 2013-08-31 23:21:27 PDT
kov's GTK+ EWS bot
Comment 61 2013-08-31 23:28:51 PDT
Gyuyoung Kim
Comment 62 2013-09-01 17:12:46 PDT
Gyuyoung Kim
Comment 63 2013-09-01 23:27:29 PDT
Gyuyoung Kim
Comment 64 2013-09-02 03:06:04 PDT
CC'ing Beth.
Gyuyoung Kim
Comment 65 2013-09-12 19:10:14 PDT
Created attachment 211496 [details] Rebased patch
Sergio Villar Senin
Comment 66 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
Gyuyoung Kim
Comment 67 2013-09-13 02:58:39 PDT
Gyuyoung Kim
Comment 68 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.
kov's GTK+ EWS bot
Comment 69 2013-09-13 03:12:06 PDT
Gyuyoung Kim
Comment 70 2013-09-13 03:32:03 PDT
Gyuyoung Kim
Comment 71 2013-09-15 21:19:40 PDT
Beth or others, could anyone review this patch ? I really want to land this patch.
Beth Dakin
Comment 72 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.
Gyuyoung Kim
Comment 73 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.
Gyuyoung Kim
Comment 74 2013-09-19 03:37:14 PDT
Created attachment 212048 [details] Patch according to Beth's comments
Gyuyoung Kim
Comment 75 2013-09-19 08:42:08 PDT
Created attachment 212070 [details] Patch according to Beth's comments
Build Bot
Comment 76 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
Build Bot
Comment 77 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
Build Bot
Comment 78 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
Build Bot
Comment 79 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
Gyuyoung Kim
Comment 80 2013-09-20 22:21:34 PDT
kov's GTK+ EWS bot
Comment 81 2013-09-20 23:50:03 PDT
Build Bot
Comment 82 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
Build Bot
Comment 83 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
Build Bot
Comment 84 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
Build Bot
Comment 85 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
Gyuyoung Kim
Comment 86 2013-10-06 06:16:29 PDT
Build Bot
Comment 87 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
Build Bot
Comment 88 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
Build Bot
Comment 89 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
Build Bot
Comment 90 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
Build Bot
Comment 91 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
Build Bot
Comment 92 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
Build Bot
Comment 93 2013-10-06 17:37:48 PDT
Gyuyoung Kim
Comment 94 2013-10-16 04:54:24 PDT
Build Bot
Comment 95 2013-10-16 06:30:32 PDT
Gyuyoung Kim
Comment 96 2013-10-16 17:17:06 PDT
Build Bot
Comment 97 2013-10-16 18:03:10 PDT
Gyuyoung Kim
Comment 98 2013-10-16 18:37:39 PDT
Gyuyoung Kim
Comment 99 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()); }
WebKit Commit Bot
Comment 100 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>
WebKit Commit Bot
Comment 101 2013-10-23 20:38:39 PDT
All reviewed patches have been landed. Closing bug.
Gyuyoung Kim
Comment 102 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.
Note You need to log in before you can comment on or make changes to this bug.