Add CSS image-orientation: from-image

Add CSS image-orientation: from-image

Support image-orientation: from-image [1] by teaching the CSS style
system and image LayoutImage call about it. To keep state about the
image-orientation, add a flag on CSS RareNonInheritedData. For now
support from-image and the default angle 0deg.

Add various tests for computed style of image-orientation as well as
its rendering effect on images with EXIF orientation and its dynamic
behavior via JS manipulation.

Dragging an oriented image works visually, i.e., if the image has a
orientation, the drag image should have the same orientation.

The behavior partly matches Firefox, in that from-image is supported
but not the CSS angle and flip values, unlike Firefox.

WebKit has code for using image-orientation CSS with angle values (no
from-image or flip), but it is not enabled for Safari.

[1] http://www.w3.org/TR/css4-images/#the-image-orientation

BUG=158753
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199231

[rwlbuis, 2015-05-29 22:21:48]

rob.buis@samsung.com changed reviewers:
+ pdr@chromium.org, tabatkins@chromium.org

[rwlbuis, 2015-05-29 22:21:49]

PTAL, but mind you it is WIP:) A lot of it based on WebKit code.

With this code I was able to verify it works for the various image-orientation
values, using a locally modified version of http://www.xanthir.com/etc/exif/
after Tab suggested that page.

Do you guys think it needs a runtime flag?

Next week I'll try to dig out WebKit/Firefox tests for image-orientation and fix
them for this patch.

@pdr feel free to assign to somebody else if it is not your cup of tea ; not
sure who is the Blink image expert.

[rwlbuis, 2015-06-03 22:14:45]

Next round, I got a bit concerned after seeing this in the spec:

