Implementing RTC debugging session objects (client and server parts).

Implementing RTC debugging session objects (client and server parts).

Pair of ClientSession and ServerSession holds WebRTC connection which tunnels DevTools UNIX socket.

Once cloud based signaling channel doesn't exist at the moment connections work on a single android device. For manual testing the testing APK has launch activity which lets to start service which tunnels Chrome Shell socket to another local socket (only works when Chrome Shell signed with the same certificate).

BUG=383418
TEST=Automatic: LocalSessionBridgeTest, SessionControlMessagesTest. See description for manual testing.
R=mnaganov@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/a25a26c53da3c9ee6bce24cf5ce9a60e81d97c39

[SeRya, 2014-09-05 15:23:22]

Patchset #1 (id:1) has been deleted

[SeRya, 2014-09-15 15:42:23]

serya@chromium.org changed reviewers:
+ mnaganov@chromium.org

[SeRya, 2014-09-23 18:04:38]

Patchset #3 (id:60001) has been deleted

[SeRya, 2014-09-25 07:56:21]

PTAL.

This patch depends on https://codereview.chromium.org/551793003/

[mnaganov (inactive), 2014-10-01 09:33:57]

LGTM % typos

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java
(right):

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java:109:
callback.onFailure("Concurent requests detected");
typo: Concurrent

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java:116:
// Give libjingle some time for gatering ice candidates.
typo: gathering

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java:141:
// New ICE candidate may improve connection even if controle channel operable.
typo: control

[SeRya, 2014-10-02 11:30:20]

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java
(right):

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java:109:
callback.onFailure("Concurent requests detected");
On 2014/10/01 09:33:57, mnaganov (cr) wrote:
> typo: Concurrent

Done.

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java:116:
// Give libjingle some time for gatering ice candidates.
On 2014/10/01 09:33:57, mnaganov (cr) wrote:
> typo: gathering

Done.

https://codereview.chromium.org/537253003/diff/80001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ServerSession.java:141:
// New ICE candidate may improve connection even if controle channel operable.
On 2014/10/01 09:33:57, mnaganov (cr) wrote:
> typo: control

Done.

[SeRya, 2014-10-10 09:07:42]

serya@chromium.org changed reviewers:
+ yfriedman@chromium.org

[SeRya, 2014-10-10 09:07:43]

Yaron, please review changes in:
build/all.gyp
build/android/findbugs_filter/findbugs_known_bugs.txt

I'm adding known bugs because I'm making use of third party code (WebRTC Java)
which constructs java objects in C++ so some fields look uninitialized for
findbugs.

[Yaron, 2014-10-14 20:02:54]

https://codereview.chromium.org/537253003/diff/230001/build/android/findbugs_...
File build/android/findbugs_filter/findbugs_known_bugs.txt (right):

https://codereview.chromium.org/537253003/diff/230001/build/android/findbugs_...
build/android/findbugs_filter/findbugs_known_bugs.txt:25: M D NP: Read of
unwritten public or protected field data in
org.chromium.components.devtools_bridge.SessionDependencyFactory$DataChannelObserverAdapter.onMessage(DataChannel$Buffer)
 At SessionDependencyFactory.java
Seems like you can just fix these by exposing a getter. Also, can you point to
how/where they're written in native? I don't see it.

https://codereview.chromium.org/537253003/diff/230001/components/devtools_bri...
File components/devtools_bridge.gyp (right):

https://codereview.chromium.org/537253003/diff/230001/components/devtools_bri...
components/devtools_bridge.gyp:8: 'target_name': 'devtools_bridge_javalib',
Somewhat unrelated: could you add a DEPS rule to prevent this dependency from
chrome/. It seems like this is intended as a standalone apk, right? We should
have some add'l discussion if this would be bundled.

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

https://codereview.chromium.org/537253003/diff/230001/build/android/findbugs_...
File build/android/findbugs_filter/findbugs_known_bugs.txt (right):

https://codereview.chromium.org/537253003/diff/230001/build/android/findbugs_...
build/android/findbugs_filter/findbugs_known_bugs.txt:25: M D NP: Read of
unwritten public or protected field data in
org.chromium.components.devtools_bridge.SessionDependencyFactory$DataChannelObserverAdapter.onMessage(DataChannel$Buffer)
 At SessionDependencyFactory.java
On 2014/10/14 20:02:54, Yaron wrote:
> Seems like you can just fix these by exposing a getter. Also, can you point to
> how/where they're written in native? I don't see it.

