Avoid using ImageResource->imageSize() to get the marker size

Avoid using ImageResource->imageSize() to get the marker size

In cases when LayoutListMarker represents an image it has a StyleImage
to determine the size of the the marker. Prior to this patch the
computed size is saved back to the StyleImage for later use. This
should be unnecessary since the size can be computed (or saved
locally) when it's actually needed.

When looking closer at one may notice that the zoom level is stored
inside SVGImageForContainer so that SVGImageForContainer::size()
actually returns the size _including zoom_. When LayoutListMarker
asked for the image size and provided zoom, the zoom level was applied
once more resulting in double zoom. The added test exposes this.

As part of 551419, the aim it to get rid of
ImageResource::setContainerSizeForLayoutObject and friends (storing
SVG image specific data inside ImageResource) and this is a step in
that direction.

BUG=551419, 551808
Committed: https://crrev.com/2bd5bf90d71c251c9cc777ee8b89e3a283f9cb75
Cr-Commit-Position: refs/heads/master@{#358552}

[davve, 2015-11-04 15:23:09]

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

[commit-bot: I haz the power, 2015-11-04 15:23:44]

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

[davve, 2015-11-04 15:26:43]

The CQ bit was unchecked by davve@opera.com

[davve, 2015-11-05 10:53:16]

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

[commit-bot: I haz the power, 2015-11-05 10:53:53]

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

[davve, 2015-11-05 10:58:01]

Description was changed from

==========
Avoid using ImageResource->imageSize() to get the computed size

Stop using the ImageResource to save the image size in the
ImageResource. The list marker can instead compute the size when it
needs it. This prepares us for eventually removing
ImageResource::setContainerSizeForLayoutObject().

BUG=551419
==========

to

==========
Avoid using ImageResource->imageSize() to get the computed size

Stop using the ImageResource to save the image size in the
ImageResource. The list marker can instead compute the size when it
needs it.

Using setContainerSizeForLayoutObject leads to appling the zoom level
twice for SVG images. This also prepares us for eventually removing
ImageResource::setContainerSizeForLayoutObject().

BUG=551419, 551808
==========

[commit-bot: I haz the power, 2015-11-05 11:58:11]

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

[commit-bot: I haz the power, 2015-11-05 11:58:12]

Dry run: This issue passed the CQ dry run.

[davve, 2015-11-05 12:55:17]

Description was changed from

==========
Avoid using ImageResource->imageSize() to get the computed size

Stop using the ImageResource to save the image size in the
ImageResource. The list marker can instead compute the size when it
needs it.

Using setContainerSizeForLayoutObject leads to appling the zoom level
twice for SVG images. This also prepares us for eventually removing
ImageResource::setContainerSizeForLayoutObject().

BUG=551419, 551808
==========

to

==========
Avoid using ImageResource->imageSize() to get the marker size

In cases when LayoutListMarker represents an image it has a StyleImage
to determine the size of the the marker. Prior to this patch the
computed size is saved back to the StyleImage for later use. This
should be unnecessary since the size can be computed (or saved
locally) when it's actually needed.

When looking closer at one may notice that the zoom level is stored
inside SVGImageForContainer so that SVGImageForContainer::size()
actually returns the size _including zoom_. When LayoutListMarker
asked for the image size and provided zoom, the zoom level was applied
once more resulting in double zoom. The added test exposes this.

As part of 551419, the aim it to get rid of
ImageResource::setContainerSizeForLayoutObject and friends (storing
SVG image specific data inside ImageResource) and this is a step in
that direction.

BUG=551419, 551808
==========

[davve, 2015-11-05 12:57:52]

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

[davve, 2015-11-05 13:13:38]

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

[davve, 2015-11-05 13:13:39]

Dan, could you take a look?

https://codereview.chromium.org/1427943002/ has the general direction I'm aiming
for here. fs and I decided to split this part out as it can be landed
separately.

[fs, 2015-11-05 13:16:19]

fs@opera.com changed reviewers:
- dsinclair@chromium.org

[fs, 2015-11-05 13:16:19]

lgtm

I guess one could argue that imageBulletSize() should just return IntSize and
leave the impedance matching to the users (where required). The majority of
users wants LayoutUnits though so it seems ok the way it is too.

[davve, 2015-11-05 13:30:05]

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

[commit-bot: I haz the power, 2015-11-05 13:30:31]

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

[davve, 2015-11-05 13:33:19]

> I guess one could argue that imageBulletSize() should just return IntSize and
> leave the impedance matching to the users (where required). The majority of
> users wants LayoutUnits though so it seems ok the way it is too.

I was back and forth on this, especially when I initially thought I should keep
the setContainerSizeForLayoutObject() call. Returning IntSize might generate
slightly more code(?) but looks a bit nicer. Uploaded in patch set 3.

[fs, 2015-11-05 14:23:59]

On 2015/11/05 at 13:33:19, davve wrote:
> > I guess one could argue that imageBulletSize() should just return IntSize
and
> > leave the impedance matching to the users (where required). The majority of
> > users wants LayoutUnits though so it seems ok the way it is too.
> 
> I was back and forth on this, especially when I initially thought I should
keep the setContainerSizeForLayoutObject() call. Returning IntSize might
generate slightly more code(?) but looks a bit nicer. Uploaded in patch set 3.

Yeah, probably slightly more code. Still LGTM.

[fs, 2015-11-05 14:24:42]

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

[commit-bot: I haz the power, 2015-11-05 14:50:51]

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

[commit-bot: I haz the power, 2015-11-05 14:50:51]

Dry run: This issue passed the CQ dry run.

[davve, 2015-11-06 14:27:32]

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

[davve, 2015-11-06 14:27:32]

Walter, could you weigh in perhaps? It could be useful to have a layout person
at least a little bit involved here.

[wkorman, 2015-11-07 08:31:48]

lgtm

Sorry for delay -- this looks reasonable to me, thanks for fixing.

[davve, 2015-11-08 15:35:32]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2015-11-08 15:35:40]

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

[commit-bot: I haz the power, 2015-11-08 16:45:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-08 16:46:17]

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