[webkit-reviews] review denied: [Bug 11031] Another crazy counters bug : [Attachment 42655] Proposed Patch - Fixes error in RenderObject::isEmpty as suggested by Mitz

[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 11031: Another crazy counters bug
https://bugs.webkit.org/show_bug.cgi?id=11031

Attachment 42655: Proposed Patch - Fixes error in RenderObject::isEmpty as
suggested by Mitz
https://bugs.webkit.org/attachment.cgi?id=42655&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
This patch is huge! I'm not sure I can review the whole thing. Is there any way
to do this work in smaller pieces instead of all at once? It seems to me there
are definitely many independent changes here, such as the showTreeAndMark
change, which are separable from the rest.

> +	   Tests: css2.1/counter-increment-002.html
> +		  css2.1/counter-reset-000.html
> +		  css2.1/counter-reset-002.html

New counter tests should go into "fast/css/counters". The directory named
"css2.1" is for the CSS 2.1 test suite. Not something invented by the WebKit
project, but something imported. Your new test should go in there.

Patches should not include svn:mergeinfo. You should remove those with the svn
pd command before generating the patch if they have crept in.

Many of these tests say "No newline at end of file". Please put a newline at
the end of the file.

Please make the new tests use dumpAsText if possible.

> +void CounterNode::resetRenderer(AtomicStringImpl* identifier)
> +{
> +    if(m_renderer && !m_renderer->documentBeingDestroyed())
> +	   if (RenderObjectChildList* children = m_renderer->virtualChildren())

> +	       children->invalidateCounters(m_renderer, identifier);
> +}

Coding style mistakes here. There is a missing space after "if" and missing
braces. Please read the style guide and/or use the check-webkit-style command
to find these issues.

In general it is not good to have recursive algorithms that take stack space
proportional to the depth of the render tree. And so many of these functions
work that way. We need a test case with an extremely deeply nested render tree
to see what effect that has.

> +void CounterNode::resetRenderers(AtomicStringImpl *identifier)

The * here is in the wrong place. Needs to be before the space, not after.

> +    for(CounterNode* c = m_firstChild; c ; c = c->m_nextSibling)

Need a space space here after the "for".

Should not have a space after the "c" before the ";".

We prefer words rather than letters for local variable names. The word "child"
would probably be good. I see other older code in this file that is using the
letter "c" and "o" -- sorry, that was older style.

> +	   //Relayout all counters that may be affected by this change

Comments should have a space after the "//" and a period at the end of the
sentence.

> -void CounterNode::insertAfter(CounterNode* newChild, CounterNode* refChild)
> +void CounterNode::insertAfter(CounterNode* newChild,
> +    CounterNode* refChild, AtomicStringImpl *identifier)

There's no need to break this into multiple lines. It goes against our usual
style. Also, the "*" is in the wrong place.

> +	   RefPtr<AtomicStringImpl> identifierPtr(identifier);
> +	   while (m_lastChild != refChild) {
> +	       RenderCounter::destroyCounterNode(m_lastChild->renderer(),
> +		   identifierPtr);
> +	   }

I don't understand the use of RefPtr here. And breaking up the call to
destroyCounterNode into two lines seems unhelpful.

> -void CounterNode::removeChild(CounterNode* oldChild)
> +void CounterNode::removeChild(CounterNode* oldChild,
> +    AtomicStringImpl* identifier)

Same comment about multiple lines.

> +	   fprintf(stderr, "0x%08X %s: %d %d P:0x08%X PS:0x%08X NS:0x08%X\n",
> +	       (int)c, c->isReset() ? "reset" : "increment", c->value(),
> +	       c->countInParent(), c->parent(), c->previousSibling(),
> +	       c->nextSibling());

Normally we would use static_cast, not a C-style cast.

> -    bool isReset() const { return m_isReset; }
> -    int value() const { return m_value; }
> -    int countInParent() const { return m_countInParent; }
> -    RenderObject* renderer() const { return m_renderer; }
> -
> -    CounterNode* parent() const { return m_parent; }
> -    CounterNode* previousSibling() const { return m_previousSibling; }
> -    CounterNode* nextSibling() const { return m_nextSibling; }
> -    CounterNode* firstChild() const { return m_firstChild; }
> -    CounterNode* lastChild() const { return m_lastChild; }
> -
> -    void insertAfter(CounterNode* newChild, CounterNode* beforeChild);
> -    void removeChild(CounterNode*);
> +    ALWAYS_INLINE bool isReset() const { return m_isReset || !m_parent; }
> +    ALWAYS_INLINE bool isTypeReset() const { return m_isReset; }
> +    ALWAYS_INLINE int value() const { return m_value; }
> +    ALWAYS_INLINE int countInParent() const { return m_countInParent; }
> +    ALWAYS_INLINE RenderObject* renderer() const { return m_renderer; }
> +
> +    ALWAYS_INLINE CounterNode* parent() const { return m_parent; }
> +    ALWAYS_INLINE CounterNode* previousSibling() const { return
m_previousSibling; }
> +    ALWAYS_INLINE CounterNode* nextSibling() const { return m_nextSibling; }

> +    ALWAYS_INLINE CounterNode* firstChild() const { return m_firstChild; }
> +    ALWAYS_INLINE CounterNode* lastChild() const { return m_lastChild; }

Our general rule is to use ALWAYS_INLINE only when we see an actual performance
effect and have to use it to get that effect. There are tons of simple
accessors like this and we do not plan to mark all the others with
ALWAYS_INLINE, so please don't start with these.

> +    /* identifier must match the identifier of this counter or
> +	* the renderers affected by this insertion shall not be
> +	* invalidated.
> +	*/

We use "//" for comments, and sentences with capital letters and periods.

> +    void insertAfter(CounterNode* newChild, CounterNode* beforeChild,
> +	   AtomicStringImpl* identifier);

I think we should consider using const AtomicString& instead of
AtomicStringImpl* for all these new functions.

> +/* Finds the insertion point for the counter described by counterOwner,
isReset and 
> + * counterName in the CounterNode tree for counterName and sets parent and
> + * previousSibling accordingly.
> + * The function returns true if the counter whose insertion point is
searched is NOT
> + * the root of the tree.
> + * The root of the tree is a counter reference that is not in the scope of
any other
> + * counter with the same identifier.
> + * All the counter references with the same identifier as this one that are
in
> + * children or subsequent siblings of the renderer that owns the root of the
tree
> + * form the rest of of the nodes of the tree.
> + * The root of the tree is always a reset type reference.
> + * A subtree rooted at any reset node in the tree is equivalent to all
counter 
> + * references that are in the scope of the counter or nested counter defined
by that
> + * reset node.
> + * Non-reset CounterNodes cannot have descendants.
> + */

We use // for comments, not /*.

It's nice that this comment is complete, but the long comment is hard to read.
Maybe you could say less or paragraph it in some way that makes it easier to
digest.

> +static bool findPlaceForCounter(RenderObject* counterOwner,
> +				   const AtomicString& counterName,
> +				   bool isReset, CounterNode*& parent, 
> +				   CounterNode*& previousSibling)

We do not line up declarations like this when they span multiple lines. It
would be better not to reformat unless we're trying to move the code in that
direction.

> +			       && (currentRenderer->parent() ==
counterOwner->parent())) {

I find that including parentheses in common cases like this (&& or ||
expressions combined with == involved in if statements) makes things less clear
rather than more. The idiom comes up so often it is not a source of doubt in
any real case. I suggest omitting the parentheses.

The tactical comments inside the function were all removed and replaced with a
large one at the top. Partly because of that I find the new function harder to
follow than the old, even though the total amount of comments is now greater.

> +    for (CounterMaps::iterator it = counterMaps().begin();it != end;
> +	   ++it)

This formatting is unnecessarily ugly. PUtting it all in one line would make it
easier to read. Also, please include a space after the ";". Also, need braces
around something that's more than one line.

> +	   if (it->first != object) {
> +	       nodeMap = it->second;
> +	       if (CounterNode* node = nodeMap->get(counterName.impl())) {
> +		   if (!node->parent()
> +		       && findPlaceForCounter(
> +			   const_cast<RenderObject*>(it->first),
> +			   counterName, true, newParent,
> +			   newPreviousSibling)) {

Need to find a way to do this so things aren't broken into so many lines. We
don't do it this way in WebKit code.

One way to do this since this just keeps eliminating cases is to use the "early
continue" idiom, so the main logic of the loop just continues at the outer
level instead of getting more and more deeply nested.

    if (it->first == object)
	continue;
    CounterNode* node = it->second->get(counterName.impl());
    if (!node)
	continue;
    // etc.

> -void RenderCounter::invalidate()
> +void RenderCounter::invalidate(AtomicStringImpl *identifier)
>  {
> -    m_counterNode = 0;
> -    setNeedsLayoutAndPrefWidthsRecalc();
> +    if (!identifier
> +	   || (m_counter.identifier() == (const
char*)identifier->characters())) {
> +	   m_counterNode = 0;
> +	   setNeedsLayoutAndPrefWidthsRecalc();
> +    }
>  }

Early return would be easier to read than the nested code here.

It is not correct to cast identifier->characters() to const char *, and even if
it was correct, you would need to do it with static_cast.

To compare two AtomicString instances you just use "==", no casting needed.

Patch is too big for me to review the rest at this time.

Looking forward to seeing some future iterations, hopefully in some smaller
pieces.

review- for now