Add support for the object-fit CSS property.

Add support for the object-fit CSS property.

This is hidden behind an "experimental" runtime flag named
"ObjectFitPosition". Once object-fit's "sister" property, object-position,
is implemented, the plan is to also put that behind this flag, too.

This is an implementation of object-fit as described in
http://www.w3.org/TR/2012/CR-css3-images-20120417/#object-fit

Object-fit is used to maintain the aspect ratio of replaced content
within its content box. All object-fit values except the initial one
('fill') will always ensure that the aspect ratio is retained, in
different ways (fit inside the content box, cover the content box, or
use intrinsic size). Painting is always clipped against the content
box, regardless of the 'overflow' property.

Video elements used to behave like 'object-fit:contain', while the
initial value is, as mentioned above, 'fill'. Therefore we need a
section for video in the UA style sheet.

Introducing RenderImageResource::intrinsicSize(). We cannot use
imageSize(), since it may return the extrinsic size for SVG images.

Parts of this patch (both code and layout tests) are based on work
originally done by Simon Fraser <simon.fraser@apple.com>;.
See https://bugs.webkit.org/show_bug.cgi?id=52040

BUG=236331
R=pdr@chromium.org

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

[mstensho (USE GERRIT), 2013-08-07 14:16:27]

Ojan, could this be something for you?
If not, feel free to suggest another reviewer. :)

[ojan, 2013-08-08 02:36:54]

I've done a first pass through the review. It mostly looks good. I need to tak a
big of a closer look at the paint changes before I approve, but I figured I'd
get these minor comments in your hands before tomorrow.

Phillip maybe you could do a quick sanity check over the painting code here? I
think you know more about painting in general than I do.

https://codereview.chromium.org/22482004/diff/5002/LayoutTests/fast/css/parsi...
File LayoutTests/fast/css/parsing-object-fit.html (right):

https://codereview.chromium.org/22482004/diff/5002/LayoutTests/fast/css/parsi...
LayoutTests/fast/css/parsing-object-fit.html:10: <script
src="script-tests/parsing-object-fit.js"></script>
We're moving away from using script-tests wherever they're not pure JS tests
(i.e. can be run solely in the JS VM). Having the actual test content be outside
the test file adds extra maintenance overhead for that file.

You can still use js-test-pre/js-test-post. Just inline the JS here.

Also, you don't need the description/console/link elements, those get added for
you. And then you can make the doctype the usual HTML5 one.

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
File Source/core/platform/graphics/LayoutSize.h (right):

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
Source/core/platform/graphics/LayoutSize.h:174: // (expressed as a size, too).
Blink (and WebKit) style is to leave out "what" comments that say what the code
does. Comments are for "why" explanations and for high-level descriptions (e.g.
discussing how a class is intended to be used).

I think all these comment (including the one in fitLayoutSizeToAspectRatio) just
say what the code obviously does, so they can be left out.

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
Source/core/platform/graphics/LayoutSize.h:178: };
Needs another line-break here.

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
Source/core/platform/graphics/LayoutSize.h:179: inline LayoutSize
fitLayoutSizeToAspectRatio(const LayoutSize& scaledSize, const LayoutSize&
aspectRatio, AspectRatioFit fit)
I think this method makes more sense as a member of LayoutSize. It's similar to
expandedTo or transposedSize.

[mstensho (USE GERRIT), 2013-08-08 07:26:42]

https://codereview.chromium.org/22482004/diff/5002/LayoutTests/fast/css/parsi...
File LayoutTests/fast/css/parsing-object-fit.html (right):

https://codereview.chromium.org/22482004/diff/5002/LayoutTests/fast/css/parsi...
LayoutTests/fast/css/parsing-object-fit.html:10: <script
src="script-tests/parsing-object-fit.js"></script>
On 2013/08/08 02:36:55, ojan wrote:
> We're moving away from using script-tests wherever they're not pure JS tests
> (i.e. can be run solely in the JS VM). Having the actual test content be
outside
> the test file adds extra maintenance overhead for that file.
> 
> You can still use js-test-pre/js-test-post. Just inline the JS here.
> 
> Also, you don't need the description/console/link elements, those get added
for
> you. And then you can make the doctype the usual HTML5 one.

