Summary: | testharnessreport.js should sanitize the results before printing them | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | Tools / Tests | Assignee: | Chris Dumez <cdumez> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, commit-queue, darin, lforschler, rniwa, sam, youennf | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=164071 | ||||||
Attachments: |
|
Description
Chris Dumez
2016-10-27 09:48:55 PDT
Created attachment 293038 [details]
Patch
If it proves to be difficult to remove the testharness report.js copies, we could add checks. We do that for testharness.js when running rwt. In the case of test harness report.js, style checker could do it. Comment on attachment 293038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293038&action=review Thanks so much for fixing this! > LayoutTests/http/tests/w3c/resources/testharnessreport.js:56 > + // Escape null characters, otherwise diff will think the file is binary. This comment is not the reason this matters to us. Seems like a comment written by whoever made this change in Blink and so only mentions the reason that affects them. > LayoutTests/http/tests/w3c/resources/testharnessreport.js:58 > + // Escape carriage returns as they break rietveld's difftools. This comment is even more Blink-specific. I don’t even know what “rietveld” is. Comment on attachment 293038 [details] Patch Clearing flags on attachment: 293038 Committed r207995: <http://trac.webkit.org/changeset/207995> All reviewed patches have been landed. Closing bug. (In reply to comment #3) > Comment on attachment 293038 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293038&action=review > > Thanks so much for fixing this! > > > LayoutTests/http/tests/w3c/resources/testharnessreport.js:56 > > + // Escape null characters, otherwise diff will think the file is binary. > > This comment is not the reason this matters to us. Seems like a comment > written by whoever made this change in Blink and so only mentions the reason > that affects them. > > > LayoutTests/http/tests/w3c/resources/testharnessreport.js:58 > > + // Escape carriage returns as they break rietveld's difftools. > > This comment is even more Blink-specific. I don’t even know what “rietveld” > is. Sorry about this. I went quick and dirty. I am planning to sync-up all 3 scripts so they are identical in a follow-up when I have a little bit more time. I'll clean up the comments as well. |