Bug 157232 – Next batch of conversions to use C++ enum class instead of strings for enumerations

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Next batch of conversions to use C++ enum class instead of strings for enumerations

[Darin Adler]

Created attachment 277800 [details]
Patch

[Darin Adler]

Created attachment 277801 [details]
Patch

[Build Bot]

Comment on attachment 277801 [details]
Patch

Attachment 277801 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1243754

New failing tests:
media/media-source/media-source-end-of-stream.html
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
media/media-source/media-source-end-of-stream-buffered.html
http/tests/media/media-source/mediasource-remove.html
http/tests/media/media-source/mediasource-closed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
media/media-source/media-source-fastseek.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
media/media-source/media-source-end-of-stream-readyState.html

[Build Bot]

Created attachment 277803 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277801 [details]
Patch

Attachment 277801 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1243755

New failing tests:
media/media-source/media-source-end-of-stream.html
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
media/media-source/media-source-end-of-stream-buffered.html
http/tests/media/media-source/mediasource-remove.html
http/tests/media/media-source/mediasource-closed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
media/media-source/media-source-fastseek.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
media/media-source/media-source-end-of-stream-readyState.html

[Build Bot]

Created attachment 277804 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277801 [details]
Patch

Attachment 277801 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1243763

New failing tests:
media/media-source/media-source-end-of-stream.html
http/tests/media/media-source/mediasource-play.html
http/tests/media/media-source/mediasource-config-change-mp4-v-framerate.html
http/tests/media/media-source/mediasource-sourcebuffer-mode.html
media/media-source/media-source-end-of-stream-buffered.html
http/tests/media/media-source/mediasource-remove.html
http/tests/media/media-source/mediasource-closed.html
http/tests/media/media-source/SourceBuffer-abort-readyState.html
media/media-source/media-source-fastseek.html
http/tests/media/media-source/mediasource-append-buffer.html
http/tests/media/media-source/mediasource-config-change-mp4-v-bitrate.html
media/media-source/media-source-end-of-stream-readyState.html

[Build Bot]

Created attachment 277805 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Created attachment 277809 [details]
Patch

[Darin Adler]

Media source test failures were because code wasn’t properly handling optional enumeration arguments, throwing an exception instead of passing Nullopt. Fixed now.

[Chris Dumez]

Looks like it does not build on iOS.

[Darin Adler]

Created attachment 277815 [details]
Patch

[Darin Adler]

Fixed the iOS build: There was some iOS-specific code expecting a string instead of an enum. No idea why this code was in platform/cocoa, though!

[Chris Dumez]

Comment on attachment 277809 [details]
Patch

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

r=me with a few minor comments.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:131
> +ResponseType FetchResponse::type() const

We could inline this one now.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:549
> +        buffer->setMode(AppendMode::Sequence, IGNORE_EXCEPTION);

Why aren't we setting to Segments otherwise anymore? Also, the comment above still claims we do.

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.idl:27
> +    NoInterfaceObject,

Since you're touching this, we may as well alphabetize.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:314
> +    return m_state;

We could inline it now.

> Source/WebCore/Modules/webaudio/WaveShaperNode.idl:36
> +    attribute OverSampleType oversample;

So I am guessing we no longer throw here. The new behavior is more correct and no longer throwing should be fine compatibility wise.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:-123
> -    "AppendMode" => 1,

May have been nice to go a little more incrementally :D This patch is huge!

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3738
> +            push(@$outputArray, "$indent    if (!$optionalValue)\n");

This one is UNLIKELY() too

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:130
> +const char* const expectedEnumerationValuesTestEnumType = "\"\", \"EnumValue1\", \"EnumValue2\", \"EnumValue3\"";

Could use [] instead of *

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2318
> +    auto nativeValue = parseTestEnumType(*state, value);

I personally find it a little bit confusing to use auto for Optional<> types but your call.

> Source/WebCore/crypto/CryptoKey.cpp:52
> +    return m_type;

We could inline now.

> Source/WebCore/crypto/CryptoKey.h:50
> +enum class KeyUsage { Encrypt, Decrypt, Sign, Verify, DeriveKey, DeriveBits, WrapKey, UnwrapKey };

I think this should be CryptoKeyUsage (a bit too generic otherwise). We can always have a using statement inside CryptoKey class for convenience.

> Source/WebCore/crypto/CryptoKey.h:52
> +using KeyType = CryptoKeyType;

Seems risky to have something generic like "KeyType" in WebCore namespace in a header. Why do we need this using statement?
Note that I believe we can use the name we want for the enum in the IDL as it is not web-exposed.

