Bug 81652

Summary:

[chromium] MediaStream API (JSEP): Enhancing WebMediaHints and WebICEOptions

Product:

WebKit

Reporter:

Tommy Widenflycht <tommyw>

Component:

WebKit API

Assignee:

Tommy Widenflycht <tommyw>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, fishd, jamesr, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

80589

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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	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 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.