Bug 173311 - Do not mix C and C++ API error constants
Summary: Do not mix C and C++ API error constants
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-13 06:05 PDT by Carlos Garcia Campos
Modified: 2017-06-14 16:35 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.61 KB, patch)
2017-06-13 06:07 PDT, Carlos Garcia Campos
achristensen: 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 2017-06-13 06:05:30 PDT
In r214934 I added error constants to API::Error, but not for all errors exposed in the C API. Because of this we are mixing both constants in WebErrors and in GTk+ WebKitPrivate.
Comment 1 Carlos Garcia Campos 2017-06-13 06:07:44 PDT
Created attachment 312766 [details]
Patch
Comment 2 Build Bot 2017-06-13 06:08:51 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 Alex Christensen 2017-06-13 09:56:55 PDT
Comment on attachment 312766 [details]
Patch

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

> Source/WebKit2/Shared/API/APIError.h:51
> +    enum General {

We're trying to use more enum classes in WebKit unless there's a good reason not to.  And we need to be sure to have source and binary compatibility, which I think this patch has.
Comment 4 Carlos Garcia Campos 2017-06-13 10:38:01 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 312766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312766&action=review
> 
> > Source/WebKit2/Shared/API/APIError.h:51
> > +    enum General {
> 
> We're trying to use more enum classes in WebKit unless there's a good reason
> not to.  And we need to be sure to have source and binary compatibility,
> which I think this patch has.

Yes, in this particular case we don't use enum class, because we are interested in the integer values, we would need to cast everywhere if we used enum class.
Comment 5 Alex Christensen 2017-06-13 12:31:30 PDT
I think the necessity of including from c headers is the only good reason to use an enum instead of an enum class.
Comment 6 Carlos Garcia Campos 2017-06-13 23:31:22 PDT
Committed r218244: <http://trac.webkit.org/changeset/218244>
Comment 7 Darin Adler 2017-06-14 07:30:17 PDT
If there are two sets of constants, that must be identical, what keeps them in sync? If nothing else, should we write static_assert statements that check that they are equal?
Comment 8 Carlos Garcia Campos 2017-06-14 08:26:38 PDT
(In reply to Darin Adler from comment #7)
> If there are two sets of constants, that must be identical, what keeps them
> in sync? If nothing else, should we write static_assert statements that
> check that they are equal?

I think we should make WKErrorGetErrorCode return the value using a switch and checking the API::Error value, even if they are currently the same values. That's what we do in the GTK+ API implementation. I'll do it in a follow up if you agree.
Comment 9 Darin Adler 2017-06-14 16:35:03 PDT
I do agree.