Bug 89624

Created attachment 148711 [details] Patch Pending compile-time flag landing, start implementing image-orientation.

Comment on attachment 148711 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148711&action=review Please email webkit-dev about this new feature. > Source/WebCore/rendering/style/StyleRareInheritedData.h:99 > + unsigned m_imageOrientation : 2; // Quarter turns Since there are only 4 possible values, I would just make this an enum. It'll be easier to understand the code rather than knowing that 1 == 90deg, 2 == 180deg and 3 == 270deg.

> Please email webkit-dev about this new feature. From bug 89052, advertised on webkit-dev: http://thread.gmane.org/gmane.os.opendarwin.webkit.devel/21130 > > Source/WebCore/rendering/style/StyleRareInheritedData.h:99 > > + unsigned m_imageOrientation : 2; // Quarter turns > > Since there are only 4 possible values, I would just make this an enum. It'll be easier to understand the code rather than knowing that 1 == 90deg, 2 == 180deg and 3 == 270deg. How about enum QuarterTurn { NoTurn = 0, TurnRight, TurnHalf, TurnLeft}; ?

There is an existing ImageOrientationEnum in Source/WebCore/platform/graphics/ImageOrientation.h. This contains the values in this specification plus the additional flipped variants in EXIF.

Created attachment 148960 [details] Patch Rewrote patch to use ImageOrientationEnum.

Created attachment 152705 [details] Patch Rebased against r122813: <http://trac.webkit.org/changeset/122813>. Updated ChangeLogs.

Comment on attachment 152705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152705&action=review > Source/WebCore/css/CSSPrimitiveValueMappings.h:4134 > + default: > + ASSERT_NOT_REACHED(); Nit: Since there are only 9 values, I would just list the other 5 here and omit the default: case. This will allow the compiler to give a warning if someone adds a value to ImageOrientationEnum and doesn't know to update this switch. > Source/WebCore/rendering/style/RenderStyle.h:1670 > + static ImageOrientationEnum initialImageOrientation() { return DefaultImageOrientation; } I would probably use OriginTopLeft explicitly since this doesn't depend on the exif spec. > Source/WebCore/rendering/style/StyleRareInheritedData.h:99 > + unsigned m_imageOrientation : 3; // ImageOrientationEnum - 1 Why ImageOrientationEnum - 1 ? To save a bit? I would just use the extra bit to avoid the possible human error.

Comment on attachment 152705 [details] Patch On second thoughts, will address the nits and upload a patch for landing.

Created attachment 152831 [details] Patch for landing Addressed nits, prepared for landing.

Created attachment 152833 [details] Patch for landing Fixed minor whitespace damage.

Comment on attachment 152833 [details] Patch for landing Clearing flags on attachment: 152833 Committed r122895: <http://trac.webkit.org/changeset/122895>

All reviewed patches have been landed. Closing bug.

Comment on attachment 152833 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=152833&action=review > LayoutTests/fast/css/image-orientation/image-orientation.html:23 > + shouldBe('p.style.cssText', '"image-orientation: ' + test + '; "'); The GTK port at the moment enables this feature, but fails the layout test due to the trailing whitespace that's expected in this assertion. I've also enabled the feature on ToT Chromium Linux port and the same whitespace is again making the test fail (the values are otherwise correct, and the feature is disabled on Chromium while the test is skipped). Do you remember the reasoning behind the whitespace here? Can/Should it be removed?

(In reply to comment #13) > (From update of attachment 152833 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152833&action=review > > > LayoutTests/fast/css/image-orientation/image-orientation.html:23 > > + shouldBe('p.style.cssText', '"image-orientation: ' + test + '; "'); > > The GTK port at the moment enables this feature, but fails the layout test due to the trailing whitespace that's expected in this assertion. > I've also enabled the feature on ToT Chromium Linux port and the same whitespace is again making the test fail (the values are otherwise correct, and the feature is disabled on Chromium while the test is skipped). > > Do you remember the reasoning behind the whitespace here? Can/Should it be removed? I don't remember why the whitespace is there. Feel free to remove it and update the expected results.

(In reply to comment #13) > > LayoutTests/fast/css/image-orientation/image-orientation.html:23 > > + shouldBe('p.style.cssText', '"image-orientation: ' + test + '; "'); > > Do you remember the reasoning behind the whitespace here? Can/Should it be removed? The whitespace was an artefact of bug 94633 which has since been fixed. It should be removed.