Prepare SVGImage for the default sizing algorithm

Prepare SVGImage for the default sizing algorithm

This patch aligns sizing in SVGImage with sizing done in
LayoutBoxModelObject::calculateImageIntrinsicDimensions()

Split SVGImage::containerSize() into two parts. On part that
calculates the concrete size.

The other part return containerSize(), with concrete object size as
fall-back for direct SVGImage::draw() users (webgl being the only one
known).

The long term plan is to only keep this sizing implementation and
remove the one in LayoutBoxModelObject. As long as SVG is the only
user of the complex version of the algorithm it makes sense to keep it
as SVG-only code.

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

[davve, 2016-02-19 08:12:31]

Description was changed from

==========
Prepare SVGImage for the default sizing algorithm

This patch aligns sizing in SVGImage with sizing in
LayoutBoxModelObject::calculateImageIntrinsicDimensions() by
introducing an explicit default size (300x150 for now) and using that
to compute a concrete object size according to:

https://drafts.csswg.org/css-images/#default-sizing

It splits SVGImage::containerSize() into two parts. One part that
returns the containerSize() if any, and does nothing more. The other
part calculates the concrete size.

The long term plan is to only keep this sizing implementation and
remove the one in LayoutBoxModelObject. As long as SVG is the only
user of the complex version of the algorithm it makes sense to keep it
as SVG-only code. This will address the code duplication this patch
introduces.

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

to

==========
Prepare SVGImage for the default sizing algorithm

This patch aligns sizing in SVGImage with sizing done in
LayoutBoxModelObject::calculateImageIntrinsicDimensions()

Split SVGImage::containerSize() into two parts. On part that
calculates the concrete size.

The other part return containerSize(), with concrete object size as
fall-back for direct SVGImage::draw() users (webgl being the only one
known).

The long term plan is to only keep this sizing implementation and
remove the one in LayoutBoxModelObject. As long as SVG is the only
user of the complex version of the algorithm it makes sense to keep it
as SVG-only code.

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

[davve, 2016-02-19 14:17:06]

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

[davve, 2016-02-19 14:17:06]

May make more sense seen together with the planned follow-up patches:

https://codereview.chromium.org/1694263003/

where this sizing code is used by canvas.drawImage()

and

https://codereview.chromium.org/1704493002/

where the sizing code is used in all places
LBMO::calculateImageIntrinsicDimensions is currently used.

The make or break idea is whether Image::updateConcreteObjectSize() is a sane
solution to the default sizing problem. An alternative would be to pass around
the default object size in all cases a size is requested but that turns out to
be quite intrusive.

[fs, 2016-02-19 15:47:21]

On 2016/02/19 at 14:17:06, davve wrote:
> May make more sense seen together with the planned follow-up patches:
> 
> https://codereview.chromium.org/1694263003/
> 
> where this sizing code is used by canvas.drawImage()
> 
> and
> 
> https://codereview.chromium.org/1704493002/
> 
> where the sizing code is used in all places
LBMO::calculateImageIntrinsicDimensions is currently used.
> 
> The make or break idea is whether Image::updateConcreteObjectSize() is a sane
solution to the default sizing problem. An alternative would be to pass around
the default object size in all cases a size is requested but that turns out to
be quite intrusive.

