| Summary: | Add LogArgument template for PlatformMediaSession::RemoteControlCommandType | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | achristensen, commit-queue, jer.noble, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Eric Carlson
2018-08-03 10:55:38 PDT
Created attachment 346505 [details]
Patch
Comment on attachment 346505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346505&action=review > Source/WebCore/platform/audio/PlatformMediaSession.cpp:102 > + static_assert(static_cast<size_t>(PlatformMediaSession::PlayCommand == 1), "PlatformMediaSession::PlayCommand is not 1 as expected"); Why do you need to convert the booleans into size_t's? > Source/WebCore/platform/audio/PlatformMediaSession.cpp:112 > + ASSERT(static_cast<size_t>(command) < WTF_ARRAY_LENGTH(values)); > + return values[static_cast<size_t>(command)]; I think a switch statement would be more appropriate here. It would allow the compiler to give you an error if you're missing a case if you don't have a default. Comment on attachment 346505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346505&action=review >> Source/WebCore/platform/audio/PlatformMediaSession.cpp:102 >> + static_assert(static_cast<size_t>(PlatformMediaSession::PlayCommand == 1), "PlatformMediaSession::PlayCommand is not 1 as expected"); > > Why do you need to convert the booleans into size_t's? It doesn't convert boolean to size_t, it converts an enum to a size_t so it can be compared to an integer. I just copied the style of the convertEnumerationToString() generated for all enums in IDL. >> Source/WebCore/platform/audio/PlatformMediaSession.cpp:112 >> + return values[static_cast<size_t>(command)]; > > I think a switch statement would be more appropriate here. It would allow the compiler to give you an error if you're missing a case if you don't have a default. Is there a plan to do that for the 250 enums defined in IDL? Comment on attachment 346505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346505&action=review >>> Source/WebCore/platform/audio/PlatformMediaSession.cpp:102 >>> + static_assert(static_cast<size_t>(PlatformMediaSession::PlayCommand == 1), "PlatformMediaSession::PlayCommand is not 1 as expected"); >> >> Why do you need to convert the booleans into size_t's? > > It doesn't convert boolean to size_t, it converts an enum to a size_t so it can be compared to an integer. > > I just copied the style of the convertEnumerationToString() generated for all enums in IDL. I see that now, but you put the parentheses in the wrong place. Created attachment 346774 [details]
Patch for landing
Comment on attachment 346774 [details] Patch for landing Clearing flags on attachment: 346774 Committed r234703: <https://trac.webkit.org/changeset/234703> All reviewed patches have been landed. Closing bug. |