Done.

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
File Source/core/platform/graphics/LayoutSize.h (right):

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
Source/core/platform/graphics/LayoutSize.h:174: // (expressed as a size, too).
On 2013/08/08 02:36:55, ojan wrote:
> Blink (and WebKit) style is to leave out "what" comments that say what the
code
> does. Comments are for "why" explanations and for high-level descriptions
(e.g.
> discussing how a class is intended to be used).
> 
> I think all these comment (including the one in fitLayoutSizeToAspectRatio)
just
> say what the code obviously does, so they can be left out.

Done.

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
Source/core/platform/graphics/LayoutSize.h:178: };
On 2013/08/08 02:36:55, ojan wrote:
> Needs another line-break here.

Done.

https://codereview.chromium.org/22482004/diff/5002/Source/core/platform/graph...
Source/core/platform/graphics/LayoutSize.h:179: inline LayoutSize
fitLayoutSizeToAspectRatio(const LayoutSize& scaledSize, const LayoutSize&
aspectRatio, AspectRatioFit fit)
On 2013/08/08 02:36:55, ojan wrote:
> I think this method makes more sense as a member of LayoutSize. It's similar
to
> expandedTo or transposedSize.

Done.

[ojan, 2013-08-09 23:27:26]

I couple more minor things. I've enlisted pdr to make sure the paint side of
this patch is correct.

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderReplaced.cpp (right):

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderReplaced.cpp:314: LayoutRect
RenderReplaced::replacedContentRect(const LayoutSize* overriddenIntrinsicSize)
const
I don't think we typically use pointers to LayoutSizes. This should be "const
LayoutSize&".

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderReplaced.cpp:341: // This is where object-position
should be taken into account, but since it's not implemented
This should be:
// FIXME: This is where...

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyleConstants.h:209: enum EObjectFit {
ObjectFitFill, ObjectFitContain, ObjectFitCover, ObjectFitNone,
ObjectFitScaleDown };
s/EObjectFit/ObjectFit/. We're moving away from the "E" prefix naming
convention.

[pdr., 2013-08-09 23:39:21]

Just two light comments. Overall this is quite nice.

I like the implementation of intrinsicSize but I wonder if we should rename (or
at least add comments) that the other size() methods return extrinsic sizes?

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:393: paintIntoRect(context, paintRect);
In general, we try to avoid clipping because it's quite expensive for software
rasterizers.

In this case, paintIntoRect ends up calling context->drawImage which takes a
source and dest rect. Can you plumb this through instead of clipping?

[mstensho (USE GERRIT), 2013-08-12 13:37:36]

On 2013/08/09 23:39:21, pdr wrote:
> I like the implementation of intrinsicSize but I wonder if we should rename
(or
> at least add comments) that the other size() methods return extrinsic sizes?

But it doesn't really return extrinsic sizes, except if it's an SVG. As far as I
found, all images but SVGs will return the intrinsic size from the other method
as well. This is pretty confusing.

[mstensho (USE GERRIT), 2013-08-12 13:39:10]

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:393: paintIntoRect(context, paintRect);
On 2013/08/09 23:39:22, pdr wrote:
> In general, we try to avoid clipping because it's quite expensive for software
> rasterizers.
> 
> In this case, paintIntoRect ends up calling context->drawImage which takes a
> source and dest rect. Can you plumb this through instead of clipping?

Done.

Please take a look at what I did. I was considering adding a paint offset
LayoutPoint parameter (and untranslated rectangle(s)), but decided against it in
the end, and instead used what paintReplaced() (and the other caller in
RenderVideo) calculates right away. That may be a bit more convoluted.
Especially the "srcRect.moveBy(-paintRect.location())" thing may cause some
eyebrow-raising. Not sure.

