Continue impementation SessionDependencyFactoryNative.

Partially implemented native SessionDependencyFactory (PeerConnection works, DataChannel postponed).

JNI conversions separated from the WebRTC code.

BUG=383418
R=mnaganov@chromium.org
TBR=darin@chromium.org, yfriedman@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/24f61abef70c81d7529e8f908d7beeea48f40641

[SeRya, 2014-11-10 08:05:05]

Patchset #1 (id:1) has been deleted

[SeRya, 2014-11-10 09:21:44]

Patchset #1 (id:20001) has been deleted

[SeRya, 2014-11-10 09:21:52]

Patchset #1 (id:40001) has been deleted

[SeRya, 2014-11-10 09:22:15]

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

[SeRya, 2014-11-10 09:22:16]

PTAL

[mnaganov (inactive), 2014-11-10 18:10:26]

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java
(right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java:45:
private final long mPtr;
I would suggest using a more descriptive name, e.g. mConnectionPtr.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java:100:
private final long mPtr;
and here -- mChannelPtr, perhaps?

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/android/session_dependency_factory_android.cc
(right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/android/session_dependency_factory_android.cc:20:
class JNIInvokeUtil {
We don't usually use such helper classes: this one and JNIHandlingUtil below.
They may save some typing effort, but they definitely don't add clarity, because
now you need to guess what "cast" or "convert" means. You may instead use "using
base::android::ConvertUTF8ToJavaString" in order to make names shorter.

Existing code have explicit local vars for JNIEnv, uses casts directly in
places, where they are needed, and uses ConvertUTF8ToJavaString directly. Take a
look at this class, for example:
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/rtc_configuration.h (right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/rtc_configuration.h:8: #import <string>
Why #import?

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/rtc_configuration.h:29: virtual Impl const& impl()
const = 0;
Shouldn't it be "const& Impl" instead?

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/rtc_configuration.h:33: private:
Bad indentation.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/session_dependency_factory.cc (right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:34: Impl const& impl()
const override {
Also "const& Impl"

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:141: :
signaling_thread_(signaling_thread),
I think indentation is a bit odd here.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/session_dependency_factory.h (right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.h:8: #import <string>
#import again. I thought this is some Objective-C thing.

[SeRya, 2014-11-11 08:07:05]

Patchset #2 (id:80001) has been deleted

[SeRya, 2014-11-11 08:08:19]

Commends addressed.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java
(right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java:45:
private final long mPtr;
On 2014/11/10 18:10:25, mnaganov (cr) wrote:
> I would suggest using a more descriptive name, e.g. mConnectionPtr.

Done.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java:100:
private final long mPtr;
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> and here -- mChannelPtr, perhaps?

Done.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/android/session_dependency_factory_android.cc
(right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/android/session_dependency_factory_android.cc:20:
class JNIInvokeUtil {
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> We don't usually use such helper classes: this one and JNIHandlingUtil below.
> They may save some typing effort, but they definitely don't add clarity,
because
> now you need to guess what "cast" or "convert" means. You may instead use
"using
> base::android::ConvertUTF8ToJavaString" in order to make names shorter.
> 
> Existing code have explicit local vars for JNIEnv, uses casts directly in
> places, where they are needed, and uses ConvertUTF8ToJavaString directly. Take
a
> look at this class, for example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...

Done.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/rtc_configuration.h (right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/rtc_configuration.h:8: #import <string>
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> Why #import?

Apparently java-reflex. Fixed here and in other places. Surprised it worked just
fine.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/rtc_configuration.h:29: virtual Impl const& impl()
const = 0;
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> Shouldn't it be "const& Impl" instead?

Checked in the styleguide. It requires "const Type&" instead of "Type const&".
Fixed everywhere.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/rtc_configuration.h:33: private:
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> Bad indentation.

Done.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/session_dependency_factory.cc (right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:34: Impl const& impl()
const override {
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> Also "const& Impl"

Done.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.cc:141: :
signaling_thread_(signaling_thread),
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> I think indentation is a bit odd here.

Done.

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
File components/devtools_bridge/session_dependency_factory.h (right):

https://codereview.chromium.org/716433003/diff/60001/components/devtools_brid...
components/devtools_bridge/session_dependency_factory.h:8: #import <string>
On 2014/11/10 18:10:26, mnaganov (cr) wrote:
> #import again. I thought this is some Objective-C thing.

Done.

[mnaganov (inactive), 2014-11-11 17:11:47]

LGTM % comments

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java
(right):

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java:109:
throw new RuntimeException("Not implemented");
I think you should throw UnsupportedOperationException instead.

See e.g.
https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22Not%20implem...

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
File components/devtools_bridge/session_dependency_factory.cc (right):

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
components/devtools_bridge/session_dependency_factory.cc:126:
AbstractPeerConnection::Delegate* const delegate_;
It should be "const Type", not "Type const". Here and the classes below.

[SeRya, 2014-11-12 08:25:06]

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

[SeRya, 2014-11-12 08:25:08]

+darin@chromium.org for components/devtools_bridge/DEPS
+yfriedman@chromium.org for
build/android/findbugs_filter/findbugs_known_bugs.txt

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java
(right):

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java:109:
throw new RuntimeException("Not implemented");
On 2014/11/11 17:11:47, mnaganov (cr) wrote:
> I think you should throw UnsupportedOperationException instead.
> 
> See e.g.
>
https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22Not%20implem...

Done.

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
File components/devtools_bridge/session_dependency_factory.cc (right):

https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
components/devtools_bridge/session_dependency_factory.cc:126:
AbstractPeerConnection::Delegate* const delegate_;
On 2014/11/11 17:11:47, mnaganov (cr) wrote:
> It should be "const Type", not "Type const". Here and the classes below.

This is not the case: "Type* const" is not like "Type const". For pointers we
use formatting like this (for instance -
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...).

However there are cases "Type const" (not only in this file). Fixed.

[mnaganov (inactive), 2014-11-12 18:52:37]

On 2014/11/12 08:25:08, SeRya wrote:
> mailto:+darin@chromium.org for components/devtools_bridge/DEPS
> mailto:+yfriedman@chromium.org for
> build/android/findbugs_filter/findbugs_known_bugs.txt
> 
>
https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
> File
>
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java
> (right):
> 
>
https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
>
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/SessionDependencyFactoryNative.java:109:
> throw new RuntimeException("Not implemented");
> On 2014/11/11 17:11:47, mnaganov (cr) wrote:
> > I think you should throw UnsupportedOperationException instead.
> > 
> > See e.g.
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22Not%20implem...
> 
> Done.
> 
>
https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
> File components/devtools_bridge/session_dependency_factory.cc (right):
> 
>
https://codereview.chromium.org/716433003/diff/100001/components/devtools_bri...
> components/devtools_bridge/session_dependency_factory.cc:126:
> AbstractPeerConnection::Delegate* const delegate_;
> On 2014/11/11 17:11:47, mnaganov (cr) wrote:
> > It should be "const Type", not "Type const". Here and the classes below.
> 
> This is not the case: "Type* const" is not like "Type const". For pointers we
> use formatting like this (for instance -
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...).
> 
> However there are cases "Type const" (not only in this file). Fixed.

Ah, right. Thanks for pointing this out!

[SeRya, 2014-11-17 21:20:34]

serya@chromium.org changed reviewers:
- darin@chromium.org, yfriedman@chromium.org

[SeRya, 2014-11-17 21:21:27]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-11-17 21:22:39]

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

[commit-bot: I haz the power, 2014-11-17 21:29:34]

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

[commit-bot: I haz the power, 2014-11-17 21:29:34]

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

[commit-bot: I haz the power, 2014-11-17 21:54:30]

Patchset 4 (id:??) landed as
https://crrev.com/24f61abef70c81d7529e8f908d7beeea48f40641
Cr-Commit-Position: refs/heads/master@{#304488}

[SeRya, 2014-11-17 21:54:35]

Committed patchset #4 (id:140001) manually as
24f61abef70c81d7529e8f908d7beeea48f40641 (presubmit successful).