Bug 100164

Created attachment 170248 [details] Patch

Attachment 170248 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/RenderObject.cpp:2225: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

+noel.gordon fyi

Comment on attachment 170248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170248&action=review > Source/WebCore/ChangeLog:14 > + default). However, the feature needs port-specific modifications to > + GraphicsContext (without this, webcore will use the rotated bounds but > + draw the image as if it hadn't be rotated, resulting in squished > + pixels), and most ports don't implement these yet -- so put > + turning this on for image documents behind a port-specific #ifdef. Were bugs filed about this? Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels. Perhaps I'm not understanding. > Source/WebCore/platform/graphics/ImageSource.cpp:110 > } Good. > Source/WebCore/platform/graphics/ImageSource.cpp:122 > + if (!m_decoder) > + return IntSize(); > + > + IntSize size = m_decoder->frameSizeAtIndex(index); > + if ((shouldRespectOrientation == RespectImageOrientation) && m_decoder->orientation().usesWidthAsHeight()) > + return IntSize(size.height(), size.width()); > > - return m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize(); > + return size; > } m_decoder->frameSizeAtIndex(index) could return an empty IntSize, right? Maybe write this function body as: IntSize size = m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize(); if (shouldRespectOrientation == RespectImageOrientation && orientationAtIndex(index).usesWidthAsHeight()) return IntSize(size.height(), size.width()); return size; > Source/WebCore/platform/graphics/ImageSource.cpp:184 > + return m_decoder ? m_decoder->orientation() : ImageOrientation(); return m_decoder ? m_decoder->orientation() : DefaultImageOrientation; > Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:156 > + return m_actualDecoder ? m_actualDecoder->orientation() : ImageOrientation(); return m_actualDecoder ? m_actualDecoder->orientation() : DefaultImageOrientation; You should prefix this statement with: // FIXME: Make this work with deferred image decoding. > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:182 > + // The JPEG decoder looks at EXIF metadata. > + // TODO: Possibly implement XMP and IPTC support. If you renamed the function to readExifOrientation, the first comment would be redundant :) The rename could happen in another patch. The second comment should be a FIXME, but I'm not sure this is a great place for it. Drop it, or file a bug about it. > Source/WebCore/rendering/RenderObject.cpp:2226 > +#endif Use an if statement to avoid the style nit @2225

Created attachment 170414 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170248&action=review >> Source/WebCore/ChangeLog:14 >> + turning this on for image documents behind a port-specific #ifdef. > > Were bugs filed about this? Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels. Perhaps I'm not understanding. Before this CL, orientation was never set on images, so the bug wasn't visible. This CL keeps it that way. If you want to see the squishing, patch in this CL to trunk webkit, remove the ifdef, and look at a exif-rotated jpeg. >> Source/WebCore/platform/graphics/ImageSource.cpp:122 >> } > > m_decoder->frameSizeAtIndex(index) could return an empty IntSize, right? Maybe write this function body as: > > IntSize size = m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize(); > > if (shouldRespectOrientation == RespectImageOrientation && orientationAtIndex(index).usesWidthAsHeight()) > return IntSize(size.height(), size.width()); > > return size; The current code is more similar to ImageSourceCG. Do you see a problem with the corrent code if frameSizeAtIndex() returns an empty size? >> Source/WebCore/platform/graphics/ImageSource.cpp:184 >> + return m_decoder ? m_decoder->orientation() : ImageOrientation(); > > return m_decoder ? m_decoder->orientation() : DefaultImageOrientation; Done. >> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:156 >> + return m_actualDecoder ? m_actualDecoder->orientation() : ImageOrientation(); > > return m_actualDecoder ? m_actualDecoder->orientation() : DefaultImageOrientation; > > You should prefix this statement with: > > // FIXME: Make this work with deferred image decoding. Done. Re FIXME: Does it not already work? Why not? >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:182 >> + // TODO: Possibly implement XMP and IPTC support. > > If you renamed the function to readExifOrientation, the first comment would be redundant :) The rename could happen in another patch. The second comment should be a FIXME, but I'm not sure this is a great place for it. Drop it, or file a bug about it. Changed to FIXME >> Source/WebCore/rendering/RenderObject.cpp:2226 >> +#endif > > Use an if statement to avoid the style nit @2225 I think the style nit isn't critical.

Attachment 170414 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/RenderObject.cpp:2225: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Looks fine to me, but Noel may have further comments. I think SYD is asleep right now.

Comment on attachment 170414 [details] Patch r=me. Please consult with Noel before landing.

Created attachment 170506 [details] Patch

Created attachment 170510 [details] Patch

Created attachment 170511 [details] Patch

(In reply to comment #6) > View in context: https://bugs.webkit.org/attachment.cgi?id=170248&action=review > > >> Source/WebCore/ChangeLog:14 > >> + turning this on for image documents behind a port-specific #ifdef. > > > > Were bugs filed about this? Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels. Perhaps I'm not understanding. > > Before this CL, orientation was never set on images, so the bug wasn't visible. This CL keeps it that way. If you want to see the squishing, patch in this CL to trunk webkit, remove the ifdef, and look at a exif-rotated jpeg. We need BitmapImage::draw modifications whichj we're fixing elsewhere. > >> Source/WebCore/platform/graphics/ImageSource.cpp:122 > >> } > > > > m_decoder->frameSizeAtIndex(index) could return an empty IntSize, right? Maybe write this function body as: > > > > IntSize size = m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize(); > > > > if (shouldRespectOrientation == RespectImageOrientation && orientationAtIndex(index).usesWidthAsHeight()) > > return IntSize(size.height(), size.width()); > > > > return size; > > The current code is more similar to ImageSourceCG. Do you see a problem with the current code if frameSizeAtIndex() returns an empty size? Nope. > > // FIXME: Make this work with deferred image decoding. > > Done. > Re FIXME: Does it not already work? Why not? Because m_actualDecoder becomes NULL when deferred decoding starts. > >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:182 > >> + // TODO: Possibly implement XMP and IPTC support. > > > > If you renamed the function to readExifOrientation, the first comment would be redundant :) The rename could happen in another patch. The second comment should be a FIXME, but I'm not sure this is a great place for it. Drop it, or file a bug about it. > > Changed to FIXME > > >> Source/WebCore/rendering/RenderObject.cpp:2226 > >> +#endif > > > > Use an if statement to avoid the style nit @2225 > > I think the style nit isn't critical. Not critical but it makes noise for me and others. I'm fine with this patch though.

Lots of mid-air collisions while trying to say the above :)

Comment on attachment 170511 [details] Patch Clearing flags on attachment: 170511 Committed r132433: <http://trac.webkit.org/changeset/132433>

All reviewed patches have been landed. Closing bug.

(In reply to comment #13) > (In reply to comment #6) > > View in context: https://bugs.webkit.org/attachment.cgi?id=170248&action=review > > > > >> Source/WebCore/ChangeLog:14 > > >> + turning this on for image documents behind a port-specific #ifdef. > > > > > > Were bugs filed about this? Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels. Perhaps I'm not understanding. > > > > Before this CL, orientation was never set on images, so the bug wasn't visible. This CL keeps it that way. If you want to see the squishing, patch in this CL to trunk webkit, remove the ifdef, and look at a exif-rotated jpeg. > > We need BitmapImage::draw modifications which we're fixing elsewhere. And to be precise, since ImageSource now returns orientation on all ports, those without the BitmapImage::draw modifications will squish. Hence the #ifdef. The #ifdef is missing an || PLATFORM(CHROMIUM) fyi, so imageDocument orientation is not yet working on chromium.