Bug 40616 – [DRT/Chromium] Implement --testshell-startup-dialog

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kent Tamura]

[DRT/Chromium] Implement --testshell-startup-dialog

[Kent Tamura]

Created attachment 58763 [details]
Patch

[Tony Chang]

Comment on attachment 58763 [details]
Patch

One small nit:

WebKitTools/DumpRenderTree/chromium/TestShellGtk.cpp:196
 +      gtk_init(0, 0);
Nit: Can we call this in platformInit() instead?  It would be nice to pass in the actual argc/argv, but I guess it's not a big deal.

[Kent Tamura]

(In reply to comment #2)
> (From update of attachment 58763 [details])
> One small nit:
> 
> WebKitTools/DumpRenderTree/chromium/TestShellGtk.cpp:196
>  +      gtk_init(0, 0);
> Nit: Can we call this in platformInit() instead?  It would be nice to pass in the actual argc/argv, but I guess it's not a big deal.

I wanted to avoid to open X display as possible. But it might make no sense.

[WebKit Review Bot]

Attachment 58763 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3314156

[Tony Chang]

(In reply to comment #3)
> I wanted to avoid to open X display as possible. But it might make no sense.

I didn't even realize that we could run without an X display.  You're right, we should try to preserve that so this seems fine.  Maybe add a comment about the weird placement of init?

In practice, I don't think Linux people use the startup dialog to attach a debugger.  I think it's more common to try to use --wrapper="gdb --args" with new-run-webkit-tests, although I'm not sure that works anymore.

[Kent Tamura]

Created attachment 58765 [details]
Patch 2 (add a comment about gtk_init)

[WebKit Review Bot]

Attachment 58765 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3277179

[Eric Seidel (no email)]

I've never understood why Chromium has this option.

Why not just use:
> gdb DumpRenderTree
 % run

to run the application inside the debugger?  That's what we do with the existing DumpRenderTree.

[Tony Chang]

(In reply to comment #8)
> I've never understood why Chromium has this option.
> 
> Why not just use:
> > gdb DumpRenderTree
>  % run
> 
> to run the application inside the debugger?  That's what we do with the existing DumpRenderTree.

Don't you have to manually pipe it file paths to run tests?  This also makes it easier to debug http tests since you don't have to remember to run new-run-webkit-httpd.

[Eric Seidel (no email)]

The other DumpRenderTree implementations take a list of tests as arguments and run those.  I don't really know how to speak DRT's http-like syntax off the top of my head, so I'm impressed that you all have been piping arguments to it!

[Eric Seidel (no email)]

DRT has two modes
"-" is server mode.  wait for http-like commands on stdin.
[LIST OF TESTS] runs a list of tests and dumps their render trees to stdout.  It used to not spit out extra #EOFs in that mode, but it looks like it does these days. :(

[Kent Tamura]

This option is useful on Windows.
Attaching debugger while the dialog opening is much easier than changing target command-line setting every time in Visual Studio.

[Eric Seidel (no email)]

Maybe the webkit-windows folks will want something similar for DRT then?

[Adam Roben (:aroben)]

Maybe it would be even better to have a flag that makes DRT/TestShell call DebugBreak() before it has run any tests? That would cause the just-in-time debugging dialog to appear, which seems even easier than having to attach to the process manually.

[Kent Tamura]

(In reply to comment #14)
> Maybe it would be even better to have a flag that makes DRT/TestShell call DebugBreak() before it has run any tests? That would cause the just-in-time debugging dialog to appear, which seems even easier than having to attach to the process manually.

I tried DebugBreak() today.  But It didn't work if a parent processes is a Cygwin process.

[Tony Chang]

tkent: Can you rebase now that http://trac.webkit.org/changeset/64463 has landed?

[Kent Tamura]

Created attachment 63301 [details]
Patch

[Kent Tamura]

(In reply to comment #16)
> tkent: Can you rebase now that http://trac.webkit.org/changeset/64463 has landed?

Yes.  I have uploaded a rebased patch.

[Dimitri Glazkov (Google)]

Comment on attachment 63301 [details]
Patch

ok!

[Kent Tamura]

Comment on attachment 63301 [details]
Patch

Clearing flags on attachment: 63301

Committed r64615: <http://trac.webkit.org/changeset/64615>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.