Remove Image::computeIntrinsicDimensions()

Remove Image::computeIntrinsicDimensions()

There were only one user of Image::computeIntrinsicDimensions() left,
SVGImagePainter. It was used to calculate the container size in such a
way to force non-uniform scaling in case of
preserveAspectRatio=none. Use SVGImage::concreteObjectSize to get the
viewport size for SVG images.

BUG=581357
Committed: https://crrev.com/cf0913b5fc214804d5731c43a19472fbcffaf26b
Cr-Commit-Position: refs/heads/master@{#380204}

[davve, 2016-03-04 14:40:51]

Description was changed from

==========
Remove Image::computeIntrinsicDimensions()

There were only one user of Image::computeIntrinsicDimensions() left,
SVGImagePainter. It was used to calculate the container size in such a
way to force non-uniform scaling in case of
preserveAspectRatio=none. Plainly using the concrete object size
should mean the same thing since it's the ratio that matters in this
case.

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

to

==========
Remove Image::computeIntrinsicDimensions()

There were only one user of Image::computeIntrinsicDimensions() left,
SVGImagePainter. It was used to calculate the container size in such a
way to force non-uniform scaling in case of
preserveAspectRatio=none. Use SVGImage::concreteObjectSize to get the
viewport size for SVG images.

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

[davve, 2016-03-04 14:42:06]

davve@opera.com changed reviewers:
+ fs@opera.com

[davve, 2016-03-04 14:42:07]

No hurry here but I think this is close to ready, assuming the other CLs land
and stick.

[fs, 2016-03-04 15:10:10]

fs@opera.com changed reviewers:
+ pdr@chromium.org

[fs, 2016-03-04 15:10:11]

LGTM!

+pdr for platform/ clog dancing

https://codereview.chromium.org/1720853002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGImagePainter.cpp (right):

https://codereview.chromium.org/1720853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGImagePainter.cpp:93: return
FloatSize(cachedImage->imageSize(DoNotRespectImageOrientation, 1));
Any reason in particular to use imageSize() here and not just
cachedImage->image()->size() here?

[davve, 2016-03-04 15:22:42]

https://codereview.chromium.org/1720853002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGImagePainter.cpp (right):

https://codereview.chromium.org/1720853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGImagePainter.cpp:93: return
FloatSize(cachedImage->imageSize(DoNotRespectImageOrientation, 1));
On 2016/03/04 15:10:10, fs wrote:
> Any reason in particular to use imageSize() here and not just
> cachedImage->image()->size() here?

None other than vague plans to look over (and possibly exterminate)
ImageResource::imageSize() at a later point and until then keep things
reasonably consistent. But might as well not introduce this one in the first
place.

Also the pattern emerging here could (should?) probably be moved into a virtual
Image::size(FloatSize& defaultObjectSize) along side Image::size() and
SVGImage::concreteObjectSize can move back into being an implementation detail
in SVGImage.

[fs, 2016-03-04 15:29:05]

https://codereview.chromium.org/1720853002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGImagePainter.cpp (right):

https://codereview.chromium.org/1720853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGImagePainter.cpp:93: return
FloatSize(cachedImage->imageSize(DoNotRespectImageOrientation, 1));
On 2016/03/04 at 15:22:41, David Vest wrote:
> On 2016/03/04 15:10:10, fs wrote:
> > Any reason in particular to use imageSize() here and not just
> > cachedImage->image()->size() here?
> 
> None other than vague plans to look over (and possibly exterminate)
ImageResource::imageSize() at a later point and until then keep things
reasonably consistent. But might as well not introduce this one in the first
place.
> 
> Also the pattern emerging here could (should?) probably be moved into a
virtual Image::size(FloatSize& defaultObjectSize) along side Image::size() and
SVGImage::concreteObjectSize can move back into being an implementation detail
in SVGImage.

Yeah, I don't really mind if you want to play it one way or the other. I did
somewhat expect it to receive some love in the future.

[davve, 2016-03-08 12:15:53]

davve@opera.com changed reviewers:
+ junov@chromium.org

[davve, 2016-03-08 12:15:54]

Justin, could you take a look at platform/?

[davve, 2016-03-08 12:16:37]

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

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

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

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

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

[commit-bot: I haz the power, 2016-03-08 13:37:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[davve, 2016-03-08 14:52:33]

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

[commit-bot: I haz the power, 2016-03-08 14:52:47]

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

[commit-bot: I haz the power, 2016-03-08 16:21:56]

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

[commit-bot: I haz the power, 2016-03-08 16:21:57]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2016-03-08 21:56:35]

https://codereview.chromium.org/1720853002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GeneratedImage.h (left):

https://codereview.chromium.org/1720853002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GeneratedImage.h:41: void
computeIntrinsicDimensions(FloatSize& intrinsicSize, FloatSize& intrinsicRatio)
override;
Why is intrinsicRatio a FloatSize rather than a float?

[Justin Novosad, 2016-03-08 22:00:06]

> Why is intrinsicRatio a FloatSize rather than a float?

Follow-up question: when does intrinsicRatio express a ratio that is different
from the ratio that is implicitly expressed by intrinsicSize?

[davve, 2016-03-09 08:06:10]

https://codereview.chromium.org/1720853002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GeneratedImage.h (left):

https://codereview.chromium.org/1720853002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GeneratedImage.h:41: void
computeIntrinsicDimensions(FloatSize& intrinsicSize, FloatSize& intrinsicRatio)
override;
On 2016/03/08 21:56:35, Justin Novosad wrote:
> Why is intrinsicRatio a FloatSize rather than a float?

I assume it is/was to delay actual computation of the ratio to actual use. I did
a similar thing in https://codereview.chromium.org/1690253002/ and the layout
test results showed only progressions.

> Follow-up question: when does intrinsicRatio express a ratio that is different
> from the ratio that is implicitly expressed by intrinsicSize?

Heh, yeah, the interface was confusing.

For SVG images the intrinsic size would be zero/empty for SVG lacking
width/height attributes or if any of them would be a percentage values. If the
SVG had the viewBox attribute, however, the intrinsic aspect ratio would be
derived from there. I.e <svg viewBox="0 0 1 1"/> would have empty intrinsic size
but 1:1 intrinsic aspect ratio. This in turn would affect the subsequent size
calculations.

Now all these details are hidden inside SVGImage::concreteObjectSize() enabling
us to remove this interface.

[Justin Novosad, 2016-03-09 17:42:43]

Wow!  lgtm

[davve, 2016-03-09 18:38:17]

The CQ bit was checked by davve@opera.com

[davve, 2016-03-09 18:38:17]

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/1720853002/#ps60001
(title: "Avoid using the size of the error image. Null-check in ImageResource
saved the day in previous patc…")

[commit-bot: I haz the power, 2016-03-09 18:38:31]

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

[commit-bot: I haz the power, 2016-03-09 20:36:13]

Description was changed from

==========
Remove Image::computeIntrinsicDimensions()

There were only one user of Image::computeIntrinsicDimensions() left,
SVGImagePainter. It was used to calculate the container size in such a
way to force non-uniform scaling in case of
preserveAspectRatio=none. Use SVGImage::concreteObjectSize to get the
viewport size for SVG images.

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

to

==========
Remove Image::computeIntrinsicDimensions()

There were only one user of Image::computeIntrinsicDimensions() left,
SVGImagePainter. It was used to calculate the container size in such a
way to force non-uniform scaling in case of
preserveAspectRatio=none. Use SVGImage::concreteObjectSize to get the
viewport size for SVG images.

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

[commit-bot: I haz the power, 2016-03-09 20:36:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-09 20:37:12]

Description was changed from

==========
Remove Image::computeIntrinsicDimensions()

There were only one user of Image::computeIntrinsicDimensions() left,
SVGImagePainter. It was used to calculate the container size in such a
way to force non-uniform scaling in case of
preserveAspectRatio=none. Use SVGImage::concreteObjectSize to get the
viewport size for SVG images.

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

to

==========
Remove Image::computeIntrinsicDimensions()

There were only one user of Image::computeIntrinsicDimensions() left,
SVGImagePainter. It was used to calculate the container size in such a
way to force non-uniform scaling in case of
preserveAspectRatio=none. Use SVGImage::concreteObjectSize to get the
viewport size for SVG images.

BUG=581357

Committed: https://crrev.com/cf0913b5fc214804d5731c43a19472fbcffaf26b
Cr-Commit-Position: refs/heads/master@{#380204}
==========

[commit-bot: I haz the power, 2016-03-09 20:37:14]

Patchset 4 (id:??) landed as
https://crrev.com/cf0913b5fc214804d5731c43a19472fbcffaf26b
Cr-Commit-Position: refs/heads/master@{#380204}