Fix directly composited image path for CSS image-orientation

Fix directly composited image path for CSS image-orientation

Use the DragImage::resizeAndOrientImage helper added in this
CL to obtain a SkImage of correct orientation to send to the
graphics layer for compositing.

Add new layout tests:

image-orientation-from-image-composited-dynamic.html
 - test dynamically adding CSS image-orientation: from-image
image-orientation-default.html
 - check that when no image-orientation is specified, images
   with an EXIF orientation are not transformed.

BUG=502267
Committed: https://crrev.com/8cac6805bdac3a41ac76a72e9e1e58d26105895c
Cr-Commit-Position: refs/heads/master@{#352647}

[rwlbuis, 2015-09-24 17:25:19]

rob.buis@samsung.com changed reviewers:
+ noel@chromium.org

[rwlbuis, 2015-09-24 17:25:19]

PTAL. I got the impression that changing compositor to handle image orientation
was not preferred, so in this patch it is all handled on the blink side.

[Noel Gordon, 2015-09-24 23:58:55]

Yes, I did find the the lack of help you received from the compositor side of
things disturbing.  Working around that by doing it in blink makes sense to me,
and it allows us to actually move forward.

[Noel Gordon, 2015-09-25 00:12:19]

heading in the right direction, needs a test or two ...

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp
(right):

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1866:
return true;
Very cool, and I can see why layout tests are passing here, maybe because there
are no tests :)

We should add a test if none exists; an <img> with non-default orientation that
is in its own layer

<style>
   img { transform: translateZ(0); }
</style>

and perhaps another test that dynamically adds that CSS style to the <img>
element under test.

Note that since the current file is marked deprecated, and will be once
display-lists-v2 (SPv2) becomes a real thing, then their no layout test
regression policy will keep our tests working, no matter how they implement
layered images in SPv2.

Harder, bonus points test: make the image have a layer _and_ object-fit style. 
The later is an Opera contribution to CSS, but no layout test was ever added for
a layered <img>.  Shame really, and it is well-busted, but perhaps one of your
Opera colleagues could separately look at that issue,
https://code.google.com/p/chromium/issues/detail?id=518509 as penance.

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/DragController.cpp (left):

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/DragController.cpp:802: if
(image->size().height() * image->size().width() <= MaxOriginalImageArea
Add a space before this if clause: this is not a line squishing competition :)
prefer readability.

If this was English language layout, we are starting a new paragraph here, and
paragraphs are set off from their surroundings with a space before for some
reason.  We conjecture that the reason is that walls of text (or code) without
paragraphs are unreadable.

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/DragController.cpp:804: ImageOrientation
orientation;
nit: add a space before this line.

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/DragController.cpp:806: BitmapImage*
bitmapImage = toBitmapImage(image);
Remove the {} on this if clause and instead write it as:

  if ([that condition])
      orientation = toBitmapImage(image)->currentFrameOrientation();

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/DragImage.h (right):

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/DragImage.h:73: const AffineTransform&, float
opacity, InterpolationQuality = InterpolationNone);
style: no line limit.

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1021:
ImageOrientation imageOrientation =
static_cast<BitmapImage*>(image)->currentFrameOrientation();
Let's avoid the static cast: we could for example write it as ...

ImageOrientation orientation = toBitmapImage(image)->currentFrameOrientation();

right?

[Noel Gordon, 2015-09-25 00:15:54]

Other thoughts: let's not close https://codereview.chromium.org/1361413004 . We
know we won't need it, good riddance, but I have further business to attend to
there when I get the time.

[rwlbuis, 2015-09-29 21:37:22]

On 2015/09/25 00:12:19, noel gordon wrote:
> heading in the right direction, needs a test or two ...

Sure.

https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp:1866:
> return true;
> Very cool, and I can see why layout tests are passing here, maybe because
there
> are no tests :)
> 
> We should add a test if none exists; an <img> with non-default orientation
that
> is in its own layer
> 
> <style>
>    img { transform: translateZ(0); }
> </style>

We have image-orientation-from-image-composited.html for this :)

> and perhaps another test that dynamically adds that CSS style to the <img>
> element under test.

Good idea, added image-orientation-from-image-composited-dynamic.html for that.

> Note that since the current file is marked deprecated, and will be once
> display-lists-v2 (SPv2) becomes a real thing, then their no layout test
> regression policy will keep our tests working, no matter how they implement
> layered images in SPv2.

Good to know.

> Harder, bonus points test: make the image have a layer _and_ object-fit style.

> The later is an Opera contribution to CSS, but no layout test was ever added
for
> a layered <img>.  Shame really, and it is well-busted, but perhaps one of your
> Opera colleagues could separately look at that issue,
> https://code.google.com/p/chromium/issues/detail?id=518509 as penance.

Maybe later :)

>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/page/DragController.cpp (left):
> 
>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/page/DragController.cpp:802: if
> (image->size().height() * image->size().width() <= MaxOriginalImageArea
> Add a space before this if clause: this is not a line squishing competition :)
> prefer readability.