If we let paintIntoRect() take a LayoutPoint offset + an untranslated paint
rectangle instead, we could consider letting paintIntoRect() calculate the
content rectangle on its own.

My current solution results in fewer code lines and calculations, since it is
slightly more coupled against what paintReplaced() already calculates.

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderReplaced.cpp (right):

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderReplaced.cpp:314: LayoutRect
RenderReplaced::replacedContentRect(const LayoutSize* overriddenIntrinsicSize)
const
On 2013/08/09 23:27:26, ojan wrote:
> I don't think we typically use pointers to LayoutSizes. This should be "const
> LayoutSize&".

But it's an optional parameter, currently only specified by RenderVideo. Or do
you think it would it be better to pass a zero LayoutSize object in the other
cases, although that's more expensive?

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderReplaced.cpp:341: // This is where object-position
should be taken into account, but since it's not implemented
On 2013/08/09 23:27:26, ojan wrote:
> This should be:
> // FIXME: This is where...

Done.

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyleConstants.h:209: enum EObjectFit {
ObjectFitFill, ObjectFitContain, ObjectFitCover, ObjectFitNone,
ObjectFitScaleDown };
On 2013/08/09 23:27:26, ojan wrote:
> s/EObjectFit/ObjectFit/. We're moving away from the "E" prefix naming
> convention.

Done.

[ojan, 2013-08-12 17:25:56]

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderReplaced.cpp (right):

https://codereview.chromium.org/22482004/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderReplaced.cpp:314: LayoutRect
RenderReplaced::replacedContentRect(const LayoutSize* overriddenIntrinsicSize)
const
On 2013/08/12 13:39:11, Morten Stenshorne wrote:
> On 2013/08/09 23:27:26, ojan wrote:
> > I don't think we typically use pointers to LayoutSizes. This should be
"const
> > LayoutSize&".
> 
> But it's an optional parameter, currently only specified by RenderVideo. Or do
> you think it would it be better to pass a zero LayoutSize object in the other
> cases, although that's more expensive?

Ah right. Nevermind. What you have here is fine.

[pdr., 2013-08-14 22:00:53]

Looking great. Lets do one more round on the RenderImage.cpp part.

https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:254: updateInnerContentRect();
I don't understand why this is needed. Layout must occur before paint. Layout
should update the inner content rect. If we don't need layout, the inner content
rect should be correct already.

https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:486: // cheaper than applying a clip
rectangle.
This is tough code and, while we both understand it, I worry that the next
person that finds this code will have difficulty. You've extracted out the
common case when the entire image is visible which is great. I wonder if we
should clip in the less-common case instead of using 5 lines of pretty difficult
src/dest rect math? This is just an idea.

[mstensho (USE GERRIT), 2013-08-15 15:14:01]

https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:254: updateInnerContentRect();
On 2013/08/14 22:00:54, pdr wrote:
> I don't understand why this is needed. Layout must occur before paint. Layout
> should update the inner content rect. If we don't need layout, the inner
content
> rect should be correct already.

If you lay out before you have received the image, and the image arrives later,
you need to update the "container size" of the image resource. Afterwards you
need to repaint. This only matters for SVG images, though.

So this is only interesting when resource loading is slow, and only for SVG
images. I've made a test case here, which fails if you remove the code:

http://bitten.servebeer.com/tmp/object-fit-img-svg.html

I don't know. Could be better to just ask for another layout when this happens,
if object-fit is non-initial. I'm not sure what feels more convoluted. It's not
the RenderImage itself that needs layout, but something SVG-specific in the
image cache. Although we do specify the correct desired size of the image when
calling context->drawImage(), SVG doesn't seem to respect this, but rather uses
its stored "container size". Another option would be to simply update the
"container size" during painting (and only then), but that sounds wasteful to
me.

https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:486: // cheaper than applying a clip
rectangle.
On 2013/08/14 22:00:54, pdr wrote:
> This is tough code and, while we both understand it, I worry that the next
> person that finds this code will have difficulty. You've extracted out the
> common case when the entire image is visible which is great. I wonder if we
> should clip in the less-common case instead of using 5 lines of pretty
difficult
> src/dest rect math? This is just an idea.

