Relanding: Break peerconnection_jni.cc into multiple files, in "pc" directory.

Relanding: Break peerconnection_jni.cc into multiple files, in "pc" directory.

Relanding after adding "androidnetworkmonitor_jni.h" header to jni/
directory, since some clients were including it directly.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

BUG=webrtc:8055
Review-Url: https://codereview.webrtc.org/2992103002
Cr-Commit-Position: refs/heads/master@{#19241}
Committed: https://chromium.googlesource.com/external/webrtc/+/0d48c693312895505580bca568632e7560cc8376

[Taylor Brandstetter, 2017-08-01 00:23:08]

deadbeef@webrtc.org changed reviewers:
+ magjed@webrtc.org, sakal@webrtc.org

[Taylor Brandstetter, 2017-08-01 00:23:10]

PTAL. There shouldn't be any changes here besides moving and renaming things.
I'm not attached to any specific naming scheme ("JavaToNativeX",
"JavaXToNativeX", "JavaXToNativeType"), as long as the one we choose is
consistent, so feel free to suggest something else.

[magjed_webrtc, 2017-08-01 12:52:17]

Thanks for doing this!

Can you try one thing before I stamp - if you lower similarity with 'git cl
upload --similarity=SIM', can you see if the diff gets smaller so it's easier to
verify the code is unchanged?

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/jni_helpers.h (right):

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/jni_helpers.h:39: #define JOW(rettype, name) \
This is unrelated to your CL, but I have been planning to deprecate this macro
as I think it's counterproductive. It's more transparent and easier on my eyes
to see a normal function declaration than this macro. Also, clang format works
much better without this macro which also improves readability.

If you agree, please add a comment that this macro is deprecated and that the
preferred way is to write out the full function declaration.

[Taylor Brandstetter, 2017-08-01 20:53:54]

Changed similarity threshold to 5%. It still doesn't always help, since a bunch
of things were renamed, and namespace prefixes added. But I think it's enough to
see what things changed overall.

I forgot to mention, I got rid of the big list of "using" directives; this is
generally discouraged by our style guide (as I interpret it), and personally,
it's a maintenance annoyance for me. I'd prefer always having to type out
"webrtc::"/"cricket::"/etc. rather than having to figure out if I need to. Plus,
with peerconnection_jni being split up, each file would need its own "using"
statements, adding to the maintenance cost.

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/jni_helpers.h (right):

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/jni_helpers.h:39: #define JOW(rettype, name) \
On 2017/08/01 12:52:17, magjed_webrtc wrote:
> This is unrelated to your CL, but I have been planning to deprecate this macro
> as I think it's counterproductive. It's more transparent and easier on my eyes
> to see a normal function declaration than this macro. Also, clang format works
> much better without this macro which also improves readability.
> 
> If you agree, please add a comment that this macro is deprecated and that the
> preferred way is to write out the full function declaration.

