[webkit-reviews] review denied: [Bug 4980] CSS2: Counters not supported : [Attachment 10763] Patch with minor adjustments

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 4980: CSS2: Counters not supported
http://bugzilla.opendarwin.org/show_bug.cgi?id=4980

Attachment 10763: Patch with minor adjustments
http://bugzilla.opendarwin.org/attachment.cgi?id=10763&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+    CounterNode *newNode = lookupCounter(counter);

Need to move the * on that one.

The function named getCounter I would name findCounter. I find it very
difficult to understand the "view" and "counters" parameters to that function.
Perhaps they need different names.

I don't understand why a linked list is a good data structure for something we
are looking up by name.

The function destroyCounters walks the linked list in what seems to be a
slightly obfuscated way.

The class CounterList is not well named for a linked list node. I think it
should be named CounterListNode perhaps.

Why does CounterList have one member named m_counterNode and two others named
counter and next. It's a brand new class so its members should be named
consistently.

Why does this brand new code use DeprecatedString? Performance will be better
if it doesn't.

+    RenderCounter(Node* node, CounterData* counter);
+    CounterResetNode(RenderObject* o);

We omit parameter names in cases like this.

+#ifndef _Counter_Reset_Node_h_

Names with a leading underscore like this are reserved for the implementation.
Please leave it off.

+    virtual void insertAfter (CounterNode* newChild, CounterNode* refChild);
+    virtual void removeChild (CounterNode* oldChild);

We don't put spaces before parentheses like this.

In RenderCounter.h:

+#include "config.h"
+#include "CounterNode.h"
+#include "CounterResetNode.h"
+#include "RenderCounter.h"
+#include "RenderStyle.h"

We put the include of the file's own header first to make sure it compiles
standalone. So RenderCounter.h needs to be first before the other headers.

+using namespace WebCore;

The entire file implements something in namespace WebCore, so it should be
inside a namespace WebCore declaration (with braces), not after a "using"
declaration.

+// -------------------------------------------------------------------------

I don't think this long line adds anything.

+CounterNode::CounterNode(RenderObject *o)
+    : m_hasCounters(false), m_isVisual(false),
+      m_value(0), m_count(0), m_parent(0), m_previous(0), m_next(0),
+      m_renderer(o) {}

Strange formatting here.

I think the counter nodes and counter list nodes should be arena-allocated like
the rest of the render tree.

+		 String v =
static_cast<Element*>(element())->getAttribute("value");
+		 if (!v.isEmpty()) {

The above code is unnecessarily inefficient. Instead it should use const
AtomicString& for v.

\ No newline at end of file

Should not have any of those.

+	 if(!value->isValueList())

I see a lot of if statements without spaces after the "if" like this one.

Sorry, I didn't get through every last bit of the patch, but those are my
comments so far.