Summary: | [Qt] Move options handling in QtTestBrowser to separate OptionsHandler class | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Kyzivat <kamaji> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED INVALID | ||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, nancy.piedra, Ramesh.Bapanapalli, tonikitoo, webkit.review.bot | ||||||
Priority: | P3 | Keywords: | EasyFix, Qt, QtTriaged | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Keith Kyzivat
2010-11-02 09:17:22 PDT
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.
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.
Including config.h I don't think makes sense here... does it? Add [Qt] to summary, as this is Qt specific. 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 <keith.kyzivat@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@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. Please follow http://trac.webkit.org/wiki/QtWebKitBugs when reporing bugs here (missing Qt keyword). Created attachment 87107 [details]
Improved patch, updated with latest changes in Webkit
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.
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. 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 ;-) 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 <keith.kyzivat@nokia.com> 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. === 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. |