I'm not sure I agree; it may be more transparent, but 'extern "C" JNIEXPORT void
JNICALL Java_org_webrtc_nativeFoo' isn't really easy on the eyes to me. And
having to type it out could result in mistakes that were otherwise not possible,
or code reviewers missing a detail in the noise. Is there another reason I'm
missing? Because I don't think transparency is reason enough. It's easy to look
at the definition here if one actually cares (which shouldn't be too frequent).

For the clang format point, it would be easy to change the macro to:

#define JOW(rettype, name, ...) \
   extern "C" JNIEXPORT rettype JNICALL Java_org_webrtc_##name(__VA_ARGS__)

In other words, putting the arguments in the same set of parens. Which
clang-format is much kinder to.

[magjed_webrtc, 2017-08-02 14:15:47]

On 2017/08/01 20:53:54, Taylor Brandstetter wrote:
> Changed similarity threshold to 5%. It still doesn't always help, since a
bunch
> of things were renamed, and namespace prefixes added. But I think it's enough
to
> see what things changed overall.
> 
> I forgot to mention, I got rid of the big list of "using" directives; this is
> generally discouraged by our style guide (as I interpret it), and personally,
> it's a maintenance annoyance for me. I'd prefer always having to type out
> "webrtc::"/"cricket::"/etc. rather than having to figure out if I need to.
Plus,
> with peerconnection_jni being split up, each file would need its own "using"
> statements, adding to the maintenance cost.
I agree, thanks for cleaning that up.

Also, I'm happy with the JavaToNativeX/NativeToJavaX naming, as long as it's
consistent as you say.

lgtm

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/jni_helpers.h (right):

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/jni_helpers.h:39: #define JOW(rettype, name) \
On 2017/08/01 20:53:54, Taylor Brandstetter wrote:
> On 2017/08/01 12:52:17, magjed_webrtc wrote:
> > This is unrelated to your CL, but I have been planning to deprecate this
macro
> > as I think it's counterproductive. It's more transparent and easier on my
eyes
> > to see a normal function declaration than this macro. Also, clang format
works
> > much better without this macro which also improves readability.
> > 
> > If you agree, please add a comment that this macro is deprecated and that
the
> > preferred way is to write out the full function declaration.
> 
> I'm not sure I agree; it may be more transparent, but 'extern "C" JNIEXPORT
void
> JNICALL Java_org_webrtc_nativeFoo' isn't really easy on the eyes to me. And
> having to type it out could result in mistakes that were otherwise not
possible,
> or code reviewers missing a detail in the noise. Is there another reason I'm
> missing? Because I don't think transparency is reason enough. It's easy to
look
> at the definition here if one actually cares (which shouldn't be too
frequent).
> 
> For the clang format point, it would be easy to change the macro to:
> 
> #define JOW(rettype, name, ...) \
>    extern "C" JNIEXPORT rettype JNICALL Java_org_webrtc_##name(__VA_ARGS__)
> 
> In other words, putting the arguments in the same set of parens. Which
> clang-format is much kinder to.

So we have three different options. A concrete example looks like:

extern "C" JNIEXPORT void JNICALL
Java_org_webrtc_VideoTrack_nativeAddRenderer(JNIEnv* jni,
                                             jclass,
                                             jlong j_video_track_pointer,
                                             jlong j_renderer_pointer) {
  ...
}

JOW(void, VideoTrack_nativeAddRenderer)
(JNIEnv* jni, jclass, jlong j_video_track_pointer, jlong j_renderer_pointer) {
  ...
}

JOW(void,
    VideoTrack_nativeAddRenderer,
    JNIEnv* jni,
    jclass,
    jlong j_video_track_pointer,
    jlong j_renderer_pointer) {
  ...
}

I'm leaning towards the #define JOW(rettype, name, ...) option which looks much
nicer than the current #define JOW(rettype, name) option (thanks for suggesting
it!). If we change it, we should probably also change the name from JOW to
something more descriptive like JNI_FUNCTION_DECLARATION.

It's not a big deal, so it's perfectly fine if you just want to leave the macro
as is for now.

https://codereview.webrtc.org/2992103002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/pc/peerconnectionfactory_jni.cc (right):

https://codereview.webrtc.org/2992103002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/pc/peerconnectionfactory_jni.cc:39: // need video, it
can link with
Finish this comment.

[sakal, 2017-08-02 16:48:10]

Unfortunately I don't have time to fully review this right now but lgtm on
concept level.

[Taylor Brandstetter, 2017-08-02 21:13:50]

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/jni_helpers.h (right):

