Bug 31212 – showTree(CounterNode*) generates too little info and has too many spaces.

• Component: New Bugs
• Status: RESOLVED FIXED

[Carol Szabo]

I worked on fixing the issue of DOM update reactivity in webkit's counter implementation. I found the information provided by showTree(CounterNode*) insufficient for my needs and I did not like the usage of tab for indentation as it would create too much whitespace. Here I am proposing an alternative (hopefully better) solution. Patch coming soon.

[Carol Szabo]

Created attachment 42671 [details]
Proposed Patch

Adds information about the address of the counters in the tree, the renderer that they are attached to.
Also allows for the inspection of the integrity of the tree structure.

[Eric Seidel (no email)]

It would be helpful if you could show an example of old vs. new behavior.

[Carol Szabo]

New behavior:
 0x09432630 reset: 0 0 P:0x00000000 PS:0x00000000 NS:0x00000000 R:0x0942D6EC
   0x0942DC60 increment: 1 1 P:0x09432630 PS:0x00000000 NS:0x0942C660 R:0x0942ECDC
   0x0942C660 increment: 0 1 P:0x09432630 PS:0x0942DC60 NS:0x09438EE0 R:0x0942EE64
*  0x09438EE0 reset: 0 1 P:0x09432630 PS:0x0942C660 NS:0x00000000 R:0x0942F8E4
     0x09439178 increment: 1 1 P:0x09438EE0 PS:0x00000000 NS:0x00000000 R:0x09430164

Where P stands for parent, PS for previous sibling, NS for next sibling R for the renderer that the counter is attached to.
The first hex number is the address of the counter dumped.

OldBehavior:
reset: 0 0
        increment: 1 1
        increment: 0 1
        reset: 0 1
                increment: 1 1

The decimal numbers represent the value and countInParent members of the counter.

Note: On modern terminals that support more than 100 characters per line the text in the new format would not wrap till 14 levels deep nesting.

[Eric Seidel (no email)]

Wow. The new format looks super-confusing!  I think we should have other editing folks (those who use these methods regularly for debugging) weigh in here.

[Darin Adler]

Comment on attachment 42671 [details]
Proposed Patch

This is great!

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

This won't compile without warnings under gcc. The specifier %08X takes an "unsigned" as its argument, but we are passing in a pointer. The most portable way to do this is probably to use %p instead of 0x%08X. Other solutions involve converting the type by adding a typecast.

review- because this will break the build

[Darin Adler]

(In reply to comment #4)
> Wow. The new format looks super-confusing!  I think we should have other
> editing folks (those who use these methods regularly for debugging) weigh in
> here.

Counter is not about editing, so I don’t see the relevance of "editing folks".

But despite my positive comment in the review, I think that dumping all those pointers is not extremely useful. On 64-bit systems they will be even wider too.

But on the other hand, the only person who will do this is someone debugging counters. It still seems OK to change it like this.

[Eric Seidel (no email)]

Oh, I guess I took this to be the generic showTreeForNode(Node*).  My apologies for my confusion.

[Carol Szabo]

Created attachment 42762 [details]
Proposed Patch

This patch addresses Darin's comments about pointer versus int in printf.

[Darin Adler]

Comment on attachment 42762 [details]
Proposed Patch

Heh, I would have said "counter" instead of "current" -- nouns work better than adjectives for variable names.

And the best function for writing one or two characters is probably fputc rather than fwrite.

And on reflection I would probably use things like "parent", "prev" and "next" instead of "P", "PS" and "NS".

r=me as is though, no need to go over and over this!

[WebKit Commit Bot]

Comment on attachment 42762 [details]
Proposed Patch

Clearing flags on attachment: 42762

Committed r50673: <http://trac.webkit.org/changeset/50673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.