Loosening the coupling between WebRTC and //third_party/protobuf

To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

We moved the PRESUBMIT check to another CL [1]. The goal of this
presubmit is to avoid the direct usage of google::protobuf outside
of the webrtc/base/protobuf_utils.h header file.

[1] - https://codereview.webrtc.org/2753823003/

BUG=webrtc:7340
NOTRY=True

Review-Url: https://codereview.webrtc.org/2747863003
Cr-Commit-Position: refs/heads/master@{#17466}
Committed: https://chromium.googlesource.com/external/webrtc/+/16ab93b952f9e8268f2e663ffe49548e8043d5af

[mbonadei, 2017-03-14 15:42:36]

Description was changed from

==========
Introducing protobuf_utils.h

BUG=None
==========

to

==========
To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

BUG=webrtc:7340
==========

[mbonadei, 2017-03-14 15:42:36]

mbonadei@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[mbonadei, 2017-03-14 16:07:35]

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/protobuf_utils.h
File webrtc/base/protobuf_utils.h (right):

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/protobuf_util...
webrtc/base/protobuf_utils.h:17: // #include
"third_party/protobuf/src/google/protobuf/repeated_field.h"
I am struggling with this one.

I need to include this in order to remove the comment from line
22 and remove the reference to google::protobuf::RepeatedPtrField
from webrtc/logging/rtc_event_log/rtc_event_log_parser.cc.

But when I try I get this error:

[...]
error: cannot use typeid with -fno-rtti
    T* t = new (AllocateAligned(RTTI_TYPE_ID(T), sizeof(T))) T(arg);
[...]
/third_party/protobuf/src/google/protobuf/arena.h:162:30: note: expanded from
macro 'RTTI_TYPE_ID'

[kjellander_webrtc, 2017-03-14 21:28:05]

I think I might have found the solution to your problem.

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:82: # TODO(mbonadei): wrap into an if to check if
rtc_enable_protobuf is true?
Yes, put this inside an if(rtc_enable_protobuf) condition.

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:87: include_dirs = [ "//third_party/protobuf/src" ]
I think you should replace this with a deps entry for
//third_party/protobuf:protobuf_lite

that will give you the include dir added via
https://cs.chromium.org/chromium/src/third_party/protobuf/BUILD.gn?rcl=761f60...
but also necessary defines (it seems).

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/protobuf_utils.h
File webrtc/base/protobuf_utils.h (right):

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/protobuf_util...
webrtc/base/protobuf_utils.h:17: // #include
"third_party/protobuf/src/google/protobuf/repeated_field.h"
On 2017/03/14 16:07:35, mbonadei wrote:
> I am struggling with this one.
> 
> I need to include this in order to remove the comment from line
> 22 and remove the reference to google::protobuf::RepeatedPtrField
> from webrtc/logging/rtc_event_log/rtc_event_log_parser.cc.
> 
> But when I try I get this error:
> 
> [...]
> error: cannot use typeid with -fno-rtti
>     T* t = new (AllocateAligned(RTTI_TYPE_ID(T), sizeof(T))) T(arg);
> [...]
> /third_party/protobuf/src/google/protobuf/arena.h:162:30: note: expanded from
> macro 'RTTI_TYPE_ID'

I think you can solve this by depending on //third_party/protobuf:protobuf_lite
in the protobuf_utils GN target (instead of just including the src dir).
That way, you'll get this config:
https://cs.chromium.org/chromium/src/third_party/protobuf/BUILD.gn?rcl=761f60...
which might solve this error.

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:853: include_dirs += [
"//third_party/protobuf/src" ]
We should avoid adding include_dirs whenever we can and inherit them by adding
the right dependencies instead. See previous comment.

[mbonadei, 2017-03-15 08:13:27]

mbonadei@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, kwiberg@webrtc.org

[mbonadei, 2017-03-15 10:08:05]

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:82: # TODO(mbonadei): wrap into an if to check if
rtc_enable_protobuf is true?
On 2017/03/14 21:28:05, kjellander_webrtc wrote:
> Yes, put this inside an if(rtc_enable_protobuf) condition.

Done.

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/BUILD.gn#newc...
webrtc/base/BUILD.gn:87: include_dirs = [ "//third_party/protobuf/src" ]
On 2017/03/14 21:28:05, kjellander_webrtc wrote:
> I think you should replace this with a deps entry for
> //third_party/protobuf:protobuf_lite
> 
> that will give you the include dir added via
>
https://cs.chromium.org/chromium/src/third_party/protobuf/BUILD.gn?rcl=761f60...
> but also necessary defines (it seems).

Done.

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/protobuf_utils.h
File webrtc/base/protobuf_utils.h (right):

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/base/protobuf_util...
webrtc/base/protobuf_utils.h:17: // #include
"third_party/protobuf/src/google/protobuf/repeated_field.h"
On 2017/03/14 21:28:05, kjellander_webrtc wrote:
> On 2017/03/14 16:07:35, mbonadei wrote:
> > I am struggling with this one.
> > 
> > I need to include this in order to remove the comment from line
> > 22 and remove the reference to google::protobuf::RepeatedPtrField
> > from webrtc/logging/rtc_event_log/rtc_event_log_parser.cc.
> > 
> > But when I try I get this error:
> > 
> > [...]
> > error: cannot use typeid with -fno-rtti
> >     T* t = new (AllocateAligned(RTTI_TYPE_ID(T), sizeof(T))) T(arg);
> > [...]
> > /third_party/protobuf/src/google/protobuf/arena.h:162:30: note: expanded
from
> > macro 'RTTI_TYPE_ID'
> 
> I think you can solve this by depending on
//third_party/protobuf:protobuf_lite
> in the protobuf_utils GN target (instead of just including the src dir).
> That way, you'll get this config:
>
https://cs.chromium.org/chromium/src/third_party/protobuf/BUILD.gn?rcl=761f60...
> which might solve this error.

Done.

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/BUILD.gn:853: include_dirs += [
"//third_party/protobuf/src" ]
On 2017/03/14 21:28:05, kjellander_webrtc wrote:
> We should avoid adding include_dirs whenever we can and inherit them by adding
> the right dependencies instead. See previous comment.

Done.

[kjellander_webrtc, 2017-03-15 12:02:31]

I think it would make sense to implement a PRESUBMIT.py check that is looking
for usage of the 'google::protobuf::' namespace in C++ source code and raises an
error if that's the case. That way we would protect ourselves from future uses
sneaking back in. That can be done in a follow-up CL though.

This lgtm with my comment fixed.

https://codereview.webrtc.org/2747863003/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
(right):

https://codereview.webrtc.org/2747863003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:15:
#include <string>
Sort alphabetically.

[mbonadei, 2017-03-15 13:21:48]

The pre-submit check is a good idea. Let me implement it right away.

https://codereview.webrtc.org/2747863003/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
(right):

https://codereview.webrtc.org/2747863003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:15:
#include <string>
On 2017/03/15 12:02:30, kjellander_webrtc wrote:
> Sort alphabetically.

Done.

[mbonadei, 2017-03-15 14:27:10]

On 2017/03/15 12:02:31, kjellander_webrtc wrote:
> I think it would make sense to implement a PRESUBMIT.py check that is looking
> for usage of the 'google::protobuf::' namespace in C++ source code and raises
an
> error if that's the case. That way we would protect ourselves from future uses
> sneaking back in. That can be done in a follow-up CL though.
> 

I have updated PRESUBMIT.py with a prints a error if 'google::protobuf' is used
outside of 'webrtc/base/protobuf_utils.h'.

PTAL.

[hlundin-webrtc, 2017-03-15 17:11:41]

henrik.lundin@webrtc.org changed reviewers:
+ michaelt@webrtc.org, peah@webrtc.org

[hlundin-webrtc, 2017-03-15 17:11:42]

michaelt: Please, take a look at the ANA changed.
peah: Please, look at APM

Thanks!

[michaelt, 2017-03-16 08:16:25]

https://codereview.webrtc.org/2747863003/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
(right):

https://codereview.webrtc.org/2747863003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:162:
const ProtoString& config_string,
ProtoString is a very good idea. I would like to use it for all the place where
we used just string for a "proto buffer string".
To be able to do this we could use some ifdefs in protobuf_utils.h.
WDYT ?

[mbonadei, 2017-03-16 11:35:53]

On 2017/03/16 08:16:25, michaelt wrote:
>
https://codereview.webrtc.org/2747863003/diff/120001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc
> (right):
> 
>
https://codereview.webrtc.org/2747863003/diff/120001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:162:
> const ProtoString& config_string,
> ProtoString is a very good idea. I would like to use it for all the place
where
> we used just string for a "proto buffer string".
> To be able to do this we could use some ifdefs in protobuf_utils.h.
> WDYT ?

I agree, it is more clean. 

PTAL, I have added a new define to trigger the right 'includes' and 
'using' directives in the protobuf_utils.h file (we define 
WEBRTC_ENABLE_PROTOBUF because it was tricky to depend on
3 different things like ENABLE_RTC_EVENT_LOG, 
WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP and WEBRTC_AUDIOPROC_DEBUG_DUMP).

We always define the type ProtoString when that header is included,
this should cause no problems.

[kjellander_webrtc, 2017-03-16 13:29:55]

Other reviewers: would it make sense to replace some of  ENABLE_RTC_EVENT_LOG, 
WEBRTC_AUDIO_NETWORK_ADAPTOR_DEBUG_DUMP and WEBRTC_AUDIOPROC_DEBUG_DUMP with the
more generic WEBRTC_ENABLE_PROTOBUF define?

Mirko: please link to the separate CL for PRESUBMIT.py in the CL description.
Still lgtm from my perspective.

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:82: if (rtc_enable_protobuf) {
Hmm, I thought we would need to have this target available outside this
condition but as the linux_more_configs bot is passing (it does a build with
rtc_enable_protobuf=false) I guess it's not needed.

Or could it be that some of the targets including the header doesn't have GN
check enabled?

[mbonadei, 2017-03-16 13:35:50]

Description was changed from

==========
To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

BUG=webrtc:7340
==========

to

==========
To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

We moved the PRESUBMIT check to another CL [1]. The goal of this
presubmit is to avoid the direct usage of google::protobuf outside
of the webrtc/base/protobuf_utils.h header file.

[1] - https://codereview.webrtc.org/2753823003/

BUG=webrtc:7340
==========

[michaelt, 2017-03-16 13:39:16]

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h
(right):

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:53: const
ProtoString& config_string,
As in other places, this ProtoString is passed to this function and was before a
std::string. We may could declare the string which is passed to here ProtoString
as well and this all the way up.

[mbonadei, 2017-03-16 14:48:57]

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:82: if (rtc_enable_protobuf) {
On 2017/03/16 13:29:54, kjellander_webrtc wrote:
> Hmm, I thought we would need to have this target available outside this
> condition but as the linux_more_configs bot is passing (it does a build with
> rtc_enable_protobuf=false) I guess it's not needed.
> 
> Or could it be that some of the targets including the header doesn't have GN
> check enabled?

Mmmh... You are right! That's an awesome catch!

I checked the target webrtc/logging:rtc_event_log_impl and it is not triggering
the error even if we always include that header.
I am missing something here. Good that you spotted this. I will follow up.

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h
(right):

https://codereview.webrtc.org/2747863003/diff/180001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:53: const
ProtoString& config_string,
On 2017/03/16 13:39:15, michaelt wrote:
> As in other places, this ProtoString is passed to this function and was before
a
> std::string. We may could declare the string which is passed to here
ProtoString
> as well and this all the way up.

Done.

[peah-webrtc, 2017-03-17 10:43:24]

Hi,
I think the audio_processing/* code looks good but I'm concerned with the change
of int32 to int. Will this be a problem regarding backwards compatibility and
regarding that the format is the same for all platforms?

https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:1878:
msg->set_num_input_channels(static_cast<int>(
Is this backwards compatible? Does this not change the protobuf format? Would it
be safer to cast it to int32_t?

https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_unittest.cc (right):

https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_unittest.cc:62: const int
kChannels[] = {1, 2};
Will this be backwards compatible? Also, can we be sure that int always maps to
the same value (maybe not a problem in protobufs?).?

[mbonadei, 2017-03-20 09:53:39]

The CQ bit was checked by mbonadei@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 09:53:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[mbonadei, 2017-03-20 10:17:26]

On 2017/03/17 10:43:24, peah-webrtc wrote:
> Hi,
> I think the audio_processing/* code looks good but I'm concerned with the
change
> of int32 to int. Will this be a problem regarding backwards compatibility and
> regarding that the format is the same for all platforms?
> 
>
https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/audio_processing_impl.cc (right):
> 
>
https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/audio_processing_impl.cc:1878:
> msg->set_num_input_channels(static_cast<int>(
> Is this backwards compatible? Does this not change the protobuf format? Would
it
> be safer to cast it to int32_t?
> 
>
https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/audio_processing_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2747863003/diff/200001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/audio_processing_unittest.cc:62: const int
> kChannels[] = {1, 2};
> Will this be backwards compatible? Also, can we be sure that int always maps
to
> the same value (maybe not a problem in protobufs?).?

Thanks for the feedback. You are right! It is safer to use int32_t instead of
int.

PTAL. :)

[commit-bot: I haz the power, 2017-03-20 10:23:29]

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

[commit-bot: I haz the power, 2017-03-20 10:23:30]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-03-22 10:03:23]

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/BUILD.gn#newcode106
webrtc/BUILD.gn:106: }
(See the comment about -Wundef.) This would become

  if (rtc_enable_protobuf) {
    defines += [ "WEBRTC_ENABLE_PROTOBUF=1" ]
  } else {
    defines += [ "WEBRTC_ENABLE_PROTOBUF=0" ]
  }

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:85: "protobuf_utils.h",
This file contains #ifdefs on WEBRTC_ENABLE_PROTOBUF, so it seems like it's
intended to be included even when we're not using protobufs. So why is this
build target conditional on us using protobufs? I would have thought that only
the dependency below would be conditional.

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:88: "//third_party/protobuf:protobuf_lite",
Why is this a public dependency?

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/DEPS
File webrtc/base/DEPS (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/DEPS#newcode5
webrtc/base/DEPS:5: "+third_party/protobuf",
Can you have this apply only to a specific file---protobuf_utils.h or whatever
it was called?

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/protobuf_uti...
File webrtc/base/protobuf_utils.h (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/protobuf_uti...
webrtc/base/protobuf_utils.h:16: typedef std::string ProtoString;
using ProtoString = std::string;

How are you making sure that no one uses std::string instead of this alias?

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/protobuf_uti...
webrtc/base/protobuf_utils.h:18: #ifdef WEBRTC_ENABLE_PROTOBUF
When adding new preprocessor define, please always set it to 1 for enable and 0
for disable, and test it with #if instead of #ifdef; that way, the compiler
(which uses -Wundef) will warn you if you forget to set the define in build
targets that need it.

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/protobuf_uti...
webrtc/base/protobuf_utils.h:28: #endif  // WEBRTC_BASE_PROTOBUF_UTILS_H_
The things in this header are put in the root namespace. Why not put them in
e.g. namespace webrtc?

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:88: }
Why would you ever want a conditional dep on protobuf_utils? I thought the idea
was that protobuf_utils / protobuf_utils.h would always be there, but would be
empty if we don't have protobuf support.

In the .h and .cc files, you #include protobuf_utils.h unconditionally. That's
what you should do if you follow my advice, but if you want to do the
conditional deps like you do here, you need to protect those #includes with #if
WEBRTC_PROTOBUF_ENABLED. Which would be ugly... but if you don't do it, gn check
ought to complain if you build with rtc_enable_protobuf=false. (Did you try
that?)

[peah-webrtc, 2017-03-22 11:43:20]

Great! Nice!

lgtm regarding the files
webrtc/modules/audio_processing/test/protobuf_utils.*
webrtc/modules/audio_processing/audio_processing*.*

[mbonadei, 2017-03-23 16:13:59]

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/BUILD.gn#newcode106
webrtc/BUILD.gn:106: }
On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> (See the comment about -Wundef.) This would become
> 
>   if (rtc_enable_protobuf) {
>     defines += [ "WEBRTC_ENABLE_PROTOBUF=1" ]
>   } else {
>     defines += [ "WEBRTC_ENABLE_PROTOBUF=0" ]
>   }

Done.

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:85: "protobuf_utils.h",
On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> This file contains #ifdefs on WEBRTC_ENABLE_PROTOBUF, so it seems like it's
> intended to be included even when we're not using protobufs. So why is this
> build target conditional on us using protobufs? I would have thought that only
> the dependency below would be conditional.

Already explained in the other comment. I was doing the wrong thing.

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:88: "//third_party/protobuf:protobuf_lite",
On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> Why is this a public dependency?

This is a good question.

I tried to avoid the public dependency but in that case I get this error:

../../third_party/protobuf/src/google/protobuf/message_lite.h:42:10: fatal
error: 'google/protobuf/stubs/common.h' file not found
#include <google/protobuf/stubs/common.h>

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/DEPS
File webrtc/base/DEPS (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/DEPS#newcode5
webrtc/base/DEPS:5: "+third_party/protobuf",
On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> Can you have this apply only to a specific file---protobuf_utils.h or whatever
> it was called?

Done.

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/protobuf_uti...
File webrtc/base/protobuf_utils.h (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/protobuf_uti...
webrtc/base/protobuf_utils.h:16: typedef std::string ProtoString;
On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> using ProtoString = std::string;
> 
> How are you making sure that no one uses std::string instead of this alias?

Done.

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/protobuf_uti...
webrtc/base/protobuf_utils.h:18: #ifdef WEBRTC_ENABLE_PROTOBUF
On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> When adding new preprocessor define, please always set it to 1 for enable and
0
> for disable, and test it with #if instead of #ifdef; that way, the compiler
> (which uses -Wundef) will warn you if you forget to set the define in build
> targets that need it.

Done and thanks for pointing this out. I was not aware of this.

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:88: }
On 2017/03/22 10:03:23, kwiberg-webrtc wrote:
> Why would you ever want a conditional dep on protobuf_utils? I thought the
idea
> was that protobuf_utils / protobuf_utils.h would always be there, but would be
> empty if we don't have protobuf support.
> 
> In the .h and .cc files, you #include protobuf_utils.h unconditionally. That's
> what you should do if you follow my advice, but if you want to do the
> conditional deps like you do here, you need to protect those #includes with
#if
> WEBRTC_PROTOBUF_ENABLED. Which would be ugly... but if you don't do it, gn
check
> ought to complain if you build with rtc_enable_protobuf=false. (Did you try
> that?)

Done and thanks for spotting this inconsistency. My bad, I was going in two
opposite directions.

Yes, I tried 'gn check' (both with rtc_enable_protobuf = true and
rtc_enable_protobuf = false) but it was somehow happy with all the conditions on
rtc_enable_protobuf. I guess that if the #include is not part of a defined
target it doesn't complain.

[mbonadei, 2017-03-24 10:33:08]

The CQ bit was checked by mbonadei@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 10:33:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-24 11:02:54]

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

[commit-bot: I haz the power, 2017-03-24 11:02:55]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-03-24 13:02:08]

lgtm; I'd *like* to get rid of the public_dep, but maybe there's genuinely a
good reason for it

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:88: "//third_party/protobuf:protobuf_lite",
On 2017/03/23 16:13:58, mbonadei wrote:
> On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> > Why is this a public dependency?
> 
> This is a good question.
> 
> I tried to avoid the public dependency but in that case I get this error:
> 
> ../../third_party/protobuf/src/google/protobuf/message_lite.h:42:10: fatal
> error: 'google/protobuf/stubs/common.h' file not found
> #include <google/protobuf/stubs/common.h>

public_deps is used to "export" dependencies from the current target to targets
that depend on it; in this case, other build targets can depend on
protobuf_utils instead of having to declare a dependency on protobuf_lite even
though they use stuff from protobuf_lite.

In which target do you get this error? And can it be made to depend directly on
protobuf_lite instead?

[michaelt, 2017-03-28 11:39:55]

lgtm

[mbonadei, 2017-03-30 07:27:27]

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn
File webrtc/base/BUILD.gn (right):

https://codereview.webrtc.org/2747863003/diff/240001/webrtc/base/BUILD.gn#new...
webrtc/base/BUILD.gn:88: "//third_party/protobuf:protobuf_lite",
On 2017/03/24 13:02:07, kwiberg-webrtc wrote:
> On 2017/03/23 16:13:58, mbonadei wrote:
> > On 2017/03/22 10:03:22, kwiberg-webrtc wrote:
> > > Why is this a public dependency?
> > 
> > This is a good question.
> > 
> > I tried to avoid the public dependency but in that case I get this error:
> > 
> > ../../third_party/protobuf/src/google/protobuf/message_lite.h:42:10: fatal
> > error: 'google/protobuf/stubs/common.h' file not found
> > #include <google/protobuf/stubs/common.h>
> 
> public_deps is used to "export" dependencies from the current target to
targets
> that depend on it; in this case, other build targets can depend on
> protobuf_utils instead of having to declare a dependency on protobuf_lite even
> though they use stuff from protobuf_lite.
> 
> In which target do you get this error? And can it be made to depend directly
on
> protobuf_lite instead?

Sorry for the late reply.

I am getting the error on the following two targets:

FAILED:
obj/webrtc/modules/audio_coding/audio_coding_perf_tests/opus_complexity_unittest.o

FAILED:
obj/webrtc/modules/audio_coding/audio_decoder_unittests/audio_decoder_unittest.o

And now that I look carefully they are testing targets.

Ok, let me land this and I will come back with a follow up CL to remove that
public_deps.

[mbonadei, 2017-03-30 07:45:01]

The CQ bit was checked by mbonadei@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 07:45:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-30 08:18:35]

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

[commit-bot: I haz the power, 2017-03-30 08:18:38]

Dry run: This issue passed the CQ dry run.

[mbonadei, 2017-03-30 08:20:54]

Description was changed from

==========
To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

We moved the PRESUBMIT check to another CL [1]. The goal of this
presubmit is to avoid the direct usage of google::protobuf outside
of the webrtc/base/protobuf_utils.h header file.

[1] - https://codereview.webrtc.org/2753823003/

BUG=webrtc:7340
==========

to

==========
To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

We moved the PRESUBMIT check to another CL [1]. The goal of this
presubmit is to avoid the direct usage of google::protobuf outside
of the webrtc/base/protobuf_utils.h header file.

[1] - https://codereview.webrtc.org/2753823003/

BUG=webrtc:7340
NOTRY=True
==========

[mbonadei, 2017-03-30 08:21:10]

The CQ bit was checked by mbonadei@webrtc.org

[mbonadei, 2017-03-30 08:21:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, peah@webrtc.org, kwiberg@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2747863003/#ps300001 (title:
"Rebasing")

[commit-bot: I haz the power, 2017-03-30 08:21:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-30 08:24:21]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1490862070572650,
"parent_rev": "6bfe49c00a1908e3bf05f36c7d6b420817f6304f", "commit_rev":
"16ab93b952f9e8268f2e663ffe49548e8043d5af"}

[commit-bot: I haz the power, 2017-03-30 08:24:27]

Description was changed from

==========
To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

We moved the PRESUBMIT check to another CL [1]. The goal of this
presubmit is to avoid the direct usage of google::protobuf outside
of the webrtc/base/protobuf_utils.h header file.

[1] - https://codereview.webrtc.org/2753823003/

BUG=webrtc:7340
NOTRY=True
==========

to

==========
To accommodate some downstream WebRTC users we need to loosen
the coupling between our code and the //third_party/protobuf.

This includes using typedefs to define strings instead of
assuming std::string.

After this refactoring it will be possible to link with other
protobuf implementations than the current one.

We moved the PRESUBMIT check to another CL [1]. The goal of this
presubmit is to avoid the direct usage of google::protobuf outside
of the webrtc/base/protobuf_utils.h header file.

[1] - https://codereview.webrtc.org/2753823003/

BUG=webrtc:7340
NOTRY=True

Review-Url: https://codereview.webrtc.org/2747863003
Cr-Commit-Position: refs/heads/master@{#17466}
Committed:
https://chromium.googlesource.com/external/webrtc/+/16ab93b952f9e8268f2e663ff...
==========

[commit-bot: I haz the power, 2017-03-30 08:24:28]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/external/webrtc/+/16ab93b952f9e8268f2e663ff...

[mbonadei, 2017-03-31 09:43:02]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.webrtc.org/2786363002/ by mbonadei@webrtc.org.

The reason for reverting is: I will try to reland next week because it is
causing some problems..