Teach SVGImage::containerSize() to deduce intrinsic values

Teach SVGImage::containerSize() to deduce intrinsic values

When an SVG has either intrinsic width or height but not both, and a
intrinsic ratio, the missing intrinsic width or height can be
calculated. Doing so enables us to return correct intrinsic width and
height for SVG images in more cases when the image has no renderer.

Also, this patch removes the unspecified use of the viewBox attribute
as the source of intrinsic size. A small number of tests depended on
this and has been updated. The reasoning to make this change is
because we don't use the viewBox for intrinsic size in any other
context than SVGImage. And we only use it until the renderer has a
containerSize() anyway.

http://www.whatwg.org/specs/web-apps/current-work/multipage/edits.html#dom-img-width
http://www.whatwg.org/specs/web-apps/current-work/multipage/edits.html#dom-img-height

BUG=376895
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175784

[pdr., 2014-06-06 03:54:50]

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/css3/composi...
File LayoutTests/css3/compositing/background-blend-mode-svg-color.html (right):

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/css3/composi...
LayoutTests/css3/compositing/background-blend-mode-svg-color.html:8: background:
url('resources/blue-circle.svg'), green;
Was this change required?

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/svg/as-image...
File LayoutTests/svg/as-image/svg-as-image-intrinsic-size.html (right):

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/svg/as-image...
LayoutTests/svg/as-image/svg-as-image-intrinsic-size.html:15:
setTimeout(t.step_func(waitForComplete.bind(this, img, i + 1)), 10);
10!!1!? Lets crank this down to 1ms :)

https://codereview.chromium.org/308643002/diff/60001/Source/core/svg/graphics...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/308643002/diff/60001/Source/core/svg/graphics...
Source/core/svg/graphics/SVGImage.cpp:169: if (intrinsicSize.isEmpty() &&
intrinsicRatio) {
This logic seems correct but why isn't it needed elsewhere such as
RenderReplaced::computeAspectRatioInformationForRenderBox?

computeAspectRatioInformationForRenderBox seems to have some similar logic
through calling computeReplacedLogicalHeight.

https://codereview.chromium.org/308643002/diff/60001/Source/core/svg/graphics...
Source/core/svg/graphics/SVGImage.cpp:177: return
IntSize(static_cast<int>(ceilf(intrinsicSize.width())),
static_cast<int>(ceilf(intrinsicSize.height())));
I think this is just expandedIntSize

[davve, 2014-06-06 20:26:27]

Thanks for the review! (Was off today celebrating the very important
https://en.wikipedia.org/wiki/National_Day_of_Sweden. :)

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/css3/composi...
File LayoutTests/css3/compositing/background-blend-mode-svg-color.html (right):

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/css3/composi...
LayoutTests/css3/compositing/background-blend-mode-svg-color.html:8: background:
url('resources/blue-circle.svg'), green;
On 2014/06/06 03:54:50, pdr wrote:
> Was this change required?

Yes, otherwise the background shorthand overwrites the background-size rule. And
we need the background-size to scale the background image properly.

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/svg/as-image...
File LayoutTests/svg/as-image/svg-as-image-intrinsic-size.html (right):

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/svg/as-image...
LayoutTests/svg/as-image/svg-as-image-intrinsic-size.html:15:
setTimeout(t.step_func(waitForComplete.bind(this, img, i + 1)), 10);
On 2014/06/06 03:54:50, pdr wrote:
> 10!!1!? Lets crank this down to 1ms :)

Sure. (I'm hoping it doesn't matter in practice since the first branch will be
hit (it's a short data uri after all))

https://codereview.chromium.org/308643002/diff/60001/Source/core/svg/graphics...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/308643002/diff/60001/Source/core/svg/graphics...
Source/core/svg/graphics/SVGImage.cpp:169: if (intrinsicSize.isEmpty() &&
intrinsicRatio) {
On 2014/06/06 03:54:50, pdr wrote:
> This logic seems correct but why isn't it needed elsewhere such as
> RenderReplaced::computeAspectRatioInformationForRenderBox?
> 
> computeAspectRatioInformationForRenderBox seems to have some similar logic
> through calling computeReplacedLogicalHeight.

AFAICT, private method computeAspectRatioInformationForRenderBox is a workhorse
for computeReplacedLogicalHeight and computeReplacedLogicalWidth that has
corresponding logic as part of the replaced element layout.

One difference is that computeAspectRatioInformationForRenderBox() returns both
size and ratio, containerSize() only returns size. Not resolving using ratio
loses information.

https://codereview.chromium.org/308643002/diff/60001/Source/core/svg/graphics...
Source/core/svg/graphics/SVGImage.cpp:177: return
IntSize(static_cast<int>(ceilf(intrinsicSize.width())),
static_cast<int>(ceilf(intrinsicSize.height())));
On 2014/06/06 03:54:50, pdr wrote:
> I think this is just expandedIntSize

Done.

[pdr., 2014-06-06 20:28:37]

LGTM

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/css3/composi...
File LayoutTests/css3/compositing/background-blend-mode-svg-color.html (right):

https://codereview.chromium.org/308643002/diff/60001/LayoutTests/css3/composi...
LayoutTests/css3/compositing/background-blend-mode-svg-color.html:8: background:
url('resources/blue-circle.svg'), green;
On 2014/06/06 20:26:26, David Vest wrote:
> On 2014/06/06 03:54:50, pdr wrote:
> > Was this change required?
> 
> Yes, otherwise the background shorthand overwrites the background-size rule.
And
> we need the background-size to scale the background image properly.

Ahh, I understand. I thought this was dependent on the ordering, but that was
wrong.

[davve, 2014-06-06 20:37:08]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2014-06-06 20:38:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davve@opera.com/308643002/80001

[commit-bot: I haz the power, 2014-06-06 22:11:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10663)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11062)

[commit-bot: I haz the power, 2014-06-06 22:20:51]

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

[commit-bot: I haz the power, 2014-06-06 22:20:52]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10678)

[davve, 2014-06-09 08:30:25]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2014-06-09 08:30:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davve@opera.com/308643002/80001

[commit-bot: I haz the power, 2014-06-09 09:31:24]

Change committed as 175784