Next batch of conversions to use C++ enum class instead of strings for enumerations
Created attachment 277800 [details] Patch
Created attachment 277801 [details] Patch
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
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
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
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
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
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
Created attachment 277809 [details] Patch
Media source test failures were because code wasn’t properly handling optional enumeration arguments, throwing an exception instead of passing Nullopt. Fixed now.
Looks like it does not build on iOS.
Created attachment 277815 [details] Patch
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!
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"
Looks like iOS is still red though.
Created attachment 277817 [details] Patch
Fixed iOS build. Didn’t address all of Chris’s comments yet.
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.
Created attachment 277819 [details] Patch
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.
(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.
Committed r200291: <http://trac.webkit.org/changeset/200291>
Oops, forgot to reset binding test results after last change.