Bug 129740 – [EFL][WK2] Add ewk APIs to control TLS error policy on WebContext.

• Component: WebKit2
• Status: RESOLVED FIXED

[Peter Molnar]

Add WK2 API to control TLS error policy on WebContext.

[Peter Molnar]

Created attachment 225884 [details]
patch

[WebKit Commit Bot]

Attachment 225884 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 12 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Molnar]

(In reply to comment #2)
> Attachment 225884 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
> ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
> Total errors found: 12 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

These style errors are caused by the certificate embedded into the code. This seems to be a better option than including it from a file, this way we can make the test more self-containing.

[Gyuyoung Kim]

Comment on attachment 225884 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225884&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:361
> +EAPI void ewk_context_set_ignore_tls(Ewk_Context* ewkContext, Eina_Bool ignoreTLSErrors);

1. Wrong * place and wrong efl function style. Ewk_Context* ewkContext => Ewk_Context *ewk_context, ignoreTLSErrors => ignore_tls_errors.

2. Generally EFL function naming has placed *verb* at the end of function. For instance, ewk_context_ignore_tls_set(...)

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:24
> +EWK2UnitTestServer::EWK2UnitTestServer(GTlsCertificate* tls_cert)

We only use EFL coding style in public EFL APIs. So, tls_cert => tlsCert

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:29
> +    EWK2UnitTestServer(GTlsCertificate* tls_cert = 0);

ditto

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:41
> +static bool finish_test = false;

ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:152
> +    Ewk_Error* error = 0;

0 => nullptr ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:178
> +    Ewk_Error* error = 0;

ditto.

[Peter Molnar]

Created attachment 225990 [details]
patch

Fixed the style problems above.

[WebKit Commit Bot]

Attachment 225990 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 12 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 225990 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225990&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:361
> +EAPI void ewk_context_ignore_tls_set(Ewk_Context *ewkContext, Eina_Bool ignore_tls_errors);

I find this name misleading: it looks like one wants to ignore TLS as a whole (?). How about ewk_context_ignore_tls_errors_set()?

[Gyuyoung Kim]

Comment on attachment 225990 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225990&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.h:361
>> +EAPI void ewk_context_ignore_tls_set(Ewk_Context *ewkContext, Eina_Bool ignore_tls_errors);
> 
> I find this name misleading: it looks like one wants to ignore TLS as a whole (?). How about ewk_context_ignore_tls_errors_set()?

BTW, don't we need to support _get() as well ?

[Peter Molnar]

(In reply to comment #8)
> (From update of attachment 225990 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225990&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_context.h:361
> >> +EAPI void ewk_context_ignore_tls_set(Ewk_Context *ewkContext, Eina_Bool ignore_tls_errors);
> > 
> > I find this name misleading: it looks like one wants to ignore TLS as a whole (?). How about ewk_context_ignore_tls_errors_set()?

Sure, I've renamed it. It will be more clear this way.

> 
> BTW, don't we need to support _get() as well ?

Added the getter too.

[Peter Molnar]

Created attachment 226121 [details]
patch

[WebKit Commit Bot]

Attachment 226121 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 12 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 226121 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226121&action=review

I think we need to test _get() as well. Can't you test _get() in tests ?

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:362
> +Eina_Bool ewk_context_ignore_tls_errors_get(Ewk_Context *ewkContext);

1. Missing const keyword in parameter. Ewk_Context -> const Ewk_Context
2. Use context to be sync with existing APIs.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:370
> +EAPI void ewk_context_ignore_tls_errors_set(Ewk_Context *ewkContext, Eina_Bool ignore_tls_errors);

ditto.

[Peter Molnar]

Created attachment 226289 [details]
patch

(In reply to comment #12)
> (From update of attachment 226121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226121&action=review
> 
> I think we need to test _get() as well. Can't you test _get() in tests ?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:362
> > +Eina_Bool ewk_context_ignore_tls_errors_get(Ewk_Context *ewkContext);
> 
> 1. Missing const keyword in parameter. Ewk_Context -> const Ewk_Context
> 2. Use context to be sync with existing APIs.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:370
> > +EAPI void ewk_context_ignore_tls_errors_set(Ewk_Context *ewkContext, Eina_Bool ignore_tls_errors);
> 
> ditto.

Added the tests. Fixed missing const keywords.

[WebKit Commit Bot]

Attachment 226289 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 12 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jinwoo Song]

Comment on attachment 226289 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226289&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:362
> +Eina_Bool ewk_context_ignore_tls_errors_get(const Ewk_Context *ewkContext);

Regarding the naming, I would suggest 'ewk_context_tls_errors_policy_get/set()' and to add a enumeration for TLS errors policy(Ignore/Fail). 
As described in comments, this API provides the interface to control the TLS error policy.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:115
> +        if (!strcmp(path, "/index.html")) {

Why don't you use strncmp() instead of strcmp()?

[Peter Molnar]

Created attachment 226695 [details]
patch

(In reply to comment #15)
> (From update of attachment 226289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226289&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:362
> > +Eina_Bool ewk_context_ignore_tls_errors_get(const Ewk_Context *ewkContext);
> 
> Regarding the naming, I would suggest 'ewk_context_tls_errors_policy_get/set()' and to add a enumeration for TLS errors policy(Ignore/Fail). 
> As described in comments, this API provides the interface to control the TLS error policy.

Yes, this name is definitely better, I've renamed it. Also added the enumeration you suggested.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:115
> > +        if (!strcmp(path, "/index.html")) {
> 
> Why don't you use strncmp() instead of strcmp()?

Currently strcmp() is used in all of these tests, so I'm trying to conform to this here, too. If you think it needs to be an strncmp(), then it would be wise to change it in all of the other tests also.

[WebKit Commit Bot]

Attachment 226695 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 12 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Molnar]

Created attachment 227203 [details]
patch

The previous patch was bad (wrong enum->bool mapping), fixed it.
Also renamed the test callbacks and added comments to make it more clear what's going on in those tests.

[WebKit Commit Bot]

Attachment 227203 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 12 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 227203 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227203&action=review

Looks fine except for EFL coding style.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:383
> + * @param context context object to get TLS error policy for.

For @param, @return statements, we haven't used . at the end of line.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:385
> + * @return The TLS errors policy for the context.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:387
> +Ewk_TLS_Error_Policy ewk_context_tls_error_policy_get(const Ewk_Context *ewkContext);

Please use context instead of ewkContext to be compliant with existing APIs.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:392
> + * @param context context object to set TLS error policy for.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:393
> + * @param ignore_tls_errors a #Ewk_TLS_Error_Policy.

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:395
> +EAPI void ewk_context_tls_error_policy_set(const Ewk_Context *ewkContext, Ewk_TLS_Error_Policy tls_error_policy);

ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:100
> +        // The test fails if an SSL error occurs.

Wrong indentation.

[Jinwoo Song]

Comment on attachment 227203 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227203&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:384
> +        toImpl(m_context.get())->setIgnoreTLSErrors(false);

You may write in a line as following.

toImpl(m_context.get())->setIgnoreTLSErrors(TLSErrorPolicy == EWK_TLS_ERROR_POLICY_IGNORE);

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:83
> +    /// Fail on TLS errors.

It would be more readable to write one value and comment in a line. Also use *//* instead of *///*.

[Peter Molnar]

Created attachment 227445 [details]
patch

Fixed the style issues mentioned above.

[WebKit Commit Bot]

Attachment 227445 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 13 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jinwoo Song]

Comment on attachment 227445 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227445&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:87
> +/// Creates a type name for the Ewk_TLS_Error_Policy.

Use *//* instead of *///*.

[Jinwoo Song]

Comment on attachment 227445 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227445&action=review

> Source/WebKit2/ChangeLog:3
> +        [EFL] Add WK2 API to control TLS error policy on WebContext.

Let's change the bug title as below. You're not adding WK2 C APIs but ewk APIs for EFL port.

[EFL][WK2] Add ewk APIs to control TLS error policy on WebContext.

[Peter Molnar]

Created attachment 227659 [details]
patch

(In reply to comment #24)
> (From update of attachment 227445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227445&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:87
> > +/// Creates a type name for the Ewk_TLS_Error_Policy.
> 
> Use *//* instead of *///*.

Fixed.

(In reply to comment #25)
> (From update of attachment 227445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227445&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL] Add WK2 API to control TLS error policy on WebContext.
> 
> Let's change the bug title as below. You're not adding WK2 C APIs but ewk APIs for EFL port.
> 
> [EFL][WK2] Add ewk APIs to control TLS error policy on WebContext.

Changed the bug title.

[WebKit Commit Bot]

Attachment 227659 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 13 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 227659 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227659&action=review

Patch looks getting better.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:29
> +    EWK2UnitTestServer(GTlsCertificate* tlsCert = nullptr);

Missing *explicit* keyword ?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:30
>      virtual ~EWK2UnitTestServer();

Unrelated to this patch though, why this dtor was declared as virtual :(

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:182
> +    waitUntilTrue(finishTest, testTimeoutSeconds);

Any reason we have to wait until testTimeoutSeconds ?

[Peter Molnar]

Created attachment 227754 [details]
patch

(In reply to comment #28)
> (From update of attachment 227659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227659&action=review
> 
> Patch looks getting better.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:29
> > +    EWK2UnitTestServer(GTlsCertificate* tlsCert = nullptr);
> 
> Missing *explicit* keyword ?

Fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:30
> >      virtual ~EWK2UnitTestServer();
> 
> Unrelated to this patch though, why this dtor was declared as virtual :(

I assume that in this case I don't have to fix it in this patch.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:182
> > +    waitUntilTrue(finishTest, testTimeoutSeconds);
> 
> Any reason we have to wait until testTimeoutSeconds ?

Waiting for the timeout is just for the case if something really nasty happens. If either onLoadFinished or onLoadProvisionalFailed event is triggered, finishTest will become true, and the test finishes immediately (600-800 msec on average). In normal test runs either of them will be very likely to happen.

So it's just for the case when none of these events happen, so finishTest would stay false forever (e.g. the cert can't be created from the supplied PEM, server can't start for some reason, etc.).

[WebKit Commit Bot]

Attachment 227754 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 13 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 227754 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227754&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:380
> +{

Don't we need to set true or false if result of TLSErrorPolicy == EWK_TLS_ERROR_POLICY_IGNORE was already set ?

How about adding below logic ?

    bool isNewPolicy = TLSErrorPolicy == EWK_TLS_ERROR_POLICY_IGNORE;
    if (toImpl(m_context.get())->ignoreTLSErrors() == isNewPolicy)
        return;

    toImpl(m_context.get())->setIgnoreTLSErrors(isNewPolicy);

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:490
> +void ewk_context_tls_error_policy_set(const Ewk_Context *context, Ewk_TLS_Error_Policy tls_error_policy)

Wrong * place. Ewk_Context* context

We only follow EFL coding style in public header.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:381
> + * @param context context object to get TLS error policy for

What is *for* at the end of line ?

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:390
> + * @param context context object to set TLS error policy for

ditto.

[Peter Molnar]

Created attachment 227837 [details]
patch

Fixed previously mentioned problems.

[WebKit Commit Bot]

Attachment 227837 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 13 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryuan Choi]

Comment on attachment 227837 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227837&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:388
> + * Sets TLS error policy for @a context.

I hope that we have more description.
For example, what is default value?

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:391
> + * @param ignore_tls_errors a #Ewk_TLS_Error_Policy

tls_error_policy

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:393
> +EAPI void ewk_context_tls_error_policy_set(const Ewk_Context *context, Ewk_TLS_Error_Policy tls_error_policy);

Remove const

[Gyuyoung Kim]

Comment on attachment 227837 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227837&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:385
> +Ewk_TLS_Error_Policy ewk_context_tls_error_policy_get(const Ewk_Context *context);

Wrong * place.

[Gyuyoung Kim]

Comment on attachment 227837 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227837&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_context.h:385
>> +Ewk_TLS_Error_Policy ewk_context_tls_error_policy_get(const Ewk_Context *context);
> 
> Wrong * place.

I'm sorry. My mistake. Please ignore wrong comment.

[Peter Molnar]

Created attachment 228168 [details]
patch

[WebKit Commit Bot]

Attachment 228168 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 13 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Molnar]

Created attachment 228170 [details]
patch

[WebKit Commit Bot]

Attachment 228170 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:48:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:50:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:52:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:53:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:54:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:56:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:59:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:60:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:68:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:69:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:70:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:71:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:99:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 13 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 228170 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228170&action=review

LGTM except for a comment.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:34
> +        m_baseURL = soup_uri_new("https://127.0.0.1/");

We can move this to out of if ~ else scope. Can't we move this to 40 line ?

[WebKit Commit Bot]

Comment on attachment 228170 [details]
patch

Clearing flags on attachment: 228170

Committed r166497: <http://trac.webkit.org/changeset/166497>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.