ISSUE 7  This property is likely going to be deprecated and its functionality
moved to HTML. At minimum, it will likely lose all but its initial value and
from-image.
(http://dev.w3.org/csswg/css-images-3/#issues-index)

It may have come from this discussion:
http://comments.gmane.org/gmane.comp.web.css.general/52401

Anyway I tried to restrict the work to support only from-image and 0deg. Notice
the patchset becomes much more manageable since the ImageOrientationDescription
does not have to be plumbed through. Now also the dynamic testcase works
(setting from-image from js).

Let me know if this makes sense. To support all the angles and potentially flip
is easily doable but will need all of the plumbing, I wanted to avoid that for
now given the ISSUE 7 comment.

PTAL.

[pdr., 2015-06-04 03:53:34]

pdr@chromium.org changed reviewers:
+ noel@google.com, timloh@chromium.org

[pdr., 2015-06-04 03:53:35]

I'm probably not the best reviewer for this. +cc Noel and Tim, would you be up
for reviewing this?

[rwlbuis, 2015-06-04 22:49:27]

On 2015/06/04 03:53:35, pdr wrote:
> I'm probably not the best reviewer for this. +cc Noel and Tim, would you be up
> for reviewing this?

SGTM.

In addition, I was wondering if we could remove shouldRespectImageOrientation
Document setting/WebKitShouldRespectImageOrientation as it can be emulated
through image-orientation: from-image. Should simplify the code.

[Noel Gordon, 2015-06-04 23:12:17]

On 2015/06/04 22:49:27, rwlbuis wrote:

> In addition, I was wondering if we could remove shouldRespectImageOrientation
> Document setting/WebKitShouldRespectImageOrientation as it can be emulated
> through image-orientation: from-image. Should simplify the code.

The setting is used in ImageDocument so I assume you mean to simplify that code
(it does have tests for an orientated image).  Seems fine, but suggest you do
that on a separate bug.

[Noel Gordon, 2015-06-04 23:18:13]

Also, I see no changes to CompositedLayerMapping.cpp, which is called
CompositedDeprecatedPaintLayerMapping.cpp these days.  Please add test for an
exif image with translateZ(0), aka an image in a composited CSS layer.

[rwlbuis, 2015-06-08 15:43:20]

On 2015/06/04 23:18:13, noel gordon wrote:
> Also, I see no changes to CompositedLayerMapping.cpp, which is called
> CompositedDeprecatedPaintLayerMapping.cpp these days.  Please add test for an
> exif image with translateZ(0), aka an image in a composited CSS layer.

Thanks for the suggestion, I added a test for it, but is causes no problem. Hope
I got the test right!

[Noel Gordon, 2015-06-09 11:13:03]

On 2015/06/08 15:43:20, rwlbuis wrote:

> Thanks for the suggestion, I added a test for it, but is causes no problem.
Hope
> I got the test right!

Test seems about right. But there was no problem: interesting
http://trac.webkit.org/changeset/153797

[Noel Gordon, 2015-06-09 13:26:55]

On 2015/06/04 23:12:17, noel gordon wrote:
> On 2015/06/04 22:49:27, rwlbuis wrote:
> 
> > In addition, I was wondering if we could remove
shouldRespectImageOrientation
> > Document setting/WebKitShouldRespectImageOrientation as it can be emulated
> > through image-orientation: from-image. Should simplify the code.
> 
> The setting is used in ImageDocument so I assume you mean to simplify that
code
> (it does have tests for an orientated image).  Seems fine, but suggest you do
> that on a separate bug.

https://code.google.com/p/chromium/issues/detail?id=498233

[rwlbuis, 2015-06-09 13:47:41]

On 2015/06/09 13:26:55, noel gordon wrote:
> On 2015/06/04 23:12:17, noel gordon wrote:
> > On 2015/06/04 22:49:27, rwlbuis wrote:
> > 
> > > In addition, I was wondering if we could remove
> shouldRespectImageOrientation
> > > Document setting/WebKitShouldRespectImageOrientation as it can be emulated
> > > through image-orientation: from-image. Should simplify the code.
> > 
> > The setting is used in ImageDocument so I assume you mean to simplify that
> code
> > (it does have tests for an orientated image).  Seems fine, but suggest you
do
> > that on a separate bug.
> 
> https://code.google.com/p/chromium/issues/detail?id=498233

Thanks!

Any opinion as to whether this needs to go behind a runtime flag?

I'll fix the description to something more meaningful.

[Noel Gordon, 2015-06-09 14:02:06]

On 2015/06/09 13:47:41, rwlbuis wrote:
> On 2015/06/09 13:26:55, noel gordon wrote:
> > On 2015/06/04 23:12:17, noel gordon wrote:
> > > On 2015/06/04 22:49:27, rwlbuis wrote:
> > > 
> > > > In addition, I was wondering if we could remove
> > shouldRespectImageOrientation
> > > > Document setting/WebKitShouldRespectImageOrientation as it can be
emulated
> > > > through image-orientation: from-image. Should simplify the code.
> > > 
> > > The setting is used in ImageDocument so I assume you mean to simplify that
> > code
> > > (it does have tests for an orientated image).  Seems fine, but suggest you
> do
> > > that on a separate bug.
> > 
> > https://code.google.com/p/chromium/issues/detail?id=498233
> 
> Thanks!
> 
> Any opinion as to whether this needs to go behind a runtime flag?

None as yet.

> I'll fix the description to something more meaningful.

Could you add a reference to the spec section your implementing?

> [noel] Test seems about right. But there was no problem: interesting
> http://trac.webkit.org/changeset/153797

Did you look at that webkit patch / bug?  We have the same code in Blink
(DirectlyCompositedImage), right?

[rwlbuis, 2015-06-09 14:57:29]

On 2015/06/09 14:02:06, noel gordon wrote:
> > I'll fix the description to something more meaningful.
> 
> Could you add a reference to the spec section your implementing?

Done!

> > [noel] Test seems about right. But there was no problem: interesting
> > http://trac.webkit.org/changeset/153797
> 
> Did you look at that webkit patch / bug?  We have the same code in Blink
> (DirectlyCompositedImage), right?

Will have a look.

[Noel Gordon, 2015-06-09 15:17:55]

On 2015/06/09 14:57:29, rwlbuis wrote:

> Will have a look.

Ok, and because we rename everything or move things to different files, you go
here

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[rwlbuis, 2015-06-09 15:30:57]

On 2015/06/09 15:17:55, noel gordon wrote:
> On 2015/06/09 14:57:29, rwlbuis wrote:
> 
> > Will have a look.
> 
> Ok, and because we rename everything or move things to different files, you go
> here
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Thanks, I found it and included my fix in patch set 6.

One strange thing about this, I can see it fixes the testcase when run manually
(i.e. without the change the output is wrong and with it is right), though when
used in the layout test framework the test seems to PASS with or without my fix
in CompositedDeprecatedPaintLayerMapping.cpp.

[Noel Gordon, 2015-06-09 15:38:22]

On 2015/06/09 15:30:57, rwlbuis wrote:
> On 2015/06/09 15:17:55, noel gordon wrote:
> > On 2015/06/09 14:57:29, rwlbuis wrote:
> > 
> > > Will have a look.
> > 
> > Ok, and because we rename everything or move things to different files, you
go
> > here
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Thanks, I found it and included my fix in patch set 6.
> 
> One strange thing about this, I can see it fixes the testcase when run
manually
> (i.e. without the change the output is wrong and with it is right), 

Good find then :)

