Update Fetch to use enum class instead of string for enumerations

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Update Fetch to use enum class instead of string for enumerations

[Darin Adler]

Created attachment 277841 [details]
Patch

[Darin Adler]

Created attachment 277844 [details]
Patch

[Alex Christensen]

Comment on attachment 277844 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchOptions.h:36
> +enum class ReferrerPolicy { EmptyString, NoReferrer, NoReferrerWhenDowngrade, OriginOnly, OriginWhenCrossOrigin, UnsafeUrl };

These enum classes are now in namespace WebCore scope, when they were inside the scope of the class.  Do we really want all these enums to be in WebCore scope?  I'm not sure if this is a good idea.  This applies to https://bugs.webkit.org/show_bug.cgi?id=157242 too.

> Source/WebCore/platform/ReferrerPolicy.h:32
> -#define ReferrerPolicy_h
> +#pragma once

We could do this with several other headers in this patch, too.  Document.h is already done, but the others aren't.

> Source/WebCore/platform/ReferrerPolicy.h:38
> +// I am not sure which it is. We should remove the unneeded ones, and also
> +// should work around this issue somewhere else, maybe even in config.h.

This is no good.  I think someone who manages the GTK port should look at this and fix it the right way.  This causes strange, hard to find bugs.

[Darin Adler]

Comment on attachment 277844 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchOptions.h:36
>> +enum class ReferrerPolicy { EmptyString, NoReferrer, NoReferrerWhenDowngrade, OriginOnly, OriginWhenCrossOrigin, UnsafeUrl };
> 
> These enum classes are now in namespace WebCore scope, when they were inside the scope of the class.  Do we really want all these enums to be in WebCore scope?  I'm not sure if this is a good idea.  This applies to https://bugs.webkit.org/show_bug.cgi?id=157242 too.

This is pretty fundamental and I have been discussing it in all these patches with Chris Dumez as I have been doing them. This is the way these things are all specified in Web IDL. We could put them in classes but the enumerations are not scoped to an interface in the specifications. In another of these bugs Chris and I discussed an idea where we would scope all of these to the interface of the IDL file they are in and shorten their names if the interface was also a prefix of the enumeration's name. But as I did this patch I realized that if we had done that then these here's in this patch would all be members of the FetchRequest class which would make it impossible to use them in FetchOptions in this case.

I don't think there is any obvious trivial way to make this better and I like having this exactly match the IDL from the specifications.

[Chris Dumez]

Comment on attachment 277844 [details]
Patch

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

>>> Source/WebCore/Modules/fetch/FetchOptions.h:36
>>> +enum class ReferrerPolicy { EmptyString, NoReferrer, NoReferrerWhenDowngrade, OriginOnly, OriginWhenCrossOrigin, UnsafeUrl };
>> 
>> These enum classes are now in namespace WebCore scope, when they were inside the scope of the class.  Do we really want all these enums to be in WebCore scope?  I'm not sure if this is a good idea.  This applies to https://bugs.webkit.org/show_bug.cgi?id=157242 too.
> 
> This is pretty fundamental and I have been discussing it in all these patches with Chris Dumez as I have been doing them. This is the way these things are all specified in Web IDL. We could put them in classes but the enumerations are not scoped to an interface in the specifications. In another of these bugs Chris and I discussed an idea where we would scope all of these to the interface of the IDL file they are in and shorten their names if the interface was also a prefix of the enumeration's name. But as I did this patch I realized that if we had done that then these here's in this patch would all be members of the FetchRequest class which would make it impossible to use them in FetchOptions in this case.
> 
> I don't think there is any obvious trivial way to make this better and I like having this exactly match the IDL from the specifications.

I still like the idea of always defining theses enums in a class.

Couldn't we add an IDL extended attribute to indicate in which scope the enum is defined? e.g.
[Scope=FetchOptions] enum requestType { ... };

We could also assume that by default the scope would be the class corresponding the IDL where the enum is declared (as this is probably the most common).

> Source/WebCore/platform/ReferrerPolicy.h:41
> +#undef Always

