Bug 89624 – Add parsing and style application for css3-images image-orientation

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[David Barr]

Add image-orientation support to WebKit (parsing and style application.)
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.

[David Barr]

Created attachment 148711 [details]
Patch

Pending compile-time flag landing, start implementing image-orientation.

[Tony Chang]

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.

[David Barr]

> 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}; ?

[David Barr]

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.

[David Barr]

Created attachment 148960 [details]
Patch

Rewrote patch to use ImageOrientationEnum.

[David Barr]

Created attachment 152705 [details]
Patch

Rebased against r122813: <http://trac.webkit.org/changeset/122813>. Updated ChangeLogs.

[Tony Chang]

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.

[David Barr]

Comment on attachment 152705 [details]
Patch

On second thoughts, will address the nits and upload a patch for landing.

[David Barr]

Created attachment 152831 [details]
Patch for landing

Addressed nits, prepared for landing.

[David Barr]

Created attachment 152833 [details]
Patch for landing

Fixed minor whitespace damage.

[WebKit Review Bot]

Comment on attachment 152833 [details]
Patch for landing

Clearing flags on attachment: 152833

Committed r122895: <http://trac.webkit.org/changeset/122895>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

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?

[Tony Chang]

(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.

[David Barr]

(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.