[SVG2] Elements that don't render shouldn't contribute to ancestor bboxes.

[SVG2] Elements that don't render shouldn't contribute to ancestor bboxes.

Based on Nikos Andronikos' patch in https://bugs.webkit.org/show_bug.cgi?id=134184.

SVG elements that have rendering disabled should not contribute to any ancestor elements bounding box.
Examples of elements with rendering disabled:
  - basic shape with width <= 0 or height <= 0
  - path with no path data (d attribute missing or empty)
  - polyline or polygon element with no point data (points attribute missing or empty)

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

[fs, 2014-06-25 12:53:20]

https://codereview.chromium.org/355813004/diff/1/LayoutTests/svg/custom/getBB...
File LayoutTests/svg/custom/getBBox-polygon-nodata.html (right):

https://codereview.chromium.org/355813004/diff/1/LayoutTests/svg/custom/getBB...
LayoutTests/svg/custom/getBBox-polygon-nodata.html:4: function
check_rect_bbox(bbox, expected_bbox, test_name)
Really hate to see all this boilerplate and manual output generation when
there's more than one harness that could be used (and w/ testharness.js you
might even be able to get rid of the -expected.txt files! =D)

https://codereview.chromium.org/355813004/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/355813004/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.h:63: virtual bool
isRenderingDisabled() const
The implementation of this is essentially the same as isShapeEmpty (which is
what's used as gating in paint) - so maybe they should be merged (I don't think
the fallback part of isShapeEmpty is needed anymore after the recent
bbox-fixes.)

https://codereview.chromium.org/355813004/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.h:65: // Returning false by default
ensures backwards compatibility for
This will probably be mute w/ the above, but - backwards compatibility with
what? (i.e. what element doesn't implement...)

[Erik Dahlström (inactive), 2014-06-25 14:57:43]

https://codereview.chromium.org/355813004/diff/1/LayoutTests/svg/custom/getBB...
File LayoutTests/svg/custom/getBBox-polygon-nodata.html (right):

https://codereview.chromium.org/355813004/diff/1/LayoutTests/svg/custom/getBB...
LayoutTests/svg/custom/getBBox-polygon-nodata.html:4: function
check_rect_bbox(bbox, expected_bbox, test_name)
On 2014/06/25 12:53:20, fs wrote:
> Really hate to see all this boilerplate and manual output generation when
> there's more than one harness that could be used (and w/ testharness.js you
> might even be able to get rid of the -expected.txt files! =D)

Agreed. Will collect all these into a single testharness test.

https://codereview.chromium.org/355813004/diff/1/LayoutTests/svg/custom/getBB...
File LayoutTests/svg/custom/getBBox-polyline-nodata.html (right):

https://codereview.chromium.org/355813004/diff/1/LayoutTests/svg/custom/getBB...
LayoutTests/svg/custom/getBBox-polyline-nodata.html:32: result &=
check_rect_bbox(p_bbox, {"x":0, "y":0, "width":0, "height":0}, "getBBox on
polyline with no points attribute")
BUG: no polyline here, probably c&p error in the original webkit patch. Fixed in
the testharness-based test.

https://codereview.chromium.org/355813004/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/355813004/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.h:63: virtual bool
isRenderingDisabled() const
On 2014/06/25 12:53:20, fs wrote:
> The implementation of this is essentially the same as isShapeEmpty (which is
> what's used as gating in paint) - so maybe they should be merged (I don't
think
> the fallback part of isShapeEmpty is needed anymore after the recent
> bbox-fixes.)

Done.

https://codereview.chromium.org/355813004/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGShape.h:65: // Returning false by default
ensures backwards compatibility for
On 2014/06/25 12:53:20, fs wrote:
> This will probably be mute w/ the above, but - backwards compatibility with
> what? (i.e. what element doesn't implement...)

Done.

[fs, 2014-06-25 15:06:52]

LGTM w/ nit.

https://codereview.chromium.org/355813004/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGEllipse.h (right):

https://codereview.chromium.org/355813004/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGEllipse.h:48: 
Nit: Maybe drop this stray change.

[Erik Dahlström (inactive), 2014-06-25 15:13:36]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-06-25 15:14:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/355813004/40001

[f(malita), 2014-06-25 15:16:51]

LGTM

IIUC the issue is we actually use uniteEvenIfEmpty() - any reason to do that in
the first place?

https://codereview.chromium.org/355813004/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/355813004/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderSupport.cpp:141: if (current->isSVGShape() &&
toRenderSVGShape(current)->isShapeEmpty())
We have a similar check for containers in updateObjectBoundingBox() - maybe this
should live there too (and possibly the two can be merged, but not necessarily
in this patch).

[commit-bot: I haz the power, 2014-06-25 16:10:45]

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

[Erik Dahlström (inactive), 2014-06-25 16:35:38]

On 2014/06/25 15:16:51, Florin Malita wrote:
> LGTM
> 
> IIUC the issue is we actually use uniteEvenIfEmpty() - any reason to do that
in
> the first place?

I think there's a difference between an empty bbox and an element that "doesn't
render" (according to the spec). You can e.g have a path without a height, and
since it renders it should contribute to other bboxes. See
http://jsfiddle.net/WddM5/.

>
https://codereview.chromium.org/355813004/diff/40001/Source/core/rendering/sv...
> File Source/core/rendering/svg/SVGRenderSupport.cpp (right):
> 
>
https://codereview.chromium.org/355813004/diff/40001/Source/core/rendering/sv...
> Source/core/rendering/svg/SVGRenderSupport.cpp:141: if (current->isSVGShape()
&&
> toRenderSVGShape(current)->isShapeEmpty())
> We have a similar check for containers in updateObjectBoundingBox() - maybe
this
> should live there too (and possibly the two can be merged, but not necessarily
> in this patch).

Ok, will have a look at that.

[commit-bot: I haz the power, 2014-06-25 16:46:04]

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

[commit-bot: I haz the power, 2014-06-25 16:46:05]

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

[Erik Dahlström (inactive), 2014-06-26 08:11:39]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-06-26 08:12:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ed@opera.com/355813004/40001

[commit-bot: I haz the power, 2014-06-26 08:57:20]

Change committed as 176972