Bug 17585 – [gtk] get|set encoding api

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Diego Escalante Urrelo]

Attached patch implementes a get|set for encoding in webview, it's half-working because it seems I'm doing something silly with the string returned.
Attached to the patch is the epiphany part consuming this API, it's there for reference.

What ephy needs is:
- a way to get the current encoding for the page so the view > encoding menu can be displayed correctly (the proper current encoding selected)
- a way to tell the current web view to override the default-encoding of websettings for the current website (and reload the page when the encoding is changed).

The patch provides the two first thing (with bugs), the second thing should be a matter of adding a reload() somewhere in the code, right now setting the encoding and manually reloading changes nothing, I guess it's in part fault of my buggy implementation.
Please comment so I can learn the ways of C++ :)

[Diego Escalante Urrelo]

Created attachment 19427 [details]
A first draft

Go ahead and comment!

[Alp Toker]

We usually avoid g_strdup() for returned strings in favour of const gchar* and caching the value for API consistency.

I'll try to look over this for correctness soon (incl. comparing with how the other ports do this) unless someone beats me to it. Thanks for the patch.

[Alp Toker]

(In reply to comment #1)
> Created an attachment (id=19427) [edit]
> A first draft
> 
> Go ahead and comment!
> 

Does this change the encoding of the currently loaded document?

Mac WebView does it a little differently:

http://developer.apple.com/documentation/Cocoa/Reference/WebKit/Classes/WebView_Class/Reference/Reference.html#//apple_ref/occ/instm/WebView/setCustomTextEncodingName:

[Xan Lopez]

Created attachment 27827 [details]
encoding.patch

This does what Mac and Win do as far as I can tell.

[Holger Freyther]

Comment on attachment 27827 [details]
encoding.patch


> +    if (!overrideEncoding.isEmpty()) {
> +      WebKitWebViewPrivate* priv = webView->priv;
> +      g_free (priv->customEncoding);
> +      priv->customEncoding = g_strdup(overrideEncoding.utf8().data());
> +      return priv->customEncoding;
> +    } else
> +      return NULL;
> +}

looks like two style issues here. "g_free (" and two instead of four spaces to indent... please fix when landing.

[Holger Freyther]

Landed in r41247.

[Christian Dywan]

We need to add two things were overlooked initially:

A property 'custom-encoding'

A read-only function/ property 'encoding' that returns the default value

[Christian Dywan]

Created attachment 28034 [details]
Add _get_encoding and properties

[Christian Dywan]

(In reply to comment #8)
> Created an attachment (id=28034) [review]
> Add _get_encoding and properties

Meanwhile I implemented menu items for this to properly test the patch. _get_encoding seems to return the custom encoding, ie. loader()->encoding() actually has the custom value after the custom encoding set for the first time. I'm not sure yet how to overcome this.

[Gustavo Noronha (kov)]

Comment on attachment 27827 [details]
encoding.patch

Clearing the review flag so that it stops showing up as a pending commit 'till we got the other one reviewed.

[Holger Freyther]

Comment on attachment 28034 [details]
Add _get_encoding and properties


> +    /**
> +    * WebKitWebView:custom-encoding:
> +    *
> +    * The custom encoding of the web view.
> +    *
> +    * Since: 1.1.1
> +    */

may I get another space here?

[Christian Dywan]

2009-03-01  Christian Dywan  <[e-mail]>
    
            Reviewed by Holger Freyther.
    
            * webkit/webkitprivate.h:
            * webkit/webkitwebview.cpp:
            (webkit_web_view_get_encoding):
            * webkit/webkitwebview.h: Implement 'encoding' and 'custom-encoding'
            properties as well as webkit_web_view_get_encoding.

[Christian Dywan]

Note that I changed the Since tags of the properties to 0.1.2, but I'm unsure whether that's the right version. We should discuss that. Leaving the bug open for now.

[Xan Lopez]

(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=28034) [review] [review]
> > Add _get_encoding and properties
> 
> Meanwhile I implemented menu items for this to properly test the patch.
> _get_encoding seems to return the custom encoding, ie. loader()->encoding()
> actually has the custom value after the custom encoding set for the first time.
> I'm not sure yet how to overcome this.
> 

If this is really the case it might make sense to just have one function, get_encoding, and another one to check whether or not the encoding is automatic or forced by the user. Not much sense in having two getters that return the same thing, I think.

[Christian Dywan]

(In reply to comment #14)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Created an attachment (id=28034) [review] [review] [review]
> > > Add _get_encoding and properties
> > 
> > Meanwhile I implemented menu items for this to properly test the patch.
> > _get_encoding seems to return the custom encoding, ie. loader()->encoding()
> > actually has the custom value after the custom encoding set for the first time.
> > I'm not sure yet how to overcome this.
> > 
> 
> If this is really the case it might make sense to just have one function,
> get_encoding, and another one to check whether or not the encoding is automatic
> or forced by the user. Not much sense in having two getters that return the
> same thing, I think.

Well, it's not expected to make any sense, it's a bug afterall. As we discussed, the API should allow for seeing the default encoding even if it was overridden.

To me it looks like a bug in WebCore at this point, but I have to give that a closer look since I'm not familiar with where that's coming from. Otherwise we need to workaround it in WebKit I guess.

[Martin Robinson]

WebKit1GTK+ has been removed.