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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 16 14:33:09 PDT 2011


Benjamin Poulain <benjamin at webkit.org> 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 85960: patch4
https://bugs.webkit.org/attachment.cgi?id=85960&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85960&action=review

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:693
> +    if (!m_redirectOutputFileName.isEmpty()) {
> +	   if (!freopen(qPrintable(m_redirectOutputFileName), "w", stdout)) {
> +	       qDebug() << "Redirect STDOUT failed.";
> +	       return;
> +	   }
> +    }
> +    if (!m_redirectErrorFileName.isEmpty()) {
> +	   if (!freopen(qPrintable(m_redirectErrorFileName), "w", stderr)) {
> +	       qDebug() << "Redirect STDERR failed.";
> +	       return;
> +	   }
> +    }
> +

This looks like a strange place to do this.

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:109
> +    void setRedirectOutputFileName(const QString s) {
m_redirectOutputFileName = s; }
> +    void setRedirectErrorFileName(const QString s) { m_redirectErrorFileName
= s; }

This should be ref to const.

> Tools/DumpRenderTree/qt/main.cpp:73
> +    if (++index < arguments.count() && !arguments.at(index).startsWith("-"))


!arguments.at(index).startsWith("-")) -> a filename can start with "-"... :)

The side effect with ++index is a bit ugly, you can do better.

> Tools/DumpRenderTree/qt/main.cpp:87
> +bool validOptions(QStringList& args)
> +{
> +    int count = 0;
> +
> +    for (int i = 1; i < args.size(); ++i)
> +	   if (!args.at(i).startsWith('-'))
> +	       count++;
> +
> +    return count >= 1;
> +}

I don't follow here, at least the function name could be improved.

> Tools/DumpRenderTree/qt/main.cpp:93
> +    fflush(stderr);

Unecesary.

> Tools/DumpRenderTree/qt/main.cpp:192
> +    int outputIndex = args.indexOf("-o");
> +    if (outputIndex != -1) 
> +	   dumper.setRedirectOutputFileName(takeOptionValue(args,
outputIndex));
> +
> +    int errorIndex = args.indexOf("-e");
> +    if (errorIndex != -1) 
> +	   dumper.setRedirectErrorFileName(takeOptionValue(args, errorIndex));
> +

Maybe use long option name? -o and -e are not very intuitive.

You could use QLatin1String for the literals.


More information about the webkit-reviews mailing list