[webkit-reviews] review granted: [Bug 33696] let's cache nodelists instead of dynamicnodelist::cache : [Attachment 51776] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 26 14:12:55 PDT 2010
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
More information about the webkit-reviews
mailing list