Bug 81341 - [chromium] MediaStream API (JSEP): Introducing WebMediaHints and WebIceOptions
: [chromium] MediaStream API (JSEP): Introducing WebMediaHints and WebIceOptions
Status: RESOLVED FIXED
: WebKit
WebKit API
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 80589
  Show dependency treegraph
 
Reported: 2012-03-16 05:34 PST by
Modified: 2012-03-19 17:57 PST (History)


Attachments
Patch (14.38 KB, patch)
2012-03-16 05:51 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.30 KB, patch)
2012-03-16 07:17 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.38 KB, patch)
2012-03-16 07:34 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.42 KB, patch)
2012-03-19 08:17 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.42 KB, patch)
2012-03-19 12:36 PST, Tommy Widenflycht
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-16 05:34:36 PST
Simple WebKit representations of the WebCore/platform versions.
------- Comment #1 From 2012-03-16 05:51:47 PST -------
Created an attachment (id=132262) [details]
Patch
------- Comment #2 From 2012-03-16 05:55:08 PST -------
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
------- Comment #3 From 2012-03-16 06:52:43 PST -------
(From update of attachment 132262 [details])
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).
------- Comment #4 From 2012-03-16 07:17:44 PST -------
Created an attachment (id=132278) [details]
Patch
------- Comment #5 From 2012-03-16 07:18:34 PST -------
(From update of attachment 132262 [details])
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:.
------- Comment #6 From 2012-03-16 07:29:21 PST -------
(From update of attachment 132278 [details])
Attachment 132278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11961451
------- Comment #7 From 2012-03-16 07:34:29 PST -------
Created an attachment (id=132280) [details]
Patch
------- Comment #8 From 2012-03-16 07:35:50 PST -------
Added default: back since it seems that the cr-linux buildbot isn't configured to use clang yet.
------- Comment #9 From 2012-03-16 08:12:10 PST -------
(From update of attachment 132280 [details])
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
------- Comment #10 From 2012-03-16 13:29:58 PST -------
(From update of attachment 132280 [details])
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?
------- Comment #11 From 2012-03-19 08:12:48 PST -------
(From update of attachment 132280 [details])
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.
------- Comment #12 From 2012-03-19 08:17:39 PST -------
Created an attachment (id=132588) [details]
Patch
------- Comment #13 From 2012-03-19 12:36:22 PST -------
Created an attachment (id=132629) [details]
Patch
------- Comment #14 From 2012-03-19 12:37:43 PST -------
WebIceOptions is now renamed to WebICEOptions, will rename IceOptions in an follow-up patch.
------- Comment #15 From 2012-03-19 13:22:51 PST -------
(From update of attachment 132629 [details])
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
------- Comment #16 From 2012-03-19 14:27:28 PST -------
(From update of attachment 132629 [details])
Clearing flags on attachment: 132629

Committed r111247: <http://trac.webkit.org/changeset/111247>
------- Comment #17 From 2012-03-19 14:27:33 PST -------
All reviewed patches have been landed.  Closing bug.