[GTK] webkit_web_settings_copy does not copy all settings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 96831 [details]
Patch

[Xan Lopez]

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?

[Martin Robinson]

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

[Martin Robinson]

Committed r89288: <http://trac.webkit.org/changeset/89288>

[Martin Robinson]

Comment on attachment 96831 [details]
Patch

Clearing flags after landing.