Bug 131435 – [EFL][WK2] Add API to set preferred languages

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

Service provider may check thst "Accept-Language".

This API will be used to change preference of "Acceept-Language"

[Ryuan Choi]

Created attachment 228958 [details]
Patch

[Jinwoo Song]

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.

[Ryuan Choi]

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?

[Ryuan Choi]

Created attachment 229010 [details]
Patch

[Gyuyoung Kim]

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.

[Ryuan Choi]

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

[Jinwoo Song]

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.

[Ryuan Choi]

Created attachment 229016 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 229016 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 229016 [details]
Patch

Clearing flags on attachment: 229016

Committed r167062: <http://trac.webkit.org/changeset/167062>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.