Sure, why not? But this is essentially what I did in the first version of the
patch. :) I only clipped if the content box didn't contain the entire image. So
I revert this piece of rocket surgery, then?

I too suspect that what I did here is more complicated than it's worth, and that
it probably solves a problem that doesn't exist (at least not yet, obviously) (I
suppose it could make a difference if you have many object-fitted images and
clipping performance is poor, but I don't know).

[pdr., 2013-08-15 16:15:36]

On 2013/08/15 15:14:01, Morten Stenshorne wrote:
>
https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderImage.cpp (right):
> 
>
https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
> Source/core/rendering/RenderImage.cpp:254: updateInnerContentRect();
> On 2013/08/14 22:00:54, pdr wrote:
> > I don't understand why this is needed. Layout must occur before paint.
Layout
> > should update the inner content rect. If we don't need layout, the inner
> content
> > rect should be correct already.
> 
> If you lay out before you have received the image, and the image arrives
later,
> you need to update the "container size" of the image resource. Afterwards you
> need to repaint. This only matters for SVG images, though.

This should already be handled. Here's how it should work:
When you request an image, you create an ImageResource that doesn't yet contain
the actual image. When you layout, you set the size on this image resource
(ImageResource::setContainerSizeForRenderer) which stores it temporarily until
the actual image loads. Once the actual image loads, the image resource sends
the container size to it.

> 
> So this is only interesting when resource loading is slow, and only for SVG
> images. I've made a test case here, which fails if you remove the code:
> 
> http://bitten.servebeer.com/tmp/object-fit-img-svg.html
> 
> I don't know. Could be better to just ask for another layout when this
happens,
> if object-fit is non-initial. I'm not sure what feels more convoluted. It's
not
> the RenderImage itself that needs layout, but something SVG-specific in the
> image cache. Although we do specify the correct desired size of the image when
> calling context->drawImage(), SVG doesn't seem to respect this, but rather
uses
> its stored "container size". Another option would be to simply update the
> "container size" during painting (and only then), but that sounds wasteful to
> me.

Can you add this as a test? Layouttests/http/svg/resources/delayCachedLoad.cpp
has similar infrastructure as your test for testing this.

