Bug 90604 – [EFL] [WK2] Add methods to get/set a custom text encoding

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

Add ewk_view_setting_encoding_custom_{get,set} methods to WebKit2 EFL API.

[Sudarsana Nagineni (babu)]

Created attachment 150934 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150934 [details]
Patch

Looks fine.

[Kenneth Rohde Christiansen]

Comment on attachment 150934 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:717
> +const char* ewk_view_setting_encoding_custom_get(const Evas_Object* ewkView)

You really want all these to be view settings? May be it is ok, if you dont expose something like page groups in the api

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:739
> +Eina_Bool ewk_view_setting_encoding_custom_set(Evas_Object* ewkView, const char* encoding)
> +{
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);
> +
> +    WKRetainPtr<WKStringRef> wkEncodingName = encoding ? adoptWK(WKStringCreateWithUTF8CString(encoding)) : 0;
> +    if (eina_stringshare_replace(&priv->customEncoding, encoding))
> +        WKPageSetCustomTextEncodingName(toAPI(priv->pageClient->page()), wkEncodingName.get());
> +    return true;
> +}

What if I set a non-sense encoding? shouldnt it validate it some how and return false?

[Chris Dumez]

Comment on attachment 150934 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:739
>> +}
> 
> What if I set a non-sense encoding? shouldnt it validate it some how and return false?

I guess we could call WKPageCopyCustomTextEncodingName() after and make sure it changed?

> Tools/MiniBrowser/efl/main.c:88
> +        currentEncoding++;

preincrement?

> Tools/MiniBrowser/efl/main.c:89
> +        currentEncoding %= (sizeof(encodings) / sizeof(encodings[0]));

I think the currentEncoding increment and modulo should be in one line/instruction.

[Sudarsana Nagineni (babu)]

(In reply to comment #3)
> (From update of attachment 150934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150934&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:717
> > +const char* ewk_view_setting_encoding_custom_get(const Evas_Object* ewkView)
> 
> You really want all these to be view settings? May be it is ok, if you dont expose something like page groups in the api
> 

We don't have page group support enabled in efl port now. In my opinion this setting must be part of the view because it changes only override encoding and doesn't change default encoding. The setting for default encoding is going to be part of page group.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:739
> > +Eina_Bool ewk_view_setting_encoding_custom_set(Evas_Object* ewkView, const char* encoding)
> > +{
> > +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false);
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);
> > +
> > +    WKRetainPtr<WKStringRef> wkEncodingName = encoding ? adoptWK(WKStringCreateWithUTF8CString(encoding)) : 0;
> > +    if (eina_stringshare_replace(&priv->customEncoding, encoding))
> > +        WKPageSetCustomTextEncodingName(toAPI(priv->pageClient->page()), wkEncodingName.get());
> > +    return true;
> > +}
> 
> What if I set a non-sense encoding? shouldnt it validate it some how and return false?

Setting null or some invalid custom character encoding makes view to use the default one.

[Sudarsana Nagineni (babu)]

(In reply to comment #4)
> (From update of attachment 150934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150934&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:739
> >> +}
> > 
> > What if I set a non-sense encoding? shouldnt it validate it some how and return false?
> 
> I guess we could call WKPageCopyCustomTextEncodingName() after and make sure it changed?

WKPageCopyCustomTextEncodingName() doesn't help in this case because it returns the same encoding name that was set with WKPageSetCustomTextEncodingName()

> 
> > Tools/MiniBrowser/efl/main.c:88
> > +        currentEncoding++;
> 
> preincrement?
> 
> > Tools/MiniBrowser/efl/main.c:89
> > +        currentEncoding %= (sizeof(encodings) / sizeof(encodings[0]));
> 
> I think the currentEncoding increment and modulo should be in one line/instruction.

Okay.

[Sudarsana Nagineni (babu)]

Created attachment 151082 [details]
Patch

Fixed review comment #4.

[Sudarsana Nagineni (babu)]

Created attachment 152022 [details]
Patch

Rebased

[Chris Dumez]

Comment on attachment 152022 [details]
Patch

LGTM.

[Sudarsana Nagineni (babu)]

Created attachment 152533 [details]
Patch

Rebased

[Sudarsana Nagineni (babu)]

Created attachment 153270 [details]
Patch

Rebased.

[Sudarsana Nagineni (babu)]

Kenneth, could you please have a look at this patch again and land?

[Kenneth Rohde Christiansen]

Comment on attachment 153270 [details]
Patch

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

I guess this is OK, though I wonder how much it makes sense to store a local copy of the encoding name as a shared string.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:989
> +        WKPageSetCustomTextEncodingName(toAPI(priv->pageClient->page()), wkEncodingName.get());
> +    return true;

newline before return

[WebKit Review Bot]

Comment on attachment 153270 [details]
Patch

Clearing flags on attachment: 153270

Committed r123177: <http://trac.webkit.org/changeset/123177>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.