Merge image sizing algorithms

Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939, 592888, 592886
Committed: https://crrev.com/3c7b1342b32dd6174efa9a47a4b476c940462b9c
Cr-Commit-Position: refs/heads/master@{#379801}

[davve, 2016-03-03 12:16:21]

Description was changed from

==========
Images sizing algorithm merge

SVGImage::calculateConcreteObjectSize almost had the necessary bits to
support full sizing of SVG images within Style/CSS context. Only the
contain constraint on the default object size that this patch adds was
missing.

This enables removing one sizing algorithm in LayoutBoxModelObject by
re-using the one in SVGImage instead, for the cases where such an
algorithm is needed. For generated images, the default object size is
returned directly.

Some zoom juggling needed since the provided default object size is
zoomed and SVGImage has no notion of zoom. Thus the zoom is removed
before calling SVGImage::calculateConcreteObjectSize() and re-applied
on the result afterwards.

BUG=581357
==========

to

==========
Merge image sizing algorithms

SVGImage::calculateConcreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead. For generated images with no fixed size, the
default object size is returned unmodified.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::calculateConcreteObjectSize() and
reapplied on the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

Another necessary change now that decorative images such as background
and border-image now use StyleImage::imageSize() is that the
StyleFetchedImageSet implementation must return the real image size,
not the one compensated by image scale factor. The compensation of
scale factor is moved up a level to
LayoutImageResourceStyleImage::imageSize() so that content images
continue to behave as before.

BUG=581357
==========

[davve, 2016-03-03 12:16:39]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-03 12:16:51]

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

[davve, 2016-03-03 13:43:53]

Description was changed from

==========
Merge image sizing algorithms

SVGImage::calculateConcreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead. For generated images with no fixed size, the
default object size is returned unmodified.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::calculateConcreteObjectSize() and
reapplied on the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

Another necessary change now that decorative images such as background
and border-image now use StyleImage::imageSize() is that the
StyleFetchedImageSet implementation must return the real image size,
not the one compensated by image scale factor. The compensation of
scale factor is moved up a level to
LayoutImageResourceStyleImage::imageSize() so that content images
continue to behave as before.

BUG=581357
==========

to

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::calculateConcreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::calculateConcreteObjectSize() and
reapplied on the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

Another necessary change now that decorative images such as background
and border-image now use StyleImage::imageSize() is that the
StyleFetchedImageSet implementation must return the real image size,
not the one compensated by image scale factor. The compensation of
scale factor is moved up a level to
LayoutImageResourceStyleImage::imageSize() so that content images
continue to behave as before.

BUG=581357
==========

[commit-bot: I haz the power, 2016-03-03 13:44:22]

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

[commit-bot: I haz the power, 2016-03-03 13:44:23]

Dry run: This issue passed the CQ dry run.

[davve, 2016-03-03 13:59:14]

davve@opera.com changed reviewers:
+ fs@opera.com, pdr@chromium.org, yoav@yoav.ws

[davve, 2016-03-03 13:59:15]

I think this is starting to get ready for review now.

+yoav: I'm very uncertain about the StyleFetchedImageSet.cpp change, especially
removing the scale factor adjustment. I fully expect you Yoav to tell me how
wrong I am here, but if StyleFetchedImageSet::imageSize is to be used to size
things other than content images, _some_ change seems to be required here.

Also, Yoav I sense that you might mind my change to
shouldRespectImageOrientation in StyleFetchedImage(Set)::imageSize() too. So
make sure to speak up there if you have a better solution/suggestion.

+fs, +pdr: This replaces my previous attempts (dropped) at making the default
object size of the css images spec more prominent and less implicit in the Blink
code. An end goal is to give CanvasImageSource implementation the same treatment
(as given to StyleImage in this patch), so that we can finally have proper
sizing of SVG in drawImage too. But that's for a later CL.