https://codereview.webrtc.org/2992103002/diff/20001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/jni_helpers.h:39: #define JOW(rettype, name) \
On 2017/08/02 14:15:46, magjed_webrtc wrote:
> On 2017/08/01 20:53:54, Taylor Brandstetter wrote:
> > On 2017/08/01 12:52:17, magjed_webrtc wrote:
> > > This is unrelated to your CL, but I have been planning to deprecate this
> macro
> > > as I think it's counterproductive. It's more transparent and easier on my
> eyes
> > > to see a normal function declaration than this macro. Also, clang format
> works
> > > much better without this macro which also improves readability.
> > > 
> > > If you agree, please add a comment that this macro is deprecated and that
> the
> > > preferred way is to write out the full function declaration.
> > 
> > I'm not sure I agree; it may be more transparent, but 'extern "C" JNIEXPORT
> void
> > JNICALL Java_org_webrtc_nativeFoo' isn't really easy on the eyes to me. And
> > having to type it out could result in mistakes that were otherwise not
> possible,
> > or code reviewers missing a detail in the noise. Is there another reason I'm
> > missing? Because I don't think transparency is reason enough. It's easy to
> look
> > at the definition here if one actually cares (which shouldn't be too
> frequent).
> > 
> > For the clang format point, it would be easy to change the macro to:
> > 
> > #define JOW(rettype, name, ...) \
> >    extern "C" JNIEXPORT rettype JNICALL Java_org_webrtc_##name(__VA_ARGS__)
> > 
> > In other words, putting the arguments in the same set of parens. Which
> > clang-format is much kinder to.
> 
> So we have three different options. A concrete example looks like:
> 
> extern "C" JNIEXPORT void JNICALL
> Java_org_webrtc_VideoTrack_nativeAddRenderer(JNIEnv* jni,
>                                              jclass,
>                                              jlong j_video_track_pointer,
>                                              jlong j_renderer_pointer) {
>   ...
> }
> 
> JOW(void, VideoTrack_nativeAddRenderer)
> (JNIEnv* jni, jclass, jlong j_video_track_pointer, jlong j_renderer_pointer) {
>   ...
> }
> 
> JOW(void,
>     VideoTrack_nativeAddRenderer,
>     JNIEnv* jni,
>     jclass,
>     jlong j_video_track_pointer,
>     jlong j_renderer_pointer) {
>   ...
> }
> 
> I'm leaning towards the #define JOW(rettype, name, ...) option which looks
much
> nicer than the current #define JOW(rettype, name) option (thanks for
suggesting
> it!). If we change it, we should probably also change the name from JOW to
> something more descriptive like JNI_FUNCTION_DECLARATION.
> 
> It's not a big deal, so it's perfectly fine if you just want to leave the
macro
> as is for now.

Since this CL already probably makes more changes than it should, I'll save this
for a second CL and add a TODO for now.

https://codereview.webrtc.org/2992103002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/pc/peerconnectionfactory_jni.cc (right):

https://codereview.webrtc.org/2992103002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/pc/peerconnectionfactory_jni.cc:39: // need video, it
can link with
On 2017/08/02 14:15:46, magjed_webrtc wrote:
> Finish this comment.

Good catch.

[Taylor Brandstetter, 2017-08-02 21:15:43]

Description was changed from

==========
Break peerconnection_jni.cc into multiple files, in "pc" directory.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

BUG=webrtc:8055
==========

to

==========
Break peerconnection_jni.cc into multiple files, in "pc" directory.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

deadbeef@webrtc.org is added as an owner of the new "pc" subdirectory.

BUG=webrtc:8055
==========

[Taylor Brandstetter, 2017-08-02 21:18:50]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-08-02 21:18:51]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
sakal@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2992103002/#ps100001 (title:
"Finish writing comment, add TODO comment.")

[commit-bot: I haz the power, 2017-08-02 21:19:07]

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-08-02 22:05:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1501708730518490,
"parent_rev": "5db765792ea6884b767588d25a0d7fea93fa8ca6", "commit_rev":
"dd7d8f1b609d51bcf39e9585871967a694a856bb"}

[commit-bot: I haz the power, 2017-08-02 22:05:21]

Description was changed from

==========
Break peerconnection_jni.cc into multiple files, in "pc" directory.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

deadbeef@webrtc.org is added as an owner of the new "pc" subdirectory.

BUG=webrtc:8055
==========

to

==========
Break peerconnection_jni.cc into multiple files, in "pc" directory.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

deadbeef@webrtc.org is added as an owner of the new "pc" subdirectory.

BUG=webrtc:8055