I guess my biggest gripe might be the "volatile-ness", but I can't say I see any
obvious path to avoid that ATM. So this LRTM (yes, that's an 'R'!), but I expect
pdr wants to have a look as well.

[davve, 2016-02-19 16:34:15]

On 2016/02/19 15:47:21, fs wrote:
> I guess my biggest gripe might be the "volatile-ness", but I can't say I see
any
> obvious path to avoid that ATM. So this LRTM (yes, that's an 'R'!), but I
expect
> pdr wants to have a look as well.

My first idea was to tie the default object size to the
SVGImageForContainer class and avoid involving Image with it. I
quickly ran into issues with canvas.drawImage() that wants the image
size for resolving default arguments though (see
CanvasRenderingContext2D::drawImage). That happens before the
wrapping.

If we put the wrapping in a virtual method of Image, perhaps we could
avoid some of this. I'll think on it some more.

[davve, 2016-02-19 16:48:02]

On 2016/02/19 16:34:15, David Vest wrote:

> If we put the wrapping in a virtual method of Image, perhaps we could
> avoid some of this. I'll think on it some more.

Using the canvas.drawImage case as an example, moving the wrapping
earlier would make it harder to retreive the fragment URL.

Also there is generated images that's kind of in the same boat, even
though their rules are a bit simpler.

[fs, 2016-02-19 17:03:12]

On 2016/02/19 at 16:34:15, davve wrote:
> On 2016/02/19 15:47:21, fs wrote:
> > I guess my biggest gripe might be the "volatile-ness", but I can't say I see
any
> > obvious path to avoid that ATM. So this LRTM (yes, that's an 'R'!), but I
expect
> > pdr wants to have a look as well.
> 
> My first idea was to tie the default object size to the
> SVGImageForContainer class and avoid involving Image with it. I
> quickly ran into issues with canvas.drawImage() that wants the image
> size for resolving default arguments though (see
> CanvasRenderingContext2D::drawImage). That happens before the
> wrapping.

Yeah, I see that. In that case it could be more a case of being a victim of the
current interface though.

(I.e maybe a something like:

ClipRects clipRects = imageSource->computeDefaultConstraints(canvas->width(),
canvas->height());
<override fields of 'clipRects' as needed based on drawImage arguments>
drawImageInternal(imageSource, clipRects, exceptionState);

and similar for other uses.)
 
> If we put the wrapping in a virtual method of Image, perhaps we could
> avoid some of this. I'll think on it some more.

I don't really see a strong reason to gaze into the unknown in excess, so don't
spend to much time thinking =)

[fs, 2016-02-19 17:12:29]

On 2016/02/19 at 16:48:02, davve wrote:
> On 2016/02/19 16:34:15, David Vest wrote:
> 
> > If we put the wrapping in a virtual method of Image, perhaps we could
> > avoid some of this. I'll think on it some more.
> 
> Using the canvas.drawImage case as an example, moving the wrapping
> earlier would make it harder to retreive the fragment URL.
> 
> Also there is generated images that's kind of in the same boat, even
> though their rules are a bit simpler.

Right. I suppose it does end up being "insane logistics" vs. "volatile state". I
guess I prefer the latter to the former (at least until the "insane" prefix can
be removed.)

[davve, 2016-02-22 08:56:54]

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

[commit-bot: I haz the power, 2016-02-22 08:58:02]

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

[commit-bot: I haz the power, 2016-02-22 10:11:10]

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

[commit-bot: I haz the power, 2016-02-22 10:11:10]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-02-23 04:42:28]

LGTM

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

https://codereview.chromium.org/1695243004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:93: FloatSize
calculateConcreteObjectSize(const FloatSize&) const;
Is this FloatSize parameter needed? Can you just return an IntSize here?

https://codereview.chromium.org/1695243004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:118: IntSize
m_concreteObjectSize;
Can you add a comment here describing what the concrete object size is?

It may be useful to add a markdown file describing sizing in SVG in general (as
a followup).

[davve, 2016-02-23 08:34:37]

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

https://codereview.chromium.org/1695243004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:93: FloatSize
calculateConcreteObjectSize(const FloatSize&) const;
On 2016/02/23 04:42:28, pdr wrote:
> Is this FloatSize parameter needed? Can you just return an IntSize here?

I agree is looks a bit silly now, but it's used in the follow-up CL:

https://codereview.chromium.org/1694263003/

I return a FloatSize in order to decide on how to do rounding in as few places
as possible.

https://codereview.chromium.org/1695243004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:118: IntSize
m_concreteObjectSize;
On 2016/02/23 04:42:28, pdr wrote:
> Can you add a comment here describing what the concrete object size is?

I copy a snippet from the spec and provide a link.

> It may be useful to add a markdown file describing sizing in SVG in general
(as
> a followup).

Noted.

[davve, 2016-02-23 08:59:38]

The CQ bit was checked by davve@opera.com

[davve, 2016-02-23 08:59:38]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1695243004/#ps100001
(title: "Add comment about what the concrete object size is in SVGImage
context.")

[commit-bot: I haz the power, 2016-02-23 09:00:54]

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

[commit-bot: I haz the power, 2016-02-23 10:43:04]

Description was changed from

==========
Prepare SVGImage for the default sizing algorithm

This patch aligns sizing in SVGImage with sizing done in
LayoutBoxModelObject::calculateImageIntrinsicDimensions()

Split SVGImage::containerSize() into two parts. On part that
calculates the concrete size.

The other part return containerSize(), with concrete object size as
fall-back for direct SVGImage::draw() users (webgl being the only one
known).

The long term plan is to only keep this sizing implementation and
remove the one in LayoutBoxModelObject. As long as SVG is the only
user of the complex version of the algorithm it makes sense to keep it
as SVG-only code.

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

to

==========
Prepare SVGImage for the default sizing algorithm

This patch aligns sizing in SVGImage with sizing done in
LayoutBoxModelObject::calculateImageIntrinsicDimensions()

Split SVGImage::containerSize() into two parts. On part that
calculates the concrete size.

The other part return containerSize(), with concrete object size as
fall-back for direct SVGImage::draw() users (webgl being the only one
known).

The long term plan is to only keep this sizing implementation and
remove the one in LayoutBoxModelObject. As long as SVG is the only
user of the complex version of the algorithm it makes sense to keep it
as SVG-only code.

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

[commit-bot: I haz the power, 2016-02-23 10:43:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-23 10:44:27]

Description was changed from

==========
Prepare SVGImage for the default sizing algorithm

This patch aligns sizing in SVGImage with sizing done in
LayoutBoxModelObject::calculateImageIntrinsicDimensions()

Split SVGImage::containerSize() into two parts. On part that
calculates the concrete size.

The other part return containerSize(), with concrete object size as
fall-back for direct SVGImage::draw() users (webgl being the only one
known).

The long term plan is to only keep this sizing implementation and
remove the one in LayoutBoxModelObject. As long as SVG is the only
user of the complex version of the algorithm it makes sense to keep it
as SVG-only code.

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

to

==========
Prepare SVGImage for the default sizing algorithm

This patch aligns sizing in SVGImage with sizing done in
LayoutBoxModelObject::calculateImageIntrinsicDimensions()

Split SVGImage::containerSize() into two parts. On part that
calculates the concrete size.

The other part return containerSize(), with concrete object size as
fall-back for direct SVGImage::draw() users (webgl being the only one
known).

The long term plan is to only keep this sizing implementation and
remove the one in LayoutBoxModelObject. As long as SVG is the only
user of the complex version of the algorithm it makes sense to keep it
as SVG-only code.

BUG=581357

Committed: https://crrev.com/48a3d424ac967a77a648f12e5ac5058d7cef8b90
Cr-Commit-Position: refs/heads/master@{#376965}
==========

[commit-bot: I haz the power, 2016-02-23 10:44:28]

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