Another thing is that Image::computeIntrinsicDimensions almost becomes unused by
this patch. I have https://codereview.chromium.org/1720853002 lined up to remove
it.

This fixed at least two unreported bugs: One with zooming and list-item markers
and one with cross-faded svg images. I'll file them later today or tomorrow and
update the description.

[Yoav Weiss, 2016-03-03 14:50:48]

On 2016/03/03 13:59:15, David Vest wrote:
> I think this is starting to get ready for review now.
> 
> +yoav: I'm very uncertain about the StyleFetchedImageSet.cpp change,
especially
> removing the scale factor adjustment. I fully expect you Yoav to tell me how
> wrong I am here, but if StyleFetchedImageSet::imageSize is to be used to size
> things other than content images, _some_ change seems to be required here.
> 
> Also, Yoav I sense that you might mind my change to
> shouldRespectImageOrientation in StyleFetchedImage(Set)::imageSize() too. So
> make sure to speak up there if you have a better solution/suggestion.

I'm currently on the roads, but I'll do my best to review this by early next
week. (If you don't hear from me by Monday afternoon, feel free to ping me here
or on IRC).

> 
> +fs, +pdr: This replaces my previous attempts (dropped) at making the default
> object size of the css images spec more prominent and less implicit in the
Blink
> code. An end goal is to give CanvasImageSource implementation the same
treatment
> (as given to StyleImage in this patch), so that we can finally have proper
> sizing of SVG in drawImage too. But that's for a later CL.
> 
> Another thing is that Image::computeIntrinsicDimensions almost becomes unused
by
> this patch. I have https://codereview.chromium.org/1720853002 lined up to
remove
> it.
> 
> This fixed at least two unreported bugs: One with zooming and list-item
markers
> and one with cross-faded svg images. I'll file them later today or tomorrow
and
> update the description.

