| Summary: | [Chromium] DRT does not support --dump-all-pixels flag | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Stephen Chenney <schenney> | ||||
| Component: | Tools / Tests | Assignee: | Stephen Chenney <schenney> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | dpranke, eric, kbalazs, rniwa, tony | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Stephen Chenney
2012-08-27 10:11:13 PDT
Created attachment 160744 [details]
Patch
I'm not sure what --dump-all-pixels does. :) Was this a test_shell thing which was failed to port forward? 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. 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? 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? (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. 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. (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. :) 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? 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.
(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? Sorry, I meant change --dump-all-pixels to --pixel-tests and add -p as shorthand. Committed r129139: <http://trac.webkit.org/changeset/129139> |