Fix enable_webrtc

Fix the build when enable_webrtc is set to 0.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284691

[zhaoze.zhou, 2014-07-09 19:18:08]

Please take a look.

[pdr., 2014-07-10 22:23:08]

https://codereview.chromium.org/381683003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/381683003/diff/1/AUTHORS#newcode428
AUTHORS:428: Zhaoze Zhou <zhaoze.zhou@partner.samsung.com>
Can you do this as a change by itself? Just toss up a patch that says "Add
Zhaoze Zhou to the AUTHORS file"

[kangil_, 2014-07-12 02:06:23]

On 2014/07/09 19:18:08, zhaoze.zhou wrote:
> Please take a look.

Would you do 'self review' to verify if,
  *) tab character is used
  *) indentation is correct
?

[pdr., 2014-07-12 02:34:49]

This looks pretty good but there are a lot of small issues.

Once you have these addressed, just reply with the comment "PTAL" so your
reviewers get an email. I will be on vacation next week but any reviewer should
be able to finish up this review.

https://codereview.chromium.org/381683003/diff/60001/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/381683003/diff/60001/AUTHORS#newcode428
AUTHORS:428: Zhaoze Zhou <zhaoze.zhou@partner.samsung.com>
Please rebase this patch since this landed.

https://codereview.chromium.org/381683003/diff/60001/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/381683003/diff/60001/content/content_common.g...
content/content_common.gypi:15:
'../ui/accessibility/accessibility.gyp:accessibility',
Please remove the tab here.

