Add field_trial_default dependency to libjingle_peerconnection

Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True

Committed: https://crrev.com/a7a01df2aebe7108afad208ccd0341c2f0bc7b3b
Cr-Commit-Position: refs/heads/master@{#13836}

[arlolra, 2016-07-02 00:30:34]

arlolra@gmail.com changed reviewers:
+ pthatcher@webrtc.org

[pthatcher2, 2016-08-16 20:57:35]

pthatcher@chromium.org changed reviewers:
+ kjellander@chromium.org, pthatcher@chromium.org

[pthatcher2, 2016-08-16 20:57:36]

I'm fine with this if kjellander is.

[kjellander_webrtc, 2016-08-16 21:26:01]

Description was changed from

==========
For webrtc::field_trial::FindFullName in peerconnection.cc
==========

to

==========
For webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

[kjellander_webrtc, 2016-08-16 21:26:21]

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

[kjellander_webrtc, 2016-08-16 21:26:22]

This can land with NOTRY=True if the trybots I fired are happy.

https://codereview.webrtc.org/2120673004/diff/1/webrtc/api/api.gyp
File webrtc/api/api.gyp (right):

https://codereview.webrtc.org/2120673004/diff/1/webrtc/api/api.gyp#newcode103
webrtc/api/api.gyp:103:
'<(webrtc_root)/system_wrappers/system_wrappers.gyp:field_trial_default',
I verified FindFullName is invoked in peerconnection.cc.

[kjellander_webrtc, 2016-08-16 21:26:33]

kjellander@webrtc.org changed reviewers:
- kjellander@chromium.org

[kjellander_webrtc, 2016-08-16 21:26:34]

lgtm

[kjellander_webrtc, 2016-08-17 13:40:39]

Description was changed from

==========
For webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

to

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

[kjellander_webrtc, 2016-08-17 13:42:49]

Description was changed from

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

to

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc
==========

[kjellander_webrtc, 2016-08-17 13:42:57]

Actually, this does not lgtm yet, you should do the same for the GN target as
well:
https://cs.chromium.org/chromium/src/third_party/webrtc/api/BUILD.gn?rcl=0&l=201

[arlolra, 2016-08-17 15:50:10]

Thanks!  Hopefully I didn't overstep with the changes there.

[kjellander_webrtc, 2016-08-17 16:49:24]

On 2016/08/17 15:50:10, arlolra wrote:
> Thanks!  Hopefully I didn't overstep with the changes there.

Please revert some of them. See comments.

[arlolra, 2016-08-17 17:16:41]

On 2016/08/17 16:49:24, kjellander_webrtc wrote:
> On 2016/08/17 15:50:10, arlolra wrote:
> > Thanks!  Hopefully I didn't overstep with the changes there.
> 
> Please revert some of them. See comments.

Which comments?  Can you elaborate.

"../system_wrappers:field_trial_default", is redundant with "../call", so should
I omit that?

And is putting the more specific targets as in api.gyp wrong in BUILD.gn?  I
would think these should match up.

[kjellander_webrtc, 2016-08-18 10:22:40]

Sorry! I forgot to post my comment drafts in the last round.

https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (left):

https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn#oldco...
webrtc/api/BUILD.gn:114: "../call",
You're removing the dependency on ../call here. Please keep it around (it's one
of the few targets that don't have a proper 1:1 mapping between GYP+GN.

https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:114: "../media:rtc_media",
This change does nothing, the default group in media includes only rtc_media, so
I'd prefer not touching this line.

https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:115: "../pc:rtc_pc",
Same for this.

[arlolra, 2016-08-20 14:51:13]

On 2016/08/18 10:22:40, kjellander_webrtc wrote:
> Sorry! I forgot to post my comment drafts in the last round.

No problem.  I reverted the unnecessary changes.  Thanks.

[kjellander_webrtc, 2016-08-22 06:21:32]

Description was changed from

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc
==========

to

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

[kjellander_webrtc, 2016-08-22 06:21:51]

On 2016/08/20 14:51:13, arlolra wrote:
> On 2016/08/18 10:22:40, kjellander_webrtc wrote:
> > Sorry! I forgot to post my comment drafts in the last round.
> 
> No problem.  I reverted the unnecessary changes.  Thanks.

Thanks. I'll land this for you.

[kjellander_webrtc, 2016-08-22 06:34:42]

The CQ bit was checked by kjellander@webrtc.org

[commit-bot: I haz the power, 2016-08-22 06:34:46]

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, 2016-08-22 06:34:47]

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

[commit-bot: I haz the power, 2016-08-22 06:34:49]

A disapproval has been posted by following reviewers: kjellander@webrtc.org.
Please make sure to get positive review before another CQ attempt.

[kjellander_webrtc, 2016-08-22 06:38:19]

On 2016/08/22 06:34:49, commit-bot: I haz the power wrote:
> A disapproval has been posted by following reviewers:
mailto:kjellander@webrtc.org.
> Please make sure to get positive review before another CQ attempt.

lgtm

[kjellander_webrtc, 2016-08-22 06:38:26]

The CQ bit was checked by kjellander@webrtc.org

[commit-bot: I haz the power, 2016-08-22 06:38:36]

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, 2016-08-22 06:48:15]

Description was changed from

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

to

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

[commit-bot: I haz the power, 2016-08-22 06:48:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-22 06:48:20]