The "data" field initialized in the constructor
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...).

Constructor called from native code:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin....

Unfortunately adding a getter to WebRTC public API is problematic. Other
applications already use field access.

https://codereview.chromium.org/537253003/diff/230001/components/devtools_bri...
File components/devtools_bridge.gyp (right):

https://codereview.chromium.org/537253003/diff/230001/components/devtools_bri...
components/devtools_bridge.gyp:8: 'target_name': 'devtools_bridge_javalib',
On 2014/10/14 20:02:54, Yaron wrote:
> Somewhat unrelated: could you add a DEPS rule to prevent this dependency from
> chrome/. It seems like this is intended as a standalone apk, right? We should
> have some add'l discussion if this would be bundled.

In two words: it's supposed to be eventually bundled in Clank but without
dependency on libjingle_peerconnection_javalib (the SessionDependencyFactory
class specifically designated to make this transition easier). I forwarded you
email thread with discussion of the implementation plan.

Didn't get which dependency from chrome/ you are talking about and how DEPS rule
may help. Please explain.

[Yaron, 2014-10-15 19:15:40]

On Wed Oct 15 2014 at 2:05:50 AM <serya@chromium.org> wrote:

>
> https://codereview.chromium.org/537253003/diff/230001/
> build/android/findbugs_filter/findbugs_known_bugs.txt
> File build/android/findbugs_filter/findbugs_known_bugs.txt (right):
>
> https://codereview.chromium.org/537253003/diff/230001/
> build/android/findbugs_filter/findbugs_known_bugs.txt#newcode25
> build/android/findbugs_filter/findbugs_known_bugs.txt:25
>
<https://codereview.chromium.org/537253003/diff/230001/build/android/findbugs_...>:
> M D NP: Read
> of unwritten public or protected field data in
> org.chromium.components.devtools_bridge.SessionDependencyFactory$
> DataChannelObserverAdapter.onMessage(DataChannel$Buffer)
>   At SessionDependencyFactory.java
> On 2014/10/14 20:02:54, Yaron wrote:
> > Seems like you can just fix these by exposing a getter. Also, can you
> point to
> > how/where they're written in native? I don't see it.
>
> The "data" field initialized in the constructor
> (https://code.google.com/p/chromium/codesearch#chromium/
> src/third_party/libjingle/source/talk/app/webrtc/java/
> src/org/webrtc/DataChannel.java&l=73).
>
> Constructor called from native code:
> https://code.google.com/p/chromium/codesearch#chromium/
> src/third_party/libjingle/source/talk/app/webrtc/java/
> jni/peerconnection_jni.cc&l=946.
>

Thanks. Btw, I'd recommend using chromium's jni_generator if you can. It's
safer than using hand-coded JNI bindings.

>
> Unfortunately adding a getter to WebRTC public API is problematic. Other
> applications already use field access.
>
> https://codereview.chromium.org/537253003/diff/230001/
> components/devtools_bridge.gyp
> File components/devtools_bridge.gyp (right):
>
> https://codereview.chromium.org/537253003/diff/230001/
> components/devtools_bridge.gyp#newcode8
> components/devtools_bridge.gyp:8
>
<https://codereview.chromium.org/537253003/diff/230001/components/devtools_bri...>:
> 'target_name':
> 'devtools_bridge_javalib',
> On 2014/10/14 20:02:54, Yaron wrote:
> > Somewhat unrelated: could you add a DEPS rule to prevent this
> dependency from
> > chrome/. It seems like this is intended as a standalone apk, right? We
> should
> > have some add'l discussion if this would be bundled.
>
> In two words: it's supposed to be eventually bundled in Clank but
> without dependency on libjingle_peerconnection_javalib (the
> SessionDependencyFactory class specifically designated to make this
> transition easier). I forwarded you email thread with discussion of the
> implementation plan.
>
> Didn't get which dependency from chrome/ you are talking about and how
> DEPS rule may help. Please explain.
>

See how chrome/DEPS has some "-" rules. I was suggesting that we could have
chrome/browser/android have a DEPS rule with a "-" to prevent this
dependency.

lgtm with that change


>
> https://codereview.chromium.org/537253003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[SeRya, 2014-10-16 08:42:56]

