RESOLVED FIXED 134830
[GTK] Remove WebKitCertificateInfo from WebKit2GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=134830
Summary [GTK] Remove WebKitCertificateInfo from WebKit2GTK+ API
Carlos Garcia Campos
Reported 2014-07-11 03:22:13 PDT
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.
Attachments
Patch (3.79 KB, patch)
2014-07-11 03:26 PDT, Carlos Garcia Campos
no flags
Patch (24.27 KB, patch)
2014-07-29 05:50 PDT, Carlos Garcia Campos
no flags
Rebased to apply on current git master (24.17 KB, patch)
2014-07-29 05:56 PDT, Carlos Garcia Campos
gustavo: review+
Carlos Garcia Campos
Comment 1 2014-07-11 03:26:36 PDT
WebKit Commit Bot
Comment 2 2014-07-11 03:27:46 PDT
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 3 2014-07-16 11:25:31 PDT
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)
Comment 4 2014-07-16 11:26:59 PDT
… improve this documentation I mean
Michael Catanzaro
Comment 5 2014-07-16 11:41:01 PDT
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
Comment 6 2014-07-21 04:50:38 PDT
(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
Comment 7 2014-07-21 11:47:28 PDT
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
Comment 8 2014-07-29 05:43:59 PDT
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
Comment 9 2014-07-29 05:50:52 PDT
Carlos Garcia Campos
Comment 10 2014-07-29 05:56:13 PDT
Created attachment 235680 [details] Rebased to apply on current git master
Carlos Garcia Campos
Comment 11 2014-07-29 05:57:02 PDT
I think this is the last API change blocking the libs version bump
Gustavo Noronha (kov)
Comment 12 2014-07-29 08:24:13 PDT
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
Comment 13 2014-07-30 00:07:11 PDT
Note You need to log in before you can comment on or make changes to this bug.