[webkit-reviews] review denied: [Bug 48842] [Qt] Move options handling in QtTestBrowser to separate OptionsHandler class : [Attachment 87107] Improved patch, updated with latest changes in Webkit

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo 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 87107: Improved patch, updated with latest changes in Webkit
https://bugs.webkit.org/attachment.cgi?id=87107&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
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 at 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.