> though when
> used in the layout test framework the test seems to PASS with or without my
fix
> in CompositedDeprecatedPaintLayerMapping.cpp.

Somethings weird with the test framework.  Put an exit(0) at that point in the
code, and rerun the layer test (without your fix, or course).  The test should
crash.

[rwlbuis, 2015-06-09 15:50:05]

Patchset #6 (id:100001) has been deleted

[rwlbuis, 2015-06-09 16:01:19]

On 2015/06/09 15:38:22, noel gordon wrote:
> > though when
> > used in the layout test framework the test seems to PASS with or without my
> fix
> > in CompositedDeprecatedPaintLayerMapping.cpp.
> 
> Somethings weird with the test framework.  Put an exit(0) at that point in the
> code, and rerun the layer test (without your fix, or course).  The test should
> crash.

It does crash, with or without my patch, so it does get to that spot even on
layout runs. I'll try to debug some more after lunch.

[rwlbuis, 2015-06-09 21:17:28]

@noel I am not sure if the TODOs in the Description are the best place :) Let me
reply inline:

TODO: what if I drag an orientated image from the page? Does it re-orientate as
it drags (that'd look odd to the user I think).

Currently works fine for from-image. If we'd support angles and flip I think we
need code in DragController and friends, similar to what I did in patch set 3.

TODO: per
http://sethfowler.org/blog/2013/09/13/new-in-firefox-26-css-image-orientation/
seems Firefox implements [ flip ], the blog mentions it's use-cases. Seems we're
TBD if we want compat here?  perhaps we jsut not it in the changelog and ask the
editor.

Willing to work on that, but fear the patch may get big. I think flip only makes
sense if general angles are supported too, otherwise it is quite limited.

TODO: What is the Safari story for image-orientation properties?  Do we have
compat?

It seems Safari supports angles, but not flip or from-image.

[rwlbuis, 2015-06-11 21:32:54]

On 2015/06/09 21:17:28, rwlbuis wrote:
> @noel I am not sure if the TODOs in the Description are the best place :) Let
me
> reply inline:
> 
> TODO: what if I drag an orientated image from the page? Does it re-orientate
as
> it drags (that'd look odd to the user I think).
> 
> Currently works fine for from-image. If we'd support angles and flip I think
we
> need code in DragController and friends, similar to what I did in patch set 3.
> 
> TODO: per
> http://sethfowler.org/blog/2013/09/13/new-in-firefox-26-css-image-orientation/
> seems Firefox implements [ flip ], the blog mentions it's use-cases. Seems
we're
> TBD if we want compat here?  perhaps we jsut not it in the changelog and ask
the
> editor.
> 
> Willing to work on that, but fear the patch may get big. I think flip only
makes
> sense if general angles are supported too, otherwise it is quite limited.
> 
> TODO: What is the Safari story for image-orientation properties?  Do we have
> compat?
> 
> It seems Safari supports angles, but not flip or from-image.

@noel patch set #6 has I think a correct check for the direct compositition
case. Basically we can only allow this is from-image is not used or the
orientation in the actual image is default. I was looking too much at the WebKit
implementation but it not so useful since WebKit does not support from-image in
the first place. PTAL.

[rwlbuis, 2015-06-11 21:33:15]

On 2015/06/11 21:32:54, rwlbuis wrote:
> On 2015/06/09 21:17:28, rwlbuis wrote:
> > @noel I am not sure if the TODOs in the Description are the best place :)
Let
> me
> > reply inline:
> > 
> > TODO: what if I drag an orientated image from the page? Does it re-orientate
> as
> > it drags (that'd look odd to the user I think).
> > 
> > Currently works fine for from-image. If we'd support angles and flip I think
> we
> > need code in DragController and friends, similar to what I did in patch set
3.
> > 
> > TODO: per
> >
http://sethfowler.org/blog/2013/09/13/new-in-firefox-26-css-image-orientation/
> > seems Firefox implements [ flip ], the blog mentions it's use-cases. Seems
> we're
> > TBD if we want compat here?  perhaps we jsut not it in the changelog and ask
> the
> > editor.
> > 
> > Willing to work on that, but fear the patch may get big. I think flip only
> makes
> > sense if general angles are supported too, otherwise it is quite limited.
> > 
> > TODO: What is the Safari story for image-orientation properties?  Do we have
> > compat?
> > 
> > It seems Safari supports angles, but not flip or from-image.
> 
> @noel patch set #6 has I think a correct check for the direct compositition
> case. Basically we can only allow this is from-image is not used or the
> orientation in the actual image is default. I was looking too much at the
WebKit
> implementation but it not so useful since WebKit does not support from-image
in
> the first place. PTAL.

Oops I meant patch set #8.

[Noel Gordon, 2015-06-12 15:18:38]

On 2015/06/09 21:17:28, rwlbuis wrote:
> @noel I am not sure if the TODOs in the Description are the best place :) Let
me
> reply inline:

