Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
53313005:
Respect disable_webrtc command line. (Closed)
|
Created:
7 years, 1 month ago by michaelbai Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, dwkang1, wonsik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRespect disable_webrtc command line.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232936
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 10
Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 24 (0 generated)
PTAL
https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... content/renderer/render_view_impl.cc:3074: if (webrtc_enabled) { early return would be better if (cmd_line->HasSwitch(switches::kDisableWebRTC)) return;
https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... content/renderer/render_view_impl.cc:3074: if (webrtc_enabled) { We can't return and still want to create WebMediaPlayerAndroid in line 3136 when WebRTC disabled, On 2013/10/30 22:45:20, jamesr wrote: > early return would be better > > if (cmd_line->HasSwitch(switches::kDisableWebRTC)) > return;
https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... content/renderer/render_view_impl.cc:3074: if (webrtc_enabled) { On 2013/10/30 22:48:35, michaelbai wrote: > We can't return and still want to create WebMediaPlayerAndroid in line 3136 when > WebRTC disabled, This function's way too complicated. Break it up into pieces that do one thing each.
https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/1/content/renderer/render_view_... content/renderer/render_view_impl.cc:3074: if (webrtc_enabled) { Agreed, I had another CL https://codereview.chromium.org/52463004/ depends on this, how about getting this landed, then I will refactoring this code. On 2013/10/30 22:50:11, jamesr wrote: > On 2013/10/30 22:48:35, michaelbai wrote: > > We can't return and still want to create WebMediaPlayerAndroid in line 3136 > when > > WebRTC disabled, > > This function's way too complicated. Break it up into pieces that do one thing > each.
I don't think it's reasonable to make this function even more complicated than it is right now.
PTAL
https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:6249: #if defined(ENABLE_WEBRTC) OOC aren't we shipping WebRTC? do you know if we can remove this or are there bits of Chrome that require this define?
https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:6249: #if defined(ENABLE_WEBRTC) WebRTC is disabled in WebView. But, I don't know why both compile flag and command line needed. +joth, he may comment on this. On 2013/11/01 00:21:36, scherkus wrote: > OOC aren't we shipping WebRTC? do you know if we can remove this or are there > bits of Chrome that require this define?
webview compiles out webRtc to save space for unused feature. But the command line flag is also needed to hide the getUserMedia() function from javascript at runtime. Otherwise pages would think it's supported when it's not. (obviously nothing of that is intractable, just statement of the current situation) On 31 October 2013 17:31, <michaelbai@chromium.org> wrote: > > https://codereview.chromium.**org/53313005/diff/100003/** > content/renderer/render_view_**impl.cc<https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_view_impl.cc> > File content/renderer/render_view_**impl.cc (right): > > https://codereview.chromium.**org/53313005/diff/100003/** > content/renderer/render_view_**impl.cc#newcode6249<https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_view_impl.cc#newcode6249> > content/renderer/render_view_**impl.cc:6249: #if defined(ENABLE_WEBRTC) > WebRTC is disabled in WebView. But, I don't know why both compile flag > and command line needed. +joth, he may comment on this. > > On 2013/11/01 00:21:36, scherkus wrote: > >> OOC aren't we shipping WebRTC? do you know if we can remove this or >> > are there > >> bits of Chrome that require this define? >> > > https://codereview.chromium.**org/53313005/<https://codereview.chromium.org/5... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Does ps2 cause us to handle the disable-webrtc flag or is it meant to be a pure refactor?
This is not pure refactoring, see my comment in render_view_impl.cc https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... File content/renderer/render_view_impl.cc (left): https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:3080: return NULL; Here is the difference, the original code will return NULL, the new code is returned if player is created successfully, looked through the InitializeMediaStreamClient, it didn't make sense to return NULL here. Here are the cases of returning false: if (!RenderThreadImpl::current()) // Will be NULL during unit tests. return false; It only happened in unit test, from the trybot results, it seemed this is not an issue. The other 2 cases, we don't want to return NULL. #if defined(OS_ANDROID) if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableWebRTC)) return false; #endif #else return false; #endif
ping
ok, lgtm. thanks for the cleanup and explanation
https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:3079: if (player != NULL) nit: remove != NULL check https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:6246: WebFrame* frame, fix indent (should be 4-space indent) https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:6364: if (media_stream_client && media_stream_client_->IsMediaStream(url)) { this code looks broken (media_stream_client doesn't appear to be defined after refactoring)
PTAL https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:3079: if (player != NULL) On 2013/11/04 20:38:50, scherkus wrote: > nit: remove != NULL check Done. https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:6246: WebFrame* frame, On 2013/11/04 20:38:50, scherkus wrote: > fix indent (should be 4-space indent) Done. https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:6364: if (media_stream_client && media_stream_client_->IsMediaStream(url)) { media_stream_client is defined unconditionally, On 2013/11/04 20:38:50, scherkus wrote: > this code looks broken (media_stream_client doesn't appear to be defined after > refactoring) https://codereview.chromium.org/53313005/diff/100003/content/renderer/render_... content/renderer/render_view_impl.cc:6364: if (media_stream_client && media_stream_client_->IsMediaStream(url)) { The media_stream_client is defined unconditionally, and it could be NULL here, so I added media_stream_client check. Did you see any problem here? On 2013/11/04 20:38:50, scherkus wrote: > this code looks broken (media_stream_client doesn't appear to be defined after > refactoring)
https://codereview.chromium.org/53313005/diff/330001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/330001/content/renderer/render_... content/renderer/render_view_impl.cc:6371: if (media_stream_client && media_stream_client_->IsMediaStream(url)) { perhaps I'm missing it ... but where is media_stream_client (NOTE: no trailing underscore!) defined? I'm only seeing media_stream_client_ (the member variable with a trailing underscore) defined in this scope.
PTAL https://codereview.chromium.org/53313005/diff/330001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/53313005/diff/330001/content/renderer/render_... content/renderer/render_view_impl.cc:6371: if (media_stream_client && media_stream_client_->IsMediaStream(url)) { It's my bad, there was underscore there. Do we have trybot that enables the GOOGLE_TV? On 2013/11/04 21:53:10, scherkus wrote: > perhaps I'm missing it ... but where is media_stream_client (NOTE: no trailing > underscore!) defined? > > I'm only seeing media_stream_client_ (the member variable with a trailing > underscore) defined in this scope.
lgtm +ycheo as FYI for GOOGLE_TV changes
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/53313005/380001
On 2013/11/04 22:58:43, scherkus wrote: > lgtm > > +ycheo as FYI for GOOGLE_TV changes Thanks Andrew for notifying this.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/53313005/380001
Message was sent while issue was closed.
Change committed as 232936 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
