[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 #63 from anton muhin <antonm at chromium.org>  2010-03-29 08:13:47 PST ---
(In reply to comment #48)
> (From update of attachment 51776 [details])
> > +void Node::markCachedNodeLists(JSC::MarkStack& markStack, JSC::JSGlobalData& globalData)
> > +{
> > +    // NodeLists may be present.  If so, they need to be marked.
> > +    if (!hasRareData())
> > +        return;
> > +
> > +    if (NodeListsNodeData* nodeLists = rareData()->nodeLists()) {
> 
> Thanks for putting the definition inside the if statement.
> 
> Now that this is in a separate function we could instead use an early return
> for this instead, which I think is slightly better although the definition ends
> up outside the if statement in that case.
> 
> But the improvement is so slight that I almost don't want to mention it because
> I want to see this patch landed!

Done.

> > +        markNodeLists(nodeLists->m_classNodeListCache, markStack, globalData);
> > +        markNodeLists(nodeLists->m_nameNodeListCache, markStack, globalData);
> > +        markNodeLists(nodeLists->m_tagNodeListCache, markStack, globalData);
> > +    }
> > +}
> > +#if USE(JSC)
> > +namespace JSC {
> > +
> > +class JSGlobalData;
> > +class MarkStack;
> > +
> > +}
> > +#endif
> 
> Correct formatting for this is:
> 
> #if USE(JSC)
> namespace JSC {
>     class JSGlobalData;
>     class MarkStack;
> }
> #endif
> 
> Even though we don't indent the entire contents of the header when it's in a
> namespace, we do indent inside namespaces other than the ones that enclose the
> entire header, like this one. See examples in almost every header file that
> forward declares classes from the JSC namespace.

Done, but now style checking script barks at me.  And I used 4 space indent
instead of 2 spaces, was I wrong?

> 
> > +#include "ClassNodeList.h"
> >  #include "DynamicNodeList.h"
> >  #include "EventListener.h"
> > +#include "NameNodeList.h"
> > +#include "QualifiedName.h"
> >  #include "RegisteredEventListener.h"
> >  #include "StringHash.h"
> > -#include "QualifiedName.h"
> > +#include "TagNodeList.h"
> 
> I'm kind of sad we have to add these includes rather than getting away with
> forward declaring more of these types, but I presume you did that because you
> have to.

Alas, PassRefPtr requires full definition.

> How close are we to entirely getting rid of DynamicNodeList::Caches? What uses
> it now?

I hoped to do that as well, but that was necessary for child list.  I'll try to
kill it later (if possible, ok?)

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