[webkit-reviews] review denied: [Bug 32884] CSS2.1 Counters not updated when new elements are inserted in the DOM. : [Attachment 46393] Proposed Patch; A few function name changes and performance improvements over last submission.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Carol Szabo <carol.szabo at nokia.com>'s
request for review:
Bug 32884: CSS2.1 Counters not updated when new elements are inserted in the
DOM.
https://bugs.webkit.org/show_bug.cgi?id=32884

Attachment 46393: Proposed Patch; A few function name changes and performance
improvements over last submission.
https://bugs.webkit.org/attachment.cgi?id=46393&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    for (next = first;; next = next->m_nextSibling) {

Normally we put a space between the semicolons in lines like this one.

> +    newChild->m_firstChild = newChild->m_lastChild = 0;

Normally we don't merge assignment statements together like this.

>      newChild->m_countInParent = newChild->computeCountInParent();
> +    newChild->resetRenderer(identifier);
> +    first->recount(identifier);
> +    return;
>  }

Normally we don't have a return statement at the end of a function like this.

> +    for (RenderObject* currentRenderer = object->nextInPreOrder(stayWithin);
currentRenderer;currentRenderer = currentRenderer->nextInPreOrder(stayWithin))
{

Missing space after the semicolon here.

> +	   if (!currentRenderer->m_hasCounterNodeMap || !(currentCounter =
maps.get(currentRenderer)->get(identifier.impl())))
> +	       continue;

I think this would be clearer with separate if statements. There's no real
reason to have the assignment inside an if statement and having separate ones
would make that possible.

> +    // Do not delete map even if empty as this function should not be called
when all counters for a renderer
> +    // are deleted destroyCounterNodes should be called instead. The map may
be empty only temporarely while

Typo: "temporarely".

Need punctuation between "deleted" and "destroyCounterNodes".

Is there a way to do some sort of assertion to check the assumption from this
comment?

> +void RenderCounter::rendererSubtreeAttached(RenderObject* renderer)
> +{
> +    ASSERT(renderer);
> +    RenderObject* descendant = renderer;
> +    do {
> +	   updateCounters(descendant);
> +	   descendant = descendant->nextInPreOrder(renderer);
> +    } while (descendant);
> +}
>  } // namespace WebCore

At the cost of one extra null check we could write this with a for loop, which
would be even clearer and easier to read.

Would be good to have a blank line before the end of the namespace.

Since this is for the commit-queue, I'll say review- so we can at least fix the
typos.