[webkit-reviews] review denied: [Bug 92398] All ports should support per test switching of pixel testing : [Attachment 154700] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 92398: All ports should support per test switching of pixel testing
https://bugs.webkit.org/show_bug.cgi?id=92398

Attachment 154700: Patch
https://bugs.webkit.org/attachment.cgi?id=154700&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154700&action=review


Thanks for changing all of the ports! that's great. That said, R-'ing for the
typo on the mac port :)

Also, it might be nice if someone else who was more familiar with the
DRT/WTR/C++ code took a look. Tony, what do you think?

In your earlier change, I missed that the format for the line written to stdin
was "test'--pixel-test'hash" ... I was hoping to extend this to support
arbitrary arguments, which I suppose you can do by just splicing them between
single quotes, but it seems a little odd. why not just separate the args with
spaces, e.g., test hash --pixel-test --foo --bar, etc.?

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:232
> +static bool getExpectedPixelHash(const String& testURL, String&
pixelHashOut)

I don't know the EFL implementation at all, but (perhaps naively) wouldn't it
be better to parse the url in the caller to see if you needed to get the hash?
Or maybe at least change the name of this function?

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:876
> +	   printSeparators = (optind < argc - 1 || (dumpPixelsForCurrentTest &&
dumpTree);

does this compile? I think you're missing a closing paren ...