Making WebRTC Java api avaliable in Chromium.

Making WebRTC Java api avaliable in Chromium.

BUG=383418
TEST=Build target libjingle_peerconnection_javalib

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

Committed: https://crrev.com/0d8b69077ac03b7ea176d9df6f3730fe7a63a499
Cr-Commit-Position: refs/heads/master@{#298073}

Committed: https://crrev.com/6e2bfbb7098fe1a30b8b6625dd0aa023b0e08686
Cr-Commit-Position: refs/heads/master@{#298819}

[SeRya, 2014-09-08 14:30:01]

Patchset #1 (id:1) has been deleted

[hellner1, 2014-09-10 17:16:34]

hellner@chromium.org changed reviewers:
+ hellner@chromium.org

[hellner1, 2014-09-10 17:16:34]

I'm currently working on migrating libjingle to webrtc. Part of that is getting
rid of libjingle.gyp. Ideally I would like to avoid adding more stuff to it.

The libjingle_peerconnection.gypi-file seems like an orthogonal change. Since
libjingle.gyp is being deprecated I don't think there is any need to
re-factor/try to improve it.

[SeRya, 2014-09-11 06:59:12]

On 2014/09/10 17:16:34, hellner1 wrote:
> I'm currently working on migrating libjingle to webrtc. Part of that is
getting
> rid of libjingle.gyp. Ideally I would like to avoid adding more stuff to it.
> 
> The libjingle_peerconnection.gypi-file seems like an orthogonal change. Since
> libjingle.gyp is being deprecated I don't think there is any need to
> re-factor/try to improve it.

Purpose of libjingle_peerconnection.gypi is following.

libjingle_peerconnection_so needs code declared in libpeerconnection but
libpeerconnection could be either static or shared library depending on
libpeer_target_type. If libpeer_target_type == "static_library" it's OK. If it's
a shared_library then libjingle_peerconnection_so won't build properly.

I see 2 ways to address it:
1. Introduce a common predecessor (like "libpeerconnection_static" which is
statatic_library) or
2. Compile common code twice sharing it through through gypi-include.

I found the second way more appealing. Does first way looks better for you or
may be you see another alternative?

[hellner1, 2014-09-29 18:21:05]

Sorry for the delay here!

I think it is more consistent with the rest of WebRTC to do as in suggestion 1).
I added kjellander and ajm (cc) to verify my statement here.

https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:660: 'libjingle_webrtc_common',
Do you need "init_webrtc.cc/h" if you instead depend on libjingle_webrtc?

https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:684: 'includes': [ '../../build/java.gypi'
],
This might make it harder get rid of this duplicate gyp-file (see other one
here: https://code.google.com/p/webrtc/source/browse/trunk/talk/libjingle.gyp)
as build/java.gypi might be in a different relative directory in the standalone
build.

[SeRya, 2014-09-30 18:29:02]

https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:660: 'libjingle_webrtc_common',
On 2014/09/29 18:21:05, hellner1 wrote:
> Do you need "init_webrtc.cc/h" if you instead depend on libjingle_webrtc?

Reason for this was webrtc::field_trial::FindFullName referenced in 2-3 files.
But it seems like dependence on system_wrappers.gyp:field_trial_default is
better way to satisfy that need. Fixed.

https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:684: 'includes': [ '../../build/java.gypi'
],
On 2014/09/29 18:21:05, hellner1 wrote:
> This might make it harder get rid of this duplicate gyp-file (see other one
> here: https://code.google.com/p/webrtc/source/browse/trunk/talk/libjingle.gyp)
> as build/java.gypi might be in a different relative directory in the
standalone
> build.

Do you have an idea how to make it easier? It seems '<(DEPTH)/build/java.gypi'
doesn't work.

[hellner1, 2014-09-30 18:49:01]

On 2014/09/30 18:29:02, SeRya wrote:
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> File third_party/libjingle/libjingle.gyp (right):
> 
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> third_party/libjingle/libjingle.gyp:660: 'libjingle_webrtc_common',
> On 2014/09/29 18:21:05, hellner1 wrote:
> > Do you need "init_webrtc.cc/h" if you instead depend on libjingle_webrtc?
> 
> Reason for this was webrtc::field_trial::FindFullName referenced in 2-3 files.
> But it seems like dependence on system_wrappers.gyp:field_trial_default is
> better way to satisfy that need. Fixed.
> 
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> third_party/libjingle/libjingle.gyp:684: 'includes': [ '../../build/java.gypi'
> ],
> On 2014/09/29 18:21:05, hellner1 wrote:
> > This might make it harder get rid of this duplicate gyp-file (see other one
> > here:
https://code.google.com/p/webrtc/source/browse/trunk/talk/libjingle.gyp)
> > as build/java.gypi might be in a different relative directory in the
> standalone
> > build.
> 
> Do you have an idea how to make it easier? It seems '<(DEPTH)/build/java.gypi'
> doesn't work.
That's because gyp's include is done in the first pass in which variables aren't
expanded. You can work around this by turning your include into a dependency as
done e.g. here:
https://code.google.com/p/webrtc/source/detail?spec=svn4294&r=4294. I'm not
suggesting you do it here though. I will have to do it when moving your project
to a shared (by standalone and Chromium) gyp file.

[SeRya, 2014-10-01 18:10:42]

On 2014/09/30 18:49:01, hellner1 wrote:
> On 2014/09/30 18:29:02, SeRya wrote:
> >
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> > File third_party/libjingle/libjingle.gyp (right):
> > 
> >
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> > third_party/libjingle/libjingle.gyp:660: 'libjingle_webrtc_common',
> > On 2014/09/29 18:21:05, hellner1 wrote:
> > > Do you need "init_webrtc.cc/h" if you instead depend on libjingle_webrtc?
> > 
> > Reason for this was webrtc::field_trial::FindFullName referenced in 2-3
files.
> > But it seems like dependence on system_wrappers.gyp:field_trial_default is
> > better way to satisfy that need. Fixed.
> > 
> >
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> > third_party/libjingle/libjingle.gyp:684: 'includes': [
'../../build/java.gypi'
> > ],
> > On 2014/09/29 18:21:05, hellner1 wrote:
> > > This might make it harder get rid of this duplicate gyp-file (see other
one
> > > here:
> https://code.google.com/p/webrtc/source/browse/trunk/talk/libjingle.gyp)
> > > as build/java.gypi might be in a different relative directory in the
> > standalone
> > > build.
> > 
> > Do you have an idea how to make it easier? It seems
'<(DEPTH)/build/java.gypi'
> > doesn't work.
> That's because gyp's include is done in the first pass in which variables
aren't
> expanded. You can work around this by turning your include into a dependency
as
> done e.g. here:
> https://code.google.com/p/webrtc/source/detail?spec=svn4294&r=4294. I'm not
> suggesting you do it here though. I will have to do it when moving your
project
> to a shared (by standalone and Chromium) gyp file.

Any new consern?

[hellner1, 2014-10-01 18:30:34]

I pinged ajm and kjellander for second opinion.

[kjellander_chromium, 2014-10-01 18:40:59]

On 2014/10/01 18:10:42, SeRya wrote:
> On 2014/09/30 18:49:01, hellner1 wrote:
> > On 2014/09/30 18:29:02, SeRya wrote:
> > >
> >
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> > > File third_party/libjingle/libjingle.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> > > third_party/libjingle/libjingle.gyp:660: 'libjingle_webrtc_common',
> > > On 2014/09/29 18:21:05, hellner1 wrote:
> > > > Do you need "init_webrtc.cc/h" if you instead depend on
libjingle_webrtc?
> > > 
> > > Reason for this was webrtc::field_trial::FindFullName referenced in 2-3
> files.
> > > But it seems like dependence on system_wrappers.gyp:field_trial_default is
> > > better way to satisfy that need. Fixed.
> > > 
> > >
> >
>
https://codereview.chromium.org/551793003/diff/20001/third_party/libjingle/li...
> > > third_party/libjingle/libjingle.gyp:684: 'includes': [
> '../../build/java.gypi'
> > > ],
> > > On 2014/09/29 18:21:05, hellner1 wrote:
> > > > This might make it harder get rid of this duplicate gyp-file (see other
> one
> > > > here:
> > https://code.google.com/p/webrtc/source/browse/trunk/talk/libjingle.gyp)
> > > > as build/java.gypi might be in a different relative directory in the
> > > standalone
> > > > build.
> > > 
> > > Do you have an idea how to make it easier? It seems
> '<(DEPTH)/build/java.gypi'
> > > doesn't work.
> > That's because gyp's include is done in the first pass in which variables
> aren't
> > expanded. You can work around this by turning your include into a dependency
> as
> > done e.g. here:
> > https://code.google.com/p/webrtc/source/detail?spec=svn4294&r=4294. I'm not
> > suggesting you do it here though. I will have to do it when moving your
> project
> > to a shared (by standalone and Chromium) gyp file.
> 
> Any new consern?

If hellner@ is fine with doing this refactoring during his libjingle.gyp merge
work I'm okay with this for now.

[kjellander_chromium, 2014-10-01 18:44:09]

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

[kjellander_chromium, 2014-10-01 18:44:10]

lgtm with a few comments. I prefer hellner does the main review though.

https://codereview.chromium.org/551793003/diff/60001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/551793003/diff/60001/build/all.gyp#newcode832
build/all.gyp:832:
'../third_party/libjingle/libjingle.gyp:libjingle_peerconnection_javalib',
Move this line up above the comment since it's not a test apk.

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/li...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:608: # TODO(serya): Termpraty workaround for
crbug.com/418915. Remove when fixed.
nit: Termpraty -> Temporary.

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:679: '<(DEPTH)/third_party/libyuv/include',
Is this really needed?

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/ov...
File third_party/libjingle/overrides/empty.cc (right):

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/ov...
third_party/libjingle/overrides/empty.cc:5: // TODO(serya): Temporary workaround
for https://crbug.com/418915. To be removed when fixed.
nit: 80 chars max line length.

[SeRya, 2014-10-01 21:05:17]

https://codereview.chromium.org/551793003/diff/60001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/551793003/diff/60001/build/all.gyp#newcode832
build/all.gyp:832:
'../third_party/libjingle/libjingle.gyp:libjingle_peerconnection_javalib',
On 2014/10/01 18:44:10, kjellander wrote:
> Move this line up above the comment since it's not a test apk.

Done.

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/li...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:608: # TODO(serya): Termpraty workaround for
crbug.com/418915. Remove when fixed.
On 2014/10/01 18:44:10, kjellander wrote:
> nit: Termpraty -> Temporary.

Done.

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/li...
third_party/libjingle/libjingle.gyp:679: '<(DEPTH)/third_party/libyuv/include',
On 2014/10/01 18:44:10, kjellander wrote:
> Is this really needed?

Done.

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/ov...
File third_party/libjingle/overrides/empty.cc (right):

https://codereview.chromium.org/551793003/diff/60001/third_party/libjingle/ov...
third_party/libjingle/overrides/empty.cc:5: // TODO(serya): Temporary workaround
for https://crbug.com/418915. To be removed when fixed.
On 2014/10/01 18:44:10, kjellander wrote:
> nit: 80 chars max line length.

Done.

[ajm, 2014-10-02 00:47:51]

I don't have a strong opinion on this. I'll defer my part to kjellander
(thanks!).

[hellner1, 2014-10-02 01:48:59]

lgtm

[SeRya, 2014-10-02 04:19:48]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-02 04:20:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/100001

[commit-bot: I haz the power, 2014-10-02 04:23:52]

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

[commit-bot: I haz the power, 2014-10-02 04:23:54]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/63349)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[SeRya, 2014-10-02 07:26:22]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-02 07:26:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/100001

[commit-bot: I haz the power, 2014-10-02 07:34:15]

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

[commit-bot: I haz the power, 2014-10-02 07:34:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[SeRya, 2014-10-02 08:36:13]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-02 08:37:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/100001

[commit-bot: I haz the power, 2014-10-02 08:45:31]

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

[commit-bot: I haz the power, 2014-10-02 08:45:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[SeRya, 2014-10-02 10:07:05]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-02 10:07:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/100001

[commit-bot: I haz the power, 2014-10-02 11:05:53]

Committed patchset #5 (id:100001) as ead41d1c4c1e717e18bbc2ca799ba51504384502

[commit-bot: I haz the power, 2014-10-02 11:07:15]

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

[henrika (OOO until Aug 14), 2014-10-02 12:38:30]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/614263007/ by henrika@chromium.org.

The reason for reverting is: This CL breaks official Chrome..

[SeRya, 2014-10-03 09:24:05]

Looks like all problems gets solves if we exclude new targets when
libpeer_target_type != static_library.

Tommi said android doesn't use it anyway. To make sure and to support gyp
overrides I added specific condition in all.gyp.

BY the way, it compiles if libpeer_target_type == loadable_module but fails at
runtime.

[hellner1, 2014-10-03 17:53:11]

lgtm

[SeRya, 2014-10-03 17:54:11]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-03 17:55:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/120001

[commit-bot: I haz the power, 2014-10-03 19:18:39]

Committed patchset #6 (id:120001) as c4f209cee2fa7b83ac137d0fffe3abafe91aa0d8

[commit-bot: I haz the power, 2014-10-03 19:19:23]

Patchset 6 (id:??) landed as
https://crrev.com/0d8b69077ac03b7ea176d9df6f3730fe7a63a499
Cr-Commit-Position: refs/heads/master@{#298073}

[Niklas Enbom, 2014-10-03 23:24:42]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/627853002/ by niklase@chromium.org.

The reason for reverting is: Seems to break official builds on win, speculative
revert..

[mikhal1, 2014-10-03 23:28:54]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/620373003/ by mikhal@chromium.org.

The reason for reverting is: Speculative revert, breaking official win
builders..

[hellner1, 2014-10-03 23:50:30]

On 2014/10/03 23:28:54, mikhal1 wrote:
> A revert of this CL (patchset #6 id:120001) has been created in
> https://codereview.chromium.org/620373003/ by mailto:mikhal@chromium.org.
> 
> The reason for reverting is: Speculative revert, breaking official win
> builders..

Confirmed that it breaks things (and thus the revert was needed):
https://code.google.com/p/chromium/issues/detail?id=420293

[SeRya, 2014-10-08 18:25:00]

On 2014/10/03 23:50:30, hellner1 wrote:
> On 2014/10/03 23:28:54, mikhal1 wrote:
> > A revert of this CL (patchset #6 id:120001) has been created in
> > https://codereview.chromium.org/620373003/ by mailto:mikhal@chromium.org.
> > 
> > The reason for reverting is: Speculative revert, breaking official win
> > builders..
> 
> Confirmed that it breaks things (and thus the revert was needed):
> https://code.google.com/p/chromium/issues/detail?id=420293

Relanding since the fix https://webrtc-codereview.appspot.com/29659004 landed
and propagated to chromium. Checked locally on clankium.

[SeRya, 2014-10-08 19:07:21]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-08 19:09:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/140001

[commit-bot: I haz the power, 2014-10-09 03:47:12]

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

[commit-bot: I haz the power, 2014-10-09 03:47:13]

Exceeded time limit waiting for builds to trigger.

[SeRya, 2014-10-09 05:15:25]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-09 05:16:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/140001

[commit-bot: I haz the power, 2014-10-09 05:18:23]

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

[commit-bot: I haz the power, 2014-10-09 05:18:25]

Failed to commit the patch.

[commit-bot: I haz the power, 2014-10-09 05:19:14]

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

[SeRya, 2014-10-09 07:06:31]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-09 07:08:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551793003/140001

[commit-bot: I haz the power, 2014-10-09 07:10:38]

Committed patchset #7 (id:140001) as 55cf1f0a3d9877e91ff8dc7cdef038aac08e9825

[commit-bot: I haz the power, 2014-10-09 07:11:34]

Patchset 7 (id:??) landed as
https://crrev.com/6e2bfbb7098fe1a30b8b6625dd0aa023b0e08686
Cr-Commit-Position: refs/heads/master@{#298819}