Populate [Default] for some extensible enums in services/media_session [chromium/src : main]
Andrew Paseltiner (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 4 comments![]( "Open in Gerrit")
If you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
[Default] kNotInPictureInPicture,It's not obvious to me this should be the "default" state.
[Default] kDeprecatedUnknown,Isn't this deprecated? Is that OK?
Related details
- Andrew Paseltiner
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner added 1 comment![]( "Open in Gerrit")
If you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
The alternative is to add something like `[Default, MinVersion=123] kUnknown` to each of these, which is doable, but requires updating `switch` statements and so on throughout the codebase. It's also unclear to me exactly how `Default` and `MinVersion` interact in terms of addressing the underlying problem.
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Andrew PaseltinerIf you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
The alternative is to add something like `[Default, MinVersion=123] kUnknown` to each of these, which is doable, but requires updating `switch` statements and so on throughout the codebase. It's also unclear to me exactly how `Default` and `MinVersion` interact in terms of addressing the underlying problem.
- Andrew Paseltiner
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Paseltiner (Gerrit)
Andrew Paseltiner added 1 comment![]( "Open in Gerrit")
Andrew PaseltinerIf you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
The alternative is to add something like `[Default, MinVersion=123] kUnknown` to each of these, which is doable, but requires updating `switch` statements and so on throughout the codebase. It's also unclear to me exactly how `Default` and `MinVersion` interact in terms of addressing the underlying problem.
Ack. According to https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/tools/bindings/README.md#Versioned-Enums:
For versioned enum definitions, the use of a [MinVersion] attribute is strictly for documentation purposes. It has no impact on the generated code.
@hide...@chromium.org Do you have any thoughts on the addition of new enum values here, like `[Default] kUnkown` versus trying to reuse existing values as the default?
Related details
- Andrew Paseltiner
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 2 comments![]( "Open in Gerrit")
Andrew PaseltinerIf you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
Andrew PaseltinerThe alternative is to add something like `[Default, MinVersion=123] kUnknown` to each of these, which is doable, but requires updating `switch` statements and so on throughout the codebase. It's also unclear to me exactly how `Default` and `MinVersion` interact in terms of addressing the underlying problem.
Ack. According to https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/tools/bindings/README.md#Versioned-Enums:
For versioned enum definitions, the use of a [MinVersion] attribute is strictly for documentation purposes. It has no impact on the generated code.
@hide...@chromium.org Do you have any thoughts on the addition of new enum values here, like `[Default] kUnkown` versus trying to reuse existing values as the default?
I think adding clear Unknown sounds like more explicit and understandable.
Though, I think it's better to hear from ARC team explicitly. cc: khmel@.
sorry for delayed reply. back from ooo.
Related details
- Andrew Paseltiner
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yury Khmel (Gerrit)
Yury Khmel added 1 comment![]( "Open in Gerrit")
Andrew PaseltinerIf you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
Andrew PaseltinerThe alternative is to add something like `[Default, MinVersion=123] kUnknown` to each of these, which is doable, but requires updating `switch` statements and so on throughout the codebase. It's also unclear to me exactly how `Default` and `MinVersion` interact in terms of addressing the underlying problem.
Hidehiko AbeAck. According to https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/tools/bindings/README.md#Versioned-Enums:
For versioned enum definitions, the use of a [MinVersion] attribute is strictly for documentation purposes. It has no impact on the generated code.
@hide...@chromium.org Do you have any thoughts on the addition of new enum values here, like `[Default] kUnkown` versus trying to reuse existing values as the default?
I think adding clear Unknown sounds like more explicit and understandable.
Though, I think it's better to hear from ARC team explicitly. cc: khmel@.
I like the Hidehiko's idea with adding Unknown as a default. To me it safer way to do.
Andrew Paseltiner (Gerrit)
Andrew Paseltiner added 4 comments![]( "Open in Gerrit")
Andrew PaseltinerIf you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
Andrew PaseltinerThe alternative is to add something like `[Default, MinVersion=123] kUnknown` to each of these, which is doable, but requires updating `switch` statements and so on throughout the codebase. It's also unclear to me exactly how `Default` and `MinVersion` interact in terms of addressing the underlying problem.
Hidehiko AbeAck. According to https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/tools/bindings/README.md#Versioned-Enums:
For versioned enum definitions, the use of a [MinVersion] attribute is strictly for documentation purposes. It has no impact on the generated code.
@hide...@chromium.org Do you have any thoughts on the addition of new enum values here, like `[Default] kUnkown` versus trying to reuse existing values as the default?
Yury KhmelI think adding clear Unknown sounds like more explicit and understandable.
Though, I think it's better to hear from ARC team explicitly. cc: khmel@.
I like the Hidehiko's idea with adding Unknown as a default. To me it safer way to do.
I'be added `kUnknown` to all of these, but it's not clear to me that it is safer. While the compiler helps catch `switch` statements that do not handle the new value, it doesn't help with things like `if (value == kFoo) { ... } else { ... }` doing the right thing with the new value. That requires auditing all uses with domain-specific expertise.
It's not obvious to me this should be the "default" state.
Acknowledged
Isn't this deprecated? Is that OK?
Acknowledged
[Default] kInactive,Andrew PaseltinerSimilarly here.
Acknowledged
Related details
- Daniel Cheng
- Hidehiko Abe
- Yury Khmel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
Andrew PaseltinerIf you can convince the media session/ARC (which I think uses this too?) folks that this is OK, meh. But as an IPC owner without domain-specific knowledge, it's hard for me to be sure that this is OK.
Andrew PaseltinerThe alternative is to add something like `[Default, MinVersion=123] kUnknown` to each of these, which is doable, but requires updating `switch` statements and so on throughout the codebase. It's also unclear to me exactly how `Default` and `MinVersion` interact in terms of addressing the underlying problem.
Hidehiko AbeAck. According to https://chromium.googlesource.com/chromium/src/+/HEAD/mojo/public/tools/bindings/README.md#Versioned-Enums:
For versioned enum definitions, the use of a [MinVersion] attribute is strictly for documentation purposes. It has no impact on the generated code.
@hide...@chromium.org Do you have any thoughts on the addition of new enum values here, like `[Default] kUnkown` versus trying to reuse existing values as the default?
Yury KhmelI think adding clear Unknown sounds like more explicit and understandable.
Though, I think it's better to hear from ARC team explicitly. cc: khmel@.
Andrew PaseltinerI like the Hidehiko's idea with adding Unknown as a default. To me it safer way to do.
I'be added `kUnknown` to all of these, but it's not clear to me that it is safer. While the compiler helps catch `switch` statements that do not handle the new value, it doesn't help with things like `if (value == kFoo) { ... } else { ... }` doing the right thing with the new value. That requires auditing all uses with domain-specific expertise.
I think any code using comparisons like that was probably wrong anyway?
I'm not sure we shoudl be using `NOTREACHED()` generically to handle the new values; by virtue of being an extensible interface, those notreached are, in fact, reachable.
NOTREACHED();This will crash, and we are not supposed to crash on bad inputs from IPC.
Related details
- Andrew Paseltiner
- Hidehiko Abe
- Yury Khmel