The review tool does not provide a way to add review comments to your change
description either ... so I used TODO ;)

> TODO: what if I drag an orientated image from the page? Does it re-orientate
as
> it drags (that'd look odd to the user I think).
> 
> Currently works fine for from-image. If we'd support angles and flip I think
we
> need code in DragController and friends, similar to what I did in patch set 3.

I think that angles and flip, force / set an orientation on a BitmapImage and
we'd be mostly good.

> TODO: per
> http://sethfowler.org/blog/2013/09/13/new-in-firefox-26-css-image-orientation/
> seems Firefox implements [ flip ], the blog mentions it's use-cases. Seems
we're
> TBD if we want compat here?  perhaps we just not it in the changelog and ask
the
> editor.
> 
> Willing to work on that, but fear the patch may get big. I think flip only
makes
> sense if general angles are supported too, otherwise it is quite limited.

"Behavior matches Firefox."  is not quite correct then.  Resolve this TODO but
noting the difference in the change log.

General angles for orientation are not needed, orientation <angles> should be
mod 90 per spec I think, and then flip has a role.

For general angles, well we have CSS rotate() for that, and its rotation angle
is applied to the image _after_ orientation.

> TODO: What is the Safari story for image-orientation properties?  Do we have
> compat?
> 
> It seems Safari supports angles, but not flip or from-image.

I believe images on iOS Safari only, are all set to "from-image" via the
setting.  Anyho, resolve this TODO with a short note about the Safari compat
story in the change log.

[Noel Gordon, 2015-06-12 16:21:03]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[Noel Gordon, 2015-06-12 16:21:05]

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutImage.cpp (right):

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:85: bool hadStyle = (oldStyle != 0);
Remove hadStyle, doesn't really say anymore that oldStyle. Write:

   oldStyle ? oldStyle->respectImageOrientation() : ...

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:88: intrinsicSizeChanged();
Devious and I love it.

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2325: // an <img> and the setting to respect
it everywhere is set.
Comment out of date.

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2326: return document().isImageDocument()
Let's break this up into individual if statements, it's tending to unwieldy :)

Also, for the image document case, add a // FIXME: crbug.com/XXXXXX ...
as a reminder.

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1859:
|| toBitmapImage(image)->size() ==
toBitmapImage(image)->sizeRespectingOrientation();
The first part of the clause looks right to me.  Bust it out into a separate if
statement.

The second part is interesting. If an image is orientated by 180 deg, the
orientated size and the natural size() are the same. So how is that rotation
sent to the GPU? I don't think ImageLayer supports that, nor do I see a change
to ImageLayer in your patch.  /curious I expect the GPU will just draw your
image un-orientated -> bug.

[Noel Gordon, 2015-06-12 16:26:53]

On 2015/06/12 15:18:38, noel gordon wrote:

> I think that angles and flip, force / set an orientation on a BitmapImage and
> we'd be mostly good.

Or maybe not on second thoughts, we'd pull the desired orientation from CSS
system somehow.

[rwlbuis, 2015-06-15 17:21:52]

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutImage.cpp (right):

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:85: bool hadStyle = (oldStyle != 0);
On 2015/06/12 16:21:04, noel gordon wrote:
> Remove hadStyle, doesn't really say anymore that oldStyle. Write:
> 
>    oldStyle ? oldStyle->respectImageOrientation() : ...
> 
>   

