Bug 89624 - Add parsing and style application for css3-images image-orientation
Summary: Add parsing and style application for css3-images image-orientation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Barr
URL: http://www.w3.org/TR/2012/CR-css3-ima...
Keywords:
Depends on: 89055
Blocks: 89052 91566
  Show dependency treegraph
 
Reported: 2012-06-20 17:59 PDT by David Barr
Modified: 2013-01-30 15:52 PST (History)
10 users (show)

See Also:


Attachments
Patch (21.65 KB, patch)
2012-06-20 19:15 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (22.72 KB, patch)
2012-06-21 21:53 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch (23.07 KB, patch)
2012-07-17 00:20 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch for landing (23.11 KB, patch)
2012-07-17 14:26 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch for landing (23.11 KB, patch)
2012-07-17 14:29 PDT, David Barr
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Barr 2012-06-20 17:59:36 PDT
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.
Comment 1 David Barr 2012-06-20 19:15:13 PDT
Created attachment 148711 [details]
Patch

Pending compile-time flag landing, start implementing image-orientation.
Comment 2 Tony Chang 2012-06-21 09:33:30 PDT
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.
Comment 3 David Barr 2012-06-21 17:18:46 PDT
> 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}; ?
Comment 4 David Barr 2012-06-21 18:59:41 PDT
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.
Comment 5 David Barr 2012-06-21 21:53:06 PDT
Created attachment 148960 [details]
Patch

Rewrote patch to use ImageOrientationEnum.
Comment 6 David Barr 2012-07-17 00:20:48 PDT
Created attachment 152705 [details]
Patch

Rebased against r122813: <http://trac.webkit.org/changeset/122813>. Updated ChangeLogs.
Comment 7 Tony Chang 2012-07-17 09:50:42 PDT
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 8 David Barr 2012-07-17 13:34:34 PDT
Comment on attachment 152705 [details]
Patch

On second thoughts, will address the nits and upload a patch for landing.
Comment 9 David Barr 2012-07-17 14:26:45 PDT
Created attachment 152831 [details]
Patch for landing

Addressed nits, prepared for landing.
Comment 10 David Barr 2012-07-17 14:29:01 PDT
Created attachment 152833 [details]
Patch for landing

Fixed minor whitespace damage.
Comment 11 WebKit Review Bot 2012-07-17 16:23:47 PDT
Comment on attachment 152833 [details]
Patch for landing

Clearing flags on attachment: 152833

Committed r122895: <http://trac.webkit.org/changeset/122895>
Comment 12 WebKit Review Bot 2012-07-17 16:23:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Zan Dobersek 2013-01-30 11:43:59 PST
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?
Comment 14 Tony Chang 2013-01-30 11:59:24 PST
(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.
Comment 15 David Barr 2013-01-30 15:52:41 PST
(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.