Bug 81341 – [chromium] MediaStream API (JSEP): Introducing WebMediaHints and WebIceOptions

• Component: WebKit API
• Status: RESOLVED FIXED

[Tommy Widenflycht]

Simple WebKit representations of the WebCore/platform versions.

[Tommy Widenflycht]

Created attachment 132262 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adam Barth]

Comment on attachment 132262 [details]
Patch

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

This looks fine, but we need fishd to review.

> Source/WebKit/chromium/src/WebIceOptions.cpp:71
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return WebIceOptions::ALL;

Typically, in WebKit, we leave off the default case and let the compile tell us if we've forgotten an enum value (assuming the Chromium compile flags are set that way).

[Tommy Widenflycht]

Created attachment 132278 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 132262 [details]
Patch

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

>> Source/WebKit/chromium/src/WebIceOptions.cpp:71
>> +        return WebIceOptions::ALL;
> 
> Typically, in WebKit, we leave off the default case and let the compile tell us if we've forgotten an enum value (assuming the Chromium compile flags are set that way).

Just verified and Chromium is setup the same way. Removed default:.

[WebKit Review Bot]

Comment on attachment 132278 [details]
Patch

Attachment 132278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11961451

[Tommy Widenflycht]

Created attachment 132280 [details]
Patch

[Tommy Widenflycht]

Added default: back since it seems that the cr-linux buildbot isn't configured to use clang yet.

[WebKit Review Bot]

Comment on attachment 132280 [details]
Patch

Attachment 132280 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11966218

New failing tests:
fast/notifications/notifications-check-permission.html

[Darin Fisher (:fishd, Google)]

Comment on attachment 132280 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebIceOptions.h:46
> +class WebIceOptions {

"Ice" -> ICE

see style guide

> Source/WebKit/chromium/public/platform/WebIceOptions.h:48
> +    enum WebUseCandidatesOption {

nit: inner enums should not be "Web" prefixed

> Source/WebKit/chromium/public/platform/WebIceOptions.h:50
> +        NO_RELAY,

nit: use WebKit CamelCase style naming.

nit: for webkit API, name enums like so:

  enum Foo {
      FooBar,
      FooBaz
  };

I'm lacking ICE familiarity, but I find the name of the enum very confusing.
What does UseCandidatesOption mean?

> Source/WebKit/chromium/public/platform/WebIceOptions.h:68
> +    WebUseCandidatesOption useCandidates() const;

need to export this one?

nit: i'm confused by the name of this function.  "useCandidates" sounds like a command, yet
this is a const method that returns an enum value.  confusing.  is there a better name?

> Source/WebKit/chromium/public/platform/WebMediaHints.h:62
> +    bool audio() const;

nit: export methods?

[Tommy Widenflycht]

Comment on attachment 132280 [details]
Patch

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

>> Source/WebKit/chromium/public/platform/WebIceOptions.h:46
>> +class WebIceOptions {
> 
> "Ice" -> ICE
> 
> see style guide

See bug 81339 for answer.

>> Source/WebKit/chromium/public/platform/WebIceOptions.h:48
>> +    enum WebUseCandidatesOption {
> 
> nit: inner enums should not be "Web" prefixed

Fixed.

>> Source/WebKit/chromium/public/platform/WebIceOptions.h:50
>> +        NO_RELAY,
> 
> nit: use WebKit CamelCase style naming.
> 
> nit: for webkit API, name enums like so:
> 
>   enum Foo {
>       FooBar,
>       FooBaz
>   };
> 
> I'm lacking ICE familiarity, but I find the name of the enum very confusing.
> What does UseCandidatesOption mean?

I just used the JS specification naming here which indeed is quite confusing: use_candidates: "all", "no_relay", "only_relay". It means which kinds of candidates you want to use: only local, only relay or both. In the latest patch I rewrote it to:


    enum CandidateType {
        CandidateTypeAll,
        CandidateTypeNoRelay,
        CandidateTypeOnlyRelay,
    };

    WEBKIT_EXPORT CandidateType candidateTypeToUse() const;

Which hopefully is much clearer.

>> Source/WebKit/chromium/public/platform/WebIceOptions.h:68
>> +    WebUseCandidatesOption useCandidates() const;
> 
> need to export this one?
> 
> nit: i'm confused by the name of this function.  "useCandidates" sounds like a command, yet
> this is a const method that returns an enum value.  confusing.  is there a better name?

See above.

>> Source/WebKit/chromium/public/platform/WebMediaHints.h:62
>> +    bool audio() const;
> 
> nit: export methods?

Fixed.

[Tommy Widenflycht]

Created attachment 132588 [details]
Patch

[Tommy Widenflycht]

Created attachment 132629 [details]
Patch

[Tommy Widenflycht]

WebIceOptions is now renamed to WebICEOptions, will rename IceOptions in an follow-up patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 132629 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebICEOptions.h:43
> +

nit: no new line here

[WebKit Review Bot]

Comment on attachment 132629 [details]
Patch

Clearing flags on attachment: 132629

Committed r111247: <http://trac.webkit.org/changeset/111247>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.