Counters aren't updated when a new counter-increment is added

• Component: CSS
• Status: RESOLVED DUPLICATE

[Shinichiro Hamaji]

This is the first step of Bug 23262.

The values of counters aren't updated when a new counter-increment element is added. Firefox and IE handle the test URL correctly.

[Shinichiro Hamaji]

Created attachment 41401 [details]
Patch v1

[Eric Seidel (no email)]

Comment on attachment 41401 [details]
Patch v1

Shouldn't this be dumpAsText?

[Shinichiro Hamaji]

Comment on attachment 41401 [details]
Patch v1

As same as tests of Bug 23262, innerText doesn't output counter values.

I looked how innerText works a bit and found it is not easy to change innerText to output counter values. innerText uses TextIterator to gather texts and TextIterator doesn't care about generated contents like counters.

Another idea is adding layoutTestController.dumpCounterNodes(). I think this is reasonable. I'll file another bug. For now, I remove review? bit.

[Shinichiro Hamaji]

Created attachment 42237 [details]
Patch v2

[Shinichiro Hamaji]

OK, as now we have counterValueForElementById, we can test this with dumpAsText.

[Darin Adler]

Comment on attachment 42237 [details]
Patch v2

> +void RenderCounter::addCounterNodes(RenderObject* object) {

Brace needs to be on the following line, not the same line.

I don’t understand the high level concept of this patch enough to review it. I believe the idea here is that it's important to explicitly create some counter nodes in certain cases. So you added a function that calls the counter function in RenderCounter.cpp because of its side effect. But I don't understand why creating more counter nodes is the way to fix a bug.

What is the nature of the bug?

Further, what does isRooted have to do with any of this? It seems clearly wrong to be walking up the entire render tree looking for a RenderView in this code.

[Shinichiro Hamaji]

Created attachment 42339 [details]
Patch v3

[Shinichiro Hamaji]

I'll describe this issue and why I chose this way.

Now, counter values are calculated when their originalText() are called. originalText() calls counter() and it traverses the tree in ascending order to update counter values. This approach works if there are no dynamic changes. 

However, if a new increment element (say [I]) is inserted before an RenderCounter element (say [A]) and no other RenderCounter are inserted after the increment element, the counter value of [A] won't be updated because originalText() for [A] doesn't trigger counter() and it just uses the value of counter node which are already constructed.

(Note that if another RenderCounter (say [B]) is added after [I], [B]'s originalText() triggers counter(), notices [I] is added, creates a counter node for [I], and updates all counter values including [A]'s)

I came up with two solutions for this:

- Insert counter nodes when counter elements are added and this addition will update the values of descendants if needed. This is what my patch is doing.
- When a counter element is added, mark descendants of the added element as dirty and make the counter values to be updated in the next call of originalText(). If we can mark descendants as dirty, I think we can update the values of counters when we mark. That's why I didn't choose this way.

Of course, there may be another better way which fixes this issue. If someone kindly suggest better ways, I'm happy to fix my patch.

As for extra counter nodes, my patch doesn't create extra counter nodes actually. The counter() function doesn't create new counter nodes if corresponding counter node already exists.

> Further, what does isRooted have to do with any of this? It seems clearly wrong
> to be walking up the entire render tree looking for a RenderView in this code.

Ah, I think !isAnonymous() is much better here. What I want to do is

- for increment and reset, we create counter nodes when these nodes are added
- for content:counter(...), we create counter nodes when their originalText is called(). Because :before and :after are not connected to the root node in RenderObject::addChild(), we cannot create counter nodes for them with correct values here. Deferring counter node creation for them are OK because 1. they don't change the values of descendants as they aren't increment nor reset 2. originalText() will be called for them and their values will be calculated eventually.

I used isRooted() because of the second item, but I've noticed !isAnonymous() is sufficient for this purpose so I changed to use it.

> Brace needs to be on the following line, not the same line.

Sorry for this mistake... I often forgot this rule (and forgot to use check-webkit-style as well)

[Darin Adler]

Comment on attachment 42339 [details]
Patch v3

It seems more elegant to me to delete the possibly-invalid counter nodes than to create new counter nodes just because something is going into the tree. The counter value may never be used and it seems unfortunate to compute its value explicitly instead of lazily. That having been said, the fix seems OK to me and won't have any effect on pages not using counters, so I'll say r=me

I think the name of the "counter" function, which I named, is extremely weak. It's really unclear you would call it to create a counter for its side effects.

[Shinichiro Hamaji]

Created attachment 42751 [details]
Patch v4

[Shinichiro Hamaji]

(In reply to comment #9)
> (From update of attachment 42339 [details])
> It seems more elegant to me to delete the possibly-invalid counter nodes than
> to create new counter nodes just because something is going into the tree. The
> counter value may never be used and it seems unfortunate to compute its value
> explicitly instead of lazily. That having been said, the fix seems OK to me and
> won't have any effect on pages not using counters, so I'll say r=me

OK, I uploaded another patch with invalidating approach. Indeed, this approach would be faster than the previous approach for usual (no-dynamic-change) cases. Thank you for the comment!
 
> I think the name of the "counter" function, which I named, is extremely weak.
> It's really unclear you would call it to create a counter for its side effects.

Agreed. I'd like to create another patch to fix this naming, but I couldn't figure out a good name for this function. Do you have any good idea for this? createCounterNodeIfNeeded or something like this?

[Darin Adler]

(In reply to comment #11)
> Agreed. I'd like to create another patch to fix this naming, but I couldn't
> figure out a good name for this function. Do you have any good idea for this?
> createCounterNodeIfNeeded or something like this?

I think just calling it create without the "if needed" may be OK even if it doesn't always create a new counter node. To avoid the "create" rule for object ownership, we should probably call it makeCounterNode.

[Darin Adler]

Comment on attachment 42751 [details]
Patch v4

Better. r=me

[Shinichiro Hamaji]

Comment on attachment 42751 [details]
Patch v4

Putting cq- as this patch cannot be committed until Bug 31286 is resolved.

[Shinichiro Hamaji]

Comment on attachment 42751 [details]
Patch v4

Rejecting patch 42751 from commit-queue.

Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
gfzddrsaowyo/WebCorePrefix.h -c /Users/hamaji/GitKit/WebCore/xml/XMLHttpRequest.cpp -o /Users/hamaji/GitKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/XMLHttpRequest.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/hamaji/GitKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/RenderCounter.o /Users/hamaji/GitKit/WebCore/rendering/RenderCounter.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)

[Shinichiro Hamaji]

Conflicted with Bug 31213. As it seems Carol is also working for this issue, I'll discuss on Bug 11031.

[Shinichiro Hamaji]

Created attachment 43300 [details]
Resolving conflict for r50960

[Shinichiro Hamaji]

(In reply to comment #17)
> Created an attachment (id=43300) [details]
> Resolving conflict for r50960

I added a comment on this patch in Bug 11031. I don't set the review? bit for this patch until the discussion in Bug 11031 finishes.

[Eric Seidel (no email)]

PIng?  Is Patch v4 waiting to be landed?

[Shinichiro Hamaji]

Sorry, this bug will be fixed by Carol.

*** This bug has been marked as a duplicate of bug 11031 ***