Bug 90604

Created attachment 150934 [details] Patch

Comment on attachment 150934 [details] Patch Looks fine.

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 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.

(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.

(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.

Created attachment 151082 [details] Patch Fixed review comment #4.

Created attachment 152022 [details] Patch Rebased

Comment on attachment 152022 [details] Patch LGTM.

Created attachment 152533 [details] Patch Rebased

Created attachment 153270 [details] Patch Rebased.

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

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 on attachment 153270 [details] Patch Clearing flags on attachment: 153270 Committed r123177: <http://trac.webkit.org/changeset/123177>

All reviewed patches have been landed. Closing bug.