> Source/WebCore/css/FontFaceSet.h:55
> +    FontFaceSetLoadStatus status() const;

We could have a "using" statement inside FontFaceSet to make this name a lot shorter (e.g. LoadStatus).

> Tools/ChangeLog:9
> +        checking. We don't want to wast time trying to make our generated code match our style.

Typo: "waste"

[Chris Dumez]

Looks like iOS is still red though.

[Darin Adler]

Created attachment 277817 [details]
Patch

[Darin Adler]

Fixed iOS build. Didn’t address all of Chris’s comments yet.

[Darin Adler]

Comment on attachment 277809 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchResponse.cpp:131
>> +ResponseType FetchResponse::type() const
> 
> We could inline this one now.

Done.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:549
>> +        buffer->setMode(AppendMode::Sequence, IGNORE_EXCEPTION);
> 
> Why aren't we setting to Segments otherwise anymore? Also, the comment above still claims we do.

The mode is Segments already in a newly created buffer, so it wasn’t important to set it in that case. And the comment is quoting the spec, and the implementation doesn’t have to do it literally.

But I will change it back; no good reason for me to have changed it.

>> Source/WebCore/Modules/mediastream/MediaDeviceInfo.idl:27
>> +    NoInterfaceObject,
> 
> Since you're touching this, we may as well alphabetize.

Done.

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:314
>> +    return m_state;
> 
> We could inline it now.

Done.

>> Source/WebCore/Modules/webaudio/WaveShaperNode.idl:36
>> +    attribute OverSampleType oversample;
> 
> So I am guessing we no longer throw here. The new behavior is more correct and no longer throwing should be fine compatibility wise.

That’s what I thought too.

I am a little embarrassed I didn’t add a test, though.

>> Source/WebCore/bindings/scripts/CodeGenerator.pm:-123
>> -    "AppendMode" => 1,
> 
> May have been nice to go a little more incrementally :D This patch is huge!

OK, I’ll try to keep the patch sizes down for the next step.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3738
>> +            push(@$outputArray, "$indent    if (!$optionalValue)\n");
> 
> This one is UNLIKELY() too

Done.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:130
>> +const char* const expectedEnumerationValuesTestEnumType = "\"\", \"EnumValue1\", \"EnumValue2\", \"EnumValue3\"";
> 
> Could use [] instead of *

Done.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2318
>> +    auto nativeValue = parseTestEnumType(*state, value);
> 
> I personally find it a little bit confusing to use auto for Optional<> types but your call.

I’ll stick with auto. Generated and template code benefits even more from not specifically naming types so I am going to use auto even more there; I think we should be using it a lot more in the output of the bindings generator. If this was hand-written code I would think more about your preference to not use auto for this.

>> Source/WebCore/crypto/CryptoKey.cpp:52
>> +    return m_type;
> 
> We could inline now.

Done.

>> Source/WebCore/crypto/CryptoKey.h:50
>> +enum class KeyUsage { Encrypt, Decrypt, Sign, Verify, DeriveKey, DeriveBits, WrapKey, UnwrapKey };
> 
> I think this should be CryptoKeyUsage (a bit too generic otherwise). We can always have a using statement inside CryptoKey class for convenience.

I agree that if I was naming this I would have named it CryptoKeyUsage.

But I didn’t chose this name. This is the name from the IDL and from the W3C Web Cryptography API specification. I chose to leave the IDL as is and follow the specification for now. If we want to do better, there are these obvious options:

1) Use a different name in our IDL, don’t worry about matching the specification.
2) Use a different name in our IDL, work to get the specification changed.
3) Add [ImplementedAs] support to our WebIDL implementation and use a different type name in the C++ code from the name in the IDL.

For now I prefer to just leave it like this, but maybe you can convince me to do one of these other things. Even if I land it like this I am happy to come back and fix it depending on what you suggest.

>> Source/WebCore/crypto/CryptoKey.h:52
>> +using KeyType = CryptoKeyType;
> 
> Seems risky to have something generic like "KeyType" in WebCore namespace in a header. Why do we need this using statement?
> Note that I believe we can use the name we want for the enum in the IDL as it is not web-exposed.

I agree that it’s inelegant. Not sure I would really call it “risky”, though. I don’t expect this to cause any subtle failures but it could conflict with another name in the future.

To answer your question about why we need this, it is here for the same reason that KeyUsage above is named "KeyUsage". "KeyType" is the name from the IDL and the specification.

