[webkit-reviews] review granted: [Bug 69524] [GTK] Add methods to get/set a custom text enconding to WebKit2 GTK+ API : [Attachment 111005] Patch updated to current git master
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 14 08:58:17 PDT 2011
Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 69524: [GTK] Add methods to get/set a custom text enconding to WebKit2 GTK+
API
https://bugs.webkit.org/show_bug.cgi?id=69524
Attachment 111005: Patch updated to current git master
https://bugs.webkit.org/attachment.cgi?id=111005&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111005&action=review
Looks good, but please makethe fixes below.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:46
> + CString customEncoding;
Nit: I think this should be called customTextEncoding.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:320
> + * not the default one.
You can remove "not the default one" here.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:337
> + const CString& customEncoding =
toImpl(wkCustomEncoding.get())->string().utf8();
> + if (webView->priv->customEncoding != customEncoding)
> + webView->priv->customEncoding = customEncoding;
> +
Unfortunately, you should not use a reference here since utf8() is a
temporary. It's probably faster just to always assign this value to
priv->customEncoding.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:344
> + * @charset: (allow-none): a character encoding name, or %NULL
No comma after "name"
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:350
> + * Sets the current custom character encoding name of @web_view. Any load
> + * operation will be stopped and the page will be reloaded. This method
> + * doesn't change the default character enconding.
> + * Setting custom character encoding to %NULL, makes @web_view to use the
> + * default one.
Please consider something like this for the documentation:
Sets the current custom character encoding override of @web_view. The custom
character encoding will override any text encoding detected via HTTP headers or
META tags. Calling this method will stop any current load operation and reload
the
current page. Setting the custom character encoding to %NULL removes the
character
encoding override.
Note: This does not change the default character enconding, which is set via
#WebSettings.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:34
> + /* Go back to the default charset */
Please use a C++ style comment here.
More information about the webkit-reviews
mailing list