[webkit-reviews] review denied: [Bug 56323] [Qt] Add a command line option to capture stdout and stderr for DumpRenderTree : [Attachment 85807] patch

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied qi
<qi.2.zhang at nokia.com>'s request for review:
Bug 56323: [Qt] Add a command line option to capture stdout and stderr for
DumpRenderTree
https://bugs.webkit.org/show_bug.cgi?id=56323

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

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=85807&action=review

Overall the patch looks great. r- to fix the build break.

> Tools/ChangeLog:8
> +	   Provide an option for DumpRenderTree to redirect stdout and stderr.

This line does not seem to add information as it just repeats the bug title.
Perhaps you should describe how the patch work in plain English.

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:684
> +	   freopen(qPrintable(m_redirectErrorFileName), "w", stderr);

This does not build in my environment (Linux) - not sure why it passed on EWS.
Fails with the following error:

DumpRenderTreeQt.cpp: In member function ‘void
WebCore::DumpRenderTree::processArgsLine(const QStringList&)’:
DumpRenderTreeQt.cpp:682: error: ignoring return value of ‘FILE* freopen(const
char*, const char*, FILE*)’, declared with attribute warn_unused_result
DumpRenderTreeQt.cpp:684: error: ignoring return value of ‘FILE* freopen(const
char*, const char*, FILE*)’, declared with attribute warn_unused_result

You can perhaps check the return value of the freopen call - if
(freopen(qPrintable(m_redirectOutputFileName), "w", stdout) == 0) {..}.

> Tools/DumpRenderTree/qt/main.cpp:155
>	   qDebug() << "Or folder containing test files: DumpRenderTree
[-v|--pixel-tests] dirpath";

I would prefer to have more error-handling for the command-line. It appears
that "DumpRenderTree -o FileName" is crashing (as there is no test specified)
without any warning.