LayoutSVGImage should work correctly even when image size is unavailable

LayoutSVGImage should work correctly even when image size is unavailable

Currently LayoutSVGImage::calculateObjectSize returns an invalid size (i.e.,
size containing NaN) when the image size is unavailable. This change fixes that.

BUG=675895
R=fs@opera.com, hiroshige@chromium.org
TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html

Committed: https://crrev.com/43c7ce1974915502d40e84e8c3a2ae80d4c9037e
Cr-Commit-Position: refs/heads/master@{#440177}

[yhirano, 2016-12-20 09:30:19]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kouhei (in TOK), 2016-12-20 09:34:33]

kouhei@chromium.org changed reviewers:
+ fmalita@chromium.org, kouhei@chromium.org

[kouhei (in TOK), 2016-12-20 09:34:33]

+fmalita

lgtm

[kouhei (in TOK), 2016-12-20 09:35:05]

Add TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html ?

[kouhei (in TOK), 2016-12-20 09:35:05]

Add TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html ?

[yhirano, 2016-12-20 09:40:42]

Description was changed from

==========
LayoutSVGImage should work correctly even when image size is unavailable

Currently LayoutSVGImage::calculateObjectSize returns an invalid size (i.e.,
size containing NaN) when the image size is unavailable. This change fixes that.

BUG=675895
R=fs@opera.com, hiroshige@chromium.org
==========

to

==========
LayoutSVGImage should work correctly even when image size is unavailable

Currently LayoutSVGImage::calculateObjectSize returns an invalid size (i.e.,
size containing NaN) when the image size is unavailable. This change fixes that.

BUG=675895
R=fs@opera.com, hiroshige@chromium.org
TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html
==========

[yhirano, 2016-12-20 10:06:41]

On 2016/12/20 09:35:05, kouhei wrote:
> Add TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html ?

Done.

[fs, 2016-12-20 10:08:12]

lgtm

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:73:
!cachedImage->isSizeAvailable())
Would this not affect other similar code-paths simply because they either check
for the "empty" condition, or because the don't happen to use the (zero)
dimensions as denominators in a division? I'm not opposed to having this check
as such, but I wonder if makes sense to diverge w.r.t other similar uses.

[commit-bot: I haz the power, 2016-12-20 10:54:49]

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

[commit-bot: I haz the power, 2016-12-20 10:54:51]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-12-20 12:06:30]

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:73:
!cachedImage->isSizeAvailable())
On 2016/12/20 10:08:12, fs wrote:
> Would this not affect other similar code-paths simply because they either
check
> for the "empty" condition, or because the don't happen to use the (zero)
> dimensions as denominators in a division? I'm not opposed to having this check
> as such, but I wonder if makes sense to diverge w.r.t other similar uses.

I'm not sure (I'm not familiar with the layout / image code). It looks dangerous
to me to read size when it is not available.

It is possible that we need more protection against 0-width and 0-height, but it
is a different issue, I think.

[fs, 2016-12-20 12:15:45]

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:73:
!cachedImage->isSizeAvailable())
On 2016/12/20 at 12:06:30, yhirano wrote:
> On 2016/12/20 10:08:12, fs wrote:
> > Would this not affect other similar code-paths simply because they either
check
> > for the "empty" condition, or because the don't happen to use the (zero)
> > dimensions as denominators in a division? I'm not opposed to having this
check
> > as such, but I wonder if makes sense to diverge w.r.t other similar uses.
> 
> I'm not sure (I'm not familiar with the layout / image code). It looks
dangerous to me to read size when it is not available.

My observation was mostly that other code seems to rely on that "no size
available" implies "size 0x0" (meaning one less property of the image to check.)
I'm fine with landing this change as-is though.

[f(malita), 2016-12-21 15:47:18]

lgtm

[hiroshige, 2016-12-21 17:51:06]

lgtm

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/2592613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:73:
!cachedImage->isSizeAvailable())
On 2016/12/20 12:15:45, fs wrote:
> On 2016/12/20 at 12:06:30, yhirano wrote:
> > On 2016/12/20 10:08:12, fs wrote:
> > > Would this not affect other similar code-paths simply because they either
> check
> > > for the "empty" condition, or because the don't happen to use the (zero)
> > > dimensions as denominators in a division? I'm not opposed to having this
> check
> > > as such, but I wonder if makes sense to diverge w.r.t other similar uses.
> > 
> > I'm not sure (I'm not familiar with the layout / image code). It looks
> dangerous to me to read size when it is not available.
> 
> My observation was mostly that other code seems to rely on that "no size
> available" implies "size 0x0" (meaning one less property of the image to
check.)
> I'm fine with landing this change as-is though.

If we can replace isSizeAvailable() here and in ImageResource-related code with
0x0 size check, then it would be simpler (one less property that
ImageResourceContent should keep track), but it doesn't block this CL.

[yhirano, 2016-12-21 17:53:26]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-12-21 17:53:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 19:30:33]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482342806859510, "parent_rev":
"850ccdb2216b63b32b8743e7697a17478e13dd7d", "commit_rev":
"a533db804e50f3db025118171b2f2a74a0e79386"}

[commit-bot: I haz the power, 2016-12-21 19:30:59]

Description was changed from

==========
LayoutSVGImage should work correctly even when image size is unavailable

Currently LayoutSVGImage::calculateObjectSize returns an invalid size (i.e.,
size containing NaN) when the image size is unavailable. This change fixes that.

BUG=675895
R=fs@opera.com, hiroshige@chromium.org
TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html
==========

to

==========
LayoutSVGImage should work correctly even when image size is unavailable

Currently LayoutSVGImage::calculateObjectSize returns an invalid size (i.e.,
size containing NaN) when the image size is unavailable. This change fixes that.

BUG=675895
R=fs@opera.com, hiroshige@chromium.org
TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html

Review-Url: https://codereview.chromium.org/2592613002
==========

[commit-bot: I haz the power, 2016-12-21 19:31:01]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-21 19:34:03]

Description was changed from

==========
LayoutSVGImage should work correctly even when image size is unavailable

Currently LayoutSVGImage::calculateObjectSize returns an invalid size (i.e.,
size containing NaN) when the image size is unavailable. This change fixes that.

BUG=675895
R=fs@opera.com, hiroshige@chromium.org
TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html

Review-Url: https://codereview.chromium.org/2592613002
==========

to

==========
LayoutSVGImage should work correctly even when image size is unavailable

Currently LayoutSVGImage::calculateObjectSize returns an invalid size (i.e.,
size containing NaN) when the image size is unavailable. This change fixes that.

BUG=675895
R=fs@opera.com, hiroshige@chromium.org
TEST=svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html

Committed: https://crrev.com/43c7ce1974915502d40e84e8c3a2ae80d4c9037e
Cr-Commit-Position: refs/heads/master@{#440177}
==========

[commit-bot: I haz the power, 2016-12-21 19:34:04]

Patchset 1 (id:??) landed as
https://crrev.com/43c7ce1974915502d40e84e8c3a2ae80d4c9037e
Cr-Commit-Position: refs/heads/master@{#440177}