Bug 101820 – axObjectCache code is more complicated than necessary

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

axObjectCache code is more complicated than necessary

[Adam Barth]

Created attachment 173416 [details]
Patch

[Darin Adler]

Comment on attachment 173416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173416&action=review

> Source/WebCore/dom/Document.cpp:2204
> +    // Clear the cache member variable before calling delete because attempts
> +    // are made to access it during destruction.
> +    OwnPtr<AXObjectCache> axObjectCache = topDocument()->m_axObjectCache.release();

No need to put the result of release into a local variable. This can just be:

    topDocument()->m_axObjectCache.release();

> Source/WebCore/dom/Document.cpp:2223
> +    Document* document = topDocument();
> +    ASSERT(document == this || !m_axObjectCache);
> +    if (document->m_axObjectCache)
> +        return document->m_axObjectCache.get();
> +    document->m_axObjectCache = adoptPtr(new AXObjectCache(this));
> +    return document->m_axObjectCache.get();

I prefer to write such things as:

    if (!document->m_axObjectCache)
        document->m_axObjectCache = adoptPtr(new AXObjectCache(this));
    return document->m_axObjectCache.get();

But also, I suggest writing this instead:

    ASSERT(this = topDocument() || !m_axObjectCache);
    if (!m_axObjectCache)
        m_axObjectCache = adoptPtr(new AXObjectCache(this));
    return m_axObjectCache.get();

[Darin Adler]

Comment on attachment 173416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173416&action=review

>> Source/WebCore/dom/Document.cpp:2223
>> +    return document->m_axObjectCache.get();
> 
> I prefer to write such things as:
> 
>     if (!document->m_axObjectCache)
>         document->m_axObjectCache = adoptPtr(new AXObjectCache(this));
>     return document->m_axObjectCache.get();
> 
> But also, I suggest writing this instead:
> 
>     ASSERT(this = topDocument() || !m_axObjectCache);
>     if (!m_axObjectCache)
>         m_axObjectCache = adoptPtr(new AXObjectCache(this));
>     return m_axObjectCache.get();

Oops!

    ASSERT(this == topDocument() || !m_axObjectCache);

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 173416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173416&action=review
> 
> > Source/WebCore/dom/Document.cpp:2204
> > +    // Clear the cache member variable before calling delete because attempts
> > +    // are made to access it during destruction.
> > +    OwnPtr<AXObjectCache> axObjectCache = topDocument()->m_axObjectCache.release();
> 
> No need to put the result of release into a local variable. This can just be:
> 
>     topDocument()->m_axObjectCache.release();

Done.

> > Source/WebCore/dom/Document.cpp:2223
> > +    Document* document = topDocument();
> > +    ASSERT(document == this || !m_axObjectCache);
> > +    if (document->m_axObjectCache)
> > +        return document->m_axObjectCache.get();
> > +    document->m_axObjectCache = adoptPtr(new AXObjectCache(this));
> > +    return document->m_axObjectCache.get();
> 
> I prefer to write such things as:
> 
>     if (!document->m_axObjectCache)
>         document->m_axObjectCache = adoptPtr(new AXObjectCache(this));
>     return document->m_axObjectCache.get();

Done.

> But also, I suggest writing this instead:
> 
>     ASSERT(this = topDocument() || !m_axObjectCache);
>     if (!m_axObjectCache)
>         m_axObjectCache = adoptPtr(new AXObjectCache(this));
>     return m_axObjectCache.get();

I'm nit sure I understand.  If Document::axObjectCache() is called on a non-top document, won't this code create a new object cache for that document?

It's a bit strange that we have a m_axObjectCache pointer for every Document but we only use it for top documents.  It seems like it might be better to store that pointer on Page.

I wandered into this code because Document::axObjectCache is showing up on profiles for Dromeo's dom-modify benchmark (because it is called in Element::attributeChanged).  I thing was I would first modernize the code to use OwnPtr and then see about getting it off the profile.

[Adam Barth]

s/nit/not/

[Adam Barth]

Created attachment 173520 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173520 [details]
Patch for landing

Clearing flags on attachment: 173520

Committed r134178: <http://trac.webkit.org/changeset/134178>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #4)
> I'm nit sure I understand.  If Document::axObjectCache() is called on a non-top document, won't this code create a new object cache for that document?

Yes, I misunderstood the code.

> It's a bit strange that we have a m_axObjectCache pointer for every Document but we only use it for top documents.  It seems like it might be better to store that pointer on Page.

Possibly. Page comes with its own problems because it can be 0 for during the end of a frame’s lifetime. But we did create Page originally to try to move top document and top frame data structures out of the Frame/Document family of classes.

> I wandered into this code because Document::axObjectCache is showing up on profiles for Dromeo's dom-modify benchmark (because it is called in Element::attributeChanged).  I thing was I would first modernize the code to use OwnPtr and then see about getting it off the profile.

Sure does sound like a great plan!

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #4)
> > I'm nit sure I understand.  If Document::axObjectCache() is called on a non-top document, won't this code create a new object cache for that document?
> 
> Yes, I misunderstood the code.

If I had understood better what was going on, I would have suggested naming the local variable topDocument:

    Document* topDocument = this->topDocument();

[Adam Barth]

> If I had understood better what was going on, I would have suggested naming the local variable topDocument:

Good idea.  Will fix shortly.

[Adam Barth]

Name change is in bug 101966.