Bug 56458 – Media Streaming API patch 0: adding compilation guards

• Component: DOM
• Status: RESOLVED FIXED

[Leandro Graciá Gil]

This patch introduces the compilation guards to be used by the next incoming patches implementing the HTML5 user media videoconferencing.

[Leandro Graciá Gil]

Created attachment 85924 [details]
Patch

[Steve Block]

Comment on attachment 85924 [details]
Patch

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

A few general comments ...

- Have you checked with the owners of each port that they want these flags added to their config files? I haven't seen this approach used before. Given that a new feature is typically initially enabled by default on only a couple of ports (as is the case here), the plumbing for the config flag is only provided for those ports. If a port later decides to experiment with the feature, they add the flag plumbing. While I understand that you're trying to help, it might be seen as adding needless bloat if they never plan to use the feature. It might be worth CC'ing representatives from other ports on this bug to get their feedback. You could split the non-Chromium ports into a separate patch if you're blocked on fixing this for Chromium.
- Is there a spec you can link to from the 'URL' field of the bug?
- Rather than using 'patch number' in the bug title, I'd recommend creating a meta-bug for the whole feature. Individual components are then tracked with bugs that block the meta-bug.

> ChangeLog:7
> +        https://bugs.webkit.org/show_bug.cgi?id=56458

ChangeLog style is ...
<bug title>
<bug URL>

<more detailed description>

> Source/WebCore/ChangeLog:8
> +

WebCore ChangeLog requires a list of the tests that test the functionality on this patch, or a description of why no tests are required.

> Source/WebKit/chromium/features.gypi:87
> +        'ENABLE_USER_MEDIA=1',

Ordering

[Leandro Graciá Gil]

Created attachment 86049 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 85924 [details]
Patch

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

>> ChangeLog:7
>> +        https://bugs.webkit.org/show_bug.cgi?id=56458
> 
> ChangeLog style is ...
> <bug title>
> <bug URL>
> 
> <more detailed description>

Fixed.

>> Source/WebCore/ChangeLog:8
>> +
> 
> WebCore ChangeLog requires a list of the tests that test the functionality on this patch, or a description of why no tests are required.

Changes to WebCore removed.

>> Source/WebKit/chromium/features.gypi:87
>> +        'ENABLE_USER_MEDIA=1',
> 
> Ordering

Fixed.

[Leandro Graciá Gil]