https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
content/content_renderer.gypi:842: },{  # enable_webrtc==0
There is an extra space at the end of the line here.

https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
content/content_renderer.gypi:843: 'sources': [
Please correct this indentation.

https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
content/content_renderer.gypi:848: ['enable_webrtc==0 and enable_plugins==1',{
This is not indented properly.

https://codereview.chromium.org/381683003/diff/60001/content/shell/renderer/t...
File content/shell/renderer/test_runner/web_frame_test_proxy.h (right):

https://codereview.chromium.org/381683003/diff/60001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_frame_test_proxy.h:284: }
You added an extra space here.

[zhaoze.zhou, 2014-07-14 20:39:36]

On 2014/07/12 02:34:49, pdr wrote:
> This looks pretty good but there are a lot of small issues.
> 
> Once you have these addressed, just reply with the comment "PTAL" so your
> reviewers get an email. I will be on vacation next week but any reviewer
should
> be able to finish up this review.
> 
> https://codereview.chromium.org/381683003/diff/60001/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/381683003/diff/60001/AUTHORS#newcode428
> AUTHORS:428: Zhaoze Zhou <mailto:zhaoze.zhou@partner.samsung.com>
> Please rebase this patch since this landed.
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/content_common.gypi
> File content/content_common.gypi (right):
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/content_common.g...
> content/content_common.gypi:15:
> '../ui/accessibility/accessibility.gyp:accessibility',
> Please remove the tab here.
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
> File content/content_renderer.gypi (right):
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
> content/content_renderer.gypi:842: },{  # enable_webrtc==0
> There is an extra space at the end of the line here.
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
> content/content_renderer.gypi:843: 'sources': [
> Please correct this indentation.
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/content_renderer...
> content/content_renderer.gypi:848: ['enable_webrtc==0 and enable_plugins==1',{
> This is not indented properly.
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/web_frame_test_proxy.h (right):
> 
>
https://codereview.chromium.org/381683003/diff/60001/content/shell/renderer/t...
> content/shell/renderer/test_runner/web_frame_test_proxy.h:284: }
> You added an extra space here.

PTAL

[jam, 2014-07-15 02:05:43]

can you provide more background about why you care about building without
enable_webrtc?

https://codereview.chromium.org/381683003/diff/140001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/381683003/diff/140001/content/public/browser/...
content/public/browser/render_process_host.h:201: typedef
base::Callback<void(scoped_ptr<uint8[]> packet_header,
why move this out instead of ifdef'ing out the code which uses it?

[lgombos, 2014-07-15 02:33:23]

> can you provide more background about why you care about building without
> enable_webrtc?

Our interest with this work is to save on binary size for an embedded
environment. Additionally - for the health of the code base - we should either
maintain the ability to flip these enable_ flags or we should remove them.

[zhaoze.zhou, 2014-07-15 13:21:21]

Reason of why take  WebRtcRtpPacketCallback out of ifdef. Please take a look.

https://codereview.chromium.org/381683003/diff/140001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/381683003/diff/140001/content/public/browser/...
content/public/browser/render_process_host.h:201: typedef
base::Callback<void(scoped_ptr<uint8[]> packet_header,
On 2014/07/15 02:05:43, jam wrote:
> why move this out instead of ifdef'ing out the code which uses it?

In file included from
../../content/browser/renderer_host/render_process_host_impl.cc:89:0:
../../content/browser/renderer_host/p2p/socket_dispatcher_host.h:53:13: error:
‘WebRtcRtpPacketCallback’ in ‘class content::RenderProcessHost’ does not name a
type
       const RenderProcessHost::WebRtcRtpPacketCallback& packet_callback);
             ^
../../content/browser/renderer_host/p2p/socket_dispatcher_host.h:53:57: error:
ISO C++ forbids declaration of ‘packet_callback’ with no type [-fpermissive]
       const RenderProcessHost::WebRtcRtpPacketCallback& packet_callback);
                                                         ^
../../content/browser/renderer_host/p2p/socket_dispatcher_host.h:112:3: error:
‘WebRtcRtpPacketCallback’ in ‘class content::RenderProcessHost’ does not name a
type
   RenderProcessHost::WebRtcRtpPacketCallback packet_callback_;
   ^

This error is in the condition of enable_webrtc = 0

[jam, 2014-07-15 16:20:40]

On 2014/07/15 02:33:23, lgombos wrote:
> > can you provide more background about why you care about building without
> > enable_webrtc?
> 
> Our interest with this work is to save on binary size for an embedded
> environment. Additionally - for the health of the code base - we should either
> maintain the ability to flip these enable_ flags or we should remove them.

I'm curious how much binary size enable_webrtc saves?

https://codereview.chromium.org/381683003/diff/140001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/381683003/diff/140001/content/public/browser/...
content/public/browser/render_process_host.h:201: typedef
base::Callback<void(scoped_ptr<uint8[]> packet_header,
On 2014/07/15 13:21:21, zhaoze.zhou wrote:
> On 2014/07/15 02:05:43, jam wrote:
> > why move this out instead of ifdef'ing out the code which uses it?
> 
> In file included from
> ../../content/browser/renderer_host/render_process_host_impl.cc:89:0:
> ../../content/browser/renderer_host/p2p/socket_dispatcher_host.h:53:13: error:
> ‘WebRtcRtpPacketCallback’ in ‘class content::RenderProcessHost’ does not name
a
> type
>        const RenderProcessHost::WebRtcRtpPacketCallback& packet_callback);
>              ^
> ../../content/browser/renderer_host/p2p/socket_dispatcher_host.h:53:57: error:
> ISO C++ forbids declaration of ‘packet_callback’ with no type [-fpermissive]
>        const RenderProcessHost::WebRtcRtpPacketCallback& packet_callback);
>                                                          ^
> ../../content/browser/renderer_host/p2p/socket_dispatcher_host.h:112:3: error:
> ‘WebRtcRtpPacketCallback’ in ‘class content::RenderProcessHost’ does not name
a
> type
>    RenderProcessHost::WebRtcRtpPacketCallback packet_callback_;
>    ^
> 
> This error is in the condition of enable_webrtc = 0

seems like "#if defined(ENABLE_WEBRTC)" should be added to
socket_dispatcher_host.h

[lgombos, 2014-07-15 16:53:17]

On 2014/07/15 16:20:40, jam wrote:
> On 2014/07/15 02:33:23, lgombos wrote:
> > > can you provide more background about why you care about building without
> > > enable_webrtc?
> > 
> > Our interest with this work is to save on binary size for an embedded
> > environment. Additionally - for the health of the code base - we should
either
> > maintain the ability to flip these enable_ flags or we should remove them.
> 
> I'm curious how much binary size enable_webrtc saves?
> 

We can probably measure that. Not sure if this is achieved by this CL but the
significant saving would come from not linking in libjingle - as for content
shell webrtc seems to be the only user of libjingle.

[kbalazs, 2014-07-15 17:14:29]

On 2014/07/15 16:53:17, lgombos wrote:
> On 2014/07/15 16:20:40, jam wrote:
> > On 2014/07/15 02:33:23, lgombos wrote:
> > > > can you provide more background about why you care about building
without
> > > > enable_webrtc?
> > > 
> > > Our interest with this work is to save on binary size for an embedded
> > > environment. Additionally - for the health of the code base - we should
> either
> > > maintain the ability to flip these enable_ flags or we should remove them.
> > 
> > I'm curious how much binary size enable_webrtc saves?
> > 
> 
> We can probably measure that. Not sure if this is achieved by this CL but the
> significant saving would come from not linking in libjingle - as for content
> shell webrtc seems to be the only user of libjingle.

Which is more than 700K! http://andrewhayden.github.io/binary_size_example/

[jam, 2014-07-16 18:27:23]

On 2014/07/15 17:14:29, kbalazs wrote:
> On 2014/07/15 16:53:17, lgombos wrote:
> > On 2014/07/15 16:20:40, jam wrote:
> > > On 2014/07/15 02:33:23, lgombos wrote:
> > > > > can you provide more background about why you care about building
> without
> > > > > enable_webrtc?
> > > > 
> > > > Our interest with this work is to save on binary size for an embedded
> > > > environment. Additionally - for the health of the code base - we should
> > either
> > > > maintain the ability to flip these enable_ flags or we should remove
them.
> > > 
> > > I'm curious how much binary size enable_webrtc saves?
> > > 
> > 
> > We can probably measure that. Not sure if this is achieved by this CL but
the
> > significant saving would come from not linking in libjingle - as for content
> > shell webrtc seems to be the only user of libjingle.
> 
> Which is more than 700K! http://andrewhayden.github.io/binary_size_example/


thanks, that's very helpful to know

https://codereview.chromium.org/381683003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

https://codereview.chromium.org/381683003/diff/160001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:50: #if
defined(ENABLE_WEBRTC)
nit: put the comment inside the ifdef. also, if you're ifdef'ing out the
StartRtpDump method, you should also ifdef out StopRtpDumpOnUIThread method as
well since they're related

you also need to update the cc file to match

[zhaoze.zhou, 2014-07-16 21:12:39]

Please take a look

https://codereview.chromium.org/381683003/diff/200001/content/browser/rendere...
File content/browser/renderer_host/p2p/socket_dispatcher_host.h (right):

https://codereview.chromium.org/381683003/diff/200001/content/browser/rendere...
content/browser/renderer_host/p2p/socket_dispatcher_host.h:100: void
StopRtpDumpOnIOThread(bool incoming, bool outgoing);
hi, I think the StopRtpDumpOnIOThread method also related with the StartRtpDump
method.

[zhaoze.zhou, 2014-07-16 22:10:10]

Please take a look

[jam, 2014-07-17 17:40:05]

looks like the cl added more files. specifically now that you added changes in
p2p_messages.h, I looked closer and I have some more questions.

it doesn't seem necessasry to include "#if defined(ENABLE_WEBRTC)" inside files
that are only compiled (and included) when enable_webrtc is 1. for example
socket_dispatcher_host.cc is only compiled with enable_webrtc.
socket_dispatcher_host.h is only included in render_process_host_impl.cc for
enable_webrtc. so i don't know why they need to be changed.

for p2p_messages.h, it's only included in files with enable_webrtc. the one
exception is content/common/content_message_generator.h, so you can put it
behind an include there

[zhaoze.zhou, 2014-07-17 22:24:47]

On 2014/07/17 17:40:05, jam wrote:
> looks like the cl added more files. specifically now that you added changes in
> p2p_messages.h, I looked closer and I have some more questions.
> 
> it doesn't seem necessasry to include "#if defined(ENABLE_WEBRTC)" inside
files
> that are only compiled (and included) when enable_webrtc is 1. for example
> socket_dispatcher_host.cc is only compiled with enable_webrtc.
> socket_dispatcher_host.h is only included in render_process_host_impl.cc for
> enable_webrtc. so i don't know why they need to be changed.
> 
> for p2p_messages.h, it's only included in files with enable_webrtc. the one
> exception is content/common/content_message_generator.h, so you can put it
> behind an include there

Thanks, you are right.

[zhaoze.zhou, 2014-07-18 20:49:47]

nasko@chromium.org: Please take a look

[nasko, 2014-07-21 07:57:05]

On 2014/07/18 20:49:47, zhaoze.zhou wrote:
> mailto:nasko@chromium.org: Please take a look

Can you be more specific what I need to look at? jam@ is OWNER for content/, so
his LGTM should be sufficient.

[zhaoze.zhou, 2014-07-21 13:11:12]

On 2014/07/21 07:57:05, nasko (in Munich) wrote:
> On 2014/07/18 20:49:47, zhaoze.zhou wrote:
> > mailto:nasko@chromium.org: Please take a look
> 
> Can you be more specific what I need to look at? jam@ is OWNER for content/,
so
> his LGTM should be sufficient.

In patch set 8, I added something on content_message_generator.h . Thanks

[nasko, 2014-07-21 13:12:23]

On 2014/07/21 13:11:12, zhaoze.zhou wrote:
> On 2014/07/21 07:57:05, nasko (in Munich) wrote:
> > On 2014/07/18 20:49:47, zhaoze.zhou wrote:
> > > mailto:nasko@chromium.org: Please take a look
> > 
> > Can you be more specific what I need to look at? jam@ is OWNER for content/,
> so
> > his LGTM should be sufficient.
> 
> In patch set 8, I added something on content_message_generator.h . Thanks

messages LGTM

[jam, 2014-07-21 17:24:20]

lgtm with nits

https://codereview.chromium.org/381683003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/381683003/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:89: #if
defined(ENABLE_WEBRTC)
ditto, move this to 158

https://codereview.chromium.org/381683003/diff/280001/content/common/content_...
File content/common/content_message_generator.h (right):

https://codereview.chromium.org/381683003/diff/280001/content/common/content_...
content/common/content_message_generator.h:48: #if defined(ENABLE_WEBRTC)
nit: please move this to line 68 so that the ifdef'd sections are at the bottom
(makes readability easier)

[zhaoze.zhou, 2014-07-21 18:23:37]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-07-21 18:25:11]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/381683...

[commit-bot: I haz the power, 2014-07-21 18:47:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31119)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/35519)

[commit-bot: I haz the power, 2014-07-21 18:49:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-21 18:49:40]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31127)

[zhaoze.zhou, 2014-07-21 19:11:51]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-07-21 19:13:18]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/381683...

[commit-bot: I haz the power, 2014-07-21 20:17:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31143)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/35543)

[commit-bot: I haz the power, 2014-07-21 20:20:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-21 20:20:13]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[zhaoze.zhou, 2014-07-22 14:16:17]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-07-22 14:17:41]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/381683...

[commit-bot: I haz the power, 2014-07-22 16:10:40]

Change committed as 284691