[webkit-reviews] review denied: [Bug 56458] User media videoconferencing patch 0: adding compilation guards : [Attachment 85924] Patch

[bugzilla-daemon at webkit.org]

Steve Block <steveblock at google.com> has denied Leandro Graciá Gil
<leandrogracia at chromium.org>'s request for review:
Bug 56458: User media videoconferencing patch 0: adding compilation guards
https://bugs.webkit.org/show_bug.cgi?id=56458

Attachment 85924: Patch
https://bugs.webkit.org/attachment.cgi?id=85924&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
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