137331 - rewrite viewer in XUL

Status: RESOLVED FIXED

(Core Graveyard :: Viewer App, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[Håkan Waara]

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?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[Bernd]

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[Bernd]

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.

Comment 9

[karnaze (gone)]

Adding dcone to cc list. 

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: updated work in progress

another .tar.gz

Comment 11

[Marc Attinasi]

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.

Comment 12

[dcone (gone)]

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.  

Comment 13

[Kevin McCluskey (gone)]

bug 139911 covers the debug plugin work

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: updated work in progress

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

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 16

[Simon Fraser [no longer active]]

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.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: rename / reorganize things in extensions/layout-debug/

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).

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: rename / reorganize things in extensions/layout-debug/

oops, wrong (version of) patch.

Comment 19

[Brian Ryner (not reading)]

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).

Comment 20

[Daniel (Leaf) Nunes]

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.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: add UI code into extensions/layout-debug/ and remove UI features from regression tester (changes to old files)

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: add UI code into extensions/layout-debug/ and remove UI features from regression tester (new files)

Comment 23

[Brian Ryner (not reading)]

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 24

[Brian Ryner (not reading)]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: add UI code into extensions/layout-debug/ and remove UI features from regression tester

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

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 28

[Bernd]

Attachment: patch to generate the necessary makefiles

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

Comment 29

[Bernd]

Attachment: cosmetics

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

Comment 30

[Bernd]

Attachment: patch

in addition one needs an ioservice to instantiate a nsIFileURL
http://www.mail-archive.com/[e-mail]/msg01524.html

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.