Review-Url: https://codereview.webrtc.org/2992103002
Cr-Commit-Position: refs/heads/master@{#19223}
Committed:
https://chromium.googlesource.com/external/webrtc/+/dd7d8f1b609d51bcf39e95858...
==========

[commit-bot: I haz the power, 2017-08-02 22:05:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/dd7d8f1b609d51bcf39e95858...

[Zhi Huang, 2017-08-03 01:00:41]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.webrtc.org/2989323002/ by zhihuang@webrtc.org.

The reason for reverting is: Borken in the internal projects..

[Taylor Brandstetter, 2017-08-03 16:04:58]

Description was changed from

==========
Break peerconnection_jni.cc into multiple files, in "pc" directory.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

deadbeef@webrtc.org is added as an owner of the new "pc" subdirectory.

BUG=webrtc:8055

Review-Url: https://codereview.webrtc.org/2992103002
Cr-Commit-Position: refs/heads/master@{#19223}
Committed:
https://chromium.googlesource.com/external/webrtc/+/dd7d8f1b609d51bcf39e95858...
==========

to

==========
Relanding: Break peerconnection_jni.cc into multiple files, in "pc" directory.

Relanding after adding "androidnetworkmonitor_jni.h" header to jni/
directory, since some clients were including it directly.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

deadbeef@webrtc.org is added as an owner of the new "pc" subdirectory.

BUG=webrtc:8055
==========

[Taylor Brandstetter, 2017-08-03 16:05:17]

Description was changed from

==========
Relanding: Break peerconnection_jni.cc into multiple files, in "pc" directory.

Relanding after adding "androidnetworkmonitor_jni.h" header to jni/
directory, since some clients were including it directly.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

deadbeef@webrtc.org is added as an owner of the new "pc" subdirectory.

BUG=webrtc:8055
==========

to

==========
Relanding: Break peerconnection_jni.cc into multiple files, in "pc" directory.

Relanding after adding "androidnetworkmonitor_jni.h" header to jni/
directory, since some clients were including it directly.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

BUG=webrtc:8055
==========

[Taylor Brandstetter, 2017-08-03 16:05:34]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-08-03 16:05:35]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
sakal@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2992103002/#ps120001 (title:
"Add jni/androidnetworkmonitor_jni.h include for backwards comaptibility.")

[commit-bot: I haz the power, 2017-08-03 16:05:38]

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-08-03 17:20:18]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1501776334333520,
"parent_rev": "367aaa7ca5eeac7ca18ace3fce16636a8dccf0ff", "commit_rev":
"0d48c693312895505580bca568632e7560cc8376"}

[commit-bot: I haz the power, 2017-08-03 17:20:22]

Description was changed from

==========
Relanding: Break peerconnection_jni.cc into multiple files, in "pc" directory.

Relanding after adding "androidnetworkmonitor_jni.h" header to jni/
directory, since some clients were including it directly.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

BUG=webrtc:8055
==========

to

==========
Relanding: Break peerconnection_jni.cc into multiple files, in "pc" directory.

Relanding after adding "androidnetworkmonitor_jni.h" header to jni/
directory, since some clients were including it directly.

This CL breaks peerconnection_jni.cc apart, into one file for each
class. It also moves the methods for converting between C++/Java
structs into "java_native_conversion.cc", and uses a consistent naming
scheme ("JavaToNativeX, NativeToJavaX"). These files go into a new
"pc" directory, of which deadbeef@ is added as an owner.

It also moves some relevant files to the "pc" directory that belong
there: ownedfactoryandthreads, androidnetworkmonitor_jni, and
rtcstatscollectorcallbackwrapper. This directory is intended to hold
all the files that deal with the PeerConnection API specifically, or
related classes (like DataChannel, RtpSender, MediaStreamTrack) that
are tied to it closely.

BUG=webrtc:8055

Review-Url: https://codereview.webrtc.org/2992103002
Cr-Commit-Position: refs/heads/master@{#19241}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0d48c693312895505580bca56...
==========

[commit-bot: I haz the power, 2017-08-03 17:20:25]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/0d48c693312895505580bca56...