Bug 129740

Summary:

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

Product:

WebKit

Reporter:

Peter Molnar <pmolnar.u-szeged>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bunhere, cdumez, commit-queue, gyuyoung.kim, rakuco, sergio

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	gyuyoung.kim: review-
patch	none
patch	gyuyoung.kim: review-
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none

Description Peter Molnar 2014-03-05 07:47:19 PST

Add WK2 API to control TLS error policy on WebContext.

Comment 1 Peter Molnar 2014-03-05 07:48:45 PST

Created attachment 225884 [details]
patch

Comment 2 WebKit Commit Bot 2014-03-05 07:50:16 PST

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.

Comment 3 Peter Molnar 2014-03-05 07:52:15 PST

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

Comment 4 Gyuyoung Kim 2014-03-05 16:52:51 PST

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.

Comment 5 Peter Molnar 2014-03-06 07:28:23 PST

Created attachment 225990 [details]
patch

Fixed the style problems above.

Comment 6 WebKit Commit Bot 2014-03-06 07:29:47 PST

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.

Comment 7 Raphael Kubo da Costa (:rakuco) 2014-03-07 00:50:40 PST

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()?

Comment 8 Gyuyoung Kim 2014-03-07 04:27:49 PST

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 ?

Comment 9 Peter Molnar 2014-03-07 05:58:58 PST

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

Comment 10 Peter Molnar 2014-03-07 05:59:30 PST

Created attachment 226121 [details]
patch

Comment 11 WebKit Commit Bot 2014-03-07 06:00:17 PST

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.

Comment 12 Gyuyoung Kim 2014-03-07 22:22:06 PST

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.

Comment 13 Peter Molnar 2014-03-10 03:22:30 PDT

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.

Comment 14 WebKit Commit Bot 2014-03-10 03:23:21 PDT

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.

Comment 15 Jinwoo Song 2014-03-13 21:41:06 PDT

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()?

Comment 16 Peter Molnar 2014-03-14 05:51:35 PDT

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.

Comment 17 WebKit Commit Bot 2014-03-14 05:52:49 PDT

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.

Comment 18 Peter Molnar 2014-03-19 11:26:41 PDT

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.

Comment 19 WebKit Commit Bot 2014-03-19 11:28:23 PDT

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.

Comment 20 Gyuyoung Kim 2014-03-20 18:45:16 PDT

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.

Comment 21 Jinwoo Song 2014-03-20 22:30:16 PDT

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

Comment 22 Peter Molnar 2014-03-21 09:27:27 PDT

Created attachment 227445 [details]
patch

Fixed the style issues mentioned above.

Comment 23 WebKit Commit Bot 2014-03-21 09:30:10 PDT

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.

Comment 24 Jinwoo Song 2014-03-21 19:10:45 PDT

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

Comment 25 Jinwoo Song 2014-03-21 19:14:11 PDT

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.

Comment 26 Peter Molnar 2014-03-24 09:44:17 PDT

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.

Comment 27 WebKit Commit Bot 2014-03-24 09:46:03 PDT

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.

Comment 28 Gyuyoung Kim 2014-03-24 16:53:37 PDT

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 ?

Comment 29 Peter Molnar 2014-03-25 08:55:25 PDT

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

Comment 30 WebKit Commit Bot 2014-03-25 08:58:28 PDT

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.

Comment 31 Gyuyoung Kim 2014-03-25 23:03:28 PDT

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.

Comment 32 Peter Molnar 2014-03-26 01:56:43 PDT

Created attachment 227837 [details]
patch

Fixed previously mentioned problems.

Comment 33 WebKit Commit Bot 2014-03-26 01:59:40 PDT

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.

Comment 34 Ryuan Choi 2014-03-26 02:55:11 PDT

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

Comment 35 Gyuyoung Kim 2014-03-31 00:50:54 PDT

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.

Comment 36 Gyuyoung Kim 2014-03-31 02:49:03 PDT

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.

Comment 37 Peter Molnar 2014-03-31 03:50:22 PDT

Created attachment 228168 [details]
patch

Comment 38 WebKit Commit Bot 2014-03-31 03:53:41 PDT

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.

Comment 39 Peter Molnar 2014-03-31 03:55:09 PDT

Created attachment 228170 [details]
patch

Comment 40 WebKit Commit Bot 2014-03-31 03:56:47 PDT

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.

Comment 41 Gyuyoung Kim 2014-03-31 04:01:33 PDT

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 ?

Comment 42 WebKit Commit Bot 2014-03-31 06:01:31 PDT

Comment on attachment 228170 [details]
patch

Clearing flags on attachment: 228170

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

Comment 43 WebKit Commit Bot 2014-03-31 06:01:40 PDT

All reviewed patches have been landed.  Closing bug.