95098 2012-08-27 10:11:13 -0700 [Chromium] DRT does not support --dump-all-pixels flag 2012-09-20 08:57:16 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 schenney schenney dpranke eric kbalazs rniwa tony oldest_to_newest 705355 0 schenney 2012-08-27 10:11:13 -0700 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. 705369 1 160744 schenney 2012-08-27 10:17:42 -0700 Created attachment 160744 Patch 705561 2 eric 2012-08-27 13:12:12 -0700 I'm not sure what --dump-all-pixels does. :) Was this a test_shell thing which was failed to port forward? 705580 3 schenney 2012-08-27 13:20:13 -0700 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. 706619 4 dpranke 2012-08-28 12:33:20 -0700 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? 706647 5 schenney 2012-08-28 12:49:21 -0700 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? 708565 6 kbalazs 2012-08-30 06:36:08 -0700 (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. 708572 7 schenney 2012-08-30 06:50:06 -0700 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. 708577 8 kbalazs 2012-08-30 06:52:17 -0700 (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. :) 720771 9 schenney 2012-09-14 08:03:33 -0700 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? 720962 10 160744 dpranke 2012-09-14 11:13:51 -0700 Comment on attachment 160744 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. 721004 11 schenney 2012-09-14 12:00:07 -0700 (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? 721014 12 dpranke 2012-09-14 12:08:11 -0700 Sorry, I meant change --dump-all-pixels to --pixel-tests and add -p as shorthand. 724553 13 schenney 2012-09-20 08:57:16 -0700 Committed r129139: <http://trac.webkit.org/changeset/129139> 160744 2012-08-27 10:17:42 -0700 2012-09-14 11:13:51 -0700 Patch bug-95098-20120827131734.patch text/plain 3654 schenney SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDEyNjc3MykKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE5IEBACisyMDEyLTA4LTI3ICBTdGVwaGVuIENoZW5uZXkgIDxzY2hlbm5leUBjaHJvbWl1 bS5vcmc+CisKKyAgICAgICAgW0Nocm9taXVtXSBEUlQgZG9lcyBub3Qgc3VwcG9ydCAtLWR1bXAt YWxsLXBpeGVscyBmbGFnCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD05NTA5OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIEFkZCBzdXBwb3J0IGZvciB0aGUgLS1kdW1wLWFsbC1waXhlbHMgb3B0aW9uIGluIENo cm9taXVtIER1bXBSZW5kZXJUcmVlLiBVc2UKKyAgICAgICAgb2YgdGhpcyBmbGFnIGNhdXNlcyBw aXhlbHMgcmVzdWx0cyB0byBiZSBjcmVhdGVkIGZvciBhbGwgdGVzdHMsIHJlZ2FyZGxlc3Mgb2YK KyAgICAgICAgaW5kaXZpZHVhbCB0ZXN0IG9wdGlvbnMuIElmIGFuIGluZGl2aWR1YWwgdGVzdCBw cHJvdmlkZXMgYSBwaXhlbCBoYXNoIGl0IHdpbGxiZSB1c2VkLAorICAgICAgICBvdGhlcndpc2Ug dGhlIGhhc2ggd2lsbCBiZSBlbXB0eS4KKworICAgICAgICAqIER1bXBSZW5kZXJUcmVlL2Nocm9t aXVtL0R1bXBSZW5kZXJUcmVlLmNwcDoKKyAgICAgICAgKHJ1blRlc3QpOiBBZGQgYSBwYXJhbWV0 ZXIgYW5kIGNvZGUgdG8gZm9yY2UgcGl4ZWwgcmVzdWx0cyBmb3IgdGhlIHRlc3QuCisgICAgICAg IChtYWluKTogQWRkIHBhcmFtZXRlciBoYW5kbGluZyBmb3IgLS1kdW1wLWFsbC1waXhlbHMuCisK IDIwMTItMDgtMjcgIFBoaWxpcHBlIE5vcm1hbmQgIDxwbm9ybWFuZEBpZ2FsaWEuY29tPgogCiAg ICAgICAgIFtHU3RyZWFtZXJdW1F0XSBXZWJBdWRpbyBzdXBwb3J0CkluZGV4OiBUb29scy9EdW1w UmVuZGVyVHJlZS9jaHJvbWl1bS9EdW1wUmVuZGVyVHJlZS5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gVG9v bHMvRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vRHVtcFJlbmRlclRyZWUuY3BwCShyZXZpc2lvbiAx MjY3NjkpCisrKyBUb29scy9EdW1wUmVuZGVyVHJlZS9jaHJvbWl1bS9EdW1wUmVuZGVyVHJlZS5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTg1LDcgKzg1LDcgQEAgcHJpdmF0ZToKICAgICBPd25QdHI8 TW9ja1dlYktpdFBsYXRmb3JtU3VwcG9ydD4gbV9tb2NrUGxhdGZvcm07CiB9OwogCi1zdGF0aWMg dm9pZCBydW5UZXN0KFRlc3RTaGVsbCYgc2hlbGwsIFRlc3RQYXJhbXMmIHBhcmFtcywgY29uc3Qg c3RyaW5nJiBpbnB1dExpbmUpCitzdGF0aWMgdm9pZCBydW5UZXN0KFRlc3RTaGVsbCYgc2hlbGws IFRlc3RQYXJhbXMmIHBhcmFtcywgY29uc3Qgc3RyaW5nJiBpbnB1dExpbmUsIGNvbnN0IGJvb2wg Zm9yY2VEdW1wUGl4ZWxzKQogewogICAgIGludCBvbGRUaW1lb3V0TXNlYyA9IHNoZWxsLmxheW91 dFRlc3RUaW1lb3V0KCk7CiAgICAgVGVzdENvbW1hbmQgY29tbWFuZCA9IHBhcnNlSW5wdXRMaW5l KGlucHV0TGluZSk7CkBAIC0xMDMsMTEgKzEwMywxMSBAQCBzdGF0aWMgdm9pZCBydW5UZXN0KFRl c3RTaGVsbCYgc2hlbGwsIFRlCiAgICAgICAgICAgYm9vbCBpc0xhc3RMb2FkID0gKGkgPT0gKHY4 OjpUZXN0aW5nOjpHZXRTdHJlc3NSdW5zKCkgLSAxKSk7CiAgICAgICAgICAgc2hlbGwuc2V0RHVt cFdoZW5GaW5pc2hlZChpc0xhc3RMb2FkKTsKICAgICAgICAgICBzaGVsbC5yZXNldFRlc3RDb250 cm9sbGVyKCk7Ci0gICAgICAgICAgc2hlbGwucnVuRmlsZVRlc3QocGFyYW1zLCBjb21tYW5kLnNo b3VsZER1bXBQaXhlbHMpOworICAgICAgICAgIHNoZWxsLnJ1bkZpbGVUZXN0KHBhcmFtcywgY29t bWFuZC5zaG91bGREdW1wUGl4ZWxzIHx8IGZvcmNlRHVtcFBpeGVscyk7CiAgICAgICB9CiAgICAg fSBlbHNlIHsKICAgICAgIHNoZWxsLnJlc2V0VGVzdENvbnRyb2xsZXIoKTsKLSAgICAgIHNoZWxs LnJ1bkZpbGVUZXN0KHBhcmFtcywgY29tbWFuZC5zaG91bGREdW1wUGl4ZWxzKTsKKyAgICAgIHNo ZWxsLnJ1bkZpbGVUZXN0KHBhcmFtcywgY29tbWFuZC5zaG91bGREdW1wUGl4ZWxzIHx8IGZvcmNl RHVtcFBpeGVscyk7CiAgICAgfQogICAgIHNoZWxsLnNldExheW91dFRlc3RUaW1lb3V0KG9sZFRp bWVvdXRNc2VjKTsKIH0KQEAgLTEyMCw2ICsxMjAsNyBAQCBpbnQgbWFpbihpbnQgYXJnYywgY2hh ciogYXJndltdKQogICAgIFRlc3RQYXJhbXMgcGFyYW1zOwogICAgIFZlY3RvcjxzdHJpbmc+IHRl c3RzOwogICAgIGJvb2wgc2VydmVyTW9kZSA9IGZhbHNlOworICAgIGJvb2wgZHVtcEFsbFBpeGVs cyA9IGZhbHNlOwogICAgIGJvb2wgYWxsb3dFeHRlcm5hbFBhZ2VzID0gZmFsc2U7CiAgICAgYm9v bCBzdGFydHVwRGlhbG9nID0gZmFsc2U7CiAgICAgYm9vbCBhY2NlbGVyYXRlZENvbXBvc2l0aW5n Rm9yVmlkZW9FbmFibGVkID0gZmFsc2U7CkBAIC0xMzksNiArMTQwLDggQEAgaW50IG1haW4oaW50 IGFyZ2MsIGNoYXIqIGFyZ3ZbXSkKICAgICAgICAgc3RyaW5nIGFyZ3VtZW50KGFyZ3ZbaV0pOwog ICAgICAgICBpZiAoYXJndW1lbnQgPT0gIi0iKQogICAgICAgICAgICAgc2VydmVyTW9kZSA9IHRy dWU7CisgICAgICAgIGVsc2UgaWYgKGFyZ3VtZW50ID09IG9wdGlvbkR1bXBBbGxQaXhlbHMpCisg ICAgICAgICAgICBkdW1wQWxsUGl4ZWxzID0gdHJ1ZTsKICAgICAgICAgZWxzZSBpZiAoYXJndW1l bnQgPT0gb3B0aW9uTm90cmVlKQogICAgICAgICAgICAgcGFyYW1zLmR1bXBUcmVlID0gZmFsc2U7 CiAgICAgICAgIGVsc2UgaWYgKGFyZ3VtZW50ID09IG9wdGlvbkRlYnVnUmVuZGVyVHJlZSkKQEAg LTIzOSwxNCArMjQyLDE0IEBAIGludCBtYWluKGludCBhcmdjLCBjaGFyKiBhcmd2W10pCiAgICAg ICAgICAgICAgICAgLy8gRXhwbGljaXRseSBxdWl0IG9uIHBsYXRmb3JtcyB3aGVyZSBFT0YgaXMg bm90IHJlbGlhYmxlLgogICAgICAgICAgICAgICAgIGlmICghc3RyY21wKHRlc3RTdHJpbmcsICJR VUlUIikpCiAgICAgICAgICAgICAgICAgICAgIGJyZWFrOwotICAgICAgICAgICAgICAgIHJ1blRl c3Qoc2hlbGwsIHBhcmFtcywgdGVzdFN0cmluZyk7CisgICAgICAgICAgICAgICAgcnVuVGVzdChz aGVsbCwgcGFyYW1zLCB0ZXN0U3RyaW5nLCBkdW1wQWxsUGl4ZWxzKTsKICAgICAgICAgICAgIH0K ICAgICAgICAgfSBlbHNlIGlmICghdGVzdHMuc2l6ZSgpKQogICAgICAgICAgICAgcHV0cygiI0VP RiIpOwogICAgICAgICBlbHNlIHsKICAgICAgICAgICAgIHBhcmFtcy5wcmludFNlcGFyYXRvcnMg PSB0ZXN0cy5zaXplKCkgPiAxOwogICAgICAgICAgICAgZm9yICh1bnNpZ25lZCBpID0gMDsgaSA8 IHRlc3RzLnNpemUoKTsgaSsrKQotICAgICAgICAgICAgICAgIHJ1blRlc3Qoc2hlbGwsIHBhcmFt cywgdGVzdHNbaV0pOworICAgICAgICAgICAgICAgIHJ1blRlc3Qoc2hlbGwsIHBhcmFtcywgdGVz dHNbaV0sIGR1bXBBbGxQaXhlbHMpOwogICAgICAgICB9CiAKICAgICAgICAgc2hlbGwuY2FsbEpT R0MoKTsK