Based on the initial error, I suspect only the one for "Always" is needed. Also, maybe we could limit this to the GTK port? e.g.
// FIXME: ...
#if PLATFORM(GTK)
#undef Always
#endif

I agree someone from the GTK port should investigate although I don't think this should necessarily block this patch from landing.

[Michael Catanzaro]

(In reply to comment #3)
> This is no good.  I think someone who manages the GTK port should look at
> this and fix it the right way.  This causes strange, hard to find bugs.

Unfortunately changing this header triggers a rebuild of 2000 files, so it's hard to test to figure out where it's coming from, but it indeed seems some header felt it appropriate to define "Always". This is... unimpressive. I think #undef is the way to go until we figure out where this is coming from.

[Michael Catanzaro]

Trick is to try redefining it again, then the compiler will spit out the line it was first defined on: /usr/include/X11/X.h:441

/* Used in CreateWindow for backing-store hint */

#define NotUseful               0
#define WhenMapped              1
#define Always                  2

This file is full of unnamespaced macros. I think #undef is the only option. :(

[Darin Adler]

(In reply to comment #7)
> This file is full of unnamespaced macros. I think #undef is the only option.

Not a surprise; but the question is where to put that #undef.

[Darin Adler]

(In reply to comment #5)
> I still like the idea of always defining theses enums in a class.

I’m happy to move to that model. I’m almost done getting rid of the string-based enums, and I would be happy to return to all these new enums and move them and change their names as needed to fit the new model.

We can also choose whether to use enum class or old-school enum.

> Couldn't we add an IDL extended attribute to indicate in which scope the
> enum is defined? e.g.
> [Scope=FetchOptions] enum requestType { ... };
> 
> We could also assume that by default the scope would be the class
> corresponding the IDL where the enum is declared (as this is probably the
> most common).

Sure, we can do that if you think we get a better result that way. I was starting with literal translation of the IDL to our C++, and preferred something that didn’t require a new extended attribute, but we can do easily do whatever mapping we like.

I don’t know that Scope is the right name for this attribute, though. Seems a lot like ImplementedAs to me. Could be:

    [ImplementedAs=FetchOptions::RequestType] enum RequestType

It might be considered inelegant to repeat the enumeration name, but it’s more flexible.

> Also, maybe we could limit this to the GTK port? e.g.
> // FIXME: ...
> #if PLATFORM(GTK)
> #undef Always
> #endif

We could, but as long as the comment explains it’s for GTK, I can’t think of a reason it’s better to actually put the #undef inside an #if.

[Darin Adler]

Thanks for the comments from all of you.

Chris, I noticed that you did not say review+ yet. What changes do you consider mandatory before I land this? I’m getting really close to eliminating the old string-based enums entirely, and I’d love to get this step landed, even if there is some follow-up.

[Chris Dumez]

Comment on attachment 277844 [details]
Patch

I was hoping we would do the [Scope] or [ImplementedAs] support instead of moving all of the enums to WebCore but r=me as long as we revisit later.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #5)
> > I still like the idea of always defining theses enums in a class.
> 
> I’m happy to move to that model. I’m almost done getting rid of the
> string-based enums, and I would be happy to return to all these new enums
> and move them and change their names as needed to fit the new model.
> 
> We can also choose whether to use enum class or old-school enum.
> 
> > Couldn't we add an IDL extended attribute to indicate in which scope the
> > enum is defined? e.g.
> > [Scope=FetchOptions] enum requestType { ... };
> > 
> > We could also assume that by default the scope would be the class
> > corresponding the IDL where the enum is declared (as this is probably the
> > most common).
> 
> Sure, we can do that if you think we get a better result that way. I was
> starting with literal translation of the IDL to our C++, and preferred
> something that didn’t require a new extended attribute, but we can do easily
> do whatever mapping we like.
> 
> I don’t know that Scope is the right name for this attribute, though. Seems
> a lot like ImplementedAs to me. Could be:
> 
>     [ImplementedAs=FetchOptions::RequestType] enum RequestType

Yes, this is good too although the parser might need tweaking to allow "::" in there since this is not very IDL.

[Darin Adler]

Committed r200313: <http://trac.webkit.org/changeset/200313>