Bug 31212 - showTree(CounterNode*) generates too little info and has too many spaces.
Summary: showTree(CounterNode*) generates too little info and has too many spaces.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-06 12:52 PST by Carol Szabo
Modified: 2009-11-09 11:43 PST (History)
7 users (show)

See Also:


Attachments
Proposed Patch (2.13 KB, patch)
2009-11-06 14:40 PST, Carol Szabo
darin: review-
Details | Formatted Diff | Diff
Proposed Patch (2.12 KB, patch)
2009-11-09 09:58 PST, Carol Szabo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carol Szabo 2009-11-06 12:52:13 PST
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.
Comment 1 Carol Szabo 2009-11-06 14:40:57 PST
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.
Comment 2 Eric Seidel (no email) 2009-11-06 16:36:11 PST
It would be helpful if you could show an example of old vs. new behavior.
Comment 3 Carol Szabo 2009-11-06 18:33:23 PST
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.
Comment 4 Eric Seidel (no email) 2009-11-07 00:06:28 PST
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.
Comment 5 Darin Adler 2009-11-07 11:43:22 PST
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
Comment 6 Darin Adler 2009-11-07 11:52:25 PST
(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.
Comment 7 Eric Seidel (no email) 2009-11-07 13:53:49 PST
Oh, I guess I took this to be the generic showTreeForNode(Node*).  My apologies for my confusion.
Comment 8 Carol Szabo 2009-11-09 09:58:34 PST
Created attachment 42762 [details]
Proposed Patch

This patch addresses Darin's comments about pointer versus int in printf.
Comment 9 Darin Adler 2009-11-09 10:28:10 PST
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!
Comment 10 WebKit Commit Bot 2009-11-09 11:43:25 PST
Comment on attachment 42762 [details]
Proposed Patch

Clearing flags on attachment: 42762

Committed r50673: <http://trac.webkit.org/changeset/50673>
Comment 11 WebKit Commit Bot 2009-11-09 11:43:30 PST
All reviewed patches have been landed.  Closing bug.