[webkit-reviews] review granted: [Bug 33696] let's cache nodelists instead of dynamicnodelist::cache : [Attachment 51776] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted anton muhin <antonm at chromium.org>'s
request for review:
Bug 33696: let's cache nodelists instead of dynamicnodelist::cache
https://bugs.webkit.org/show_bug.cgi?id=33696

Attachment 51776: Patch
https://bugs.webkit.org/attachment.cgi?id=51776&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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