Summary: | [EFL][WK2] Add API to set preferred languages | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||
Component: | WebKit EFL | Assignee: | Ryuan Choi <ryuan.choi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, sergio | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ryuan Choi
2014-04-09 06:11:52 PDT
Created attachment 228958 [details]
Patch
Comment on attachment 228958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228958&action=review > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:573 > + if (languages) { Why don't you make an early return when the language parameter is null? > Source/WebKit2/UIProcess/API/efl/ewk_context.h:437 > + * @param languages the list of preferred languages (char * as data) s/char */char* ? Move this line below the description. > Source/WebKit2/UIProcess/API/efl/ewk_context.h:440 > + * header that will be included in the network requests Put a period at the end of the sentence. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:276 > + ASSERT_STREQ("en-US", s_acceptLanguages.utf8().data()); The default locale may be different from the system environment. Comment on attachment 228958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228958&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:573 >> + if (languages) { > > Why don't you make an early return when the language parameter is null? Null is used to clear user preferred langague. I will mention it in doxygen. >> Source/WebKit2/UIProcess/API/efl/ewk_context.h:437 >> + * @param languages the list of preferred languages (char * as data) > > s/char */char* ? > Move this line below the description. OK. >> Source/WebKit2/UIProcess/API/efl/ewk_context.h:440 >> + * header that will be included in the network requests > > Put a period at the end of the sentence. OK >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:276 >> + ASSERT_STREQ("en-US", s_acceptLanguages.utf8().data()); > > The default locale may be different from the system environment. If I am right, run-efl-tests will change the locale for non-english user. Isn't it enough? Created attachment 229010 [details]
Patch
Comment on attachment 229010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229010&action=review > Source/WebKit2/ChangeLog:8 > + Application may need to change the list of "Accept-Language". According to this description, this APIs may not be used by application. I'm not sure we have to add new APIs without clear reason. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:243 > +static String s_acceptLanguages; I prefer to use a primitive type in out of WebKit. Because it may occur symbol visibility problem when running layout test. In this case, "const char*" looks proper that String. (In reply to comment #5) > (From update of attachment 229010 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229010&action=review > > > Source/WebKit2/ChangeLog:8 > > + Application may need to change the list of "Accept-Language". > > According to this description, this APIs may not be used by application. I'm not sure we have to add new APIs without clear reason. > Some contents providers can check Accept-Language field of request header and send different contents to the applications. http://www.w3.org/International/questions/qa-lang-priorities.en.php So, good browsers have the power to change it. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:243 > > +static String s_acceptLanguages; > > I prefer to use a primitive type in out of WebKit. Because it may occur symbol visibility problem when running layout test. > > In this case, "const char*" looks proper that String. OK, I will. Comment on attachment 228958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228958&action=review >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:276 >>> + ASSERT_STREQ("en-US", s_acceptLanguages.utf8().data()); >> >> The default locale may be different from the system environment. > > If I am right, run-efl-tests will change the locale for non-english user. > > Isn't it enough? If then, that's fine. Created attachment 229016 [details]
Patch
Comment on attachment 229016 [details]
Patch
LGTM
Comment on attachment 229016 [details] Patch Clearing flags on attachment: 229016 Committed r167062: <http://trac.webkit.org/changeset/167062> All reviewed patches have been landed. Closing bug. |