Summary: | [DRT/Chromium] Implement --testshell-startup-dialog | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aroben, bweinstein, dglazkov, eric, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 28398 | ||||||||||
Attachments: |
|
Description
Kent Tamura
2010-06-15 01:22:28 PDT
Created attachment 58763 [details]
Patch
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.
(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. Attachment 58763 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3314156 (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. Created attachment 58765 [details]
Patch 2 (add a comment about gtk_init)
Attachment 58765 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3277179 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.
(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. 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! 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. :( 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. Maybe the webkit-windows folks will want something similar for DRT then? 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. (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. tkent: Can you rebase now that http://trac.webkit.org/changeset/64463 has landed? Created attachment 63301 [details]
Patch
(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. Comment on attachment 63301 [details]
Patch
ok!
Comment on attachment 63301 [details] Patch Clearing flags on attachment: 63301 Committed r64615: <http://trac.webkit.org/changeset/64615> All reviewed patches have been landed. Closing bug. |