Summary: | [OpenWebRTC] RealtimeMediaSourceCenter implementation | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | adam.bergkvist, agouaillard, pnormand | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 142393, 142778, 145119 | ||||||
Bug Blocks: | 79203, 143211, 124288 | ||||||
Attachments: |
|
Description
Philippe Normand
2015-03-09 02:47:56 PDT
Created attachment 248833 [details]
patch
I'll file a bug for EFL's jhbuild Comment on attachment 248833 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=248833&action=review > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:67 > + DEPRECATED_DEFINE_STATIC_LOCAL(RealtimeMediaSourceCenterOwr, center, ()); Nit: is there any reason to not use NeverDestroyed? > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:80 > +void RealtimeMediaSourceCenterOwr::validateRequestConstraints(PassRefPtr<MediaStreamCreationClient> prpClient, PassRefPtr<MediaConstraints> prpAudioConstraints, PassRefPtr<MediaConstraints> prpVideoConstraints) Nit: it would be good to switch from PassRefPtr/RefPtr to Ref before the WebRTC implementation(s) get any bigger. Not necessarily in this patch, but soon. > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:146 > + GList* item; > + > + for (item = sources; item; item = item->next) { Nit: this isn't needed outside of the loop so you shouldn't need to pre-declare it. Can you use "auto"? > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:159 > + if (mediaType & OWR_MEDIA_TYPE_AUDIO) > + sourceType = RealtimeMediaSource::Audio; > + else if (mediaType & OWR_MEDIA_TYPE_VIDEO) > + sourceType = RealtimeMediaSource::Video; else ASSERT() (In reply to comment #3) > Comment on attachment 248833 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248833&action=review > > > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:67 > > + DEPRECATED_DEFINE_STATIC_LOCAL(RealtimeMediaSourceCenterOwr, center, ()); > > Nit: is there any reason to not use NeverDestroyed? > Hum, nope :) > > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:80 > > +void RealtimeMediaSourceCenterOwr::validateRequestConstraints(PassRefPtr<MediaStreamCreationClient> prpClient, PassRefPtr<MediaConstraints> prpAudioConstraints, PassRefPtr<MediaConstraints> prpVideoConstraints) > > Nit: it would be good to switch from PassRefPtr/RefPtr to Ref before the > WebRTC implementation(s) get any bigger. Not necessarily in this patch, but > soon. > Good idea, I'll file a separate patch for this. > > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:146 > > + GList* item; > > + > > + for (item = sources; item; item = item->next) { > > Nit: this isn't needed outside of the loop so you shouldn't need to > pre-declare it. Can you use "auto"? > Sure! > > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:159 > > + if (mediaType & OWR_MEDIA_TYPE_AUDIO) > > + sourceType = RealtimeMediaSource::Audio; > > + else if (mediaType & OWR_MEDIA_TYPE_VIDEO) > > + sourceType = RealtimeMediaSource::Video; > > else ASSERT() Ok, I can add an ASSERT_NOT_REACHED() here indeed. Committed r181893: <http://trac.webkit.org/changeset/181893> |