RESOLVED FIXED 81652
[chromium] MediaStream API (JSEP): Enhancing WebMediaHints and WebICEOptions 
https://bugs.webkit.org/show_bug.cgi?id=81652
Summary [chromium] MediaStream API (JSEP): Enhancing WebMediaHints and WebICEOptions
Tommy Widenflycht
Reported 2012-03-20 05:21:09 PDT
Adding initialize method to WebMediaHints and WebICEOptions. This is needed for Chromium unittests.
Attachments
Patch (3.73 KB, patch)
2012-03-20 05:25 PDT, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Patch (4.37 KB, patch)
2012-03-20 06:59 PDT, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Patch (6.37 KB, patch)
2012-03-21 05:53 PDT, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Patch (6.55 KB, patch)
2012-03-21 06:02 PDT, Tommy Widenflycht 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Tommy Widenflycht
Comment 1 2012-03-20 05:25:37 PDT
Created attachment 132807 [details] Patch
WebKit Review Bot
Comment 2 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.
Tommy Widenflycht
Comment 3 2012-03-20 06:59:59 PDT
Created attachment 132819 [details] Patch
Darin Fisher (:fishd, Google)
Comment 4 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?
Darin Fisher (:fishd, Google)
Comment 5 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.
Tommy Widenflycht
Comment 6 2012-03-21 05:53:36 PDT
Created attachment 133025 [details] Patch
WebKit Review Bot
Comment 7 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.
Tommy Widenflycht
Comment 8 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.
Tommy Widenflycht
Comment 9 2012-03-21 06:02:54 PDT
Created attachment 133027 [details] Patch
Adam Barth
Comment 10 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?
Tommy Widenflycht
Comment 11 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?
Adam Barth
Comment 12 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.
WebKit Review Bot
Comment 13 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>
WebKit Review Bot
Comment 14 2012-03-21 11:30:13 PDT
All reviewed patches have been landed. Closing bug.
Darin Fisher (:fishd, Google)
Comment 15 2012-03-22 10:41:30 PDT
Comment on attachment 133027 [details] Patch LGTM too
Note You need to log in before you can comment on or make changes to this bug.