Bug 43512 – [GTK] Use GSettings to save/restore Web Inspector settings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gustavo Noronha (kov)]

The Web Inspector has a number of settings it uses to decide whether, for instance, the javascript debugger should always be enabled. Similar settings exist for the profiler, resource tracker, and along with those there are a few other settings. Now that glib has GSettings, it's a good time to add support for those hooks into WebKitGTK+.

[Gustavo Noronha (kov)]

Created attachment 63499 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:121:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:137:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:145:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 12 files


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

[Gustavo Noronha (kov)]

Created attachment 63501 [details]
proposed patch

Fixed style issues.

[Martin Robinson]

Comment on attachment 63499 [details]
proposed patch

Awesome patch!

> +gsettings_SCHEMAS = $(srcdir)/WebKit/gtk/org.webkit.gtk.gschema.xml

Perhaps the identifier should be org.webkitgtk.gschema.xml to match our
domain name? I'm not sure what the typical procedure is here.

> +GSettings* webkit_get_inspector_g_settings()
> +{
> +    static GSettings* settings = g_settings_new("org.webkit.gtk.inspector");
> +    return settings;
> +}

I think it makes sense to follow WebKit-style naming conventions
for this helper, since it isn't part of the API.

[Gustavo Noronha (kov)]

(In reply to comment #4)
> (From update of attachment 63499 [details])
> Awesome patch!

Thanks =)
 
> > +gsettings_SCHEMAS = $(srcdir)/WebKit/gtk/org.webkit.gtk.gschema.xml
> 
> Perhaps the identifier should be org.webkitgtk.gschema.xml to match our
> domain name? I'm not sure what the typical procedure is here.

Agreed, makes sense!

> > +GSettings* webkit_get_inspector_g_settings()
> > +{
> > +    static GSettings* settings = g_settings_new("org.webkit.gtk.inspector");
> > +    return settings;
> > +}
> 
> I think it makes sense to follow WebKit-style naming conventions
> for this helper, since it isn't part of the API.

Hmm, yes, I think you are right here as well, though I was thinking this might become convenience API for browsers wanting to track/manipulate the settings, but I guess we can rename/relocate the helper if that turns out to be the case =).

[Gustavo Noronha (kov)]

Created attachment 63599 [details]
proposed patch

Addresses Martin's suggestions, and enables parallel-instalability of gtk2 and gtk3 versions of the schema.

[Martin Robinson]

I like this patch a lot, but I wonder if there is a way to write it in a way which basically no-ops on older versions of GLib instead of bumping the required version again. What do you think?

[Martin Robinson]

Comment on attachment 63599 [details]
proposed patch

Gustavo mentioned to me over IRC that it is possible to write this patch without bumping the required version of GIO (conditional compilation of the GSettings bits). I'll r- for now, until he gets back to this. If it isn't possible, feel free to r? again. It looks awesome to me otherwise.

[Gustavo Noronha (kov)]

Created attachment 64224 [details]
use gsettings for the inspector

Addressed Martin's suggestion (effectiveness will be tested by the EWS, which should have an earlier glib version ;D)

Also, I built with introspection this time, and found GSettings would cause us trouble because it _really_ wants the schema to be installed (it aborts the process if it is not!). I made WebKit more reliant/robust to gsettings whims in this patch.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/webkit/webkitprivate.cpp:224:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 11 files


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

[Gustavo Noronha (kov)]

Comment on attachment 64224 [details]
use gsettings for the inspector

WebKit/gtk/JSCore.gir.in:2
 +  <repository version="1.0"
err... disregard this one, it's just something I need to do to be able to build with an older gobject-introspection, I'm researching a way of detecting the version to generate it correctly automatically based on the gobject-introspection version =(

[Martin Robinson]

Comment on attachment 64224 [details]
use gsettings for the inspector

> +#if HAVE_GSETTINGS
> +template <> GVariant* refGPtr(GVariant* ptr)
> +{
> +    if (ptr)
> +        g_variant_ref(ptr);
> +    return ptr;
> +}
> +
> +template <> void derefGPtr(GVariant* ptr)
> +{
> +    g_variant_unref(ptr);
> +}
> +#endif
> +#if HAVE_GSETTINGS
> +template <> GVariant* refGPtr(GVariant* ptr);
> +template <> void derefGPtr(GVariant* ptr);
> +#endif

I'd rather this be unprotected by HAVE_GSETTINGS. It might be useful later
for non-GSettings code!

> +    while (*iter != NULL) {
> +        if (g_str_equal(schemaID, *iter))
> +            return true;
> +        iter++;
> +    }

Style error, as the bot mentioned.

But if you change those two small nits and remove the inadvertant change to the GIR
file, r=me. Thanks!

[Gustavo Noronha (kov)]

Landed as r65255.