On 2014/10/15 19:15:40, Yaron wrote:
> On Wed Oct 15 2014 at 2:05:50 AM <mailto:serya@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/537253003/diff/230001/
> > build/android/findbugs_filter/findbugs_known_bugs.txt
> > File build/android/findbugs_filter/findbugs_known_bugs.txt (right):
> >
> > https://codereview.chromium.org/537253003/diff/230001/
> > build/android/findbugs_filter/findbugs_known_bugs.txt#newcode25
> > build/android/findbugs_filter/findbugs_known_bugs.txt:25
> >
>
<https://codereview.chromium.org/537253003/diff/230001/build/android/findbugs_...>:
> > M D NP: Read
> > of unwritten public or protected field data in
> > org.chromium.components.devtools_bridge.SessionDependencyFactory$
> > DataChannelObserverAdapter.onMessage(DataChannel$Buffer)
> >   At SessionDependencyFactory.java
> > On 2014/10/14 20:02:54, Yaron wrote:
> > > Seems like you can just fix these by exposing a getter. Also, can you
> > point to
> > > how/where they're written in native? I don't see it.
> >
> > The "data" field initialized in the constructor
> > (https://code.google.com/p/chromium/codesearch#chromium/
> > src/third_party/libjingle/source/talk/app/webrtc/java/
> > src/org/webrtc/DataChannel.java&l=73).
> >
> > Constructor called from native code:
> > https://code.google.com/p/chromium/codesearch#chromium/
> > src/third_party/libjingle/source/talk/app/webrtc/java/
> > jni/peerconnection_jni.cc&l=946.
> >
> 
> Thanks. Btw, I'd recommend using chromium's jni_generator if you can. It's
> safer than using hand-coded JNI bindings.

I'm planing use it. But with third-party code I have limited choice.

> >
> > Unfortunately adding a getter to WebRTC public API is problematic. Other
> > applications already use field access.
> >
> > https://codereview.chromium.org/537253003/diff/230001/
> > components/devtools_bridge.gyp
> > File components/devtools_bridge.gyp (right):
> >
> > https://codereview.chromium.org/537253003/diff/230001/
> > components/devtools_bridge.gyp#newcode8
> > components/devtools_bridge.gyp:8
> >
>
<https://codereview.chromium.org/537253003/diff/230001/components/devtools_bri...>:
> > 'target_name':
> > 'devtools_bridge_javalib',
> > On 2014/10/14 20:02:54, Yaron wrote:
> > > Somewhat unrelated: could you add a DEPS rule to prevent this
> > dependency from
> > > chrome/. It seems like this is intended as a standalone apk, right? We
> > should
> > > have some add'l discussion if this would be bundled.
> >
> > In two words: it's supposed to be eventually bundled in Clank but
> > without dependency on libjingle_peerconnection_javalib (the
> > SessionDependencyFactory class specifically designated to make this
> > transition easier). I forwarded you email thread with discussion of the
> > implementation plan.
> >
> > Didn't get which dependency from chrome/ you are talking about and how
> > DEPS rule may help. Please explain.
> >
> 
> See how chrome/DEPS has some "-" rules. I was suggesting that we could have
> chrome/browser/android have a DEPS rule with a "-" to prevent this
> dependency.
> 
> lgtm with that change

Added DEPS rule like in google_apis/DEPS.

> >
> > https://codereview.chromium.org/537253003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[SeRya, 2014-10-16 08:48:25]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-16 08:49:32]

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

[commit-bot: I haz the power, 2014-10-16 08:55:30]

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

[commit-bot: I haz the power, 2014-10-16 08:55: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-16 09:22:59]

Patchset #8 (id:450001) has been deleted

[SeRya, 2014-10-16 09:31:44]

Patchset #8 (id:470001) has been deleted

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

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

[SeRya, 2014-10-16 09:37:04]

Committed patchset #8 (id:490001) manually as
a25a26c53da3c9ee6bce24cf5ce9a60e81d97c39.

[Yaron, 2014-10-16 15:58:17]

