Closed Bug 490909 Opened 11 years ago Closed 11 years ago

Remove old layout debugging functions InLineList, InSiblingList, & VerifyTree

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

I just noticed these warnings while rebuilding layout:

nsBlockFrame.cpp:6328: warning: ‘PRBool InLineList(nsLineList&, nsIFrame*)’ defined but not used
nsBlockFrame.cpp:6346: warning: ‘PRBool InSiblingList(nsLineList&, nsIFrame*)’ defined but not used

and indeed, these functions aren't called anywhere, as these MXR searches show:
  http://mxr.mozilla.org/mozilla-central/search?string=InLineList
  http://mxr.mozilla.org/mozilla-central/search?string=InSiblingList

AFAICT, the last chunk of code that called these methods (nsBlockFrame::IsChild) was removed in bug 379349 in 2007.  

nsBlockFrame.cpp revision 3.846 includes calls to InChildList & InSiblingList, and revision 3.847 (and later) don't:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.846
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.847

It seems like we should remove these functions if they're not ever getting called.
Attachment #375234 - Flags: superreview?(dbaron)
Attachment #375234 - Flags: review?(dbaron)
Comment on attachment 375234 [details] [diff] [review]
patch #1: remove InLineList & InSiblingList

r+sr=dbaron

We should probably remove nsIFrameDebug::VerifyTree, all implementations of it, and the associated machinery as well.
Attachment #375234 - Flags: superreview?(dbaron)
Attachment #375234 - Flags: superreview+
Attachment #375234 - Flags: review?(dbaron)
Attachment #375234 - Flags: review+
(In reply to comment #2)
> We should probably remove nsIFrameDebug::VerifyTree, all implementations of it,
> and the associated machinery as well.

Here's a patch that does that.

I think I caught all the implementations & related machinery -- I grepped for "VerifyTree", and I ignored the "StyleVerifyTree" instances and the instances in security/nss/cmd/libpkix (which look unrelated, at first glance).

Compiling right now; will re-post if I hit any compile failures.
Attachment #375234 - Attachment description: patch to remove InLineList & InSiblingList → patch #1: remove InLineList & InSiblingList
Comment on attachment 376033 [details] [diff] [review]
patch #2: remove "VerifyTree"

Yay, this compiles.

The patch removes...
  - nsIFrameDebug::VerifyTree and all implementations
  - nsIFrameDebug::Get/SetVerifyTreeEnable and its only implementation (in nsFrame.cpp)
  - The chunk of PresShell::DoVerifyReflow() that calls VerifyTree
Attachment #376033 - Flags: superreview?(dbaron)
Attachment #376033 - Flags: review?(dbaron)
(In reply to comment #5)
>   - The chunk of PresShell::DoVerifyReflow() that calls VerifyTree
I meant to say: ... calls VerifyTree **and** nsIFrameDebug::GetVerifyTreeEnable (This is the only place where GetVerifyTreeEnable was called)

Also, note that SetVerifyTreeEnable isn't actually called from anywhere right now.
cc:ing roc and bz to make sure they're also ok with removing the VerifyTree code.
Sounds good to me. VerifyTree does some setup and teardown that could get in the way of refactoring.
I shall not miss this code, esp. since it doesn't really do that much good given all the "disabled" impls.
Comment on attachment 376033 [details] [diff] [review]
patch #2: remove "VerifyTree"

ok, r+sr=dbaron.

Please do an mxr search for "VerifyTree" after you land this to check that you didn't miss anything.  (You probably need to wait an hour or more for MXR to update.)
Attachment #376033 - Flags: superreview?(dbaron)
Attachment #376033 - Flags: superreview+
Attachment #376033 - Flags: review?(dbaron)
Attachment #376033 - Flags: review+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/5ef612c5cad9

I'll do that mxr check in a bit -- I verified locally with "grep", though, and I think I've gotten everything aside from the unrelated bits mentioned in comment 4 (*StyleVerifyTree* and other unrelated code in security/nss/cmd/libpkix).
Yup, the only remaining instances are "StyleVerifyTree and stuff in security/nss/cmd/libpkix, with one exception -- the file
http://mxr.mozilla.org/mozilla-central/source/layout/doc/regression_tests.html
has a block of sample output, including this line:
"Note: frameverifytree is disabled"

I tend to think it'd be silly to change this, since
  (a) it seems bad to artificially modify a block of sample output. (The output is correct, for the date that it was generated.)
  (b) this particular document hasn't been updated at all since its creation (aside from a few minor spelling fixes), so it's undoubtedly obsolete in more significant ways than this. :)

But I'd be happy to fix this if anyone thinks it'd be useful.

Resolving as FIXED, since the code changes here are done.
Assignee: nobody → dholbert
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Summary: Remove unused functions InLineList & InSiblingList from nsBlockFrame.cpp → Remove old layout debugging functions InLineList, InSiblingList, & VerifyTree
You need to log in before you can comment on or make changes to this bug.