Bug 31213

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.

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.

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.

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.

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

Comment on attachment 42925 [details] Proposed Patch Added the crash report to bug 31461. Let's spin again.

Comment on attachment 42925 [details] Proposed Patch Clearing flags on attachment: 42925 Committed r50960: <http://trac.webkit.org/changeset/50960>

All reviewed patches have been landed. Closing bug.

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

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