On 2014/10/16 08:42:56, SeRya wrote:
> On 2014/10/15 19:15:40, Yaron wrote:
> > On Wed Oct 15 2014 at 2:05:50 AM <mailto:serya@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/537253003/diff/230001/
> > > build/android/findbugs_filter/findbugs_known_bugs.txt
> > > File build/android/findbugs_filter/findbugs_known_bugs.txt (right):
> > >
> > > https://codereview.chromium.org/537253003/diff/230001/
> > > build/android/findbugs_filter/findbugs_known_bugs.txt#newcode25
> > > build/android/findbugs_filter/findbugs_known_bugs.txt:25
> > >
> >
>
<https://codereview.chromium.org/537253003/diff/230001/build/android/findbugs_...>:
> > > M D NP: Read
> > > of unwritten public or protected field data in
> > > org.chromium.components.devtools_bridge.SessionDependencyFactory$
> > > DataChannelObserverAdapter.onMessage(DataChannel$Buffer)
> > >   At SessionDependencyFactory.java
> > > On 2014/10/14 20:02:54, Yaron wrote:
> > > > Seems like you can just fix these by exposing a getter. Also, can you
> > > point to
> > > > how/where they're written in native? I don't see it.
> > >
> > > The "data" field initialized in the constructor
> > > (https://code.google.com/p/chromium/codesearch#chromium/
> > > src/third_party/libjingle/source/talk/app/webrtc/java/
> > > src/org/webrtc/DataChannel.java&l=73).
> > >
> > > Constructor called from native code:
> > > https://code.google.com/p/chromium/codesearch#chromium/
> > > src/third_party/libjingle/source/talk/app/webrtc/java/
> > > jni/peerconnection_jni.cc&l=946.
> > >
> > 
> > Thanks. Btw, I'd recommend using chromium's jni_generator if you can. It's
> > safer than using hand-coded JNI bindings.
> 
> I'm planing use it. But with third-party code I have limited choice.
> 
> > >
> > > Unfortunately adding a getter to WebRTC public API is problematic. Other
> > > applications already use field access.
> > >
> > > https://codereview.chromium.org/537253003/diff/230001/
> > > components/devtools_bridge.gyp
> > > File components/devtools_bridge.gyp (right):
> > >
> > > https://codereview.chromium.org/537253003/diff/230001/
> > > components/devtools_bridge.gyp#newcode8
> > > components/devtools_bridge.gyp:8
> > >
> >
>
<https://codereview.chromium.org/537253003/diff/230001/components/devtools_bri...>:
> > > 'target_name':
> > > 'devtools_bridge_javalib',
> > > On 2014/10/14 20:02:54, Yaron wrote:
> > > > Somewhat unrelated: could you add a DEPS rule to prevent this
> > > dependency from
> > > > chrome/. It seems like this is intended as a standalone apk, right? We
> > > should
> > > > have some add'l discussion if this would be bundled.
> > >
> > > In two words: it's supposed to be eventually bundled in Clank but
> > > without dependency on libjingle_peerconnection_javalib (the
> > > SessionDependencyFactory class specifically designated to make this
> > > transition easier). I forwarded you email thread with discussion of the
> > > implementation plan.
> > >
> > > Didn't get which dependency from chrome/ you are talking about and how
> > > DEPS rule may help. Please explain.
> > >
> > 
> > See how chrome/DEPS has some "-" rules. I was suggesting that we could have
> > chrome/browser/android have a DEPS rule with a "-" to prevent this
> > dependency.
> > 
> > lgtm with that change
> 
> Added DEPS rule like in google_apis/DEPS.
>
where? I don't see this 
> > >
> > > https://codereview.chromium.org/537253003/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[SeRya, 2014-10-17 06:45:25]

> > > See how chrome/DEPS has some "-" rules. I was suggesting that we could
have
> > > chrome/browser/android have a DEPS rule with a "-" to prevent this
> > > dependency.
> > > 
> > > lgtm with that change
> > 
> > Added DEPS rule like in google_apis/DEPS.
> >
> where? I don't see this 

https://codereview.chromium.org/537253003/patch/490001/500004

[chromium-reviews, 2014-10-17 18:51:43]

That's not what I meant. That means that components/devtools_bridge cant'
depend on content/ and chrome/ (the latter being redundant since none can).
You need to update the DEPS in chrome/browser

On Thu Oct 16 2014 at 11:45:25 PM <serya@chromium.org> wrote:

> > > > See how chrome/DEPS has some "-" rules. I was suggesting that we
> could
> have
> > > > chrome/browser/android have a DEPS rule with a "-" to prevent this
> > > > dependency.
> > > >
> > > > lgtm with that change
> > >
> > > Added DEPS rule like in google_apis/DEPS.
> > >
> > where? I don't see this
>
> https://codereview.chromium.org/537253003/patch/490001/500004
>
>
> https://codereview.chromium.org/537253003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[SeRya, 2014-10-20 07:58:53]

On 2014/10/17 18:51:43, chromium-reviews wrote:
> That's not what I meant. That means that components/devtools_bridge cant'
> depend on content/ and chrome/ (the latter being redundant since none can).
> You need to update the DEPS in chrome/browser

Oops. Sorry. I'll fix it in a separate CL. It's looks orthogonal anyway.