[webkit-reviews] review denied: [Bug 4980] CSS2: Counters not supported : [Attachment 10831] another round

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Sat Sep 30 14:24:19 PDT 2006


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.webkit.org/show_bug.cgi?id=4980

Attachment 10831: another round
http://bugzilla.webkit.org/attachment.cgi?id=10831&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looking good!

I checked, and every use of RenderObject::hasCounter is immediately followed by
a call to RenderObject::findCounter. Perhaps we should just add yet another
boolean parameter to findCounter to determine whether or not it should create a
counter if it doesn't already exist. Then we could eliminate hasCounter
entirely.

Is there a way to avoid recounting everything in the tree as the tree is
initially parsed and when the tree is destroyed and the entire document is
going away? I'm worried that there's too much recounting.

+    if (pos >= m_length) {
+	 append(*str);
+	 return;
+    }

That needs to be append(str, length), not append(*str), which will append only
the first character!

+	 int newlen = m_length + length;

newlen should be a size_t rather than an int.

+static RenderObjectsToCounterNodeMaps* getRenderObjectsToCounterNodeMaps()

This could return a reference instead of a pointer. Either way is OK.

+		 counterNode->remove();
+		 delete counterNode;
+		 counterNode = 0;

1) No reason to set counterNode to 0.

2) Why doesn't the CounterNode destructor handle the counterNode->remove()
part? If it did, we could go back to writing this more simply using
deleteAllValues.

+    int val = 0;
+    RenderObjectsToCounterNodeMaps* objectsMap =
getRenderObjectsToCounterNodeMaps();

The definition of "val" should really be moved down to just before it's first
used. I also think it would read beter if the values of the nodes were set
where they are allocated as needed instead of sharing a single call to
setValue().

+	 CounterNodeMap* counterNodesMap = objectsMap->get(this);
+	 if (counterNodesMap)
+	     newNode = counterNodesMap->get(counterName);

In general, I like putting the definition inside the if statement in all the
cases like this, but it's an arguably matter of style.

+    objectsMap->set(this, nodeMap);

This line should go inside the if statement. It's only needed for a
newly-created node map.

+	 RenderObject* n = !isListItem() && previousSibling() ? 
+	     previousSibling()->previousSibling() : previousSibling();

I think this reads better with the ? on the start of the second line.

+	 while (n) {
+	     if (n->hasCounter(counterName)) {
+		 current = n->findCounter(counterName);
+		 break;
+	     } else
+		 n = n->previousSibling();
+	 }

I would have written this as a for loop:

    for (; n; n = n->previousSibling())
	if (n->hasCounter(counterName)) {
	    current = n->findCounter(counterName);
	    break;
	}

I think that makes it a little easier to see what's going on.

+    bool counters = !m_counter->separator().isNull();

This local variable needs a better name. I can't see how a boolean could be
named "counters"!

+	     // Found render-sibling, now search for later counter-siblings
among its render-children

I suspect that the loop below this comment could be written much more simply by
using previousInPreOrder or a similar function. Not sure though.

I noticed this comment:

    // note: do not add unnecessary bitflags, we have 32 bit already!

You should remove it -- it's inaccurate!

+    m_item = convertValueToType(value, total,
(EListStyleType)m_counter->listStyle());

+	     m_item = convertValueToType(value, total,
(EListStyleType)m_counter->listStyle()) 

You should remove the typecasts since they're not needed any more.

+	     m_item = convertValueToType(value, total,
(EListStyleType)m_counter->listStyle()) 
+		 + m_counter->separator() + m_item;

How about a += here instead of an =? Does this handle the overflow case? Is
that needed?

+    str = new StringImpl(m_item.characters(), m_item.length());

Can this be str = m_item.impl()->copy() instead?

+    , m_next (0)

Stray space here.

The CounterNode::recountAndGetNext does not seem to handle overflow. Is that
OK?

+	 assert(newChild->m_next->m_previous == refChild);

Should use ASSERT, not asssert.

+	 assert (m_last == refChild);

Should use ASSERT and omit the extra space.

+// Implementation of counter-increment and counter-content

What good is that comment?

+    bool hasSeparator() const { return m_hasSeparator; };
+    bool willNeedLayout() const { return m_willNeedLayout; };
+    void setWillNeedLayout() { m_willNeedLayout = true; };
+    void setRenderer(RenderObject* o) { m_renderer = o; };
+    RenderObject* renderer() const { return m_renderer; };

Remove incorrect trailing semicolons here, please.

+	 ASSERT(pair);

Should move this up one line so we assert before we dereference pair (two
places in RenderStyle).

+    RefPtr<CSSValue> parsedValue;

Why not put parsedValue back inside the loop where it used to be? Is there a
reason to move it out? To avoid a tiny bit of reference count churn, the code
below this should be append(parsedValue.release()) instead of just
append(parsedValue). Same with the call to addProperty, which should pass
values.release() instead of values.

These comments are all about optional cleanup and obscure edge case questions
except for the append(*str) mistake, but I need to do a review- because of that
mistake.



More information about the webkit-reviews mailing list