Bug 90604

Summary: [EFL] [WK2] Add methods to get/set a custom text encoding
Product: WebKit Reporter: Sudarsana Nagineni (babu) <naginenis>
Component: WebKit EFLAssignee: Sudarsana Nagineni (babu) <naginenis>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, kenneth, lucas.de.marchi, ryuan.choi, sw0524.lee, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Sudarsana Nagineni (babu) 2012-07-05 05:10:55 PDT
Add ewk_view_setting_encoding_custom_{get,set} methods to WebKit2 EFL API.
Comment 1 Sudarsana Nagineni (babu) 2012-07-05 06:42:01 PDT
Created attachment 150934 [details]
Patch
Comment 2 Gyuyoung Kim 2012-07-05 19:47:32 PDT
Comment on attachment 150934 [details]
Patch

Looks fine.
Comment 3 Kenneth Rohde Christiansen 2012-07-06 00:51:08 PDT
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?
Comment 4 Chris Dumez 2012-07-06 04:19:03 PDT
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.
Comment 5 Sudarsana Nagineni (babu) 2012-07-06 05:47:26 PDT
(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.
Comment 6 Sudarsana Nagineni (babu) 2012-07-06 05:55:28 PDT
(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.
Comment 7 Sudarsana Nagineni (babu) 2012-07-06 07:44:00 PDT
Created attachment 151082 [details]
Patch

Fixed review comment #4.
Comment 8 Sudarsana Nagineni (babu) 2012-07-12 12:27:51 PDT
Created attachment 152022 [details]
Patch

Rebased
Comment 9 Chris Dumez 2012-07-13 03:51:04 PDT
Comment on attachment 152022 [details]
Patch

LGTM.
Comment 10 Sudarsana Nagineni (babu) 2012-07-16 07:39:23 PDT
Created attachment 152533 [details]
Patch

Rebased
Comment 11 Sudarsana Nagineni (babu) 2012-07-19 08:32:07 PDT
Created attachment 153270 [details]
Patch

Rebased.
Comment 12 Sudarsana Nagineni (babu) 2012-07-19 15:31:54 PDT
Kenneth, could you please have a look at this patch again and land?
Comment 13 Kenneth Rohde Christiansen 2012-07-19 20:32:10 PDT
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
Comment 14 WebKit Review Bot 2012-07-19 21:32:12 PDT
Comment on attachment 153270 [details]
Patch

Clearing flags on attachment: 153270

Committed r123177: <http://trac.webkit.org/changeset/123177>
Comment 15 WebKit Review Bot 2012-07-19 21:32:19 PDT
All reviewed patches have been landed.  Closing bug.