[fs, 2016-03-03 15:22:54]

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h
(right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h:58:
scaledImageSize.scale(1 / m_styleImage->imageScaleFactor());
Won't this double-scale if m_styleImage is an image-set?

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:143: &&
!layer.image()->imageSize(&imageClient, imageClient.style()->effectiveZoom(),
LayoutSize()).isEmpty()
Not your doing, and no action necessarily required, but in many cases
canRender() above will end up in ImageResource::canRender which checks this
predicate - just without the default object size. So I'm guessing that we'd
rather want to be able to use canRender() alone to determine this (I'm guessing
it won't work though because not all subclasses implement the method in a
compatible manner AFAICS.)

Regardless though, passing effective zoom seems redundant.

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:101: // Don't let
images that have a width/height >= 1 shrink below 1 when zoomed.
Maybe we could put this sequence as a helper on StyleImage or something like
that. (Hmm, maybe the whole block even...)

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:112: const
RespectImageOrientationEnum shouldRespectImageOrientation =
DoNotRespectImageOrientation;
Nit: Fold this into the call? (Comment alone seems sufficient to notice this?
Unless I'm missing something...)

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:115: const
RespectImageOrientationEnum shouldRespectImageOrientation =
DoNotRespectImageOrientation;
Same as above.

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:124: // SVGImage.
SVGImageForContainer carried the final image size,
Nit: s/carried/carries/?

[fs, 2016-03-03 15:24:43]

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h
(right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h:58:
scaledImageSize.scale(1 / m_styleImage->imageScaleFactor());
On 2016/03/03 at 15:22:53, fs wrote:
> Won't this double-scale if m_styleImage is an image-set?

Ah, no, that code was no longer. You can disregard this then.

[davve, 2016-03-04 06:49:43]

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:143: &&
!layer.image()->imageSize(&imageClient, imageClient.style()->effectiveZoom(),
LayoutSize()).isEmpty()
On 2016/03/03 15:22:53, fs wrote:
> Not your doing, and no action necessarily required, but in many cases
> canRender() above will end up in ImageResource::canRender which checks this
> predicate - just without the default object size. So I'm guessing that we'd
> rather want to be able to use canRender() alone to determine this (I'm
guessing
> it won't work though because not all subclasses implement the method in a
> compatible manner AFAICS.)

Good observation. Using canRender() alone would be the reasonable thing here.

> Regardless though, passing effective zoom seems redundant.

I'll queue a CL for getting this of at least this part.

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:101: // Don't let
images that have a width/height >= 1 shrink below 1 when zoomed.
On 2016/03/03 15:22:53, fs wrote:
> Maybe we could put this sequence as a helper on StyleImage or something like
> that. (Hmm, maybe the whole block even...)

Yes, that probably good. Even prior to this, there are some really good code
sharing opportunities between StyleFetchedImage and StyleFetchedImageSet.

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:112: const
RespectImageOrientationEnum shouldRespectImageOrientation =
DoNotRespectImageOrientation;
On 2016/03/03 15:22:53, fs wrote:
> Nit: Fold this into the call? (Comment alone seems sufficient to notice this?
> Unless I'm missing something...)

Done.

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:115: const
RespectImageOrientationEnum shouldRespectImageOrientation =
DoNotRespectImageOrientation;
On 2016/03/03 15:22:53, fs wrote:
> Same as above.

Done.

[davve, 2016-03-04 07:12:35]

Description was changed from

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::calculateConcreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::calculateConcreteObjectSize() and
reapplied on the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

Another necessary change now that decorative images such as background
and border-image now use StyleImage::imageSize() is that the
StyleFetchedImageSet implementation must return the real image size,
not the one compensated by image scale factor. The compensation of
scale factor is moved up a level to
LayoutImageResourceStyleImage::imageSize() so that content images
continue to behave as before.

BUG=581357
==========

to

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::calculateConcreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::calculateConcreteObjectSize() and
reapplied on the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

Another necessary change now that decorative images such as background
and border-image now use StyleImage::imageSize() is that the
StyleFetchedImageSet implementation must return the real image size,
not the one compensated by image scale factor. The compensation of
scale factor is moved up a level to
LayoutImageResourceStyleImage::imageSize() so that content images
continue to behave as before.

BUG=581357, 591935, 591939
==========

[fs, 2016-03-04 09:00:56]

On 2016/03/04 at 06:49:43, davve wrote:
...
>
https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right):
> 
>
https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:101: // Don't let
images that have a width/height >= 1 shrink below 1 when zoomed.
> On 2016/03/03 15:22:53, fs wrote:
> > Maybe we could put this sequence as a helper on StyleImage or something like
> > that. (Hmm, maybe the whole block even...)
> 
> Yes, that probably good. Even prior to this, there are some really good code
sharing opportunities between StyleFetchedImage and StyleFetchedImageSet.

Looks nice!

LGTM

https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp:52: FloatSize
unzoomedDefaultObjectSize(defaultObjectSize);
(Strange as it may sound/feel, but this looks like it could use the code in the
new helper too =))

[davve, 2016-03-04 13:02:56]

https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp:52: FloatSize
unzoomedDefaultObjectSize(defaultObjectSize);
On 2016/03/04 09:00:56, fs wrote:
> (Strange as it may sound/feel, but this looks like it could use the code in
the
> new helper too =))

I'm not sure what you refer to by 'this' here but I dialed up the code sharing a
notch by sharing at least the zooming code.

[fs, 2016-03-04 13:06:57]

https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp:52: FloatSize
unzoomedDefaultObjectSize(defaultObjectSize);
On 2016/03/04 at 13:02:56, David Vest wrote:
> On 2016/03/04 09:00:56, fs wrote:
> > (Strange as it may sound/feel, but this looks like it could use the code in
the
> > new helper too =))
> 
> I'm not sure what you refer to by 'this' here but I dialed up the code sharing
a notch by sharing at least the zooming code.

