80396 - needed helper debug method to list all frames from any frame

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Alexandru Savulov]

a method is needed to make the dump of all the frames possible when calling it 
from any frame (e.g. you may want to dump the frames beginning with the 
rootframe but only within the nsTableCellFrame::Reflow)

Comment 1

[Alexandru Savulov]

Attachment: here is the patch file with the new method nsIFrameDebug::RootFrameList

Comment 2

[Marc Attinasi]

Some comments on the patch:

1) argument names always start with 'a', so 'out' should be 'aOut'
2) assert or check for null the argument 'aPresContext' before dereference
3) the comment 
   // frames are not really COMs (AddRef and Release are private)
  seems out of place in a frame class (unnecessary I think)
4) did you mean to ignore the return codes and always return NS_OK?

Otherwise, the patch looks good!

Comment 3

[Alexandru Savulov]

Attachment: modified patch to check in - see next comment

Comment 4

[Alexandru Savulov]

I added null pointer check and made the code consistent in the method. however 
the List method (still) does not check for null pointers args. I didn't changed 
that be cause of overloading methods in derivates. This should be a sepparate 
issue.

Comment 5

[rbs]

That NS_IMETHOD look a bit akward to me since the method is doing
rootFrame->List(), and has little do with the 'this' frame from where it will be
called. Since an NS_IMETHOD isn't needed to do that, what you probably want is a:
static void RootList(nsIPresContext* aPresContext)

Comment 6

[Marc Attinasi]

I think I'd prefer:

NS_IMETHOD nsIPresShell::RootList(void);

Comment 7

[rbs]

You mean:
#ifdef NS_DEBUG
...
#endif

Comment 8

[Marc Attinasi]

Absolutely, DEBUG only, thanks for the clarification.

Alex, what do you think about moving that call to the PresShell?

Comment 9

[rbs]

While at it, what about DebugRootList(void) -- like other debugging methods
so far.

Marc, but looking again, I guess what Alex wanted was a one-off method
that he could call without worrying about the necessary setup to do so.
With this method in the pres shell however, he will still have to
to do aPresContext->GetShell(), etc. If I am following correctly, he
wanted a much simplified helper.

Comment 10

[Marc Attinasi]

Personally, I think getting the shell is pretty easy given a presContext, but it
will result in a bunch of block more-or-less like:

nsCOMPtr<nsIPresShell> shell;
aPresContext->GetShell(getter_AddRefs(shell));
if (shell) shell->DebugRootList();

Not too bad, really, but if it is more than you want, then put it on the
presContext. I'm agnostic. 

Comment 11

[rbs]

Why not leave that as a static in nsIFrameDebug as Alex originally wanted?

Comment 12

[Alexandru Savulov]

Here is another improvement I'm thinking about: nsIFrameDebug could provide a 
method that allows listing the frames beginning from one of the frames that 
contains the subtree where the actual frame resides based on the type of the 
containing frame and the number of upwards occurences of that type of frame.

e.g:

NS_IMETHOD ListFromFrame((nsIPresContext* aPresContext, nsIAtom* aType, PRInt32 
aPass, FILE* out, PRInt32 aIndent)

In this way the presence of the method in nsIFrameDebug is legitimated, and 
there is the possibility to limit the dump not to list everything, but only what 
is of interrest.

The method will traverse the tree upwards (using GetParent) and stops as soon as 
the aPass-th occurence of a frame of type aType has been found or the root of 
the tree has been reached. Passing aPass < 1 results in either dumping the whole 
tree or error (dumping the whole tree is what I prefer). Wrong types could also 
dump the whole tree.

What about this variant?

Comment 13

[rbs]

I am not sure if there will be enough users of such a highly specialized 
function as you describe. Maybe you could just write a localized helper that you 
start to use for what you want.

Comment 14

[Alexandru Savulov]

ok... let's forget the hi-specialized version and go back to the old one.

the method is not bound to a particular frame, needs only the pointer to a 
nsPresContext and could be called from anywhere a pres context is available.

so what do you guys think about:

class nsIFrameDebug : public nsISupports {
...
static NS_LAYOUT void RootFrameList(nsIPresContext* aPresContext, FILE* out, 
PRInt32 aIndent);
...
};

I would like to stick to this version be cause it covers the most important of 
your requrements too. (static, part of the nsIFrameDebug, ifdef NS_DEBUG, easy 
to use , no additional lines of code at the call locations).

please let me know if everybody agrees with this one. 
(would be a step forward for me because I also need stuff for revisions in order 
to get closer to CVS write rights :-)

Comment 15

[Marc Attinasi]

So, the last patch (ID=34156) but make it static? OK by me: sr=attinasi

Comment 16

[Marc Attinasi]

sorry, patch ID=34584 - the one from 05/15/01

Comment 17

[rbs]

r=rbs (for the static version of patch ID=34584)

Comment 18

[Alexandru Savulov]

Attachment: the static version of the patch

Comment 19

[Marc Attinasi]

Patch checked in. Alex, you can mark this one fixed now (and update your CVS
tracker).

Comment 20

[Alexandru Savulov]

mark as FIXED (another check-in for me on the way to CVS read/write access)

Comment 21

[Marc Attinasi]

verifying: checked CVS logs and it is there.