(In reply to comment #2)
> (From update of attachment 85924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85924&action=review
> 
> - Is there a spec you can link to from the 'URL' field of the bug?
> - Rather than using 'patch number' in the bug title, I'd recommend creating a meta-bug for the whole feature. Individual components are then tracked with bugs that block the meta-bug.

There is a metabug already. It can be found in: https://bugs.webkit.org/show_bug.cgi?id=56459. The spec URL can be found in the metabug description.

[Eric Carlson]

Comment on attachment 86049 [details]
Patch

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

> Source/WebKit/chromium/features.gypi:86
> +        'ENABLE_USER_MEDIA=1',

'ENABLE_USER_MEDIA' doesn't tell me anything about what feature it enables. I realize that the spec uses "user media" in the interface names, eg. NavigatorUserMedia, getUserMedia(), etc, but the term didn't mean anything to me until I looked at the spec and it even after reading the spec it doesn't make me think  "video conferencing". 

I don't have any great suggestions, maybe 'ENABLE_MEDIA_STREAM_API'?

[Leandro Graciá Gil]

(In reply to comment #6)
> (From update of attachment 86049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86049&action=review
> 
> > Source/WebKit/chromium/features.gypi:86
> > +        'ENABLE_USER_MEDIA=1',
> 
> 'ENABLE_USER_MEDIA' doesn't tell me anything about what feature it enables. I realize that the spec uses "user media" in the interface names, eg. NavigatorUserMedia, getUserMedia(), etc, but the term didn't mean anything to me until I looked at the spec and it even after reading the spec it doesn't make me think  "video conferencing". 
> 
> I don't have any great suggestions, maybe 'ENABLE_MEDIA_STREAM_API'?

Good suggestion. I'm not very convinced about ENABLE_USER_MEDIA either, but couldn't figure anything much better. I'll make the changes to ENABLE_MEDIA_STREAM_API.

[Steve Block]

Comment on attachment 86049 [details]
Patch

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

>>> Source/WebKit/chromium/features.gypi:86
>>> +        'ENABLE_USER_MEDIA=1',
>> 
>> 'ENABLE_USER_MEDIA' doesn't tell me anything about what feature it enables. I realize that the spec uses "user media" in the interface names, eg. NavigatorUserMedia, getUserMedia(), etc, but the term didn't mean anything to me until I looked at the spec and it even after reading the spec it doesn't make me think  "video conferencing". 
>> 
>> I don't have any great suggestions, maybe 'ENABLE_MEDIA_STREAM_API'?
> 
> Good suggestion. I'm not very convinced about ENABLE_USER_MEDIA either, but couldn't figure anything much better. I'll make the changes to ENABLE_MEDIA_STREAM_API.

Typically enable guards don't include 'API', eg 'GEOLOCATION', 'DEVICE_ORIENTATION', 'BLOB', so maybe 'ENABLE_MEDIA_STREAM'.

> Tools/Scripts/build-webkit:264
> +      define => "ENABLE_USER_MEDIA", default => 0, value => \$userMediaSupport },

Does Chromium use this script? How does a default of o interact with a default of 1 in the gypi?

[Leandro Graciá Gil]

Created attachment 86076 [details]
Patch

[Leandro Graciá Gil]

Created attachment 86078 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 86049 [details]
Patch

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

>>>> Source/WebKit/chromium/features.gypi:86
>>>> +        'ENABLE_USER_MEDIA=1',
>>> 
>>> 'ENABLE_USER_MEDIA' doesn't tell me anything about what feature it enables. I realize that the spec uses "user media" in the interface names, eg. NavigatorUserMedia, getUserMedia(), etc, but the term didn't mean anything to me until I looked at the spec and it even after reading the spec it doesn't make me think  "video conferencing". 
>>> 
>>> I don't have any great suggestions, maybe 'ENABLE_MEDIA_STREAM_API'?
>> 
>> Good suggestion. I'm not very convinced about ENABLE_USER_MEDIA either, but couldn't figure anything much better. I'll make the changes to ENABLE_MEDIA_STREAM_API.
> 
> Typically enable guards don't include 'API', eg 'GEOLOCATION', 'DEVICE_ORIENTATION', 'BLOB', so maybe 'ENABLE_MEDIA_STREAM'.

Fixed.

>> Tools/Scripts/build-webkit:264
>> +      define => "ENABLE_USER_MEDIA", default => 0, value => \$userMediaSupport },
> 
> Does Chromium use this script? How does a default of o interact with a default of 1 in the gypi?

Default changed to isChromium() as seems more appropriate. I've left the 1 in the gypi file to make sure that the build bots always compile with this new feature enabled.

[Leandro Graciá Gil]

Created attachment 86089 [details]
Patch

[Leandro Graciá Gil]

Renaming the title in the ChangeLogs.

[Leandro Graciá Gil]

Created attachment 86174 [details]
Patch

[Leandro Graciá Gil]

No content changes. Rebasing to test the gtk bot.

[Steve Block]

Comment on attachment 86174 [details]
Patch

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

> Source/WebCore/features.pri:78
> +!contains(DEFINES, ENABLE_MEDIA_STREAM=.): DEFINES += ENABLE_MEDIA_STREAM=0

Is this change required to make build-webkit work for Chromium?

> Tools/Scripts/build-webkit:264
> +      define => "ENABLE_MEDIA_STREAM", default => isChromium(), value => \$mediaStreamSupport },

Presumably there'll be a runtime flag for Chromium too to prevent the feature from being accessible while still only partially complete?

[Leandro Graciá Gil]

Comment on attachment 86174 [details]
Patch

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

>> Source/WebCore/features.pri:78
>> +!contains(DEFINES, ENABLE_MEDIA_STREAM=.): DEFINES += ENABLE_MEDIA_STREAM=0
> 
> Is this change required to make build-webkit work for Chromium?

No, you're right. I'm removing this.

>> Tools/Scripts/build-webkit:264
>> +      define => "ENABLE_MEDIA_STREAM", default => isChromium(), value => \$mediaStreamSupport },
> 
> Presumably there'll be a runtime flag for Chromium too to prevent the feature from being accessible while still only partially complete?

Yes, you're right.

[Leandro Graciá Gil]

Created attachment 86320 [details]
Patch

[Leandro Graciá Gil]

Removed pri file.

[Steve Block]

Comment on attachment 86320 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 86320 [details]
Patch

Clearing flags on attachment: 86320

Committed r81596: <http://trac.webkit.org/changeset/81596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.