Bug 226213 – Make MediaSession readystate enums all lowercase

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jean-Yves Avenard [:jya]]

Make MediaSession readystate enums all lowercase

[Radar WebKit Bug Importer]

<rdar://problem/78437011>

[Jean-Yves Avenard [:jya]]

Created attachment 429632 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 429646 [details]
Patch

[Peng Liu]

Comment on attachment 429646 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSessionReadyState.h:37
> +    Haveenoughdata,

Oh? Why do we need these changes in C++ code?

[Chris Dumez]

Comment on attachment 429646 [details]
Patch

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

>> Source/WebCore/Modules/mediasession/MediaSessionReadyState.h:37
>> +    Haveenoughdata,
> 
> Oh? Why do we need these changes in C++ code?

That's what the C++ code generated by the IDL bindings generator expects based on the IDL enum values. I don't think there is a way around that at the moment sadly.

[Chris Dumez]

Comment on attachment 429646 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSessionReadyState.idl:29
> +    "havenothing",

Are we trying to match a spec? If so, you could use '-' as separator to end up with nicer names on c++ side, e.g. 'havenothing' -> 'have-nothing'

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 429646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429646&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaSessionReadyState.idl:29
> > +    "havenothing",
> 
> Are we trying to match a spec? If so, you could use '-' as separator to end
> up with nicer names on c++ side, e.g. 'havenothing' -> 'have-nothing'

If so -> If NOT

[Chris Dumez]

Comment on attachment 429646 [details]
Patch

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

>>> Source/WebCore/Modules/mediasession/MediaSessionReadyState.h:37
>>> +    Haveenoughdata,
>> 
>> Oh? Why do we need these changes in C++ code?
> 
> That's what the C++ code generated by the IDL bindings generator expects based on the IDL enum values. I don't think there is a way around that at the moment sadly.

I think we wouldn't need any C++ changes if we used '-' in the names in the IDL (e.g. 'have-nothing').

[Eric Carlson]

Comment on attachment 429646 [details]
Patch

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

> LayoutTests/media/media-session/mock-coordinator-expected.txt:156
> -EXPECTED (latestChange == 'coordinatorStateChanged') OK
> +EXPECTED (latestChange == 'playbackStateChanged') OK
> +EXPECTED (latestChange == 'playbackStateChanged') OK
> +EXPECTED (latestChange == 'playbackStateChanged') OK
> +EXPECTED (latestChange == 'coordinatorStateChanged'), OBSERVED 'undefined', AFTER TIMEOUT FAIL

Is this supposed to part of this change?

[Jean-Yves Avenard [:jya]]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 429646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429646&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaSessionReadyState.idl:29
> > +    "havenothing",
> 
> Are we trying to match a spec? If so, you could use '-' as separator to end
> up with nicer names on c++ side, e.g. 'havenothing' -> 'have-nothing'

yes, currently Media Session states and actions use that format: "seekbackward" "previoustrack", "togglemicrophone" 

HTML Media events are also all in lowercase "loadedmetata"

[Jean-Yves Avenard [:jya]]

(In reply to Peng Liu from comment #4)
> Comment on attachment 429646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429646&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaSessionReadyState.h:37
> > +    Haveenoughdata,
> 
> Oh? Why do we need these changes in C++ code?

The IDL binding code makes assumptions on how the actual C++ enum is named. It creates a 1:1 association between the IDL name and the C++ enum.

Maybe there's an IDL settings to force which C++ enum watches.

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 429646 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=429646&action=review
> > 
> > > Source/WebCore/Modules/mediasession/MediaSessionReadyState.idl:29
> > > +    "havenothing",
> > 
> > Are we trying to match a spec? If so, you could use '-' as separator to end
> > up with nicer names on c++ side, e.g. 'havenothing' -> 'have-nothing'
> 
> yes, currently Media Session states and actions use that format:
> "seekbackward" "previoustrack", "togglemicrophone" 
> 
> HTML Media events are also all in lowercase "loadedmetata"

how about picture-in-picture?
Source/WebCore/html/HTMLVideoElement.idl:[Conditional=VIDEO_PRESENTATION_MODE] enum VideoPresentationMode { "inline", "fullscreen", "picture-in-picture" };

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> (In reply to Jean-Yves Avenard [:jya] from comment #10)
> > (In reply to Chris Dumez from comment #6)
> > > Comment on attachment 429646 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=429646&action=review
> > > 
> > > > Source/WebCore/Modules/mediasession/MediaSessionReadyState.idl:29
> > > > +    "havenothing",
> > > 
> > > Are we trying to match a spec? If so, you could use '-' as separator to end
> > > up with nicer names on c++ side, e.g. 'havenothing' -> 'have-nothing'
> > 
> > yes, currently Media Session states and actions use that format:
> > "seekbackward" "previoustrack", "togglemicrophone" 
> > 
> > HTML Media events are also all in lowercase "loadedmetata"
> 
> how about picture-in-picture?
> Source/WebCore/html/HTMLVideoElement.idl:
> [Conditional=VIDEO_PRESENTATION_MODE] enum VideoPresentationMode { "inline",
> "fullscreen", "picture-in-picture" };

MediaDecodingType also has "media-source". MediaKeysRequirement has "not-allowed". I am sure there are other examples.

[Chris Dumez]

(In reply to Chris Dumez from comment #13)
> (In reply to Chris Dumez from comment #12)
> > (In reply to Jean-Yves Avenard [:jya] from comment #10)
> > > (In reply to Chris Dumez from comment #6)
> > > > Comment on attachment 429646 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=429646&action=review
> > > > 
> > > > > Source/WebCore/Modules/mediasession/MediaSessionReadyState.idl:29
> > > > > +    "havenothing",
> > > > 
> > > > Are we trying to match a spec? If so, you could use '-' as separator to end
> > > > up with nicer names on c++ side, e.g. 'havenothing' -> 'have-nothing'
> > > 
> > > yes, currently Media Session states and actions use that format:
> > > "seekbackward" "previoustrack", "togglemicrophone" 
> > > 
> > > HTML Media events are also all in lowercase "loadedmetata"
> > 
> > how about picture-in-picture?
> > Source/WebCore/html/HTMLVideoElement.idl:
> > [Conditional=VIDEO_PRESENTATION_MODE] enum VideoPresentationMode { "inline",
> > "fullscreen", "picture-in-picture" };
> 
> MediaDecodingType also has "media-source". MediaKeysRequirement has
> "not-allowed". I am sure there are other examples.

The fact is that there is plenty of precedent for using - as word separator in enum strings. I think it looks much nicer and our bindings generator deals with nicely with them. If we have control over the naming, seems unfortunate to end up with the ugly way :P

[Jean-Yves Avenard [:jya]]

(In reply to Chris Dumez from comment #14)
> The fact is that there is plenty of precedent for using - as word separator
> in enum strings. I think it looks much nicer and our bindings generator
> deals with nicely with them. If we have control over the naming, seems
> unfortunate to end up with the ugly way :P

From searching at the time in the spec (we had a discussion in #webkit), the use of "firstword" is much more prevalent than "first-word" and "firstWord".

Sadly, it's all over the place.

[Jean-Yves Avenard [:jya]]

Created attachment 429724 [details]
Patch

[EWS]

Committed r278143 (238187@main): <https://commits.webkit.org/238187@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429724 [details].