[Webkit-unassigned] [Bug 33696] let's cache nodelists instead of dynamicnodelist::cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33696





--- Comment #103 from anton muhin <antonm at chromium.org>  2010-04-28 06:30:45 PST ---
(In reply to comment #102)
> (From update of attachment 54443 [details])
> > +    , m_classNamesOriginal(classNames)
> 
> Instead of adding this and making every ClassNodeList bigger, you should add an
> originalString() function to the SpaceSplitString class, which already keeps a
> copy of the original string. (If you kept this, then you'd want to give it a
> better name.)

I've started what you proposed but noticed that
SpaceSplitStringData::createVector mutates string:

 49     if (m_shouldFoldCase && hasNonASCIIOrUpper(m_string))
 50         m_string = m_string.foldCase();

So for now I decided to keep a copy (renamed to m_originalClassNames), but I'll
do something about it if you still prefer to fetch original string from the
SpaceSplitString.  After all I could probably use it as a key (just thinking)

> 
> > +    ASSERT(list->hasOwnCaches());
> > +    UNUSED_PARAM(list);

Thanks a lot for pointing this out, fixed.

> 
> The ASSERT_UNUSED macro should be used in cases like this.
> 
> > +    ASSERT(list == data->m_classNodeListCache.get(className));
> 
> The ASSERT_UNUSED macro should be used in cases like this.
> 
> > +    ASSERT(list->hasOwnCaches());
> > +    UNUSED_PARAM(list);
> 
> The ASSERT_UNUSED macro should be used in cases like this.
> 
> > +    ASSERT(list == data->m_nameNodeListCache.get(nodeName));
> 
> The ASSERT_UNUSED macro should be used in cases like this.
> 
> > +    ASSERT(list->hasOwnCaches());
> > +    UNUSED_PARAM(list);
> 
> The ASSERT_UNUSED macro should be used in cases like this.
> 
> > +    ASSERT(list == data->m_tagNodeListCache.get(name));
> 
> The ASSERT_UNUSED macro should be used in cases like this.

Ditto for all above.

> 
> > +    PassRefPtr<TagNodeList> list = TagNodeList::create(this, namespaceURI.isEmpty() ? nullAtom : namespaceURI, localNameAtom);
> 
> Local variables should have a type of RefPtr, not PassRefPtr. See
> <http://webkit.org/coding/RefPtr.html>.
> 
> > +    PassRefPtr<NameNodeList> list = NameNodeList::create(this, elementName);
> 
> Local variables should have a type of RefPtr, not PassRefPtr. See
> <http://webkit.org/coding/RefPtr.html>.

Thanks a lot, fixed.

> 
> > +    m_rootNode->removeCachedTagNodeList(this, QualifiedName(nullAtom, m_localName, m_namespaceURI));
> 
> We might want to store this as a QualifiedName in the first place.

Directly in the TagNodeList object like I currently do for ClassNodeList?

I'll upload new version of the patch when it'll pass layout tests.

Thanks a lot for comments, Darin.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.