Done.

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:88: intrinsicSizeChanged();
On 2015/06/12 16:21:04, noel gordon wrote:
> Devious and I love it.

Thanks :)

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2325: // an <img> and the setting to respect
it everywhere is set.
On 2015/06/12 16:21:04, noel gordon wrote:
> Comment out of date.

Done.

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2326: return document().isImageDocument()
On 2015/06/12 16:21:04, noel gordon wrote:
> Let's break this up into individual if statements, it's tending to unwieldy :)
> 
> Also, for the image document case, add a // FIXME: crbug.com/XXXXXX ...
> as a reminder.

Done. Forgot about the breaking up though, will fix.

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/com...
File Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1162793003/diff/160001/Source/core/layout/com...
Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1859:
|| toBitmapImage(image)->size() ==
toBitmapImage(image)->sizeRespectingOrientation();
On 2015/06/12 16:21:04, noel gordon wrote:
> The first part of the clause looks right to me.  Bust it out into a separate
if
> statement.

Done.

> The second part is interesting. If an image is orientated by 180 deg, the
> orientated size and the natural size() are the same. So how is that rotation
> sent to the GPU? I don't think ImageLayer supports that, nor do I see a change
> to ImageLayer in your patch.  /curious I expect the GPU will just draw your
> image un-orientated -> bug.

Will experiment a bit with that, you could be right...

[rwlbuis, 2015-06-18 21:37:09]

@noel, PTAL. I think I fixed the Description now. As to the
CompositedDeprecatedPaintLayerMapping, I think it may be best for now to not do
the compositing if from-image is used to be on the safe side. Or, if you have
some hints to fix it in ImageLayer let me know, I am not very familiar with GPU
stuff. Could also leave a TODO/FIXME for it.

[Noel Gordon, 2015-06-19 07:41:04]

On 2015/06/18 21:37:09, rwlbuis wrote:
> @noel, PTAL. I think I fixed the Description now. 

I will at it again nearer submit time. I was curious if Safari or Firefox apply
orientation to HTMLImage only, or to other elements or CSS background images
etc.

> As to the
> CompositedDeprecatedPaintLayerMapping, I think it may be best for now to not
do
> the compositing if from-image is used to be on the safe side.

Agree. Hope you manually tested this part as well.

> Or, if you have
> some hints to fix it in ImageLayer let me know, I am not very familiar with
GPU
> stuff. 

It would require plumbing the orientation enum values through ImageLayer and
into the compositor code in src/cc

> Could also leave a TODO/FIXME for it.

A FIXME and a bug to go with it would do for now.  The plumbing could be done on
that bug.

