So, I felt like debugging a layout problem this morning, and it turned out it didn't show up in viewer, presumably because of the scroll frame. So I decided to try hacking up part of viewer in XUL, which needs to happen anyway, I think, because: * much of the UI of the existing viewer only works on Windows because it has to be maintained on three platforms, * the needs of the UI of the existing viewer are forcing us to keep a pile of native widget code that we don't need, and * the existing viewer uses old (non-model) APIs. So I hacked up extensions/ldb/ . (It stands for layout debugger, really! I was forced to pick that name since layout-debug was already taken by a shell of code that looks like it intends to become a plugin. ;-) I'm certainly willing to rename it if people have better ideas, but I'm very bad at naming things. What I did today got me to a working browser UI (URL bar, status bar, back/forward/reload/stop) and I hooked up most of the simple menu items in viewer (all the Dump things, most of the toggles except for Event Debugging / EventTargetDebugging, because I'd like to find another way to do them, partly since I want to use the nsLayoutDebugger name for the extension), although there are a few I haven't done yet. Much of this was done by copying chunks of code from nsBrowserWindow.cpp into nsLayoutDebugger.cpp. I also learned a bit more about XUL than I knew before. So I already have something that's already significantly more useful to me than the old viewer (since the UI works!), except for the regression tests, which are a bit more work. I'll attach a tar.gz of the extensions/ldb directory that I have so far if anyone else wants to play with it. I'm assuming that everybody will be using gmake by the time this is ready (except Mac), so I didn't bother with makefile.wins. (Once built, it can be started either from the Tools menu (where I probably did the overlay wrong, since I didn't give a position, but I want it at the end, and that's where it ended up), or as "./mozilla -chrome chrome://ldb/content/".) Do you think this should go into cvs?

This is a .tar.gz file, meant to be extracted within mozilla/extensions/

Comment on attachment 79092 [details] work in progress For some reason the URL bar only shows up when I start it from the command line, rather than the overlaid menu item. Bizarre.

Actually, I'm losing track of time a little. I started this late yesterday afternoon, not this morning.

I thought the point of having viewer be native UI was so that you could debug gecko without accidently having to debug the debugger (since it uses XUL -- catch-22). Is there a way to get around this in LDB?

For debugging, the native embedding apps (gtkEmbed, winEmbed, PPEmbed) should be sufficient. We don't then need to maintain the entire regression test harness and all the debugging tools in a native app.

couldn't this effort be coordianted with dcones effort for the regression test plugin at extensions/layout-debug ?

What is the plugin supposed to do? Just the regression tests? If so, the regression test code itself could be refactored so both could use it. I still don't see the point, though.

the point is if that regression plugin works it would ease the job you described as: >So I already have something that's already significantly more useful to me than >the old viewer (since the UI works!), except for the regression tests, which are >a bit more work. and it would make the switch away from the viewer faster.

Adding dcone to cc list.

another .tar.gz

Part of the idea of the plugin was to be able to plug the debugging code into any Gecko embedding client and have it work, instead of having a dedicated app to do this. Thus, Viewer would just go away,a nd we could use SeaMonkey or MFCEmbed, for example, to do the regression tests and frame dumping. That is what Kevin and Don told me anyway.

The debug object is a way that either HTML or XUL could get at specific data that is used in the layout engine. The plug in could be used anywhere the layout engine is embeded or used. The amount of code added.. is very small.. the plug-in, HTML, and Javascript take care of most of the regression testing or formating of the data. The service inside the layout engine basically give access to data that is already there.. like the Frame model, content, etc. The Scripts outside of the layout engine take care of comparing, flaging, searching directories, maintaining file list, etc. Its very versitle, make very little impact on the layout engine and can be used for regression, debugging, statistics, etc.

bug 139911 covers the debug plugin work

Updated to trunk (removing nsISizeOfHandler-related stuff and handling nsIStyleContext de-COM-tamination).

I'm thinking I want to land this, since it, combined with the debug plugin, should cover most of the features of viewer (although I still need to know how to use the debug plugin). The question is where to put it. Perhaps it belongs in extensions/layout-debug/ui/ or extensions/layout-debug/viewer/ or something like that, or perhaps it should remain (as the current tarfile expects -- extensions/ldb/) its own extension.

I've been lax in writing docs for the debug plugin (which isn't a plugin any more). Basically, build debug with the 'layout-debug' extension enabled. With the resulting build, open the file mozilla/layout/tools/tests/regression_tests.html. Use this page to create a set of 'baseline' files based on one or more testcases. Make changes to your build, rerun. Load the same file again, and now do the 'verify and compare' option. Basically the JS on this page just drives nsIFrameDebugObject.idl. Feel free to write your own test suite around this.

