Bug 95098 – [Chromium] DRT does not support --dump-all-pixels flag

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

[Stephen Chenney]

I'm trying to debug a paint crash using DumpRenderTree in a debugger with a simple command line for debugging:
DumpRenderTree --no-timeout --dump-all-pixels [testname]

But no pixel tests are dumped so no crash. It turns out we don't support --dump-all-pixels so the only way to get pixel results inside the debugger is to run DRT in server mode and provide a file with the filename and the -p flag.

I suspect I'm missing something, but in any event I will shortly put up a patch with DRT updated to handle --dump-all-pixels.

[Stephen Chenney]

Created attachment 160744 [details]
Patch

[Eric Seidel (no email)]

I'm not sure what --dump-all-pixels does. :)  Was this a test_shell thing which was failed to port forward?

[Stephen Chenney]

The --dump-all-pixels argument was added by Darin Adler way back in 2006:

http://trac.webkit.org/changeset/13611

The comment on the ChangeLog says:

DumpRenderTree/DumpRenderTree.m:
(main): Added a new --dump-all-pixels option, used when forcing run-webkit-tests
to generate new output for all tests it runs.
(dump): Dump the scroll position if it's non-zero. Always dump the image when
the --dump-all-pixels option is passed. Also tightened up the image dumping
code and replaced the incorrect use of +[NSGraphicsContext saveGraphicsState]
with code to save and restore the context.

So I think my interpretation is correct: "Always dump the image when
the --dump-all-pixels option is passed."

It wasn't needed previously because we supported --pixel-tests as an argument to DRT, but that was removed in http://trac.webkit.org/changeset/124581.

[Dirk Pranke]

If all you're trying to do is run a single test, then DumpREnderTree --no-timeout testname'-p should work. (Note the single quote which separates the test name from the -p parameter.

That said, the change sounds fine to me. So, with this change this would work the same way across all of the DRT implementations?

[Stephen Chenney]

The single quote was the trick I was missing, but at the same time I think it's much simpler for all if we support --dump-all-pixels.

I too would like to know: What do the other ports do with this parameter?

[Balazs Kelemen]

(In reply to comment #5)
> The single quote was the trick I was missing, but at the same time I think it's much simpler for all if we support --dump-all-pixels.
> 
> I too would like to know: What do the other ports do with this parameter?

Do other ports have such a parameter now? I don't think so. I think it's fine to add this.

[Stephen Chenney]

I should have done this sooner: A search for dump-all-pixels finds support only in the win port and now Chromium. It was originally added for the mac port, presumably, then picked up in the win port, which Chromium picked up the parameter name from, then removed (presumably) from the mac port at some point.

On the one hand, I find dump-all-pixels much simpler for debugging DRT and one-off testing outside of run-webkit-tests and new-run-webkit-tests. On the other hand, if it's not in other ports I tend to think we should remove it everywhere.

[Balazs Kelemen]

(In reply to comment #7)
> On the one hand, I find dump-all-pixels much simpler for debugging DRT and one-off testing outside of run-webkit-tests and new-run-webkit-tests. On the other hand, if it's not in other ports I tend to think we should remove it everywhere.

I think we should add it to everywhere. :)

[Stephen Chenney]

Bump. Do you want me to implement this on every platform, or are you fine with checking it in for chromium and leaving it to the other platforms to do as they will?

[Dirk Pranke]

Comment on attachment 160744 [details]
Patch

I think the patch looks fine (ish).  However, I think it'd be better if we just went back to supporting -p/--pixel-tests for this, rather than using --dump-all-pixels (at least that would be consistent with the per-test flag).

While I think we should implement this on all the ports, we don't need to do it in this patch (and while it would be nice of you to implement it everywhere, I don't think we need to require that). r+ w/ these changes; if you disagree, let me know.

[Stephen Chenney]

(In reply to comment #10)
> (From update of attachment 160744 [details])
> I think the patch looks fine (ish).  However, I think it'd be better if we just went back to supporting -p/--pixel-tests for this, rather than using --dump-all-pixels (at least that would be consistent with the per-test flag).
> 
> While I think we should implement this on all the ports, we don't need to do it in this patch (and while it would be nice of you to implement it everywhere, I don't think we need to require that). r+ w/ these changes; if you disagree, let me know.

What are "these changes" in this context? Did some review comments not get published? Do you mean re-adding -p/--pixel-test?

[Dirk Pranke]

Sorry, I meant change --dump-all-pixels to --pixel-tests and add -p as shorthand.

[Stephen Chenney]

Committed r129139: <http://trac.webkit.org/changeset/129139>