[SVG] Refactor getIntersectionList() and getEnclosureList()

[SVG] Refactor getIntersectionList() and getEnclosureList()

This CL moves most of the implementation out of RenderSVGModelObject and
into SVGSVGElement (which is the only client). Some notes:

* use the consolidated SVGGraphicsElement::computeCTM() (which is
  accumulating localCoordinateSpaceTransform()s and not
  localToParentTransform()s like the old getElementCTM() did <- root
  cause for the zooming bug)
* only force a layout update once, at the entry point, instead of on each
  getElementCTM() call.
* detect the stopping element on the fly, instead of an additional upfront
  ancestor crawl.
* transforms are now correctly computed relative to the target <svg>
  element instead of the nearest viewport.

BUG=155277
R=pdr@chromium.org,schenney@chromium.org,fs@opera.com,rob.buis@samsung.com

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

[fs, 2014-03-03 10:44:32]

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.h:65: AncestorScope // Used by
SVGSVGElemeng::get{Enclosure|Intersection}List()
Nit: typo SVGSVGElemeng

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
File Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:336: if (r.isEmpty() && other.isEmpty())
This function had me wondering...

Since isEmpty() is "width <= 0 || height <= 0", then intersecting, say:

<0, 5, 10, 0> with
<5, 0, 0, 10>

would return false, although it should return true - yes?

Maybe it would just be easier to open-code FloatRect::intersects w/ the
!isEmpty()-parts of the predicate removed (and potentially add checks for
width/height < 0 - if that's even possible to achieve... [for the RenderObject
side])

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:343: return intersectsAllowingEmpty(other, r);
Might make sense to just make the code in the above if-block (yet) a(nother)
helper-function? (But it depends on how the above is resolved of course.)

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:394: if (referenceElement) {
Some test(s) for this case would be nice - but I'm not going to force you to
write them. =)

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:399: // No common subtree: the spec is not
clear about this case - should we bend over backwards
I think this is in line with the spec wording, so seems fine to me.

[Stephen Chennney, 2014-03-03 13:00:22]

One significant issue.

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
File Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:334: static bool intersectsAllowingEmpty(const
FloatRect& r, const FloatRect& other)
Either this is much simpler to implement, or I'm not understanding the purpose
of the method (which means it's badly named).

The two rects intersect if max(r.x(), other.x()) > min(r.x() + r.height, other.x
+ other.height) and similar for y. That is, if their intersection is non-empty
(also called a separating lines check because you are finding out if there is a
line that separates the two rectangles in either dimension). Much cheaper to
compute that all this "contains" checking.

[f(malita), 2014-03-03 14:12:18]

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
File Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:334: static bool intersectsAllowingEmpty(const
FloatRect& r, const FloatRect& other)
On 2014/03/03 13:00:22, Stephen Chenney wrote:
> Either this is much simpler to implement, or I'm not understanding the purpose
> of the method (which means it's badly named).
> 
> The two rects intersect if max(r.x(), other.x()) > min(r.x() + r.height,
other.x
> + other.height) and similar for y. That is, if their intersection is non-empty
> (also called a separating lines check because you are finding out if there is
a
> line that separates the two rectangles in either dimension). Much cheaper to
> compute that all this "contains" checking.

TBH I haven't looked beyond the comments when moving this from
RenderSVGModelObject. As fs pointed out the current impl has functional issues
also, so it sure is a good candidate for refactoring. Will do.

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:394: if (referenceElement) {
On 2014/03/03 10:44:32, fs wrote:
> Some test(s) for this case would be nice - but I'm not going to force you to
> write them. =)

I'll add some willingly :)

https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
Source/core/svg/SVGSVGElement.cpp:399: // No common subtree: the spec is not
clear about this case - should we bend over backwards
On 2014/03/03 10:44:32, fs wrote:
> I think this is in line with the spec wording, so seems fine to me.

Awesome, I'll remove the comment then.

[fs, 2014-03-03 15:20:51]

On 2014/03/03 14:12:18, Florin Malita wrote:
>
https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
> File Source/core/svg/SVGSVGElement.cpp (right):
> 
>
https://codereview.chromium.org/185333004/diff/10010/Source/core/svg/SVGSVGEl...
> Source/core/svg/SVGSVGElement.cpp:334: static bool
intersectsAllowingEmpty(const
> FloatRect& r, const FloatRect& other)
> On 2014/03/03 13:00:22, Stephen Chenney wrote:
> > Either this is much simpler to implement, or I'm not understanding the
purpose
> > of the method (which means it's badly named).
> > 
> > The two rects intersect if max(r.x(), other.x()) > min(r.x() + r.height,
> other.x
> > + other.height) and similar for y. That is, if their intersection is
non-empty
> > (also called a separating lines check because you are finding out if there
is
> a
> > line that separates the two rectangles in either dimension). Much cheaper to
> > compute that all this "contains" checking.
> 
> TBH I haven't looked beyond the comments when moving this from
> RenderSVGModelObject. As fs pointed out the current impl has functional issues
> also, so it sure is a good candidate for refactoring. Will do.
 
Sorry, I didn't look enough at what was copied and what was new... =/

[f(malita), 2014-03-04 13:43:57]

Updated, PTAL.

[fs, 2014-03-04 14:17:20]

On 2014/03/04 13:43:57, Florin Malita wrote:
> Updated, PTAL.

Looks good to me.

[Stephen Chennney, 2014-03-04 14:40:58]

lgtm with one nit. It's time to add Google to the copyright holders.

[f(malita), 2014-03-04 17:27:36]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-04 17:28:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fmalita@chromium.org/185333004/50001

[commit-bot: I haz the power, 2014-03-05 08:54:22]

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

[commit-bot: I haz the power, 2014-03-05 08:54:23]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[f(malita), 2014-03-05 14:10:22]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-05 14:10:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fmalita@chromium.org/185333004/50001

[commit-bot: I haz the power, 2014-03-05 14:11:09]

Change committed as 168479