[webkit-reviews] review denied: [Bug 29188] [Qt] Add persistence support for the "Always enable" options in the inspector : [Attachment 39638] Patch v0.2

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at trolltech.com> has denied Jocelyn Turcotte
<jocelyn.turcotte at nokia.com>'s request for review:
Bug 29188: [Qt] Add persistence support for the "Always enable" options in the
inspector
https://bugs.webkit.org/show_bug.cgi?id=29188

Attachment 39638: Patch v0.2
https://bugs.webkit.org/attachment.cgi?id=39638&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at trolltech.com>

> -    An inspector allows you to see a page current hierarchy and loading
> +    The inspector allows you to see a page current hierarchy and loading

This change has nothing to do with the rest of the patch?

> +    // To allow QWebInspector's configuration persistence
> +    QCoreApplication::setOrganizationName("Trolltech");

Trolltech ceased to exist, we can't use it anymore.

> +    QCoreApplication::setApplicationName("QtLauncher");

> +#include <QSettings>

QtCore/QSettings, like other includes.

> +static const QString SETTINGS_STORAGE_PREFIX("Qt/QtWebKit/QWebInspector/");
> +static const QString SETTINGS_STORAGE_TYPE_SUFFIX(".type");

Since this is not a macro, no need to capital letters.
Also use QLatin1String for string constants.

> +    if (qsettings.status() == QSettings::AccessError) {
> +	   // QCoreApplication::setOrganizationName and
QCoreApplication::setApplicationName haven't been called
> +	   return;
> +    }

Don't we want a qWarning here?

> +InspectorController::Setting InspectorClientQt::variantToSetting(const
QVariant& qvariant)
> +QVariant InspectorClientQt::settingToVariant(const
InspectorController::Setting& icSetting)

Both of the functions can be just static functions in the source code.
Is there a reason why they are static member functions instead?

> +	   retVal.setValue(static_cast<QString>(icSetting.string()));

Are all the manual casts from String to QString necessary? I thought we have
automatic conversion for that.