Store SVGElement instead of SVGElementInstance for instancesForElement

Store SVGElement instead of SVGElementInstance for instancesForElement

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

[rwlbuis, 2014-05-03 20:43:52]

PTAL. I realized this works against trunk already! I think there are two tricky
things in this patch, when to mapInstanceToElement/removeInstance is one thing.
I think this is ok for now but we may have to move this logic into SVGElement
ultimately, at least we can't rely on SVGElementInstance creation/deletion
anymore :)
The other thing is the EventHandler change, I found no use for this code, i.e.
no test breaks...

[pdr., 2014-05-03 21:55:26]

Wow! Didn't think this would be so easy

https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.cpp
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.c...
Source/core/svg/SVGElement.cpp:621: if (ShadowRoot* root =
containingShadowRoot()) {
What if the <use> element is in a user-created shadow tree instead of the user
agent shadow tree we create under <use>? I think this will return the wrong
value.

Can we use correspondingElement()->userAgentShadowRoot()->host()?

[rwlbuis, 2014-05-03 22:08:52]

On 2014/05/03 21:55:26, pdr wrote:
> Wow! Didn't think this would be so easy
> 
>
https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.cpp
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.c...
> Source/core/svg/SVGElement.cpp:621: if (ShadowRoot* root =
> containingShadowRoot()) {
> What if the <use> element is in a user-created shadow tree instead of the user
> agent shadow tree we create under <use>? I think this will return the wrong
> value.
> 
> Can we use correspondingElement()->userAgentShadowRoot()->host()?

Well if I get it right we do not allow user-created shadow tree's atm, from
SVGElement.cpp:

virtual bool areAuthorShadowsAllowed() const OVERRIDE FINAL { return false; }

However it is probably good to be future proof so I'll have a look.

[pdr., 2014-05-03 22:22:41]

On 2014/05/03 22:08:52, rwlbuis wrote:
> On 2014/05/03 21:55:26, pdr wrote:
> > Wow! Didn't think this would be so easy
> > 
> >
>
https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.cpp
> > File Source/core/svg/SVGElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.c...
> > Source/core/svg/SVGElement.cpp:621: if (ShadowRoot* root =
> > containingShadowRoot()) {
> > What if the <use> element is in a user-created shadow tree instead of the
user
> > agent shadow tree we create under <use>? I think this will return the wrong
> > value.
> > 
> > Can we use correspondingElement()->userAgentShadowRoot()->host()?
> 
> Well if I get it right we do not allow user-created shadow tree's atm, from
> SVGElement.cpp:
> 
> virtual bool areAuthorShadowsAllowed() const OVERRIDE FINAL { return false; }
> 
> However it is probably good to be future proof so I'll have a look.

It's true that author shadow trees aren't allowed in SVG subtrees but we can
still create them in HTML such that a <use> element ends up inside an author
shadow tree. See svg/dom/reference-in-shadow-tree.html for an example.

I found Chrome's devtools to be helpful in understanding some of these shadow
tree scenarios. If you use Chrome, try enabling "Show user agent shadow DOM" in
the devtools settings.

[rwlbuis, 2014-05-03 23:48:53]

On 2014/05/03 21:55:26, pdr wrote:
> Wow! Didn't think this would be so easy
> 
>
https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.cpp
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/266063002/diff/1/Source/core/svg/SVGElement.c...
> Source/core/svg/SVGElement.cpp:621: if (ShadowRoot* root =
> containingShadowRoot()) {
> What if the <use> element is in a user-created shadow tree instead of the user
> agent shadow tree we create under <use>? I think this will return the wrong
> value.

I think it is enough to test for ShadowRoot::UserAgentShadowRoot like in
SVGElement::inUseShadowTree.

> Can we use correspondingElement()->userAgentShadowRoot()->host()?

I don't think so, correspondingElement() points into the non-shadow tree.

[pdr., 2014-05-04 00:03:03]

LGTM with nits

https://codereview.chromium.org/266063002/diff/50010/Source/core/svg/SVGEleme...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/266063002/diff/50010/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.cpp:619: SVGUseElement*
SVGElement::correspondingUseElement()
Can we now use this in inUseShadowTree?

bool SVGElement::inUseShadowTree() const
{
    return correspondingUseElement();
}

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

https://codereview.chromium.org/266063002/diff/50010/Source/core/svg/SVGEleme...
Source/core/svg/SVGElement.h:124: SVGUseElement* correspondingUseElement();
SVGUseElement* correspondingUseElement() const;

[rwlbuis, 2014-05-04 00:09:29]

On 2014/05/04 00:03:03, pdr wrote:
> LGTM with nits
> 
>
https://codereview.chromium.org/266063002/diff/50010/Source/core/svg/SVGEleme...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://codereview.chromium.org/266063002/diff/50010/Source/core/svg/SVGEleme...
> Source/core/svg/SVGElement.cpp:619: SVGUseElement*
> SVGElement::correspondingUseElement()
> Can we now use this in inUseShadowTree?
> 
> bool SVGElement::inUseShadowTree() const
> {
>     return correspondingUseElement();
> }
> 
>
https://codereview.chromium.org/266063002/diff/50010/Source/core/svg/SVGEleme...
> File Source/core/svg/SVGElement.h (right):
> 
>
https://codereview.chromium.org/266063002/diff/50010/Source/core/svg/SVGEleme...
> Source/core/svg/SVGElement.h:124: SVGUseElement* correspondingUseElement();
> SVGUseElement* correspondingUseElement() const;

Thanks! I like the suggestion, will see if it works...

[rwlbuis, 2014-05-04 00:16:41]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-05-04 00:16:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/266063002/70001

[commit-bot: I haz the power, 2014-05-04 00:33:11]

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

[commit-bot: I haz the power, 2014-05-04 00:33:11]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink

[pdr., 2014-05-04 00:34:38]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-05-04 00:34:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/266063002/70001

[commit-bot: I haz the power, 2014-05-04 04:48:52]

Change committed as 173275