[Noel Gordon, 2015-06-19 07:41:25]

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File LayoutTests/fast/css/image-orientation/image-orientation-dynamic.html
(right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-dynamic.html:6:
document.documentElement.className = "";
document.documentElement.className ... not needed I think.

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-dynamic.html:10:
<style>
remove style tags.

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File
LayoutTests/fast/css/image-orientation/image-orientation-from-image-composited.html
(right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-from-image-composited.html:22:
for(var i = 1; i <= 9; i++)
nit: for (var ...

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File LayoutTests/fast/css/image-orientation/image-orientation-from-image.html
(right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-from-image.html:22:
for(var i = 1; i <= 9; i++)
nit: for (var ...

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File LayoutTests/fast/css/image-orientation/image-orientation.html (right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation.html:17: var p =
document.createElement('p');
Not following: are you testing that imageOrientation does not apply to <p>
elements here, or did you mean to test an <img> element?

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2296: // FIXME: crbug.com/498233
// Respect the image's orientation if it's being used as a full-page image or
// it's an <img> and the setting to respect it everywhere is set or the <img>
// has image-orientation: from-image style. FIXME: crbug.com/498233

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2299: respectOrientation = true;
nit: this code might read better of your preferred the early return

    if (document().isImageDocument())
        return RespectImageOrientation;

    if (!isHTMLImageElement(node())))
        return DoNotRespectImageOrientation;

    if (document().settings() &&
document().settings()->shouldRespectImageOrientation())
         ...

etc, etc.  Wanna try that?

[Noel Gordon, 2015-06-19 07:43:07]

ahem: ^^ code might read better if you preferred the early return.

[rwlbuis, 2015-06-19 16:03:16]

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File LayoutTests/fast/css/image-orientation/image-orientation-dynamic.html
(right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-dynamic.html:6:
document.documentElement.className = "";
On 2015/06/19 07:41:24, noel gordon wrote:
> document.documentElement.className ... not needed I think.

Done.

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-dynamic.html:10:
<style>
On 2015/06/19 07:41:24, noel gordon wrote:
> remove style tags.

Done.

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File
LayoutTests/fast/css/image-orientation/image-orientation-from-image-composited.html
(right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-from-image-composited.html:22:
for(var i = 1; i <= 9; i++)
On 2015/06/19 07:41:24, noel gordon wrote:
> nit: for (var ...

Done.

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File LayoutTests/fast/css/image-orientation/image-orientation-from-image.html
(right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation-from-image.html:22:
for(var i = 1; i <= 9; i++)
On 2015/06/19 07:41:24, noel gordon wrote:
> nit: for (var ...

Done.

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
File LayoutTests/fast/css/image-orientation/image-orientation.html (right):

https://codereview.chromium.org/1162793003/diff/200001/LayoutTests/fast/css/i...
LayoutTests/fast/css/image-orientation/image-orientation.html:17: var p =
document.createElement('p');
On 2015/06/19 07:41:25, noel gordon wrote:
> Not following: are you testing that imageOrientation does not apply to <p>
> elements here, or did you mean to test an <img> element? 

It does not matter for the test from CSSComputedStyle POV but I agree it is
clearer, fixed.

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2296: // FIXME: crbug.com/498233
On 2015/06/19 07:41:25, noel gordon wrote:
> // Respect the image's orientation if it's being used as a full-page image or
> // it's an <img> and the setting to respect it everywhere is set or the <img>
> // has image-orientation: from-image style. FIXME: crbug.com/498233

Oh, missed this.... but it will be easy to fix.

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2299: respectOrientation = true;
On 2015/06/19 07:41:25, noel gordon wrote:
> nit: this code might read better of your preferred the early return
> 
>     if (document().isImageDocument())
>         return RespectImageOrientation;
> 
>     if (!isHTMLImageElement(node())))
>         return DoNotRespectImageOrientation;
> 
>     if (document().settings() &&
> document().settings()->shouldRespectImageOrientation())
>          ...
> 
> etc, etc.  Wanna try that?

Great suggestion, I think it looks much clearer, done.

[Noel Gordon, 2015-06-19 16:50:07]

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:2296: // FIXME: crbug.com/498233
On 2015/06/19 16:03:15, rwlbuis wrote:
> On 2015/06/19 07:41:25, noel gordon wrote:
> > // Respect the image's orientation if it's being used as a full-page image
or
> > // it's an <img> and the setting to respect it everywhere is set or the
<img>
> > // has image-orientation: from-image style. FIXME: crbug.com/498233
> 
> Oh, missed this.... but it will be easy to fix.

Ok, let's do it next round.

[rwlbuis, 2015-06-19 19:22:09]

On 2015/06/19 16:50:07, noel gordon wrote:
>
https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1162793003/diff/200001/Source/core/layout/Lay...
> Source/core/layout/LayoutObject.cpp:2296: // FIXME: crbug.com/498233
> On 2015/06/19 16:03:15, rwlbuis wrote:
> > On 2015/06/19 07:41:25, noel gordon wrote:
> > > // Respect the image's orientation if it's being used as a full-page image
> or
> > > // it's an <img> and the setting to respect it everywhere is set or the
> <img>
> > > // has image-orientation: from-image style. FIXME: crbug.com/498233
> > 
> > Oh, missed this.... but it will be easy to fix.
> 
> Ok, let's do it next round.

Done :)

[Tab Atkins, 2015-06-22 21:00:38]

> Willing to work on that, but fear the patch may get big. I think flip only
makes
> sense if general angles are supported too, otherwise it is quite limited.

There are 8 EXIF orientations, four of which "flip" the image (in a way that
can't be emulated with rotations).  "flip" was added because it's weird if
"from-image" can orient the image correctly but you can't do so manually.

I've no clue what "flip" has to do with general rotation angles, or why it's
"quite limited" without them.

[rwlbuis, 2015-06-22 22:02:11]

On 2015/06/22 21:00:38, Tab Atkins wrote:
> > Willing to work on that, but fear the patch may get big. I think flip only
> makes
> > sense if general angles are supported too, otherwise it is quite limited.
> 
> There are 8 EXIF orientations, four of which "flip" the image (in a way that
> can't be emulated with rotations).  "flip" was added because it's weird if
> "from-image" can orient the image correctly but you can't do so manually.
> 
> I've no clue what "flip" has to do with general rotation angles, or why it's
> "quite limited" without them.

I meant adding flip but not rotation angles seems quite limited to me. Again,
adding both should not be hard but touches a lot of places (heavy plumbing) as
well as needing a lot of added tests, hence maybe it is better to do in a future
patch.

[rwlbuis, 2015-06-26 14:37:46]

@noel ping :)

[rwlbuis, 2015-07-13 14:14:33]

Back from vacation, so another PING :)

[Noel Gordon, 2015-07-13 14:43:13]

Hey (and thanks for setting way status). Perhaps we should add the flag and do
the rest (#36) in another patch?  Thoughts?

Also, Tim can look over the core/CSS changes herein.

[rwlbuis, 2015-07-13 19:58:31]

On 2015/07/13 14:43:13, noel gordon wrote:
> Hey (and thanks for setting way status). Perhaps we should add the flag and do
> the rest (#36) in another patch?  Thoughts?

Sounds good! In my latest patch I added a test runtime flag, so it is tested but
by default running content_shell/chrome will not parse the property.

> Also, Tim can look over the core/CSS changes herein.

Yes that would be great. @timloh PTAL

[Timothy Loh, 2015-07-16 00:37:47]

lgtm for css

https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
Source/core/css/CSSProperties.in:198: image-orientation inherited, custom_all
Probably better with

name_for_methods=RespectImageOrientation, converter=convertImageOrientation

and writing a converter, instead of having code in StyleBuilderCustom

[Timothy Loh, 2015-07-16 03:07:26]

https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
Source/core/css/CSSProperties.in:198: image-orientation inherited, custom_all
Did you miss runtime_flag=ImageOrientation here?

[rwlbuis, 2015-07-16 18:10:57]

Patchset #14 (id:280001) has been deleted

[rwlbuis, 2015-07-16 20:03:31]

Thanks for the review!

On 2015/07/16 03:07:26, Timothy Loh wrote:
>
https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
> File Source/core/css/CSSProperties.in (right):
> 
>
https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
> Source/core/css/CSSProperties.in:198: image-orientation inherited, custom_all
> Did you miss runtime_flag=ImageOrientation here?

Right, I missed that. Fixed.

[Noel Gordon, 2015-07-20 17:07:13]

On 2015/07/16 00:37:47, Timothy Loh wrote:

>
https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
> Source/core/css/CSSProperties.in:198: image-orientation inherited, custom_all
> Probably better with
> 
> name_for_methods=RespectImageOrientation, converter=convertImageOrientation
> 
> and writing a converter, instead of having code in StyleBuilderCustom

Rob, this bit is still a TODO by the looks?

[rwlbuis, 2015-07-20 17:14:41]

On 2015/07/20 17:07:13, noel gordon wrote:
> On 2015/07/16 00:37:47, Timothy Loh wrote:
> 
> >
>
https://codereview.chromium.org/1162793003/diff/260001/Source/core/css/CSSPro...
> > Source/core/css/CSSProperties.in:198: image-orientation inherited,
custom_all
> > Probably better with
> > 
> > name_for_methods=RespectImageOrientation, converter=convertImageOrientation
> > 
> > and writing a converter, instead of having code in StyleBuilderCustom
> 
> Rob, this bit is still a TODO by the looks?

Oh, I overlooked that comment, will fix.

[Noel Gordon, 2015-07-21 15:17:51]

LGTM

[rwlbuis, 2015-07-21 15:28:40]

On 2015/07/21 15:17:51, noel gordon wrote:
> LGTM

Thanks!
Any PSAs or Intents needed or we do that later once a few more bugs have been
fixed? I prefer to wait a bit with that...

[Noel Gordon, 2015-07-21 15:37:10]

On 2015/07/21 15:28:40, rwlbuis wrote:

> Any PSAs or Intents needed or we do that later once a few more bugs have been
> fixed? I prefer to wait a bit with that...

Agree.

[rwlbuis, 2015-07-21 15:44:29]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-07-21 15:44:29]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1162793003/#ps320001
(title: "add converter")

[commit-bot: I haz the power, 2015-07-21 15:44:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162793003/320001

[commit-bot: I haz the power, 2015-07-21 16:59:25]

Committed patchset #15 (id:320001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199231