Bug 29188 – [Qt] Add persistence support for the "Always enable" options in the inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Jocelyn Turcotte]

Created attachment 39442 [details]
Patch

This patch uses QSettings to persist when a user select "Always enable" for the Resources, Scripts or Profiles tabs.

The developer have to previously initialize QSettings using QCoreApplication::setOrganizationName() and QCoreApplication::setApplicationName().
If QSettings cannot be initialized, the Always enable option is not persisted.

[Timothy Hatcher]

Comment on attachment 39442 [details]
Patch


> +    if (key == "resourceTrackingEnabled")
> +        setting = InspectorController::Setting(qsettings.value(QLatin1String("Qt/QtWebKit/QWebInspector/resourceTrackingEnabled")).toBool());
> +    else if (key == "debuggerEnabled")
> +        setting = InspectorController::Setting(qsettings.value(QLatin1String("Qt/QtWebKit/QWebInspector/debuggerEnabled")).toBool());
> +    else if (key == "profilerEnabled")
> +        setting = InspectorController::Setting(qsettings.value(QLatin1String("Qt/QtWebKit/QWebInspector/profilerEnabled")).toBool());

Can you dynamically generate the setting key path? There will be more settings added to the inspector, and making it dynamic will let them work automatically.

(There is a setting that landed recently for the color format pref.)

[Eric Seidel (no email)]

Comment on attachment 39442 [details]
Patch

Agree w/ Tim.  r- for the unecessary code duplication re: the paths.  Seems they could be dynamic.

[Jocelyn Turcotte]

Created attachment 39638 [details]
Patch v0.2

Good point, patch updated

[Ariya Hidayat]

Comment on attachment 39638 [details]
Patch v0.2


> -    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.

[Simon Hausmann]

(In reply to comment #4)
> > +    // To allow QWebInspector's configuration persistence
> > +    QCoreApplication::setOrganizationName("Trolltech");
> 
> Trolltech ceased to exist, we can't use it anymore.

I guess we could use "Nokia" instead :)
 
> > +    QCoreApplication::setApplicationName("QtLauncher");
> 
> > +#include <QSettings>
> 
> QtCore/QSettings, like other includes.

Hmm, we need this style of inclusion only in our public header
files (to avoid that customers have to change their .pro files),
but for the implementation I think the prefixless inclusion
is fine.
 
> > +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.

I was about to suggest a

static const char* settingsStorageSuffix[] = "..."

Either way the global variable should not be upper-cased if I interpret the style correctly.

[Jocelyn Turcotte]

Created attachment 39690 [details]
Patch v0.3

> > +        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.

This one is necessary since setValue is a template.
Other were to fix ambiguity operator+ error, using QLatin1String instead also did the trick so I removed them.

Other comments have been applied in the patch, thx.

[Simon Hausmann]

Comment on attachment 39690 [details]
Patch v0.3

r=me

As discussed with Jocelyn, I'm going to remove the qWarning() when landing, to prevent flooding of the terminal.

[Simon Hausmann]

Landed in r48504