Bug 81341 - [chromium] MediaStream API (JSEP): Introducing WebMediaHints and WebIceOptions
: [chromium] MediaStream API (JSEP): Introducing WebMediaHints and WebIceOptions
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit API
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Tommy Widenflycht
:
Depends on:
Blocks: 80589
  Show dependency treegraph
 
Reported: 2012-03-16 05:34 PDT by Tommy Widenflycht
Modified: 2012-03-19 17:57 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-03-16 05:34:36 PDT
Simple WebKit representations of the WebCore/platform versions.
Comment 1 Tommy Widenflycht 2012-03-16 05:51:47 PDT
Created attachment 132262 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-16 05:55:08 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Adam Barth 2012-03-16 06:52:43 PDT
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).
Comment 4 Tommy Widenflycht 2012-03-16 07:17:44 PDT
Created attachment 132278 [details]
Patch
Comment 5 Tommy Widenflycht 2012-03-16 07:18:34 PDT
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:.
Comment 6 WebKit Review Bot 2012-03-16 07:29:21 PDT
Comment on attachment 132278 [details]
Patch

Attachment 132278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11961451
Comment 7 Tommy Widenflycht 2012-03-16 07:34:29 PDT
Created attachment 132280 [details]
Patch
Comment 8 Tommy Widenflycht 2012-03-16 07:35:50 PDT
Added default: back since it seems that the cr-linux buildbot isn't configured to use clang yet.
Comment 9 WebKit Review Bot 2012-03-16 08:12:10 PDT
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
Comment 10 Darin Fisher (:fishd, Google) 2012-03-16 13:29:58 PDT
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?
Comment 11 Tommy Widenflycht 2012-03-19 08:12:48 PDT
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.
Comment 12 Tommy Widenflycht 2012-03-19 08:17:39 PDT
Created attachment 132588 [details]
Patch
Comment 13 Tommy Widenflycht 2012-03-19 12:36:22 PDT
Created attachment 132629 [details]
Patch
Comment 14 Tommy Widenflycht 2012-03-19 12:37:43 PDT
WebIceOptions is now renamed to WebICEOptions, will rename IceOptions in an follow-up patch.
Comment 15 Darin Fisher (:fishd, Google) 2012-03-19 13:22:51 PDT
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
Comment 16 WebKit Review Bot 2012-03-19 14:27:28 PDT
Comment on attachment 132629 [details]
Patch

Clearing flags on attachment: 132629

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