[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #51776|review?                     |review+
               Flag|                            |




--- Comment #48 from Darin Adler <darin at apple.com>  2010-03-26 14:12:55 PST ---
(From update of attachment 51776)
> +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!

> +        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.

> +#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.

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

r=me as-is despite my comments above

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