Bug 43512 - [GTK] Use GSettings to save/restore Web Inspector settings
Summary: [GTK] Use GSettings to save/restore Web Inspector settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-08-04 15:04 PDT by Gustavo Noronha (kov)
Modified: 2010-08-12 16:24 PDT (History)
3 users (show)

See Also:


Attachments
proposed patch (12.79 KB, patch)
2010-08-04 15:11 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
proposed patch (12.75 KB, patch)
2010-08-04 15:27 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
proposed patch (13.91 KB, patch)
2010-08-05 09:12 PDT, Gustavo Noronha (kov)
mrobinson: review-
Details | Formatted Diff | Diff
use gsettings for the inspector (15.09 KB, patch)
2010-08-12 07:50 PDT, Gustavo Noronha (kov)
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2010-08-04 15:04:31 PDT
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+.
Comment 1 Gustavo Noronha (kov) 2010-08-04 15:11:22 PDT
Created attachment 63499 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-08-04 15:14:48 PDT
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.
Comment 3 Gustavo Noronha (kov) 2010-08-04 15:27:12 PDT
Created attachment 63501 [details]
proposed patch

Fixed style issues.
Comment 4 Martin Robinson 2010-08-04 15:38:45 PDT
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.
Comment 5 Gustavo Noronha (kov) 2010-08-05 06:03:33 PDT
(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 =).
Comment 6 Gustavo Noronha (kov) 2010-08-05 09:12:41 PDT
Created attachment 63599 [details]
proposed patch

Addresses Martin's suggestions, and enables parallel-instalability of gtk2 and gtk3 versions of the schema.
Comment 7 Martin Robinson 2010-08-11 16:13:28 PDT
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?
Comment 8 Martin Robinson 2010-08-11 18:13:02 PDT
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.
Comment 9 Gustavo Noronha (kov) 2010-08-12 07:50:25 PDT
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.
Comment 10 WebKit Review Bot 2010-08-12 07:52:25 PDT
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.
Comment 11 Gustavo Noronha (kov) 2010-08-12 07:53:27 PDT
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 =(
Comment 12 Martin Robinson 2010-08-12 08:02:31 PDT
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!
Comment 13 Gustavo Noronha (kov) 2010-08-12 16:24:52 PDT
Landed as r65255.