Done.

> If this was English language layout, we are starting a new paragraph here, and
> paragraphs are set off from their surroundings with a space before for some
> reason.  We conjecture that the reason is that walls of text (or code) without
> paragraphs are unreadable.

Right but it is hard to please everybody in this regard and hard to enforce as a
style I think.

>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/page/DragController.cpp (right):
> 
>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/page/DragController.cpp:804: ImageOrientation
> orientation;
> nit: add a space before this line.

Done.

>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/page/DragController.cpp:806: BitmapImage*
> bitmapImage = toBitmapImage(image);
> Remove the {} on this if clause and instead write it as:
> 
>   if ([that condition])
>       orientation = toBitmapImage(image)->currentFrameOrientation();

Much better done.

>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/DragImage.h (right):
> 
>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/DragImage.h:73: const AffineTransform&,
float
> opacity, InterpolationQuality = InterpolationNone);
> style: no line limit.

Done.

>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):
> 
>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1021:
> ImageOrientation imageOrientation =
> static_cast<BitmapImage*>(image)->currentFrameOrientation();
> Let's avoid the static cast: we could for example write it as ...
> 
> ImageOrientation orientation =
toBitmapImage(image)->currentFrameOrientation();
> 
> right?

Right! Done.

I noticed that the default "0deg" did not work well in my first patch, i.e.
turning on the runtime flag caused it to act like "from-image" was always
enabled, fixed in latest patch.

PTAL.

[Noel Gordon, 2015-09-30 12:02:54]

https://codereview.chromium.org/1361413004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/image-orientation/image-orientation-from-image-composited-dynamic.html
(left):

https://codereview.chromium.org/1361413004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/image-orientation/image-orientation-from-image-composited-dynamic.html:20:

Thanks for the new test, Rob.  I looked at the results: they seem fine.  I would
remove the text and image result from the patch now, and instead add a line to
Layouttest/TestExpections for the new test and mark it as [ NeedsRebaseline ].

[Noel Gordon, 2015-10-01 03:52:08]

On 2015/09/29 21:37:22, rwlbuis wrote:
> On 2015/09/25 00:12:19, noel gordon wrote:

> We have image-orientation-from-image-composited.html for this :)

Ah ok, nod :)
 
> > and perhaps another test that dynamically adds that CSS style to the <img>
> > element under test.
> 
> Good idea, added image-orientation-from-image-composited-dynamic.html for
that.

Thanks.

> > If this was English language layout, we are starting a new paragraph here,
and
> > paragraphs are set off from their surroundings with a space before for some
> > reason.  We conjecture that the reason is that walls of text (or code)
without
> > paragraphs are unreadable.
> 
> Right but it is hard to please everybody in this regard and hard to enforce as
a
> style I think.

Agree, so we should please ourselves then :)

>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/DragImage.h (right):
> > 
> >
>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/DragImage.h:73: const AffineTransform&,
> float
> > opacity, InterpolationQuality = InterpolationNone);
> > style: no line limit.
> 
> Done.

But it's back in patch set #3, I note :)

> I noticed that the default "0deg" did not work well in my first patch, i.e.
> turning on the runtime flag caused it to act like "from-image" was always
> enabled, fixed in latest patch.

All working?

> PTAL.

Done.  Patch is looking good: TestExpectations and very minor nits to fix.

[rwlbuis, 2015-10-01 15:54:17]

On 2015/10/01 03:52:08, noel gordon wrote:
>
https://codereview.chromium.org/1361413004/diff/1/third_party/WebKit/Source/p...
> > > third_party/WebKit/Source/platform/DragImage.h:73: const AffineTransform&,
> > float
> > > opacity, InterpolationQuality = InterpolationNone);
> > > style: no line limit.
> > 
> > Done.
> 
> But it's back in patch set #3, I note :)

Ouch! Fixed :)

> > I noticed that the default "0deg" did not work well in my first patch, i.e.
> > turning on the runtime flag caused it to act like "from-image" was always
> > enabled, fixed in latest patch.
> 
> All working?

Sorry, I should have added a test for this earlier. I now added
image-orientation-default.html, I kept the description of the internal EXIF
image-orientation but updated the overall description that none of the imahes
should rotate, hopefully that is not too confusing.

> > PTAL.
> 
> Done.  Patch is looking good: TestExpectations and very minor nits to fix.

I updated TestExpectations, PTAL.

[Noel Gordon, 2015-10-01 16:07:40]

> > > I noticed that the default "0deg" did not work well in my first patch,
i.e.
> > > turning on the runtime flag caused it to act like "from-image" was always
> > > enabled, fixed in latest patch.
> > 
> > All working?
> 
> Sorry, I should have added a test for this earlier. I now added
> image-orientation-default.html, I kept the description of the internal EXIF
> image-orientation but updated the overall description that none of the images
> should rotate, hopefully that is not too confusing.

