Bug 31213 – The CounterNode class does not support all methods necessary to efficiently update the counter tree as needed per CSS2.1

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Carol Szabo]

I would like to add these methods to the CounterNode in support of a larger patch that makes the counter tree update able in response to DOM changes (see bug 11031).

[Carol Szabo]

In addition to tree navigation methods for non-recursive tree traversal, methods are added to support precise renderer tree updates when the Counter tree changes.

[Carol Szabo]

Created attachment 42766 [details]
Proposed patch

I know this patch in itself does not bring much benefit, but it is the first step towards implementing fully CSS2.1 counters. The only reason it is submitted by itself is to reduce the size of the bigger change that fixes 11031 and all problems with standard CSS2.1 counter tests.

[Darin Adler]

Comment on attachment 42766 [details]
Proposed patch

> +    CounterNode* next;
> +    if (!(next = m_nextSibling)) {

This seems unnecessarily strange. How about this instead?

    CounterNode* next = m_nextSibling;
    if (next)
        return next;

I don't understand why some of these member functions are calling functions like firstChild() while others are getting at data members directly. I suggest having everything get at data members directly.

> +void CounterNode::resetRenderer(const AtomicString* identifier) const

I'd like this function to take a const AtomicString& instead.

> +    if (m_renderer && !m_renderer->documentBeingDestroyed()) {
> +        if (RenderObjectChildList* children = m_renderer->virtualChildren())
> +            children->invalidateCounters(m_renderer, identifier);
> +    }

We normally prefer the early return style rather than nested ifs.

    if (!m_renderer)
        return;

Etc.

> +void CounterNode::resetRenderers(const AtomicString* identifier) const

I'd like this function to take a const AtomicString& instead.

> +{
> +    resetRenderer(identifier);
> +    for (const CounterNode* child = m_firstChild; child ; child = child->nextInPreOrder(this))
> +        child->resetRenderer(identifier);
> +}

I suggest calling the local variable "node" instead of "child", and initializing it to "this" rather than m_firstChild. That way you won't have to have the first call to resetRenderer.

Please remove the space before the semicolon.

> +void CounterNode::recount(const AtomicString* identifier)

I'd like this function to take a const AtomicString& instead.

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

I'd like this function to take a const AtomicString& instead.

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

I'd like this function to take a const AtomicString& instead.

> -void RenderCounter::invalidate()
> +void RenderCounter::invalidate(const AtomicString* identifier)

I'd like this function to take a const AtomicString& instead. You can overload with a separate function for invalidating everything. Or give it a different name such as invalidateAllIdentifiers.

> -    m_counterNode = 0;
> -    setNeedsLayoutAndPrefWidthsRecalc();
> +    if (!identifier || m_counter.identifier() == *identifier) {
> +        m_counterNode = 0;
> +        setNeedsLayoutAndPrefWidthsRecalc();
> +    }

We normally use early return.

> -static void destroyCounterNodeChildren(AtomicStringImpl* identifier, CounterNode* node)
> +static void destroyCounterNodeChildren(const AtomicString* identifier, CounterNode* node)

I'd like this function to take a const AtomicString& instead.

> -    void invalidate();
> +    void invalidate(const AtomicString* identifier = 0);

I think this should be two different functions. Maybe overloads of the same name.

> -static void invalidateCountersInContainer(RenderObject* container)
> +static void invalidateCountersInContainer(RenderObject* container, const AtomicString* identifier)

I'd like this function to take a const AtomicString& instead.

> +    //Sometimes the counter is attached directly on the container.
> +    if (container->isCounter()) {
> +        toRenderCounter(container)->invalidate(identifier);
> +        return;
> +    }

Is the above change a bug fix? If so, where is the test for this?

You should add a space after the "//".

> -void RenderObjectChildList::invalidateCounters(RenderObject* owner)
> +void RenderObjectChildList::invalidateCounters(RenderObject* owner, const AtomicString* identifier)

I'd like this function to take a const AtomicString& instead.

> -    void invalidateCounters(RenderObject* owner);
> +    void invalidateCounters(RenderObject* owner, const AtomicString* identifier = 0);

Do we really need the special behavior here for identifier of 0?

If it is important to have a large set of functions that work only on a single identifier or possible on multiple identifiers, then I suppose we can live with a pointer, but then it seems that AtomicStringImpl* is better than const AtomicString* and I am sorry I suggested changing it.

review- because there are a few things to change here -- if I am wrong about the pointer thing, you can just set me straight.

[Carol Szabo]

Created attachment 42925 [details]
Proposed Patch

This patch addresses Darin's coding style concerns.
Regarding the code that appears to be a bug fix in RenderObjectChildren.cpp, the reason why I did not attach a test case for that code is because the code does not fix any bugs since it is not properly called yet for DOM changes, where it would be important, the test case that this code contributes to fixing is part of the fix for 11031 for which this change is a prerequisite. Until that code is committed, my code only fixes a dangling reference for some test case, but it has no visible impact.

[WebKit Commit Bot]

Comment on attachment 42925 [details]
Proposed Patch

Rejecting patch 42925 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11621 test cases.
http/tests/xmlhttprequest/access-control-and-redirects.html -> crashed

Exiting early after 1 failures. 9148 tests run.
461.20s total testing time

9147 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

[Eric Seidel (no email)]

Comment on attachment 42925 [details]
Proposed Patch

Added the crash report to bug 31461.  Let's spin again.

[WebKit Commit Bot]

Comment on attachment 42925 [details]
Proposed Patch

Clearing flags on attachment: 42925

Committed r50960: <http://trac.webkit.org/changeset/50960>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This broke the leopard Debug build.  Working on a fix now.

[Eric Seidel (no email)]

Fixed build:
Committed r50966: <http://trac.webkit.org/changeset/50966>