> 
>
https://codereview.chromium.org/22482004/diff/27001/Source/core/rendering/Ren...
> Source/core/rendering/RenderImage.cpp:486: // cheaper than applying a clip
> rectangle.
> On 2013/08/14 22:00:54, pdr wrote:
> > This is tough code and, while we both understand it, I worry that the next
> > person that finds this code will have difficulty. You've extracted out the
> > common case when the entire image is visible which is great. I wonder if we
> > should clip in the less-common case instead of using 5 lines of pretty
> difficult
> > src/dest rect math? This is just an idea.
> 
> Sure, why not? But this is essentially what I did in the first version of the
> patch. :) I only clipped if the content box didn't contain the entire image.
So
> I revert this piece of rocket surgery, then?
> 
> I too suspect that what I did here is more complicated than it's worth, and
that
> it probably solves a problem that doesn't exist (at least not yet, obviously)
(I
> suppose it could make a difference if you have many object-fitted images and
> clipping performance is poor, but I don't know).

Yeah, this is my fault for sending you down that path :/ You're not clipping in
the common case though, which is good.

[mstensho (USE GERRIT), 2013-08-15 18:44:47]

On 2013/08/15 16:15:36, pdr wrote:
> This should already be handled. Here's how it should work:
> When you request an image, you create an ImageResource that doesn't yet
contain
> the actual image. When you layout, you set the size on this image resource
> (ImageResource::setContainerSizeForRenderer) which stores it temporarily until
> the actual image loads. Once the actual image loads, the image resource sends
> the container size to it.

Sounds reasonable. So why doesn't it work, then?

> > http://bitten.servebeer.com/tmp/object-fit-img-svg.html
> > [...]
> 
> Can you add this as a test? Layouttests/http/svg/resources/delayCachedLoad.cpp
> has similar infrastructure as your test for testing this.

LayoutTests/http/tests/svg/resources/delayCachedLoad.php

Yes, I can do that.

[pdr., 2013-08-17 02:06:17]

On 2013/08/15 18:44:47, Morten Stenshorne wrote:
> On 2013/08/15 16:15:36, pdr wrote:
> > This should already be handled. Here's how it should work:
> > When you request an image, you create an ImageResource that doesn't yet
> contain
> > the actual image. When you layout, you set the size on this image resource
> > (ImageResource::setContainerSizeForRenderer) which stores it temporarily
until
> > the actual image loads. Once the actual image loads, the image resource
sends
> > the container size to it.
> 
> Sounds reasonable. So why doesn't it work, then?

Thanks for updating this patch. I think this new patch looks good except the
image size issue. Can you look into why the process I described isn't happening?


@Ojan, can you do a final review of this as well?

[mstensho (USE GERRIT), 2013-08-19 14:46:38]

On 2013/08/17 02:06:17, pdr wrote:
> On 2013/08/15 18:44:47, Morten Stenshorne wrote:
> > On 2013/08/15 16:15:36, pdr wrote:
> > > This should already be handled. Here's how it should work:
> > > When you request an image, you create an ImageResource that doesn't yet
> > contain
> > > the actual image. When you layout, you set the size on this image resource
> > > (ImageResource::setContainerSizeForRenderer) which stores it temporarily
> until
> > > the actual image loads. Once the actual image loads, the image resource
> sends
> > > the container size to it.
> > 
> > Sounds reasonable. So why doesn't it work, then?
> 
> Thanks for updating this patch. I think this new patch looks good except the
> image size issue. Can you look into why the process I described isn't
happening?

The problem seems to be related to the list of pending size requests - oh, that
was added by you! :) The list of pending size requests is consumed in
ImageResource::createImage() when the image has arrived, but the pending size
request was issued too early from RenderImage::layout(), before the image had
loaded, so that the actual content box size was used (rather than something that
respects the aspect ratio), since the intrinsic size/ratio wasn't known yet. So
you end up with an SVGImageForContainer object with incorrect dimensions.

Perhaps the pending list should only contain the renderers, and that we ask the
renderers over again to calculate this information when it's time?
Or, hmm... that's basically what my patch does. Does my change in
RenderImage::imageDimensionsChanged() actually eliminate the need for the
pending list altogether? http/tests/misc/svg-image-delayed-size-negotiation.html
passes, at least. http/tests/svg/cached-image-sizing.html too.

[pdr., 2013-08-19 17:16:04]

On 2013/08/19 14:46:38, Morten Stenshorne wrote:
> On 2013/08/17 02:06:17, pdr wrote:
> > On 2013/08/15 18:44:47, Morten Stenshorne wrote:
> > > On 2013/08/15 16:15:36, pdr wrote:
> > > > This should already be handled. Here's how it should work:
> > > > When you request an image, you create an ImageResource that doesn't yet
> > > contain
> > > > the actual image. When you layout, you set the size on this image
resource
> > > > (ImageResource::setContainerSizeForRenderer) which stores it temporarily
> > until
> > > > the actual image loads. Once the actual image loads, the image resource
> > sends
> > > > the container size to it.
> > > 
> > > Sounds reasonable. So why doesn't it work, then?
> > 
> > Thanks for updating this patch. I think this new patch looks good except the
> > image size issue. Can you look into why the process I described isn't
> happening?
> 
> The problem seems to be related to the list of pending size requests - oh,
that
> was added by you! :) The list of pending size requests is consumed in
> ImageResource::createImage() when the image has arrived, but the pending size
> request was issued too early from RenderImage::layout(), before the image had
> loaded, so that the actual content box size was used (rather than something
that
> respects the aspect ratio), since the intrinsic size/ratio wasn't known yet.
So
> you end up with an SVGImageForContainer object with incorrect dimensions.
> 
> Perhaps the pending list should only contain the renderers, and that we ask
the
> renderers over again to calculate this information when it's time?
> Or, hmm... that's basically what my patch does. Does my change in
> RenderImage::imageDimensionsChanged() actually eliminate the need for the
> pending list altogether?
http/tests/misc/svg-image-delayed-size-negotiation.html
> passes, at least. http/tests/svg/cached-image-sizing.html too.