>> Source/WebCore/css/FontFaceSet.h:55
>> +    FontFaceSetLoadStatus status() const;
> 
> We could have a "using" statement inside FontFaceSet to make this name a lot shorter (e.g. LoadStatus).

I agree that’s a good technique. Sometimes I will even do that just within a single function to use a shorter name there.

But this enum is used only in this one function and the name is literally repeated only 5 times total. I don’t think it’s worthwhile to give it a second name just to improve that. The point, for me, is to keep the C++ as simple as possible and think having both the enum and an alias for it is more complex.

However it is worth thinking about the fact that this pattern may recur in WebIDL and we might want to make our C++ implementation more elegant. Long enumeration names that repeat the class name they are used in are probably a repeating pattern. Maybe we could have those enums be class members automatically when the name is like that. Or maybe we should always have the enums be class members, and just use that rule to shorten the name.

Let me know what you think.

>> Tools/ChangeLog:9
>> +        checking. We don't want to wast time trying to make our generated code match our style.
> 
> Typo: "waste"

Fixed.

[Darin Adler]

Created attachment 277819 [details]
Patch

[Chris Dumez]

Comment on attachment 277809 [details]
Patch

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

>>> Source/WebCore/crypto/CryptoKey.h:50
>>> +enum class KeyUsage { Encrypt, Decrypt, Sign, Verify, DeriveKey, DeriveBits, WrapKey, UnwrapKey };
>> 
>> I think this should be CryptoKeyUsage (a bit too generic otherwise). We can always have a using statement inside CryptoKey class for convenience.
> 
> I agree that if I was naming this I would have named it CryptoKeyUsage.
> 
> But I didn’t chose this name. This is the name from the IDL and from the W3C Web Cryptography API specification. I chose to leave the IDL as is and follow the specification for now. If we want to do better, there are these obvious options:
> 
> 1) Use a different name in our IDL, don’t worry about matching the specification.
> 2) Use a different name in our IDL, work to get the specification changed.
> 3) Add [ImplementedAs] support to our WebIDL implementation and use a different type name in the C++ code from the name in the IDL.
> 
> For now I prefer to just leave it like this, but maybe you can convince me to do one of these other things. Even if I land it like this I am happy to come back and fix it depending on what you suggest.

Ok. Not a big issue. I have a slight preference for 1) given that the name is not WebExposed but fine.

>>> Source/WebCore/css/FontFaceSet.h:55
>>> +    FontFaceSetLoadStatus status() const;
>> 
>> We could have a "using" statement inside FontFaceSet to make this name a lot shorter (e.g. LoadStatus).
> 
> I agree that’s a good technique. Sometimes I will even do that just within a single function to use a shorter name there.
> 
> But this enum is used only in this one function and the name is literally repeated only 5 times total. I don’t think it’s worthwhile to give it a second name just to improve that. The point, for me, is to keep the C++ as simple as possible and think having both the enum and an alias for it is more complex.
> 
> However it is worth thinking about the fact that this pattern may recur in WebIDL and we might want to make our C++ implementation more elegant. Long enumeration names that repeat the class name they are used in are probably a repeating pattern. Maybe we could have those enums be class members automatically when the name is like that. Or maybe we should always have the enums be class members, and just use that rule to shorten the name.
> 
> Let me know what you think.

Yes, I think it may be nicer to have those enums as part of the class (always) and shorten the enum name if it contains the class name.
Probably not in this patch though. I may turn out to be a bad idea once we try.

[Darin Adler]

(In reply to comment #20)
> I have a slight preference for 1) given that the name is not WebExposed but fine.

Not to belabor the point, but the reason I slightly prefer not doing that is that I think we have this goal of having our IDL files some day literally match the ones from specifications, except for a few extended attributes that we might need to help us generate the right code.

> Yes, I think it may be nicer to have those enums as part of the class
> (always) and shorten the enum name if it contains the class name.

Here are my guesses about some of small downsides of that plan:

1) Might make an even bigger mess the first time someone uses the same enum in more than one interface that is not related by inheritance in a web specification. Already kind of a problem, I suppose, given the way we compile one IDL file at a time, but this would add one more thing to deal with.

2) Helper functions that are not class members will have to use a longer name because they have to include the class name.

3) It won’t be as easy to search for the enum in C++ code given the name in the IDL.

[Darin Adler]

Committed r200291: <http://trac.webkit.org/changeset/200291>

[Darin Adler]

Oops, forgot to reset binding test results after last change.