Bug 69524 - [GTK] Add methods to get/set a custom text enconding to WebKit2 GTK+ API
Summary: [GTK] Add methods to get/set a custom text enconding to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-10-06 06:23 PDT by Carlos Garcia Campos
Modified: 2011-10-17 09:23 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.49 KB, patch)
2011-10-06 06:24 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch to use get/set_custom_charset (5.16 KB, patch)
2011-10-07 01:55 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch updated to current git master (5.10 KB, patch)
2011-10-14 06:04 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-10-06 06:23:15 PDT
Add webkit_web_view_get_custom_text_encoding() and webkit_web_view_set_custom_text_encoding()
Comment 1 Carlos Garcia Campos 2011-10-06 06:24:53 PDT
Created attachment 109953 [details]
Patch
Comment 2 Martin Robinson 2011-10-06 08:50:11 PDT
Comment on attachment 109953 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:48
>      GRefPtr<WebKitWebLoaderClient> loaderClient;
> +
> +    CString customEncoding;

Again, I think tihs should be above under context.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:293
> + * not the default-encoding

I guess a period is needed at the nd of this comment. default-encoding -> default encoding

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:322
> + * Sets the current custom text encoding name of @web_view. Any load
> + * operation will be stopped and the page will be reloaded. This method
> + * doesn't change the default enconding.
> + * Setting custom text encoding to %NULL, makes @web_view to use the

Might want to mention what the effect of setting it a little more clearly. I'm guessing that is makes text documents load with a different mime type, but I'm not certain.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:43
> +    WebKitWebView *view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> +    g_object_ref_sink(view);
> +
> +    g_assert(!webkit_web_view_get_custom_text_encoding(view));
> +    webkit_web_view_set_custom_text_encoding(view, "utf8");
> +    g_assert_cmpstr(webkit_web_view_get_custom_text_encoding(view), ==, "utf8");
> +    /* Go back to the default enconding */
> +    webkit_web_view_set_custom_text_encoding(view, NULL);
> +    g_assert(!webkit_web_view_get_custom_text_encoding(view));
> +    g_object_unref(view);

Here I think you should load a text document and verify that the mime type is what you expect.
Comment 3 Carlos Garcia Campos 2011-10-07 00:55:17 PDT
(In reply to comment #2)
> (From update of attachment 109953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109953&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:48
> >      GRefPtr<WebKitWebLoaderClient> loaderClient;
> > +
> > +    CString customEncoding;
> 
> Again, I think tihs should be above under context.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:293
> > + * not the default-encoding
> 
> I guess a period is needed at the nd of this comment. default-encoding -> default encoding

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:322
> > + * Sets the current custom text encoding name of @web_view. Any load
> > + * operation will be stopped and the page will be reloaded. This method
> > + * doesn't change the default enconding.
> > + * Setting custom text encoding to %NULL, makes @web_view to use the
> 
> Might want to mention what the effect of setting it a little more clearly. I'm guessing that is makes text documents load with a different mime type, but I'm not certain.

It has nothing to do with mime types, it's the character encoding that will be used to interpret the text.

http://en.wikipedia.org/wiki/Character_encodings_in_HTML

We can rename it to webkit_web_view_set_custom_text_character_encoding() or webkit_web_view_set_custom_charset(). GLib uses charset in its API, so it would be consistent.

> > Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:43
> > +    WebKitWebView *view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> > +    g_object_ref_sink(view);
> > +
> > +    g_assert(!webkit_web_view_get_custom_text_encoding(view));
> > +    webkit_web_view_set_custom_text_encoding(view, "utf8");
> > +    g_assert_cmpstr(webkit_web_view_get_custom_text_encoding(view), ==, "utf8");
> > +    /* Go back to the default enconding */
> > +    webkit_web_view_set_custom_text_encoding(view, NULL);
> > +    g_assert(!webkit_web_view_get_custom_text_encoding(view));
> > +    g_object_unref(view);
> 
> Here I think you should load a text document and verify that the mime type is what you expect.

We don't have API to get the mime-type yet.
Comment 4 Carlos Garcia Campos 2011-10-07 01:55:05 PDT
Created attachment 110104 [details]
Updated patch to use get/set_custom_charset

I hope it's clearer now
Comment 5 WebKit Review Bot 2011-10-07 01:57:39 PDT
Attachment 110104 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:303:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Carlos Garcia Campos 2011-10-14 06:04:08 PDT
Created attachment 111005 [details]
Patch updated to current git master
Comment 7 Martin Robinson 2011-10-14 08:58:17 PDT
Comment on attachment 111005 [details]
Patch updated to current git master

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.
Comment 8 Carlos Garcia Campos 2011-10-17 09:23:05 PDT
Committed r97624: <http://trac.webkit.org/changeset/97624>