[Webkit-unassigned] [Bug 31213] The CounterNode class does not support all methods necessary to efficiently update the counter tree as needed per CSS2.1

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31213


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42766|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2009-11-09 16:57:44 PDT ---
(From update of attachment 42766)
> +    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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.