Bug 102867

Summary: [EFL][WK2] Remove setting word from ewk_view_setting_encoding_custom_XXX APIs
Product: WebKit Reporter: Jongseok Yang <js45.yang>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch none

Jongseok Yang
Reported 2012-11-20 20:38:00 PST
ewk_view_setting_encoding_custom_XXX APIs might be misunderstanded as the "setting" word because ewk_view_setting_encoding_custom_set triggers the "reload" operation. And ewk_view_setting_XXX is not correct because there is ewk_settings object for settings. The setting word from ewk_view_setting_encoding_custom_XXX APIs should be removed.
Attachments
Patch (6.31 KB, patch)
2012-11-20 20:40 PST, Jongseok Yang 		no flags
DetailsFormatted DiffDiff
Patch (6.31 KB, patch)
2012-11-21 02:18 PST, Jongseok Yang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jongseok Yang
Comment 1 2012-11-20 20:40:52 PST
Created attachment 175332 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 2 2012-11-21 01:34:43 PST
Comment on attachment 175332 [details] Patch This looks like a nice improvement; I suggest a few additional ones: o Please mention in the _set() documentation that the page is going to be reloaded. o "encoding_custom" still sounds a bit weird. How about "ewk_view_custom_encoding_{get,set}" or "ewk_view_custom_text_encoding_{get,set}"?
Jongseok Yang
Comment 3 2012-11-21 01:48:43 PST
(In reply to comment #2) > (From update of attachment 175332 [details]) > This looks like a nice improvement; I suggest a few additional ones: > o Please mention in the _set() documentation that the page is going to be reloaded. > o "encoding_custom" still sounds a bit weird. How about "ewk_view_custom_encoding_{get,set}" or "ewk_view_custom_text_encoding_{get,set}"? Thanks for your review. I'll upload new patch.
Jongseok Yang
Comment 4 2012-11-21 01:58:16 PST
(In reply to comment #2) > (From update of attachment 175332 [details]) > This looks like a nice improvement; I suggest a few additional ones: > o Please mention in the _set() documentation that the page is going to be reloaded. I checked that it was already mentioned like the below. * Sets the custom character encoding and reloads the page. > o "encoding_custom" still sounds a bit weird. How about "ewk_view_custom_encoding_{get,set}" or "ewk_view_custom_text_encoding_{get,set}"? I'll change that.
Jongseok Yang
Comment 5 2012-11-21 02:18:38 PST
Created attachment 175386 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 6 2012-11-21 02:31:34 PST
Comment on attachment 175386 [details] Patch LGTM.
WebKit Review Bot
Comment 7 2012-11-21 02:56:21 PST
Comment on attachment 175386 [details] Patch Clearing flags on attachment: 175386 Committed r135372: <http://trac.webkit.org/changeset/135372>
WebKit Review Bot
Comment 8 2012-11-21 02:56:26 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.