Consider Layout tree cycles when resolving the <pattern> content element

Consider Layout tree cycles when resolving the <pattern> content element

When SVGPatternElement determines the attributes of a pattern, it will
not factor in any cycles brought upon it by the actual content elements.
The cycle solver however does this analysis when it is run on a
resource.
To bridge this DOM vs. Layout tree "gap", make sure to inspect the chain
of linked resources when determining the content element to use when
generating the pattern data.

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

[fs, 2015-09-21 13:02:37]

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

[fs, 2015-09-21 13:02:38]

Optionally, we could move SVGPatternElement::collectPatternAttributes to the
layout tree side and then walk the chain via SVGResources. This would
(hopefully) mean that we don't need to look for cycles more than once (in the
cycle solver) - we already require attached nodes so it's not too far-fetched.
That being said though, it will constitute quite a big code-shuffle (but likely
a net reduction.) If that SGTY, I might do that as a follow-up.

[f(malita), 2015-09-21 17:01:25]

On 2015/09/21 13:02:38, fs wrote:
> Optionally, we could move SVGPatternElement::collectPatternAttributes to the
> layout tree side and then walk the chain via SVGResources. This would
> (hopefully) mean that we don't need to look for cycles more than once (in the
> cycle solver) - we already require attached nodes so it's not too far-fetched.
> That being said though, it will constitute quite a big code-shuffle (but
likely
> a net reduction.) If that SGTY, I might do that as a follow-up.

I'm kinda rusty on linked resources, don't we have the same problem for
gradients?

If the cycle resolver does catch these, it does seem like a better place to
react - rather than trying to fix things up later/here.  But I'm fine with
landing this too since it fixes a real issue.

(alternative impl idea, possibly less involved: instead of predicting recursion,
use a state tracking field m_isPreparingPaintServer to detect and bail on
recursion when it occurs)

[fs, 2015-09-21 17:43:14]

On 2015/09/21 at 17:01:25, fmalita wrote:
> On 2015/09/21 13:02:38, fs wrote:
> > Optionally, we could move SVGPatternElement::collectPatternAttributes to the
> > layout tree side and then walk the chain via SVGResources. This would
> > (hopefully) mean that we don't need to look for cycles more than once (in
the
> > cycle solver) - we already require attached nodes so it's not too
far-fetched.
> > That being said though, it will constitute quite a big code-shuffle (but
likely
> > a net reduction.) If that SGTY, I might do that as a follow-up.
> 
> I'm kinda rusty on linked resources, don't we have the same problem for
gradients?

Gradients could only have this problem for self-references and the "DOM-side"
cycle-checker handles that (as well as the layout tree side...). (Since the
contents of <*Gradient> is only <stop>s, there can be no references from content
as in this case though.) Filters would be affected - if it actually did
implement the inheritance scheme...

> If the cycle resolver does catch these, it does seem like a better place to
react - rather than trying to fix things up later/here.  But I'm fine with
landing this too since it fixes a real issue.

Yes, it'd be possible to do - but I think it'd be even more round-about (we
don't necessarily - or rather typically - have collected the attributes yet at
the point the cycle solver is run.)

> (alternative impl idea, possibly less involved: instead of predicting
recursion, use a state tracking field m_isPreparingPaintServer to detect and
bail on recursion when it occurs)

Yes, some form of on-the-fly should work (I think I'd prefer independent state
though) - it's a big break with the current setup though. Hopefully we have
decent test coverage for these kinds of things now though...

[fmalita1, 2015-09-22 13:34:22]

fmalita@gmail.com changed reviewers:
+ fmalita@gmail.com

[fmalita1, 2015-09-22 13:34:22]

SGTM/LGTM

[fs, 2015-09-22 13:48:40]

On 2015/09/22 at 13:34:22, fmalita wrote:
> SGTM/LGTM

Sorry, but I think managed to rant myself into the stands here, so just to
clarify... Land this then implement one/all of the above?

/me makes note to avoid listing too many options

[f(malita), 2015-09-22 13:55:48]

On 2015/09/22 13:48:40, fs wrote:
> On 2015/09/22 at 13:34:22, fmalita wrote:
> > SGTM/LGTM
> 
> Sorry, but I think managed to rant myself into the stands here, so just to
> clarify... Land this then implement one/all of the above?
> 
> /me makes note to avoid listing too many options

I think this is good to land: it's non-intrusive and fixes a real bug.

Up to you if you want to investigate the other approaches later - I imagine you
have more important things on your plate :)

Maybe open a low-priority bug just to make a note?

[fs, 2015-09-22 14:12:34]

On 2015/09/22 at 13:55:48, fmalita wrote:
> On 2015/09/22 13:48:40, fs wrote:
> > On 2015/09/22 at 13:34:22, fmalita wrote:
> > > SGTM/LGTM
> > 
> > Sorry, but I think managed to rant myself into the stands here, so just to
> > clarify... Land this then implement one/all of the above?
> > 
> > /me makes note to avoid listing too many options
> 
> I think this is good to land: it's non-intrusive and fixes a real bug.
> 
> Up to you if you want to investigate the other approaches later - I imagine
you have more important things on your plate :)
> 
> Maybe open a low-priority bug just to make a note?

Thanks. I filed https://crbug.com/534817 for the bigger change - I can probably
knock down the "traverse in layout tree" fairly quickly (and with this landed
it's technically just a clean up).

[fs, 2015-09-22 14:12:46]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-09-22 14:12:49]

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

[commit-bot: I haz the power, 2015-09-22 15:25:43]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202631