This patch renames: nsIFrameDebugObject -> nsILayoutRegressionTester nsDebugObject -> nsRegressionTester and moves all the files into a single directory "src" since there are so few of them. (I could ask leaf to copy files in the repository, although I don't know if it's worth it...) Do these names seem reasonable? If not, now's the time to change them. (I decided to put Layout in the name of the interface but not the class for two reasons: (1) the interface name is "global" and needs to make sense outside of layout-debug and (2) we might potentially want to add other debugging features to nsRegressionTester that deserve another interface name. I don't feel strongly on either of these reasons, though, so if others have better ideas that's fine with me.) My intention is to remove the dump frames / dump content / dump views stuff from this interface and merge attachment 125047 [details] (which also contains copies of the relevant code for that, from viewer) in to the directory structure in layout-debug (with some interface renaming from that as well).

oops, wrong (version of) patch.

Comment on attachment 126159 [details] [diff] [review] rename / reorganize things in extensions/layout-debug/ r/sr=me (if it was just this change by itself I'd propose flattening src/ out into extensions/layout-debug, but dbaron tells me there will be a ui/ directory alongside src/, so this makes sense).

idl/nsIFrameDebugObject.idl -> src/nsILayoutRegressionTester.idl public/nsLayoutDebugCIID.h -> src/nsLayoutDebugCIID.h src/nsDebugObject.cpp -> src/nsRegressionTester.cpp src/nsDebugObject.h -> src/nsRegressionTester.h Completed on the cvs server.

comments on the new files (I know some of this isn't your code, but I'll comment on it anyway): nsLayoutDebuggingTools.cpp: There are now non-addreffing versions of GetStyleSet and GetViewManager on nsIPresShell; maybe it would be better to use those? Why the explicit nsCOMPtr scoping in nsLayoutDebuggingTools::Init() ? In nsLayoutDebuggingTools::SetReflowCounts and DumpReflowStats, that should probably be "you have _not_ built with MOZ_REFLOW_PERF=1" In DumpAWebShell, I think you could make 'str' a nsDependentString instead of nsAutoString. Also, some postincrements could be changed to preincrements. In DumpContentRecur and DumpFramesRecur, I'd prefer changing all |if (nsnull != foo)| to |if (foo)|. Also, use comptr's where appropriate. In GetDisplaySelection, you're assigning a PRInt16 into a PRBool. Is this intentional? nsLayoutDebuggingTools.h: I think you can now forward-declare nsIDocShell and nsIPref. nsILayoutDebuggingTools.idl: license missing ui/Makefile.in: missing license. explicit $(REGCHROME) is generally no longer needed (make-jars.pl searches for a contents.rdf and registers based on that). Also, need .cvsignore file for Makefile. layoutdebug.js: // XXX .checked doesn't work. Why not? because the menuitem binding doesn't have a property called checked. same for XXX comment in toggle() (you could use a getAttribute...). Along the same lines, I don't see anything that causes the menuitem's checked attribute to change when it's toggled by the user. Am I missing something? I'll come back in a bit and look at the actual logic in layoutdebug.js some more.

Comment on attachment 126864 [details] [diff] [review] add UI code into extensions/layout-debug/ and remove UI features from regression tester (changes to old files) This patch looks fine. r/sr=me.

I've addressed all the comments except for: > There are now non-addreffing versions of GetStyleSet and GetViewManager on > nsIPresShell; maybe > it would be better to use those? Not outside of libgklayout. > Why the explicit nsCOMPtr scoping in nsLayoutDebuggingTools::Init() ? Because the nsCOMPtr isn't needed later. I think we should do this more.

Updated patch, containing both changes to existing files and new files.

Checked in to trunk this afternoon (and made part of --enable-extensions=all). The regression test menu in the UI doesn't work, but I should file a separate bug on that... I should also file a separate bug on removing viewer (after notifying people of how to do all the different things that viewer could do, and waiting a bit).

I think the makefiles should also be listed in allmakefiles.sh

The viewer should IMHO - have a file open - dont have grippies on the toolbars, they dont work correctly here anyway - update its status line

in addition one needs an ioservice to instantiate a nsIFileURL http://www.mail-archive.com/mozilla-netlib@mozilla.org/msg01524.html

I just checked in a patch that I hoped would get the regression tests working -- baseline works fine, but verify is crashing. I also checked in new file lists (rtest.lst) that use relative URLs. It's also worth noting that attachment 128634 [review] broke the sub-list feature (see how layout/html/tests/block/rtest.lst is just a list of other rtest.lst files), which I fixed in what I just checked in.