Bug 81652 - [chromium] MediaStream API (JSEP): Enhancing WebMediaHints and WebICEOptions
Summary: [chromium] MediaStream API (JSEP): Enhancing WebMediaHints and WebICEOptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 80589
  Show dependency tree / graph
 
Reported: 2012-03-20 05:21 PDT by Tommy Widenflycht
Modified: 2012-03-22 10:41 PDT (History)
6 users (show)

See Also:

Attachments
Patch (3.73 KB, patch)
2012-03-20 05:25 PDT, Tommy Widenflycht 		no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2012-03-20 06:59 PDT, Tommy Widenflycht 		no flags Details | Formatted Diff | Diff
Patch (6.37 KB, patch)
2012-03-21 05:53 PDT, Tommy Widenflycht 		no flags Details | Formatted Diff | Diff
Patch (6.55 KB, patch)
2012-03-21 06:02 PDT, Tommy Widenflycht 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-03-20 05:21:09 PDT 
Adding initialize method to WebMediaHints and WebICEOptions. This is needed for Chromium unittests.
Comment 1 Tommy Widenflycht 2012-03-20 05:25:37 PDT 
Created attachment 132807 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-20 05:27:24 PDT 
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Tommy Widenflycht 2012-03-20 06:59:59 PDT 
Created attachment 132819 [details]
Patch
Comment 4 Darin Fisher (:fishd, Google) 2012-03-20 16:30:38 PDT 
Comment on attachment 132819 [details]
Patch

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

> Source/WebKit/chromium/src/WebICEOptions.cpp:57
> +    IceOptions::UseCandidatesOption option = IceOptions::ALL;

any reason not to just static_cast between these enum types?  add lines
to AssertMatchingEnums.cpp to ensure that the static_cast remains valid?
Comment 5 Darin Fisher (:fishd, Google) 2012-03-20 20:43:35 PDT 
Comment on attachment 132819 [details]
Patch

Yeah, seems like we should be using static_cast here.  See how similar enums are handled in other WebKit interfaces.
Comment 6 Tommy Widenflycht 2012-03-21 05:53:36 PDT 
Created attachment 133025 [details]
Patch
Comment 7 WebKit Review Bot 2012-03-21 05:55:07 PDT 
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 8 Tommy Widenflycht 2012-03-21 05:58:04 PDT 
Comment on attachment 132819 [details]
Patch

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

>> Source/WebKit/chromium/src/WebICEOptions.cpp:57
>> +    IceOptions::UseCandidatesOption option = IceOptions::ALL;
> 
> any reason not to just static_cast between these enum types?  add lines
> to AssertMatchingEnums.cpp to ensure that the static_cast remains valid?

No reason. Fixed.
Comment 9 Tommy Widenflycht 2012-03-21 06:02:54 PDT 
Created attachment 133027 [details]
Patch
Comment 10 Adam Barth 2012-03-21 10:07:00 PDT 
Comment on attachment 133027 [details]
Patch

Is this a common pattern in the API?  Why not just use a constructor?
Comment 11 Tommy Widenflycht 2012-03-21 10:19:33 PDT 
Yes, it is a very common chromium WebKit embedder pattern to have the initialize method creating the private WebCore object. As to exactly why I don't know.

(In reply to comment #10)
> (From update of attachment 133027 [details])
> Is this a common pattern in the API?  Why not just use a constructor?
Comment 12 Adam Barth 2012-03-21 10:22:29 PDT 
Comment on attachment 133027 [details]
Patch

Ok.  I'm fairly new at reviewing changes to the WebKit API, so please feel free to correct me if I've missing anything here.
Comment 13 WebKit Review Bot 2012-03-21 11:30:04 PDT 
Comment on attachment 133027 [details]
Patch

Clearing flags on attachment: 133027

Committed r111582: <http://trac.webkit.org/changeset/111582>
Comment 14 WebKit Review Bot 2012-03-21 11:30:13 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 Darin Fisher (:fishd, Google) 2012-03-22 10:41:30 PDT 
Comment on attachment 133027 [details]
Patch

LGTM too