Yes, that was the 'this' I was referring too ("this method"). Even nicer!

[Yoav Weiss, 2016-03-07 11:16:07]

Hey David!

Comments below. I don't mind the orientation change, but it'd be good to make
sure that this is what the spec says (so there won't be any compat issues) and
to test it in order to make sure it behaves the way we want it to behave.
Regarding the image-set changes, I think you'd want to take the scale factor
into account and add tests that make sure that it stays that way.

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:101: // border-image,
etc.)
Could you point to a spec indicating that this is the right behavior? Do we have
tests that cover that behavior change?

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:103: //
border-image, etc.)
Here too, pointing to a spec and testing functionality change would be good.

Otherwise, I believe you would need to take m_imageScaleFactor into account in
order to make sure that if `-webkit-image-set(url(1x.jpg) 1x, url(2x.jpg) 2x)`
loads the 2x resource, it won't be displayed at twice the size that it's
supposed to be displayed in.

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleImage.cpp:20: // Don't let images that
have a width/height >= 1 shrink below 1 when zoomed.
Do we have tests for this?

[davve, 2016-03-07 13:07:38]

Replying with what I know about what's in LayoutTests already.

Will look more into testing for zooming below 1x1 and for image sets.

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:101: // border-image,
etc.)
On 2016/03/07 11:16:07, Yoav Weiss wrote:
> Could you point to a spec indicating that this is the right behavior? Do we
have
> tests that cover that behavior change?

https://drafts.csswg.org/css-images-3/#the-image-orientation

" ... This property specifies an orthogonal rotation to be applied to an image
before it is laid out. It applies only to content images (e.g. replaced elements
and generated content), not decorative images (such as background-image)."

fast/images/exif-orientation-css.html tests background-image, but other
decorative images not covered well, AFAIK.

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:103: //
border-image, etc.)
On 2016/03/07 11:16:07, Yoav Weiss wrote:
> Here too, pointing to a spec and testing functionality change would be good.

You mean add the spec link as a comment? I'll do that.
 
> Otherwise, I believe you would need to take m_imageScaleFactor into account in
> order to make sure that if `-webkit-image-set(url(1x.jpg) 1x, url(2x.jpg) 2x)`
> loads the 2x resource, it won't be displayed at twice the size that it's
> supposed to be displayed in.

The tests:

fast/hidpi/image-set-background-repeat-without-size.html
fast/hidpi/image-set-border-image-comparison.html

break when the scale factor is compensated for here (since imageSize() now used
for backgrounds and border-image. Previously the background and border-image
code used Image::computeIntrinsicDimensions and did no such compensation).

Do you know of a test that probably would break with my patch?

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleImage.cpp (right):

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleImage.cpp:20: // Don't let images that
have a width/height >= 1 shrink below 1 when zoomed.
On 2016/03/07 11:16:07, Yoav Weiss wrote:
> Do we have tests for this?

That's a good questions. I'm assuming the reason for not wanting to zoom below
1x1 is that it would potentially change the semantics of the image. Having zero
width or height means a null image in some contexts and (I'm guessing here)
might trigger image fallback code.

I'm not aware of any explicit tests. Zooming is pretty weakly covered overall.

[davve, 2016-03-07 14:26:10]

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:103: //
border-image, etc.)
> The tests:
> 
> fast/hidpi/image-set-background-repeat-without-size.html
> fast/hidpi/image-set-border-image-comparison.html
> 
> break when the scale factor is compensated for here (since imageSize() now
used
> for backgrounds and border-image. Previously the background and border-image
> code used Image::computeIntrinsicDimensions and did no such compensation).

I looked into what happens here. The background image painting code
and the border-image painting code already corrected for the
imageScaleFactor. I'll look into removing that and keeping the
correction here, to make imageSize() return the size for layout which
is the sensible size, AFAICT.

[davve, 2016-03-07 16:53:40]

