Bug 48842 – [Qt] Move options handling in QtTestBrowser to separate OptionsHandler class

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Keith Kyzivat]

Move options handling in QtTestBrowser to separate OptionsHandler class, in preparation for adding more options and reducing complexity in main.cpp

The options handling in QtTestBrowser is currently done all within main.cpp.  This is fine when the number of options to handle is low, but I am working on adding more command line options, and this will increase the complexity and confusion of QtTestBrowser's main.cpp.  This option handling can be moved to a separate OptionsHandler class for handling, so that options handling is isolated, and app startup is easier to read and maintain.

This is being done in preparation for another patch that I will be providing that expands the command line options to provide runtime configurability of more QWebSettings settings.

[Keith Kyzivat]

Created attachment 72736 [details]
First stab at a patch providing OptionsHandler

Here is my changes to add an OptionsHandler class, moving most of the options parsing to a separate class -- those that deal with QWebSettings.

[WebKit Review Bot]

Attachment 72736 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKitTools/ChangeLog', u'WebKitTools/QtTestBrowser/QtTestBrowser.pro', u'WebKitTools/QtTestBrowser/main.cpp', u'WebKitTools/QtTestBrowser/optionshandler.cpp', u'WebKitTools/QtTestBrowser/optionshandler.h']" exit_code: 1
WebKitTools/QtTestBrowser/optionshandler.cpp:30:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Kyzivat]

Including config.h I don't think makes sense here... does it?

[Keith Kyzivat]

Add [Qt] to summary, as this is Qt specific.

[Andreas Kling]

Comment on attachment 72736 [details]
First stab at a patch providing OptionsHandler

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 <[e-mail]>

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 <[e-mail]>

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.

[Benjamin Poulain]

Please follow http://trac.webkit.org/wiki/QtWebKitBugs when reporing bugs here (missing Qt keyword).

[Keith Kyzivat]

Created attachment 87107 [details]
Improved patch, updated with latest changes in Webkit

[WebKit Review Bot]

Attachment 87107 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/Qt..." exit_code: 1

Tools/QtTestBrowser/optionshandler.cpp:30:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/QtTestBrowser/optionshandler.cpp:72:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Kyzivat]

Comment on attachment 87107 [details]
Improved patch, updated with latest changes in Webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=87107&action=review

> Tools/QtTestBrowser/main.cpp:-177
> -        qDebug() << "Usage:" << programName.toLatin1().data()

I left this in since passing a QString to qDebug results in it printing out surrounded with quotes.
If there's a better, Qtish way of printing a QString to stderr without quotes, I'd love to hear.

[Simon Hausmann]

Comment on attachment 87107 [details]
Improved patch, updated with latest changes in Webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=87107&action=review

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

It's 2011 now ;-)

[Antonio Gomes]

Comment on attachment 87107 [details]
Improved patch, updated with latest changes in Webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=87107&action=review

> Tools/QtTestBrowser/optionshandler.cpp:35
> +OptionsHandler::OptionsHandler(const QStringList& arguments, const QString& programName, QObject *parent)

"*" is on the left side.

> Tools/QtTestBrowser/optionshandler.cpp:85
> +    return m_arguments;

why does a SET method returns something?

> Tools/QtTestBrowser/optionshandler.cpp:88
> +QStringList OptionsHandler::setWindowOptions(WindowOptions& windowOptions)

Ditto.

> Tools/QtTestBrowser/optionshandler.cpp:179
> +        if (   m_arguments[i] == "-default-animations"

Style: no empty spaces needed here.

> Tools/QtTestBrowser/optionshandler.cpp:214
> +    setGlobalWebSettings();
> +    setWindowOptions(windowOptions);

You are ignoring the returned values anyways :)

> Tools/QtTestBrowser/optionshandler.cpp:216
> +    return m_arguments;

Why do you return stuff here again too?

> Tools/QtTestBrowser/optionshandler.h:3
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
> + * Copyright (C) 2010 Keith Kyzivat <[e-mail]>

2011 :)

> Tools/QtTestBrowser/optionshandler.h:42
> +    OptionsHandler(const QStringList& arguments, const QString& programName, QObject *parent = 0);

"*" is on the left  side.

> Tools/QtTestBrowser/optionshandler.h:56
> +      Returns the program arguments with the options that were handled removed.

maybe applyGlobalWebSettings? then returning something might make some sense.

> Tools/QtTestBrowser/optionshandler.h:67
> +    QStringList setWindowOptions(WindowOptions&);

Ditto.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.