Bug 62424 - [GTK] webkit_web_settings_copy does not copy all settings
Summary: [GTK] webkit_web_settings_copy does not copy all settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
Depends on: 61972
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-09 19:37 PDT by Martin Robinson
Modified: 2011-06-20 14:19 PDT (History)
2 users (show)

See Also:


Attachments
Patch (7.28 KB, patch)
2011-06-10 18:01 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-06-09 19:37:29 PDT
webkit_web_settings_copy copies some of the settings in WebKitWebSettings, but it appears to be out of date. It only copies a subset of the total settings that the GObject tracks.
Comment 1 Martin Robinson 2011-06-10 18:01:41 PDT
Created attachment 96831 [details]
Patch
Comment 2 Xan Lopez 2011-06-12 16:50:19 PDT
Comment on attachment 96831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96831&action=review

OK with those changes, I guess, but I feel this will bite us down the road somehow ;)

> Source/WebKit/gtk/tests/testwebsettings.c:52
> +    g_assert_cmpstr(defaultEncoding, ==, "utf-8");

Leaking defaultEncoding!

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1252
> +    for (size_t i = 0; i < numberOfProperties; i++) {

I believe to be at least a bit more correct you should check the property has a G_PARAM_CONSTRUCT flag. And uh... READWRITE?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1253
> +        GParamSpec* property = properties.get()[i];

There is no nicer way to write this? at least properties[i].get() ?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1254
> +        GParameter* parameter = parameters.get() + i;

And why do you do it differently in the next line?
Comment 3 Martin Robinson 2011-06-20 14:17:33 PDT
(In reply to comment #2)
> (From update of attachment 96831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96831&action=review

Thanks for the review!

> OK with those changes, I guess, but I feel this will bite us down the road somehow ;)

Perhaps. :/

> > Source/WebKit/gtk/tests/testwebsettings.c:52
> > +    g_assert_cmpstr(defaultEncoding, ==, "utf-8");
> Leaking defaultEncoding!

Fixed.

> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1252
> > +    for (size_t i = 0; i < numberOfProperties; i++) {
> 
> I believe to be at least a bit more correct you should check the property has a G_PARAM_CONSTRUCT flag. And uh... READWRITE?

Fixed. This wasn't there before since we know that all properties use the same flags in this class. I agree that it will make this code safer though.

> 
> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1253
> > +        GParamSpec* property = properties.get()[i];
> 
> There is no nicer way to write this? at least properties[i].get() ?

I don't think that GOwnPtr has that operator overload.

> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1254
> > +        GParameter* parameter = parameters.get() + i;
> And why do you do it differently in the next line?

One of them is a GParamSpec** and the other is a GParameter*, but they are both 1-dimensional arrays. They are just stored in memory differently.
Comment 4 Martin Robinson 2011-06-20 14:18:43 PDT
Committed r89288: <http://trac.webkit.org/changeset/89288>
Comment 5 Martin Robinson 2011-06-20 14:19:36 PDT
Comment on attachment 96831 [details]
Patch

Clearing flags after landing.