[Webkit-unassigned] [Bug 68996] [GTK][WEBKIT2] Add Font and Encoding properties to WebKitWebSettings.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 28 05:35:59 PDT 2011 

https://bugs.webkit.org/show_bug.cgi?id=68996





--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-09-28 05:36:00 PST ---
(From update of attachment 109007)
View in context: https://bugs.webkit.org/attachment.cgi?id=109007&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:622
> +static char* WKStringGetCString(WKStringRef string)
> +{
> +    size_t length = WKStringGetMaximumUTF8CStringSize(string);
> +    char *buffer = (char *) g_malloc(length);
> +    WKStringGetUTF8CString(string, buffer, length);
> +    return buffer;
> +}
> +

We don't need this, this is needed in MiniBrowser where you only use C api, here you can do:

toImpl(string)->utf8().data()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1143
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

it should 0 instead of FALSE

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1146
> +    const gchar* defaultFontFamily = WKStringGetCString(standardFontFamilyRef);

This is wrong, WKStringGetCString() doesn't return a const char * but a new allocated string, so you are leaking it. I like the idea of public methods returning a const char *, but for that we need to cache the uft8 string and return it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1170
> +    g_object_notify(G_OBJECT(settings), "default-font-family");

You should check whether the property actually changed before emitting notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1186
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1189
> +    const gchar* monospaceFontFamily = WKStringGetCString(fixedFontFamilyRef);

Same leak here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1213
> +    g_object_notify(G_OBJECT(settings), "monospace-font-family");

Check it changed before emitting notify

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1229
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1232
> +    const gchar* serifFontFamily = WKStringGetCString(serifFontFamilyRef);

Same leak here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1256
> +    g_object_notify(G_OBJECT(settings), "serif-font-family");

Check it changed before emitting notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1272
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1275
> +    const gchar* sansSerifFontFamily = WKStringGetCString(sansSerifFontFamilyRef);

Same leak here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1299
> +    g_object_notify(G_OBJECT(settings), "sans-serif-font-family");

Check it changed before emitting notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1315
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1318
> +    const gchar* cursiveFontFamily = WKStringGetCString(cursiveFontFamilyRef);

Same leak here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1342
> +    g_object_notify(G_OBJECT(settings), "cursive-font-family");

Check it changed before emitting notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1358
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1361
> +    const gchar* fantasyFontFamily = WKStringGetCString(fantasyFontFamilyRef);

Same leak here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1385
> +    g_object_notify(G_OBJECT(settings), "fantasy-font-family");

Check it changed before emitting notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1401
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1404
> +    const gchar* pictographFontFamily = WKStringGetCString(pictographFontFamilyRef);

Same leak here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1428
> +    g_object_notify(G_OBJECT(settings), "pictograph-font-family");

Check it changed before emitting notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1444
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1485
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1526
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1567
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +

0 instead of FALSE.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1570
> +    const gchar* defaultEncoding = WKStringGetCString(defaultEncodingRef);

Same leak here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1594
> +    g_object_notify(G_OBJECT(settings), "default-encoding");

Check it changed before emitting notify.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:150
> +    g_assert(!g_strcmp0(webkit_web_settings_get_default_font_family(settings), "sans-serif"));

Use g_assert_cmpstr().

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:152
> +    g_assert(!g_strcmp0(webkit_web_settings_get_default_font_family(settings), "monospace"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:160
> +    g_assert(!g_strcmp0(webkit_web_settings_get_monospace_font_family(settings), "monospace"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:162
> +    g_assert(!g_strcmp0(webkit_web_settings_get_monospace_font_family(settings), "sans-serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:171
> +    g_assert(!g_strcmp0(webkit_web_settings_get_serif_font_family(settings), "serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:173
> +    g_assert(!g_strcmp0(webkit_web_settings_get_serif_font_family(settings), "sans-serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:182
> +    g_assert(!g_strcmp0(webkit_web_settings_get_sans_serif_font_family(settings), "sans-serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:184
> +    g_assert(!g_strcmp0(webkit_web_settings_get_sans_serif_font_family(settings), "serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:193
> +    g_assert(!g_strcmp0(webkit_web_settings_get_cursive_font_family(settings), "serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:195
> +    g_assert(!g_strcmp0(webkit_web_settings_get_cursive_font_family(settings), "sans-serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:204
> +    g_assert(!g_strcmp0(webkit_web_settings_get_fantasy_font_family(settings), "serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:206
> +    g_assert(!g_strcmp0(webkit_web_settings_get_fantasy_font_family(settings), "sans-serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:215
> +    g_assert(!g_strcmp0(webkit_web_settings_get_pictograph_font_family(settings), "serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:217
> +    g_assert(!g_strcmp0(webkit_web_settings_get_pictograph_font_family(settings), "sans-serif"));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:225
> +    g_assert(webkit_web_settings_get_default_font_size(settings) == 12);

Use g_assert_cmpuint().

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:227
> +    g_assert(webkit_web_settings_get_default_font_size(settings) == 14);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:236
> +    g_assert(webkit_web_settings_get_default_monospace_font_size(settings) == 10);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:238
> +    g_assert(webkit_web_settings_get_default_monospace_font_size(settings) == 12);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:247
> +    g_assert(webkit_web_settings_get_minimum_font_size(settings) == 5);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:249
> +    g_assert(webkit_web_settings_get_minimum_font_size(settings) == 7);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:257
> +    g_assert(!g_strcmp0(webkit_web_settings_get_default_encoding(settings), "iso-8859-1"));

Use g_assert_cmpstr().

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:259
> +    g_assert(!g_strcmp0(webkit_web_settings_get_default_encoding(settings), "utf8"));

Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list