Bug 134830 – [GTK] Remove WebKitCertificateInfo from WebKit2GTK+ API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Add webkit_certificate_info_new() to allow the user to create a WebKitCertificateInfo for a given GTlsCertificate and GTlsCertificateFlags. This would be needed to be able to add exceptions with webkit_web_context_allow_tls_certificate_for_host() for a certificate saved permanently, for example.

[Carlos Garcia Campos]

Created attachment 234755 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Gustavo Noronha (kov)]

Comment on attachment 234755 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:54
> + * @tls_errors: #GTlsCertificateFlags verification status

I don't understand this parameter very well, I don't remember certificate errors reporting very well, but this is a good indication that we should probably improve this. Does this list the types of errors that should be accepted by the exception?

[Gustavo Noronha (kov)]

… improve this documentation I mean

[Michael Catanzaro]

In the context of WebKitCertificateInfo, it's only used to indicate what errors actually occurred. It does not have any effect on the behavior of webkit_web_context_allow_tls_certificate_for_host().

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 234755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234755&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:54
> > + * @tls_errors: #GTlsCertificateFlags verification status
> 
> I don't understand this parameter very well, I don't remember certificate errors reporting very well, but this is a good indication that we should probably improve this. Does this list the types of errors that should be accepted by the exception?

That pair is what you get from soup_message_get_https_status() or webkit_web_view_get_tls_info(). WebKitCertificateInfo was added later. I agree it's a bit confusing, WebKitCertificateInfo contains the certificate itself and possible TLS errors of the certificate or 0 for valid certificates. In WebCore CertificateInfo contains the pair, because DidCommitLoad message contains a CertificateInfo parameter, and in that case we need both. WebKitCertificateInfo was exposed when we added the API to handle TLS errors and add exceptions for certificates. The idea was that you get a WebKitCertificateInfo in WebKitWebView::failed-with-tls-errors that you can use to get the TLS info and use also later to add an exception. This made the API a bit inconsistent with webkit_web_view_get_tls_info(). So, maybe we could rethink the whole thing now that we are breaking the API. The WebKitCertificateInfo is not really needed for webkit_web_context_allow_tls_certificate_for_host() since only the certificate is used in that case. WebKitWebView::failed-with-tls-errors might receive the pair as parameters instead of WebKitCertificateInfo. That would probably simplify the API since we can get rid of the WebKitCertificateInfo entirely.

[Michael Catanzaro]

I was going to say that the webkit_certificate_info_new() would not really be needed if webkit_web_view_get_tls_info() were to return a WebKitCertificateInfo instead, but if you're planning to remove WebKitCertificateInfo from the API that'd probably be better.

[Carlos Garcia Campos]

This was added to make the API more convenient but it has ended up making it more complicated, so in the end I think it's better to remove it.

[Carlos Garcia Campos]

Created attachment 235679 [details]
Patch

[Carlos Garcia Campos]

Created attachment 235680 [details]
Rebased to apply on current git master

[Carlos Garcia Campos]

I think this is the last API change blocking the libs version bump

[Gustavo Noronha (kov)]

Comment on attachment 235680 [details]
Rebased to apply on current git master

This looks good to me. I don't think the likelihood of needing to extend the API is big enough to warrant a webkit object just for it, so let's do it =)

[Carlos Garcia Campos]

Committed r171792: <http://trac.webkit.org/changeset/171792>