Nope, that test reads fine to me.

> > > PTAL.
> > 
> > Done.  Patch is looking good: TestExpectations and very minor nits to fix.
> 
> I updated TestExpectations, PTAL.

Yeah that looks good.

[Noel Gordon, 2015-10-01 16:09:29]

LGTM - you'll need a OWNER for the core changes, but Platform looking good to
me.

Thanks for tests, perhaps list them in your change description?

[rwlbuis, 2015-10-01 16:42:01]

On 2015/10/01 16:09:29, noel gordon wrote:
> LGTM - you'll need a OWNER for the core changes, but Platform looking good to
> me.

Thanks, will add chrishtr.

> Thanks for tests, perhaps list them in your change description?

Will do!

[rwlbuis, 2015-10-01 16:53:16]

rob.buis@samsung.com changed reviewers:
+ chrishtr@chromium.org

[rwlbuis, 2015-10-01 16:53:17]

@chrishtr PTAL at the core changes. One change is to make sure images with
image-orientation other than OriginTopLeft can be directly composited.

The other change is for DragController to make sure images with special
image-orientation look correct when dragged. I am not sure there is test
infrastructure for this?

[chrishtr, 2015-10-02 21:17:56]

I left a comment on September 3 on bug 502267 about why we need to teach the
compositor anything about image-orientation. That needs to be answered first.

[rwlbuis, 2015-10-02 21:38:30]

On 2015/10/02 21:17:56, chrishtr wrote:
> I left a comment on September 3 on bug 502267 about why we need to teach the
> compositor anything about image-orientation. That needs to be answered first.

This patch avoids the need to change the compositor. When I first started, not
being very familiar with CompositedDeprecatedPaintLayerMapping and
GraphicsLayer, I thought changing the compositor was the only way to support
image-orientation and direct compositing, later I noticed DragImage code which
provides a SkImage correctly transformed according to the image orientation, so
this patch exposes and reuses that code. Let me know if anything is unclear.

[chrishtr, 2015-10-02 21:53:03]

https://codereview.chromium.org/1361413004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/DragImage.h (right):

https://codereview.chromium.org/1361413004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/DragImage.h:72: PassRefPtr<SkImage>
adjustedImage(PassRefPtr<SkImage>, const IntSize&, const AffineTransform&, float
opacity, InterpolationQuality = InterpolationNone);
This is not a descriptive enough method name. Also, please attach it as a method
on some class.

[rwlbuis, 2015-10-05 22:52:38]

On 2015/10/02 21:53:03, chrishtr wrote:
>
https://codereview.chromium.org/1361413004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/DragImage.h (right):
> 
>
https://codereview.chromium.org/1361413004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/DragImage.h:72: PassRefPtr<SkImage>
> adjustedImage(PassRefPtr<SkImage>, const IntSize&, const AffineTransform&,
float
> opacity, InterpolationQuality = InterpolationNone);
> This is not a descriptive enough method name. Also, please attach it as a
method
> on some class.

Good point, I tried to address this in patch set#5, PTAL.

[chrishtr, 2015-10-05 23:19:46]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-10-05 23:19:47]

lgtm

https://codereview.chromium.org/1361413004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/DragImage.h (right):

https://codereview.chromium.org/1361413004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/DragImage.h:35: #include
"third_party/skia/include/core/SkImage.h"
Instead of #include, forward-declare SkImage.

[chrishtr, 2015-10-05 23:19:47]

The patchset sent to the CQ was uploaded after l-g-t-m from noel@chromium.org
Link to the patchset: https://codereview.chromium.org/1361413004/#ps80001
(title: "Address review comments")

[chrishtr, 2015-10-05 23:20:04]

The CQ bit was unchecked by chrishtr@chromium.org

[Noel Gordon, 2015-10-06 04:10:34]

The CQ bit was checked by noel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-06 04:10:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361413004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361413004/80001

[commit-bot: I haz the power, 2015-10-06 05:37:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-06 05:37:28]

Dry run: This issue passed the CQ dry run.

[rwlbuis, 2015-10-06 14:43:15]

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

[rwlbuis, 2015-10-06 14:43:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, noel@chromium.org
Link to the patchset: https://codereview.chromium.org/1361413004/#ps100001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-10-06 14:43:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361413004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361413004/100001

[rwlbuis, 2015-10-06 14:45:00]

Patchset #7 (id:120001) has been deleted

[rwlbuis, 2015-10-06 17:41:08]

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

[rwlbuis, 2015-10-06 17:41:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, noel@chromium.org
Link to the patchset: https://codereview.chromium.org/1361413004/#ps100001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-10-06 17:41:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361413004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361413004/100001

[commit-bot: I haz the power, 2015-10-06 18:23:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-06 18:24:16]

Patchset 6 (id:??) landed as
https://crrev.com/8cac6805bdac3a41ac76a72e9e1e58d26105895c
Cr-Commit-Position: refs/heads/master@{#352647}