I'm not convinced this is correct, but I don't think we should hold this patch
up just for this minor issue. What do you think about landing this as-is, and
then addressing this issue as a followup? If that sounds reasonable to you, just
add "FIXME(crbug.com/12345): we should not setContainerSize here." in
updateInnerContentRect.

[pdr., 2013-08-21 05:43:11]

LGTM

[commit-bot: I haz the power, 2013-08-21 05:43:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/22482004/62001

[commit-bot: I haz the power, 2013-08-21 05:44:02]

Can't process patch for file
LayoutTests/fast/css/resources/circles-portrait-small.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[pdr., 2013-08-21 05:48:46]

On 2013/08/21 05:44:02, I haz the power (commit-bot) wrote:
> Can't process patch for file
> LayoutTests/fast/css/resources/circles-portrait-small.png.
> Binary file support is temporarilly disabled due to a bug. Please commit
blindly
> the binary files first then commit the source change as a separate CL. Sorry
for
> the annoyance.

Oh, darn.
My understanding is you need to add [NeedsRebaseline] to the TestExpectations
file and let the autorebaseline bot generate the binary png images for you. This
automatically handles the cross-platform expectations.

See this post for details:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/J4i52waev9w

[mstensho (USE GERRIT), 2013-08-21 06:09:42]

On 2013/08/21 05:48:46, pdr wrote:
> On 2013/08/21 05:44:02, I haz the power (commit-bot) wrote:
> > Can't process patch for file
> > LayoutTests/fast/css/resources/circles-portrait-small.png.
> > Binary file support is temporarilly disabled due to a bug. Please commit
> blindly
> > the binary files first then commit the source change as a separate CL. Sorry
> for
> > the annoyance.
> 
> Oh, darn.
> My understanding is you need to add [NeedsRebaseline] to the TestExpectations
> file and let the autorebaseline bot generate the binary png images for you.
This
> automatically handles the cross-platform expectations.
> 
> See this post for details:
> https://groups.google.com/a/chromium.org/forum/#%21topic/blink-dev/J4i52waev9w

But none of the png files are expectations; they are resources used in tests. I
think someone with commit privileges need to do what the bot says; i.e. commit
them blindly. Then, perhaps the bot could take care of the rest and refuse to
commit the source changes if they happen to break any tests?

[pdr., 2013-08-21 06:23:55]

On 2013/08/21 06:09:42, Morten Stenshorne wrote:
> On 2013/08/21 05:48:46, pdr wrote:
> > On 2013/08/21 05:44:02, I haz the power (commit-bot) wrote:
> > > Can't process patch for file
> > > LayoutTests/fast/css/resources/circles-portrait-small.png.
> > > Binary file support is temporarilly disabled due to a bug. Please commit
> > blindly
> > > the binary files first then commit the source change as a separate CL.
Sorry
> > for
> > > the annoyance.
> > 
> > Oh, darn.
> > My understanding is you need to add [NeedsRebaseline] to the
TestExpectations
> > file and let the autorebaseline bot generate the binary png images for you.
> This
> > automatically handles the cross-platform expectations.
> > 
> > See this post for details:
> >
https://groups.google.com/a/chromium.org/forum/#%2521topic/blink-dev/J4i52waev9w
> 
> But none of the png files are expectations; they are resources used in tests.
I
> think someone with commit privileges need to do what the bot says; i.e. commit
> them blindly. Then, perhaps the bot could take care of the rest and refuse to
> commit the source changes if they happen to break any tests?

I completely misread that. You're right.

I will commit this tomorrow.

[pdr., 2013-08-22 03:33:38]

Committed patchset #9 manually as r156535 (presubmit successful).