[webkit-reviews] review granted: [Bug 55399] Rework object grouping to never produce huge grouper array : [Attachment 85166] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted anton muhin <antonm at chromium.org>'s
request for review:
Bug 55399: Rework object grouping to never produce huge grouper array
https://bugs.webkit.org/show_bug.cgi?id=55399

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85166&action=review

I like the approach.  This stuff is very subtle, so I'm not 100% sure that this
change is correct, but I don't see anything wrong with it.  Some very minor
style nits below.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1715
> +    # find the super descriptor

This should be a complete sentence that starts with a capital letter and ends
with a period.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1720
> +	   if ($parent eq "EventTarget") { next; }

Each statement should be on its own line.

> Source/WebCore/bindings/v8/V8GCController.cpp:237
> +    if (root->isStyleSheet())
> +	   if (Node* ownerNode = static_cast<StyleSheet*>(root)->ownerNode())
> +	       return calculateGroupId(ownerNode);

We need { } around the body of this if block.

> Source/WebCore/bindings/v8/V8GCController.cpp:281
> +	       // CSSProperty (type of items in mutable style declaration)
doesn't have a pointer to
> +	       // its parent. Therefore we cannot treat them like other
objects---traverse up to the
> +	       // root when visiting a wrapper---and need to put them into the
group directly.

I see.	We still need to walk down for these.  Ok.

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:67
> +	       

This blank line is empty.

> Source/WebCore/css/CSSRuleList.h:59
> +    StyleList* styleList()
> +    {
> +	   return m_list.get();
> +    }

You can just put this all on one line if you like.  Simple getters can be on
one line to preserve vertical space.

> Source/WebCore/css/StyleSheetList.h:57
> +    Document* document()
> +    {
> +	   return m_doc;
> +    }

Same here.