Created attachment 132262 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

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

Created attachment 132278 [details] Patch

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 on attachment 132278 [details] Patch Attachment 132278 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11961451

Created attachment 132280 [details] Patch

Added default: back since it seems that the cr-linux buildbot isn't configured to use clang yet.

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

Created attachment 132588 [details] Patch

Created attachment 132629 [details] Patch

WebIceOptions is now renamed to WebICEOptions, will rename IceOptions in an follow-up patch.

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 on attachment 132629 [details] Patch Clearing flags on attachment: 132629 Committed r111247: <http://trac.webkit.org/changeset/111247>

All reviewed patches have been landed. Closing bug.