Add font related properties (such as font-family and font-size) and encoding property (default-encoding) to WebKitWebSettings.
Created attachment 109007 [details] Font and Encoding properties.
Comment on attachment 109007 [details] Font and Encoding properties. 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.
Comment on attachment 109007 [details] Font and Encoding properties. View in context: https://bugs.webkit.org/attachment.cgi?id=109007&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1143 >> + > > it should 0 instead of FALSE Done >> 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. New approach caches the string and deletes in upon WebKitWebSetting unref. It also updates the cached string upon calling *_set_font_family functions. >> 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. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1186 >> + > > 0 instead of FALSE Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1189 >> + const gchar* monospaceFontFamily = WKStringGetCString(fixedFontFamilyRef); > > Same leak here. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1213 >> + g_object_notify(G_OBJECT(settings), "monospace-font-family"); > > Check it changed before emitting notify Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1229 >> + > > 0 instead of FALSE Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1232 >> + const gchar* serifFontFamily = WKStringGetCString(serifFontFamilyRef); > > Same leak here. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1256 >> + g_object_notify(G_OBJECT(settings), "serif-font-family"); > > Check it changed before emitting notify. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1272 >> + > > 0 instead of FALSE Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1275 >> + const gchar* sansSerifFontFamily = WKStringGetCString(sansSerifFontFamilyRef); > > Same leak here. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1299 >> + g_object_notify(G_OBJECT(settings), "sans-serif-font-family"); > > Check it changed before emitting notify. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1315 >> + > > 0 instead of FALSE. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1318 >> + const gchar* cursiveFontFamily = WKStringGetCString(cursiveFontFamilyRef); > > Same leak here. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1342 >> + g_object_notify(G_OBJECT(settings), "cursive-font-family"); > > Check it changed before emitting notify. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1358 >> + > > 0 instead of FALSE. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1361 >> + const gchar* fantasyFontFamily = WKStringGetCString(fantasyFontFamilyRef); > > Same leak here. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1385 >> + g_object_notify(G_OBJECT(settings), "fantasy-font-family"); > > Check it changed before emitting notify. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1401 >> + > > 0 instead of FALSE. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1404 >> + const gchar* pictographFontFamily = WKStringGetCString(pictographFontFamilyRef); > > Same leak here. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1428 >> + g_object_notify(G_OBJECT(settings), "pictograph-font-family"); > > Check it changed before emitting notify. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1444 >> + > > 0 instead of FALSE Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1485 >> + > > 0 instead of FALSE. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1526 >> + > > 0 instead of FALSE. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1567 >> + > > 0 instead of FALSE. Done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1570 >> + const gchar* defaultEncoding = WKStringGetCString(defaultEncodingRef); > > Same leak here. Done. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1594 >> + g_object_notify(G_OBJECT(settings), "default-encoding"); > > Check it changed before emitting notify. Done. >> 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(). Quite useful macro! Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:152 >> + g_assert(!g_strcmp0(webkit_web_settings_get_default_font_family(settings), "monospace")); > > Ditto. Done >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:160 >> + g_assert(!g_strcmp0(webkit_web_settings_get_monospace_font_family(settings), "monospace")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:162 >> + g_assert(!g_strcmp0(webkit_web_settings_get_monospace_font_family(settings), "sans-serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:171 >> + g_assert(!g_strcmp0(webkit_web_settings_get_serif_font_family(settings), "serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:173 >> + g_assert(!g_strcmp0(webkit_web_settings_get_serif_font_family(settings), "sans-serif")); > > Ditto. Done. >> 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. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:184 >> + g_assert(!g_strcmp0(webkit_web_settings_get_sans_serif_font_family(settings), "serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:195 >> + g_assert(!g_strcmp0(webkit_web_settings_get_cursive_font_family(settings), "sans-serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:204 >> + g_assert(!g_strcmp0(webkit_web_settings_get_fantasy_font_family(settings), "serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:206 >> + g_assert(!g_strcmp0(webkit_web_settings_get_fantasy_font_family(settings), "sans-serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:215 >> + g_assert(!g_strcmp0(webkit_web_settings_get_pictograph_font_family(settings), "serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:217 >> + g_assert(!g_strcmp0(webkit_web_settings_get_pictograph_font_family(settings), "sans-serif")); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:225 >> + g_assert(webkit_web_settings_get_default_font_size(settings) == 12); > > Use g_assert_cmpuint(). Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:227 >> + g_assert(webkit_web_settings_get_default_font_size(settings) == 14); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:236 >> + g_assert(webkit_web_settings_get_default_monospace_font_size(settings) == 10); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:238 >> + g_assert(webkit_web_settings_get_default_monospace_font_size(settings) == 12); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:247 >> + g_assert(webkit_web_settings_get_minimum_font_size(settings) == 5); > > Ditto. Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:249 >> + g_assert(webkit_web_settings_get_minimum_font_size(settings) == 7); > > Ditto. Done. >> 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(). Done. >> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:259 >> + g_assert(!g_strcmp0(webkit_web_settings_get_default_encoding(settings), "utf8")); > > Ditto. Done.
Created attachment 109023 [details] Added Font and Encoding related API's
Comment on attachment 109007 [details] Font and Encoding properties. View in context: https://bugs.webkit.org/attachment.cgi?id=109007&action=review > Source/WebKit2/ChangeLog:8 > + Add font and encoding releated properties to WebKitWebSettings. > + > + Provision to query and set the font related properties (such as > + font-family, font-size) and encoding properties (such as default- > + encoding) is added. > + https://bugs.webkit.org/show_bug.cgi?id=68996 ChangeLogs should be of the form [bug title] [bug url] [other stuff] > Source/WebKit2/ChangeLog:31 > + * UIProcess/API/gtk/WebKitWebSettings.cpp: > + (webkitWebSettingsSetProperty): > + (webkitWebSettingsGetProperty): > + (webkit_web_settings_class_init): > + (WKStringGetCString): > + (webkit_web_settings_get_default_font_family): > + (webkit_web_settings_set_default_font_family): > + (webkit_web_settings_get_monospace_font_family): > + (webkit_web_settings_set_monospace_font_family): > + (webkit_web_settings_get_serif_font_family): > + (webkit_web_settings_set_serif_font_family): > + (webkit_web_settings_get_sans_serif_font_family): > + (webkit_web_settings_set_sans_serif_font_family): > + (webkit_web_settings_get_cursive_font_family): > + (webkit_web_settings_set_cursive_font_family): > + (webkit_web_settings_get_fantasy_font_family): > + (webkit_web_settings_set_fantasy_font_family): > + (webkit_web_settings_get_pictograph_font_family): > + (webkit_web_settings_set_pictograph_font_family): > + (webkit_web_settings_get_default_font_size): Either fill these out or remove each method and make a summary. Having them blank is just noise. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:481 > + * The default Serif font family used to display text. Serif, sans-serif, cursive etc are not proper nounds and should not be capitalized. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1468 > + WKPreferencesSetDefaultFontSize(priv->preferences, (uint32_t)fontSize); Do not use C style casts. Can this be an implicit cast? The documentation for all these methods should be explicit that the number accepted is in pixels and not points. > Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:214 > + // Default Pictograph Font Family font family is "serif" Missing a period. > Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:224 > + // Default Default Font Size is 12. Default Default. > Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:246 > + // Default Minimum Font Size is 5. minimum font size shouldn't be capitalized. > Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:256 > + // Default Default encoding is "iso-8859-1" Same issues + missing a period. > Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:290 > + g_test_add_func("/webkit2/websettings/defaultfontfamily", testWebKitWebSettingsDefaultFontFamily); > + g_test_add_func("/webkit2/websettings/monospacefontfamily", testWebKitWebSettingsMonospaceFontFamily); > + g_test_add_func("/webkit2/websettings/seriffontfamily", testWebKitWebSettingsSerifFontFamily); > + g_test_add_func("/webkit2/websettings/sansseriffontfamily", testWebKitWebSettingsSansSerifFontFamily); > + g_test_add_func("/webkit2/websettings/cursivefontfamily", testWebKitWebSettingsCursiveFontFamily); > + g_test_add_func("/webkit2/websettings/fanstasyfontfamily", testWebKitWebSettingsFantasyFontFamily); > + g_test_add_func("/webkit2/websettings/pictographfontfamily", testWebKitWebSettingsPictographFontFamily); > + g_test_add_func("/webkit2/websettings/defaultfontsize", testWebKitWebSettingsDefaultFontSize); > + g_test_add_func("/webkit2/websettings/defaultMonospaceFontSize", testWebKitWebSettingsDefaultMonospaceFontSize); > + g_test_add_func("/webkit2/websettings/minimumFontSize", testWebKitWebSettingsMinimumFontSize); > + g_test_add_func("/webkit2/websettings/defaultEncoding", testWebKitWebSettingsDefaultEncoding); This can be one test.
Comment on attachment 109023 [details] Added Font and Encoding related API's View in context: https://bugs.webkit.org/attachment.cgi?id=109023&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:42 > +static gchar* WKStringGetCString(WKStringRef stringRef) > +{ > + gchar* buffer = static_cast<gchar *>(g_malloc0(WKStringGetLength(stringRef) + 1)); > + WKStringGetUTF8CString(stringRef, buffer, WKStringGetLength(stringRef) + 1); > + WKRelease(stringRef); > + return buffer; > +} Use toImpl(string)->utf8().data() instead. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:54 > + gchar* defaultFontFamily; > + gchar* monospaceFontFamily; > + gchar* serifFontFamily; > + gchar* sansSerifFontFamily; > + gchar* cursiveFontFamily; > + gchar* fantasyFontFamily; > + gchar* pictographFontFamily; > + gchar* defaultEncoding; Use GOwnPtr<char> for these. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:87 > + ~_WebKitWebSettingsPrivate() > + { > + if(defaultFontFamily) > + g_free(defaultFontFamily); > + if(monospaceFontFamily) > + g_free(monospaceFontFamily); > + if(serifFontFamily) > + g_free(serifFontFamily); > + if(sansSerifFontFamily) > + g_free(sansSerifFontFamily); > + if(cursiveFontFamily) > + g_free(cursiveFontFamily); > + if(fantasyFontFamily) > + g_free(fantasyFontFamily); > + if(pictographFontFamily) > + g_free(pictographFontFamily); > + if(defaultEncoding) > + g_free(defaultEncoding); You don't need this when using GOwnPtr > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:305 > +static void webkitWebSettingsFinalize(GObject* gObject) > +{ > + WEBKIT_WEB_SETTINGS(gObject)->priv->~_WebKitWebSettingsPrivate(); > + G_OBJECT_CLASS(webkit_web_settings_parent_class)->finalize(gObject); > +} > + We don't need this either. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:676 > +/* > +static char* WKStringGetCString(WKStringRef string) > +{ > + size_t length = WKStringGetMaximumUTF8CStringSize(string); > + char *buffer = (char *) g_malloc(length); > + WKStringGetUTF8CString(string, buffer, length); > + return buffer; > +} > +*/ > + I guess you forgot to remove this. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1218 > + if (g_ascii_strcasecmp(priv->defaultFontFamily, defaultFontFamily) == 0) > + return; Use if (!g_ascii_strcasecmp(priv->defaultFontFamily, defaultFontFamily)) instead. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1263 > + if (g_ascii_strcasecmp(priv->monospaceFontFamily, monospaceFontFamily) == 0) > + return; Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1314 > + g_object_notify(G_OBJECT(settings), "serif-font-family"); Check here settings actually changed before emitting notify > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1357 > + g_object_notify(G_OBJECT(settings), "sans-serif-font-family"); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1400 > + g_object_notify(G_OBJECT(settings), "cursive-font-family"); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1443 > + g_object_notify(G_OBJECT(settings), "fantasy-font-family"); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1486 > + g_object_notify(G_OBJECT(settings), "pictograph-font-family"); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1651 > + g_object_notify(G_OBJECT(settings), "default-encoding"); Ditto.
Comment on attachment 109023 [details] Added Font and Encoding related API's View in context: https://bugs.webkit.org/attachment.cgi?id=109023&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:54 >> + gchar* defaultEncoding; > > Use GOwnPtr<char> for these. I recommend using CString instead. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:305 >> + > > We don't need this either. If he uses GOwnptr or CString he does. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1225 > + WKStringRef standardFontFamilyRef = WKStringCreateWithUTF8CString(defaultFontFamily); > + WKPreferencesSetStandardFontFamily(priv->preferences, standardFontFamilyRef); > + priv->defaultFontFamily = static_cast<gchar *> (g_realloc(priv->defaultFontFamily, WKStringGetLength(standardFontFamilyRef) + 1)); > + WKStringGetUTF8CString(standardFontFamilyRef, priv->defaultFontFamily, WKStringGetLength(standardFontFamilyRef) + 1); > + WKRelease(standardFontFamilyRef); > + Is it important to refetch the font name from WebKit?
(In reply to comment #7) > (From update of attachment 109023 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109023&action=review > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:54 > >> + gchar* defaultEncoding; > > > > Use GOwnPtr<char> for these. > > I recommend using CString instead. Yes, or CString :-) > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:305 > >> + > > > > We don't need this either. > > If he uses GOwnptr or CString he does. I think we should use placement new syntax and we don't need that. Or am I wrong? I think I already proposed using it in the other bug about web settings.
(In reply to comment #8) > I think we should use placement new syntax and we don't need that. Or am I wrong? I think I already proposed using it in the other bug about web settings. Sorry, I should have been more clear here. Iswas referring to explicitly calling the destructor. If we use placement new syntax, we need to do it so that the destructors of all the members are called. If we use "new Private" we can just call "delete Private" later to have the same effect.
(In reply to comment #9) > Sorry, I should have been more clear here. Iswas referring to explicitly calling the destructor. If we use placement new syntax, we need to do it so that the destructors of all the members are called. If we use "new Private" we can just call "delete Private" later to have the same effect. Ah!, I didn't know we had to manually call the destructor . . . I need to fix it in other places then :-P Sorry for the noise.
(In reply to comment #7) > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1225 > > + WKStringRef standardFontFamilyRef = WKStringCreateWithUTF8CString(defaultFontFamily); > > + WKPreferencesSetStandardFontFamily(priv->preferences, standardFontFamilyRef); > > + priv->defaultFontFamily = static_cast<gchar *> (g_realloc(priv->defaultFontFamily, WKStringGetLength(standardFontFamilyRef) + 1)); > > + WKStringGetUTF8CString(standardFontFamilyRef, priv->defaultFontFamily, WKStringGetLength(standardFontFamilyRef) + 1); > > + WKRelease(standardFontFamilyRef); > > + > > Is it important to refetch the font name from WebKit? I believe only way in which the preferences stored in WebKitWebSettings and WKPreferences differs is when WebKit changes it, which is very unlikely. Hence, I don' think its necessary to re-fetch the font name from WebKit.
Created attachment 109142 [details] Font and encoding related properties.
Comment on attachment 109142 [details] Font and encoding related properties. View in context: https://bugs.webkit.org/attachment.cgi?id=109142&action=review this patch needs to be updated to reflect the current state of the tree. WebKitWebSettings is now called WebKitSettings and the testing looks a bit different. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:62 > + WKStringRef defaultFontFamilyRef = WKPreferencesCopyStandardFontFamily(preferences); > + defaultFontFamily = WebKit::toImpl(defaultFontFamilyRef)->string().utf8(); > + WKRelease(defaultFontFamilyRef); > + > + WKStringRef monospaceFontFamilyRef = WKPreferencesCopyFixedFontFamily(preferences); > + monospaceFontFamily = WebKit::toImpl(monospaceFontFamilyRef)->string().utf8(); > + WKRelease(monospaceFontFamilyRef); > + > + WKStringRef serifFontFamilyRef = WKPreferencesCopySerifFontFamily(preferences); Please use smart pointers to hold these values and move this initialization to the GObject instance initialization. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:300 > + WEBKIT_WEB_SETTINGS(gObject)->priv->~_WebKitWebSettingsPrivate(); > + G_OBJECT_CLASS(webkit_web_settings_parent_class)->finalize(gObject); You are not freeing the WKPreferences object here! Better to store it in the private section as a smart pointer so this happens automatically. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:508 > + _("Default Font Family"), Why is this Title Cased? > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:522 > + _("Monospace Font Family"), Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:536 > + _("Serif Font Family"), Ditto and continued below. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:600 > + * The default font size used to display text. You should say explicitly that this font size is in pixels. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:608 > + 5, G_MAXINT, 12, Why the arbitrary minimum bottom limit on font size? > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:623 > + * > + * The default font size used to display monospace text. > + * > + */ > + g_object_class_install_property(gObjectClass, > + PROP_DEFAULT_MONOSPACE_FONT_SIZE, > + g_param_spec_uint("default-monospace-font-size", > + _("Default Monospace Font Size"), > + _("The default font size used to display monospace text."), > + 5, G_MAXINT, 10, Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:629 > + * The minimum font size used to display text. You should say this is in points and also mention that choosing a value other than 0 can break the layout of some sites. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:637 > + 0, G_MAXINT, 5, The default minimum font size should be zero, I think. Why 5? > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:645 > + * WebKitWebSettings:default-encoding > + * > + * The default encoding used to display text. > + * > + */ I believe Carlos used "charset" in another patch. Whatever it is, we need to make the name standard. > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:651 > + "iso-8859-1", Why did you choose this encoding as the default? > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:1175 > + * Get property #Object:default-font-family. The format should be: (Get/Set) the #WebKitSettings:default-font-family-property. for all of these getters and setters.
Created attachment 114303 [details] Patch
Comment on attachment 109142 [details] Font and encoding related properties. View in context: https://bugs.webkit.org/attachment.cgi?id=109142&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:608 >> + 5, G_MAXINT, 12, > > Why the arbitrary minimum bottom limit on font size? I took these numbers from WebKit1. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:629 >> + * The minimum font size used to display text. > > You should say this is in points and also mention that choosing a value other than 0 can break the layout of some sites. I quite didn't understood what 'minimum-font-size' is expected to do!. Please help me. I took these numbers from WebKit1 again.
(In reply to comment #14) > Created an attachment (id=114303) [details] > Patch Changes included in this patch are a). Changing WebKitWebSettings to WebKitSettings. b). Followed the new test design c). Rebase'ed to latest WebKit.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 114305 [details] WebKitSettings patch with new APIs added in gtk-doc sections file
Comment on attachment 114305 [details] WebKitSettings patch with new APIs added in gtk-doc sections file View in context: https://bugs.webkit.org/attachment.cgi?id=114305&action=review Looks okay, but I have some nittish things below. The minimum font size controls how the absolute smallest size fonts will be rendered. If I'm not mistaken there is another setting default-minimum-logical-font-size. This patch should probably include that as well. Some notes in general for the rest of the patch: 1. parameter names in the headers should_be_like_this, but shouldBeLikeThis in the C++ files. gtkdoc comments should reflect the names of the paramters in the headers. 2. The string that comes after parameters are not complete sentences so should not begin with a capital letter. 3. Please run Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc and ensure your patch does not introduce any new warnings. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:559 > + * The default font size (in pixels) to use for content displayed if "in pixels" does not need to be in parenthesis. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:568 > + 5, G_MAXINT, 12, It's probably best just to set the minimum value as zero, unless there's a reason to use 5 that you can see. This parameter is a uint, so the maximum should probably be G_MAXUINT, right? > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:574 > + * The default font size (in pixels) to use for content displayed in "in pixels" does not need to be in parenthesis. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:583 > + 5, G_MAXINT, 10, Again, unless there's a reason you think that 5 should be the minimum, it's probably best to let it go to zero. G_MAXUINT here as well. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:589 > + * The minimum font size (in points) used to display text. Ditto. You should mention that values other than zero can potentially break page layouts. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:597 > + 0, G_MAXINT, 5, Again, G_MAXUINT here as well. The default minimum font size should be zero. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:603 > + * The default text charset used when interpreting content whose charset is not specified. This should read: "The default text charset used when interpreting content with an unspecified charset." > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1115 > + * Get the #WebKitSettings:default-font-family. Please change this to "Gets the #WebKitSettings:default-font-family property" Please make similar changes to methods below. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1124 > + WebKitSettingsPrivate* priv = settings->priv; > + return priv->defaultFontFamily.data(); This can be one line. Please make similar changes below. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1130 > + * @defaultFontFamily: String to be set This should read "default_font_family: the new default font family" The first word should not be capitalized. The parameter name here should match the one specified in the header. The parameter names in the header should use this_kind_of_case. Please make similar changes below. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1132 > + * Set the #WebKitSettings:default-font-family. Set the #WebKitSettings:default-font-family *property*. Please make similar changes below. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1134 > +void webkit_settings_set_default_font_family(WebKitSettings* settings, const gchar* defaultFontFamily) defaultFontFamily should be called defaultFontFamily here even though the parameter name in the header is default_font_family. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1141 > + if (!g_ascii_strcasecmp(priv->defaultFontFamily.data(), defaultFontFamily)) > + return; How about g_string_equal instead of g_ascii_strcasecmp. I don't think there's any guarantee that font names are ASCII and I think we should support changing the case. Please make similar changes below. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1491 > + WKPreferencesSetMinimumFontSize(priv->preferences.get(), (uint32_t)fontSize); Is it necessary to cast here? Please try removing the cast and checking if it creates a warning. If it does result in a warning, it's fine to cast, but you must always use C++ style casts in WebKit code. Please make the same check/change in other places in this patch you use casts. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:42 > + This seems like it was added accidentally. Please remove this.
Created attachment 114461 [details] Incorporated review comments
Comment on attachment 114305 [details] WebKitSettings patch with new APIs added in gtk-doc sections file View in context: https://bugs.webkit.org/attachment.cgi?id=114305&action=review WKPreference object doesn't expose any 'LogicalMinimumFont' API. >> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1141 >> + return; > > How about g_string_equal instead of g_ascii_strcasecmp. I don't think there's any guarantee that font names are ASCII and I think we should support changing the case. Please make similar changes below. g_string_equal uses GString. Instead I used g_strcmp0 in newly uploaded patch.
(In reply to comment #21) > g_string_equal uses GString. Instead I used g_strcmp0 in newly uploaded patch. Sorry I meant g_str_equal.
(In reply to comment #22) > (In reply to comment #21) > > > g_string_equal uses GString. Instead I used g_strcmp0 in newly uploaded patch. > > Sorry I meant g_str_equal. Documentation of g_str_equal says "Note that this function is primarily meant as a hash table comparison function. For a general-purpose, NULL-safe string comparison function, see g_strcmp0()." Seems like g_strcmp0 is more appropriate for this scenario?
(In reply to comment #23) > Seems like g_strcmp0 is more appropriate for this scenario? You're absolutely correct.
(In reply to comment #24) > (In reply to comment #23) > > > Seems like g_strcmp0 is more appropriate for this scenario? > > You're absolutely correct. In that case, my last patch already does that. :)
Comment on attachment 114461 [details] Incorporated review comments View in context: https://bugs.webkit.org/attachment.cgi?id=114461&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:475 > + * The font family used as the default for content marked with a monospace font. content marked -> content using > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:489 > + * The font family used as the default for content marked with a serif font. Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:503 > + * The font family used as the default for content marked with a sans-serif font. Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:517 > + * The font family used as the default for content marked with a cursive font. Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:531 > + * The font family used as the default for content marked with a fantasy font. Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:545 > + * The font family used as the default for content marked with a pictograph font. Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:560 > + * The default font size in pixels to use for content displayed if > + * no font size or font family is specified. This seems inaccurate. I believe the default size is used even when a font family is specified. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:599 > + 0, G_MAXUINT, 5, The default value for this property should be zero.
Created attachment 114623 [details] Incorporated review comments
Comment on attachment 114623 [details] Incorporated review comments View in context: https://bugs.webkit.org/attachment.cgi?id=114623&action=review Looks good, but please fix the small things below when landing. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:509 > + _("Sans-serif font Family"), Nit: Familly is capitalized here, but not above. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:523 > + _("Cursive font Family"), Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:581 > + _("Default monospace font Size"), Same issue for "Size" > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:590 > + * controls how the absolute smallest size fonts will be rendered. Values This is a little unclear. Might be better as "controls the absolute smallest size"
Comment on attachment 114623 [details] Incorporated review comments View in context: https://bugs.webkit.org/attachment.cgi?id=114623&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:462 > + * This extra new line is not needed here, please fix it here and for other properties before landing. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1120 > + **/ Don't use double ** here, please fix it here and for other methods before landing.
Created attachment 114731 [details] Incorporated review comments
(In reply to comment #30) > Created an attachment (id=114731) [details] > Incorporated review comments Thank you Martin, Carlos for the reviews. I have uploaded the re-worked patch. Since I do not yet have the commit right, could you please commit this patch on behalf of me?
Comment on attachment 114731 [details] Incorporated review comments Clearing flags on attachment: 114731 Committed r100056: <http://trac.webkit.org/changeset/100056>
All reviewed patches have been landed. Closing bug.