[webkit-reviews] review denied: [Bug 20980] Split off uncommonly used data from Node similar to ElementRareData : [Attachment 23700] Now with ChangeLog and a few cleanups

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Smith <catfish.man at gmail.com>'s
request for review:
Bug 20980: Split off uncommonly used data from Node similar to ElementRareData
https://bugs.webkit.org/show_bug.cgi?id=20980

Attachment 23700: Now with ChangeLog and a few cleanups
https://bugs.webkit.org/attachment.cgi?id=23700&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Great work!

 #include "Node.h"
+#include "ContainerNode.h"
 
We don't need both. The Node.h include can be replaced by the ContainerNode
one. Same applies in the other places you made this change

-ContainerNode::ContainerNode(Document* doc, bool isElement)
-    : EventTargetNode(doc, isElement)
+ContainerNode::ContainerNode(Document* doc, bool isElement, bool isContainer)
+    : EventTargetNode(doc, isElement, isContainer)

+    ContainerNode(Document*, bool isElement = false, bool isContainer = true);


There's no need for the constructor of ContainerNode to take an isContainer
argument. Just pass true!

+    if (hasRareData())
+    {	 

Brace goes on the same line as the if in the Safari coding style.

 class CSSStyleDeclaration;
-class ElementRareData;
 class IntSize;
+class ElementRareData;

Please put this back into alphabetical order.

+    virtual NodeRareData* createRareData();

Please make this private instead of protected. There's no need for Element or
any class derived from it to call this function.

+#ifndef ElementRareData_h

This file needs a header.

+#include "NodeRareData.h"
+#include "Element.h"

Sort includes alphabetically.

+static inline IntSize defaultMinimumSizeForResizing()
+{
+    return IntSize(INT_MAX, INT_MAX);
+}

Functions in headers should not be marked "static" because that gives internal
linkage. Just make this inline.

+void ElementRareData::resetComputedStyle(Element* element)
+{
+    if (!m_computedStyle)
+	 return;
+    m_computedStyle->deref(element->document()->renderArena());
+    m_computedStyle = 0;
+}

This function either needs to be marked inline or moved to a .cpp file.

+    void setTabIndex(short index) { m_tabIndex = index;
m_tabIndexSetExplicitly = true; }

I think this should be named setTabIndexExplicitly.

+void Node::setFocus(bool b)
+{ 
+    ensureRareData()->m_focused = b;
+}

Maybe this have a short-circuit case if both hasRareData() and b are false?

+    if(hasRareData())

Needs a space after "if". Also, probably easier to read if written with &&.

-    virtual bool childTypeAllowed(NodeType) { return false; }
+    virtual bool childTypeAllowed(NodeType t) { return false; }

Argument name "t" should be omitted here. This should not have been changed.

+    virtual NodeRareData* createRareData();

This function should be private.

+    NodeRareData* rareData();
+    const NodeRareData* rareData() const;

One of these should be an inline that calls the other and does const_cast.

+#ifndef NodeRareData_h

This file needs a header.

+#include "StringHash.h"
+#include <wtf/HashSet.h>
+#include <wtf/OwnPtr.h>
+#include "DynamicNodeList.h"

Sort includes alphabetically.

review- because there are a lot of suggested changes, but this is looking good
and getting close to ready to land.