[webkit-reviews] review denied: [Bug 48842] [Qt] Move options handling in QtTestBrowser to separate OptionsHandler class : [Attachment 72736] First stab at a patch providing OptionsHandler

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Keith Kyzivat
<keith.kyzivat at nokia.com>'s request for review:
Bug 48842: [Qt] Move options handling in QtTestBrowser to separate
OptionsHandler class
https://bugs.webkit.org/show_bug.cgi?id=48842

Attachment 72736: First stab at a patch providing OptionsHandler
https://bugs.webkit.org/attachment.cgi?id=72736&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72736&action=review

I'm not a huge fan of this code (or any code in QtTestBrowser, really) but I
don't see any harm in cleaning it up a bit.
Splitting out the argument handling logic in this is fine with me, but this
patch will need some work. :)

> WebKitTools/QtTestBrowser/main.cpp:2
> - * Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

"2009, 2010" or "2009-2010"

> WebKitTools/QtTestBrowser/optionshandler.cpp:3
> + * Copyright (C) 2009 Keith Kyzivat <keith.kyzivat at nokia.com>

This falls under the Nokia copyright, we don't add our own names.

> WebKitTools/QtTestBrowser/optionshandler.cpp:37
> +    : QObject(parent), m_args(args), m_programName(programName)

Member initializations go on separate lines.

> WebKitTools/QtTestBrowser/optionshandler.cpp:44
> +QString OptionsHandler::usage()
> +{
> +    return usage(m_programName);
> +}

No need for two usage() functions, just inline usage(QString) in this one.

> WebKitTools/QtTestBrowser/optionshandler.cpp:69
> +QStringList OptionsHandler::setGlobalWebSettings()
> +{
> +    return m_args;
> +}

I don't understand the purpose of this function.
It's called setGlobalWebSettings, yet takes no arguments and modifies nothing.

> WebKitTools/QtTestBrowser/optionshandler.cpp:136
> +

Extra newline.

> WebKitTools/QtTestBrowser/optionshandler.cpp:138
> +    for (int i = 0; i < m_args.size(); i++) {

We normally use prefix increments (++i) in 'for' loops.

> WebKitTools/QtTestBrowser/optionshandler.cpp:157
> +	   qDebug() << usage().toLatin1().data();

usage() returns a QString which can be passed directly to qDebug(), no need for
the .toLatin1().data()

> WebKitTools/QtTestBrowser/optionshandler.h:3
> + * Copyright (C) 2010 Keith Kyzivat <keith.kyzivat at nokia.com>

This falls under the Nokia copyright, we don't add our own names.

> WebKitTools/QtTestBrowser/optionshandler.h:33
> +#include <QList>

Unnecessary include.

> WebKitTools/QtTestBrowser/optionshandler.h:38
> +// Forward Decls

Unnecessary comment.

> WebKitTools/QtTestBrowser/optionshandler.h:44
> +    OptionsHandler(QStringList args, QString programName, QObject *parent =
0);

OptionsHandler(const QStringList& arguments, const QString& programName,
QObject* parent = 0);

> WebKitTools/QtTestBrowser/optionshandler.h:51
> +    static QString usage(QString programName);

static QString usage(const QString& programName);

> WebKitTools/QtTestBrowser/optionshandler.h:82
> +signals:
> +
> +public slots:

Empty sections, please remove.

> WebKitTools/QtTestBrowser/optionshandler.h:85
> +private:
> +    void requiresGraphicsView(WindowOptions& windowOptions, const QString&
option);

Duplicate "private" section, merge with the one below.

> WebKitTools/QtTestBrowser/optionshandler.h:89
> +    // Class data

Unnecessary comment.

> WebKitTools/QtTestBrowser/optionshandler.h:90
> +    static QList<QString> s_updateModes;

QList<QString> -> QStringList

> WebKitTools/QtTestBrowser/optionshandler.h:92
> +    // Instance Data

Unnecessary comment.

> WebKitTools/QtTestBrowser/optionshandler.h:97
> +#endif // OPTIONSHANDLER_H

Nit: It's optionshandler_h above and OPTIONSHANDLER_H below.