Description was changed from

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True
==========

to

==========
Add field_trial_default dependency to libjingle_peerconnection

This is needed for webrtc::field_trial::FindFullName in peerconnection.cc

NOTRY=True

Committed: https://crrev.com/a7a01df2aebe7108afad208ccd0341c2f0bc7b3b
Cr-Commit-Position: refs/heads/master@{#13836}
==========

[commit-bot: I haz the power, 2016-08-22 06:48:22]

Patchset 3 (id:??) landed as
https://crrev.com/a7a01df2aebe7108afad208ccd0341c2f0bc7b3b
Cr-Commit-Position: refs/heads/master@{#13836}

[sakal, 2016-08-22 07:25:50]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/2263063002/ by sakal@webrtc.org.

The reason for reverting is: Breaks chromium..

[arlolra, 2016-08-22 14:14:18]

On 2016/08/22 07:25:50, sakal wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.webrtc.org/2263063002/ by mailto:sakal@webrtc.org.
> 
> The reason for reverting is: Breaks chromium..

Well that's sad.

I assume the reason is that,

"../call",
"../system_wrappers:field_trial_default",

is redundant in BUILD.gn

Can we try again w/ just "../call" this time?
Or have I lost too much capital?

[kjellander_webrtc, 2016-08-22 14:31:28]

On 2016/08/22 07:25:50, sakal wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.webrtc.org/2263063002/ by mailto:sakal@webrtc.org.
> 
> The reason for reverting is: Breaks chromium..

sakal: please provide a link to a breakage in the revert message.

Here's what broke:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2...

FAILED: lib_content_browsertests__library.so
lib_content_browsertests__library.so.TOC
lib.unstripped/lib_content_browsertests__library.so 
python
"/mnt/data/b/c/b/Android_Builder__dbg_/src/build/toolchain/gcc_solink_wrapper.py"
--readelf="../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-readelf"
--nm="../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-nm"
--strip=../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-strip
--sofile="./lib.unstripped/lib_content_browsertests__library.so"
--tocfile="./lib_content_browsertests__library.so.TOC"
--output="./lib_content_browsertests__library.so" --
../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++
-shared -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro
-Wl,-z,defs -fuse-ld=gold -Wl,--icf=all -Wl,--build-id=sha1 -Wl,--no-undefined
-Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a
-Wl,--exclude-libs=libvpx_assembly_arm.a -Wl,--warn-shared-textrel -Wl,-O1
-Wl,--as-needed -nostdlib -Wl,--warn-shared-textrel
--sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm 
-Wl,--version-script=/mnt/data/b/c/b/Android_Builder__dbg_/src/build/android/android_no_jni_exports.lst
-Wl,-wrap,calloc -Wl,-wrap,free -Wl,-wrap,malloc -Wl,-wrap,memalign
-Wl,-wrap,posix_memalign -Wl,-wrap,pvalloc -Wl,-wrap,realloc -Wl,-wrap,valloc
-L../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a
-o "./lib.unstripped/lib_content_browsertests__library.so"
-Wl,-soname="lib_content_browsertests__library.so"
@"./lib_content_browsertests__library.so.rsp"
/mnt/data/b/c/b/Android_Builder__dbg_/src/third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold:
error: obj/third_party/libjingle/libjingle.a(field_trial.o): multiple definition
of 'webrtc::field_trial::FindFullName(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&)'
/mnt/data/b/c/b/Android_Builder__dbg_/src/third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold:
obj/third_party/webrtc/system_wrappers/field_trial_default/field_trial_default.o:
previous definition here

https://cs.chromium.org/chromium/src/third_party/libjingle/BUILD.gn?rcl=0&l=70

Ideally we would just wait for further cleanup in
https://bugs.chromium.org/p/webrtc/issues/detail?id=4256 to happen before
attempting to reland this. Or if you can figure out another way to avoid
breaking Chromium?

[arlolra, 2016-08-22 15:21:28]

Isn't the solution just to remove,

```
} else {
    # Otherwise, we just add the field_trial which redirects to base.
    sources = [
      "../webrtc_overrides/field_trial.cc",
    ]
}
```

here
https://cs.chromium.org/chromium/src/third_party/libjingle/BUILD.gn?rcl=0&l=70

since the symbol is already defined with this patch?

That seems to work for the `is_nacl` case.

[kjellander_webrtc, 2016-10-12 03:16:42]

On 2016/08/22 15:21:28, arlolra wrote:
> Isn't the solution just to remove,
> 
> ```
> } else {
>     # Otherwise, we just add the field_trial which redirects to base.
>     sources = [
>       "../webrtc_overrides/field_trial.cc",
>     ]
> }
> ```
> 
> here
> https://cs.chromium.org/chromium/src/third_party/libjingle/BUILD.gn?rcl=0&l=70
> 
> since the symbol is already defined with this patch?
> 
> That seems to work for the `is_nacl` case.

I wasn't fully aware of how the field trial stuff worked before. The idea is
that you provide either your own implementation (to allow hooking up your own
statistics) or you link with one of the default implementations, which is not
linked by default for any of our production code (to provide the flexibility to
downstream users). This is also being reworked a little recently in
https://codereview.webrtc.org/2403463002/.
I'm afraid this is not well documented, which is also something that could be
improved.