Bug 56458

Summary: Media Streaming API patch 0: adding compilation guards
Product: WebKit Reporter: Leandro Graciá Gil <leandrogracia>
Component: DOMAssignee: Leandro Graciá Gil <leandrogracia>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, satish, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 56459, 56586, 56921, 56949    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Leandro Graciá Gil 2011-03-16 05:27:05 PDT
This patch introduces the compilation guards to be used by the next incoming patches implementing the HTML5 user media videoconferencing.
Comment 1 Leandro Graciá Gil 2011-03-16 05:41:15 PDT
Created attachment 85924 [details]
Patch
Comment 2 Steve Block 2011-03-17 03:21:52 PDT
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
Comment 3 Leandro Graciá Gil 2011-03-17 05:21:24 PDT
Created attachment 86049 [details]
Patch
Comment 4 Leandro Graciá Gil 2011-03-17 05:21:53 PDT
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.
Comment 5 Leandro Graciá Gil 2011-03-17 05:22:57 PDT
(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.
Comment 6 Eric Carlson 2011-03-17 09:55:05 PDT
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'?
Comment 7 Leandro Graciá Gil 2011-03-17 10:30:35 PDT
(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.
Comment 8 Steve Block 2011-03-17 10:38:12 PDT
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?
Comment 9 Leandro Graciá Gil 2011-03-17 11:51:27 PDT
Created attachment 86076 [details]
Patch
Comment 10 Leandro Graciá Gil 2011-03-17 12:09:06 PDT
Created attachment 86078 [details]
Patch
Comment 11 Leandro Graciá Gil 2011-03-17 12:09:44 PDT
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.
Comment 12 Leandro Graciá Gil 2011-03-17 13:59:46 PDT
Created attachment 86089 [details]
Patch
Comment 13 Leandro Graciá Gil 2011-03-17 14:00:16 PDT
Renaming the title in the ChangeLogs.
Comment 14 Leandro Graciá Gil 2011-03-18 10:11:56 PDT
Created attachment 86174 [details]
Patch
Comment 15 Leandro Graciá Gil 2011-03-18 10:12:21 PDT
No content changes. Rebasing to test the gtk bot.
Comment 16 Steve Block 2011-03-21 02:49:59 PDT
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?
Comment 17 Leandro Graciá Gil 2011-03-21 08:44:26 PDT
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.
Comment 18 Leandro Graciá Gil 2011-03-21 08:51:55 PDT
Created attachment 86320 [details]
Patch
Comment 19 Leandro Graciá Gil 2011-03-21 08:52:20 PDT
Removed pri file.
Comment 20 Steve Block 2011-03-21 10:09:36 PDT
Comment on attachment 86320 [details]
Patch

r=me
Comment 21 WebKit Commit Bot 2011-03-21 12:08:39 PDT
Comment on attachment 86320 [details]
Patch

Clearing flags on attachment: 86320

Committed r81596: <http://trac.webkit.org/changeset/81596>
Comment 22 WebKit Commit Bot 2011-03-21 12:08:46 PDT
All reviewed patches have been landed.  Closing bug.