Bug 92257 – 2% of all samples running grid demo show up in StyleResolver::canShareStyleWithElement, 20% of those due to getAttribute instead of fastGetAttribute

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

2% of all samples running grid demo show up in StyleResolver::canShareStyleWithElement, 20% of those due to getAttribute instead of fastGetAttribute

[Eric Seidel (no email)]

Created attachment 154362 [details]
Patch

[Eric Seidel (no email)]

The last time this was touched was for kling's rollout in bug 80245.

[Eric Seidel (no email)]

Created attachment 154363 [details]
the grid demo

[Eric Seidel (no email)]

Thoughts, Kling?

[Eric Seidel (no email)]

This is actually higher than I had originally measured, because the getAttribute() call itself shows up as nearly 1%!

[Andreas Kling]

(In reply to comment #4)
> Thoughts, Kling?

This looks like a great low-hanging fruit to go after, though I'm afraid that this won't properly serialize the typeAttr on an SVG element that's the target of a property animation.

We have this check earlier in the function:
if (element->isSVGElement() && static_cast<SVGElement*>(element)->animatedSMILStyleProperties())
    return false;

But animatedSMILStyleProperties() is only concerned with animations on CSS properties.

CC'ing some SVG folks who might know more about this.

[Eric Seidel (no email)]

OK.  I suspect we can detect that case and only take the slow path when necessary.  WildFox? pdr?

[Nikolas Zimmermann]

(In reply to comment #6)
> (In reply to comment #4)
> > Thoughts, Kling?
> 
> This looks like a great low-hanging fruit to go after, though I'm afraid that this won't properly serialize the typeAttr on an SVG element that's the target of a property animation.
SVGs typeAttr is SVGNames::typeAttr, and shouldn't be affected by this at all.
We don't use HTMLNames::typeAttr in SVG except for <script>, which doesn't have a renderer and thus no style as well.

I think it's safe from the SVG side.

[Eric Seidel (no email)]

DEFINE_GLOBAL(QualifiedName, typeAttr, nullAtom, "type", svgNamespaceURI)
vs.
DEFINE_GLOBAL(QualifiedName, typeAttr, nullAtom, "type", xhtmlNamespaceURI)

So they only differ in namespace.

inline Attribute* findAttributeInVector(const Vector<Attribute>& attributes, const QualifiedName& name)
{
    for (unsigned i = 0; i < attributes.size(); ++i) {
        if (attributes.at(i).name().matches(name))
            return &const_cast<Vector<Attribute>& >(attributes).at(i);
    }
    return 0;
}

uses 

    bool matches(const QualifiedName& other) const { return m_impl == other.m_impl || (localName() == other.localName() && namespaceURI() == other.namespaceURI()); }


Which does use namespace.

So it appears that HTMLNames::typeAttr and SVGNames::typeAttr function independently, as WildFox suggests.  Meaning this patch should have no affect on SVG.

[Eric Seidel (no email)]

Woh, wait a sec.  HTMLNames later initializes typeAttr to not include the namespace:

    new ((void*)&typeAttr) QualifiedName(nullAtom, "type", nullAtom);


Clearly makeNames needs to be fixed!

Similarly SVGNames does:

    new ((void*)&typeAttr) QualifiedName(nullAtom, "type", nullAtom);

[Eric Seidel (no email)]

Created attachment 154591 [details]
Patch

[Andreas Kling]

Comment on attachment 154591 [details]
Patch

Fabulous! r=me

[Eric Seidel (no email)]

Created attachment 154593 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154593 [details]
Patch for landing

Clearing flags on attachment: 154593

Committed r123730: <http://trac.webkit.org/changeset/123730>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.