Bug 134830 - [GTK] Remove WebKitCertificateInfo from WebKit2GTK+ API
: [GTK] Remove WebKitCertificateInfo from WebKit2GTK+ API
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on:
Blocks: 133724
  Show dependency treegraph
 
Reported: 2014-07-11 03:22 PDT by Carlos Garcia Campos
Modified: 2014-07-30 00:07 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.79 KB, patch)
2014-07-11 03:26 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (24.27 KB, patch)
2014-07-29 05:50 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased to apply on current git master (24.17 KB, patch)
2014-07-29 05:56 PDT, Carlos Garcia Campos
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 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.
Comment 1 Carlos Garcia Campos 2014-07-11 03:26:36 PDT
Created attachment 234755 [details]
Patch
Comment 2 WebKit Commit Bot 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
Comment 3 Gustavo Noronha (kov) 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?
Comment 4 Gustavo Noronha (kov) 2014-07-16 11:26:59 PDT
… improve this documentation I mean
Comment 5 Michael Catanzaro 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().
Comment 6 Carlos Garcia Campos 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.
Comment 7 Michael Catanzaro 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.
Comment 8 Carlos Garcia Campos 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.
Comment 9 Carlos Garcia Campos 2014-07-29 05:50:52 PDT
Created attachment 235679 [details]
Patch
Comment 10 Carlos Garcia Campos 2014-07-29 05:56:13 PDT
Created attachment 235680 [details]
Rebased to apply on current git master
Comment 11 Carlos Garcia Campos 2014-07-29 05:57:02 PDT
I think this is the last API change blocking the libs version bump
Comment 12 Gustavo Noronha (kov) 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 =)
Comment 13 Carlos Garcia Campos 2014-07-30 00:07:11 PDT
Committed r171792: <http://trac.webkit.org/changeset/171792>