I moved back the scaling compensation to StyleFetchedImageSet::imageSize() so
that imageSize always returns what layout should see and fixed necessary fallout
from that.

Turns out this fixes two bugs, for which I've now added tests: hidpi css list
item images and hidpi shape outside image.

PTAL.

[davve, 2016-03-08 05:43:54]

Description was changed from

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::calculateConcreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::calculateConcreteObjectSize() and
reapplied on the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

Another necessary change now that decorative images such as background
and border-image now use StyleImage::imageSize() is that the
StyleFetchedImageSet implementation must return the real image size,
not the one compensated by image scale factor. The compensation of
scale factor is moved up a level to
LayoutImageResourceStyleImage::imageSize() so that content images
continue to behave as before.

BUG=581357, 591935, 591939
==========

to

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939
==========

[davve, 2016-03-08 06:01:15]

Description was changed from

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939
==========

to

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939, 592888, 592886
==========

[davve, 2016-03-08 07:31:55]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 07:32:04]

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

[commit-bot: I haz the power, 2016-03-08 07:42:53]

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

[commit-bot: I haz the power, 2016-03-08 07:42:54]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[davve, 2016-03-08 07:50:06]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 07:50:19]

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

[commit-bot: I haz the power, 2016-03-08 09:11:30]

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

[commit-bot: I haz the power, 2016-03-08 09:11:31]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-03-08 09:11:49]

LGTM

[davve, 2016-03-08 10:19:01]

The CQ bit was checked by davve@opera.com

[davve, 2016-03-08 10:19:02]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1756763004/#ps180001
(title: "Fix unused variable 'styleImage' in release")

[commit-bot: I haz the power, 2016-03-08 10:19:11]

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

[commit-bot: I haz the power, 2016-03-08 10:30:19]

Description was changed from

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939, 592888, 592886
==========

to

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939, 592888, 592886
==========

[commit-bot: I haz the power, 2016-03-08 10:30:21]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-08 10:32:10]

Description was changed from

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939, 592888, 592886
==========

to

==========
Merge image sizing algorithms

Let users of StyleImage use StyleImage::imageSize() to get the image
size instead of fetching intrinsic information and calculating the
size outside StyleImage. This let's us remove the sizing algorithm in
LayoutBoxModelObject::calculateImageIntrinsicDimensions.

By passing along the default object size to StyleImage::imageSize, we
can remove the sizing algorithm in LayoutBoxModelObject and reuse the
one in SVGImage instead for the one image type that needs the
complicated sizing algorithm. Simpler algorithms can remain simple,
e.g. for generated images with no fixed size, the default object size
is returned unmodified.

SVGImage::concreteObjectSize almost had the necessary bits to
support full sizing of SVG images within a style context, i.e. through
StyleImage. The only missing bit was the the contain constraint on the
default object size added by this patch.

Some zoom juggling needed since the provided default object size is
sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is
removed before calling SVGImage::concreteObjectSize() and reapplied on
the result afterwards.

Background images and other decorative images should never respect the
exif rotation[1], so StyleImage::imageSize now requests the image size
from ImageResource without exif rotation applied. Presumably
StyleImage::imageSize() was broken but unused in this regard before.

In contrast to LBMO::calculateImageIntrinsicDimensions(),
StyleImage::imageSize returns the size for layout, i.e. the size
compensated for the image scale factor. This fixes two hidpi bugs, one
for list item marker images and one for shape-outside.

BUG=581357, 591935, 591939, 592888, 592886

Committed: https://crrev.com/3c7b1342b32dd6174efa9a47a4b476c940462b9c
Cr-Commit-Position: refs/heads/master@{#379801}
==========

[commit-bot: I haz the power, 2016-03-08 10:32:12]

Patchset 10 (id:??) landed as
https://crrev.com/3c7b1342b32dd6174efa9a47a4b476c940462b9c
Cr-Commit-Position: refs/heads/master@{#379801}