Created attachment 85924 [details] Patch

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

Created attachment 86049 [details] Patch

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.

(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 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'?

(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 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?

Created attachment 86076 [details] Patch

Created attachment 86078 [details] Patch

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.

Created attachment 86089 [details] Patch

Renaming the title in the ChangeLogs.

Created attachment 86174 [details] Patch

No content changes. Rebasing to test the gtk bot.

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 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.

Created attachment 86320 [details] Patch

Removed pri file.

Comment on attachment 86320 [details] Patch r=me

Comment on attachment 86320 [details] Patch Clearing flags on attachment: 86320 Committed r81596: <http://trac.webkit.org/changeset/81596>

All reviewed patches have been landed. Closing bug.