Log to the webrtc log stream from webrtc/modules java code.

Log to the webrtc log stream from  webrtc/modules java code.
The purpose is to gather all webrtc logging in a single place and allow the app to redirect all webrtc logging to a single stream for offline debugging.

Moved Logging.java to webrtc/base to be shared by talk/ and modules/.

R=glaznev@webrtc.org, henrika@webrtc.org, magjed@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/7754285f7c2651c564a48de978c41b141ecfcb02
Cr-Commit-Position: refs/heads/master@{#9959}

[jiayl2, 2015-09-14 23:26:23]

jiayl@webrtc.org changed reviewers:
+ glaznev@webrtc.org, henrika@webrtc.org

[jiayl2, 2015-09-14 23:26:23]

PTAL

[henrika_webrtc, 2015-09-15 08:21:50]

henrika@webrtc.org changed reviewers:
+ henrikg@webrtc.org

[henrika_webrtc, 2015-09-15 08:22:57]

+henrikg since he knows the logging parts in WebRTC well.

[Henrik Grunell WebRTC, 2015-09-15 08:27:17]

What is the problem with using the existing org.webrtc.Logging?

[henrika_webrtc, 2015-09-15 08:33:47]

Sorry for delaying one round but could you give a short summary of what the
actual difference in terms of logging will be using this CL. Will I no longer be
able to see the logs using adb logcat?

https://codereview.webrtc.org/1335103004/diff/1/webrtc/modules/audio_device/a...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/Logging.java
(right):

https://codereview.webrtc.org/1335103004/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/Logging.java:27:
nativeLog(severity.ordinal(), tag, message);
It is not clear to me where this log "lands"?
If we use e.g. AppRTC; does it mean that the logs no longer will be seen in the
system output (where we can use adb logcat to trace the logs)?

[henrika_webrtc, 2015-09-15 15:26:16]

LGTM

[AlexG, 2015-09-15 16:41:35]

May be move Logging.java to a separate package - something like
org.webrtc.logging.Logging.java to avoid code duplication?

[jiayl2, 2015-09-15 17:30:09]

On 2015/09/15 16:41:35, AlexG wrote:
> May be move Logging.java to a separate package - something like
> org.webrtc.logging.Logging.java to avoid code duplication?

Done. Moved org.webrtc.Logging to webrtc/base and shared now.

[jiayl2, 2015-09-15 17:30:50]

jiayl@webrtc.org changed reviewers:
+ tommi@webrtc.org

[jiayl2, 2015-09-15 17:30:51]

Adding tommi.
PTAL

[jiayl2, 2015-09-15 17:52:05]

Patchset #2 (id:20001) has been deleted

[jiayl2, 2015-09-15 17:52:13]

Patchset #2 (id:40001) has been deleted

[AlexG, 2015-09-15 17:55:02]

lgtm

[jiayl2, 2015-09-15 17:58:45]

On 2015/09/15 08:33:47, henrika_webrtc wrote:
> Sorry for delaying one round but could you give a short summary of what the
> actual difference in terms of logging will be using this CL. Will I no longer
be
> able to see the logs using adb logcat?
> 
>
https://codereview.webrtc.org/1335103004/diff/1/webrtc/modules/audio_device/a...
> File
>
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/Logging.java
> (right):
> 
>
https://codereview.webrtc.org/1335103004/diff/1/webrtc/modules/audio_device/a...
>
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/Logging.java:27:
> nativeLog(severity.ordinal(), tag, message);
> It is not clear to me where this log "lands"?
> If we use e.g. AppRTC; does it mean that the logs no longer will be seen in
the
> system output (where we can use adb logcat to trace the logs)?

Explained offline.
The log will go to the logging sinks set by the application, same as the native
logs.
The app can set all webrtc (native+java) log printed to logcat. AppRTC does that
already.

[Henrik Grunell WebRTC, 2015-09-16 07:19:59]

https://codereview.webrtc.org/1335103004/diff/80001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1335103004/diff/80001/talk/libjingle.gyp#newcod...
talk/libjingle.gyp:141: # TODO(fischman): extract this into a webrtc gyp var
that can be
Nit: maybe add similar a todo for peerconnection_java_files since we use a
webrtc file now. Or move it to the top of the target and update to cover both.

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/base/java/src/org/...
File webrtc/base/java/src/org/webrtc/Logging.java (right):

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/base/java/src/org/...
webrtc/base/java/src/org/webrtc/Logging.java:19: private static final String TAG
= "Logging";
I don't see this being used. Remove?

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/modules/video_capt...
File
webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java
(right):

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/modules/video_capt...
webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java:106:
Logging.d(TAG, "startCapture: " + width + "x" + height + "@" +
Nit: Maybe introduce private Loge(), Logd(), etc. functions? It's up to you.
(Same in other files.)

[Henrik Grunell WebRTC, 2015-09-16 07:21:26]

Nit: add bug or remove BUG=.

[tommi, 2015-09-16 08:21:24]

Magnus, since you're an owner I'm assuming I'm not needed (can't review just now
either)

[jiayl2, 2015-09-16 17:20:11]

jiayl@webrtc.org changed reviewers:
+ magjed@webrtc.org

[jiayl2, 2015-09-16 17:20:11]

Magnus,
PTAL

[jiayl2, 2015-09-16 17:26:04]

All comments addressed.

https://codereview.webrtc.org/1335103004/diff/80001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1335103004/diff/80001/talk/libjingle.gyp#newcod...
talk/libjingle.gyp:141: # TODO(fischman): extract this into a webrtc gyp var
that can be
On 2015/09/16 07:19:59, Henrik Grunell (webrtc) wrote:
> Nit: maybe add similar a todo for peerconnection_java_files since we use a
> webrtc file now. Or move it to the top of the target and update to cover both.

Done

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/base/java/src/org/...
File webrtc/base/java/src/org/webrtc/Logging.java (right):

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/base/java/src/org/...
webrtc/base/java/src/org/webrtc/Logging.java:19: private static final String TAG
= "Logging";
On 2015/09/16 07:19:59, Henrik Grunell (webrtc) wrote:
> I don't see this being used. Remove?

Done.

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/modules/video_capt...
File
webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java
(right):

https://codereview.webrtc.org/1335103004/diff/80001/webrtc/modules/video_capt...
webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java:106:
Logging.d(TAG, "startCapture: " + width + "x" + height + "@" +
On 2015/09/16 07:19:59, Henrik Grunell (webrtc) wrote:
> Nit: Maybe introduce private Loge(), Logd(), etc. functions? It's up to you.
> (Same in other files.)

I think it's OK to leave as it is.

[magjed_webrtc, 2015-09-16 18:06:30]

I'm not an owner of any of these files, but it lgtm :) I'm owner of
talk/app/webrtc/java/android/. Will files in talk/ be updated as well? I think
webrtc/modules/video_capture/android/ is deprecated and about to be deleted.

[tommi, 2015-09-16 18:32:23]

Sorry, I thought only the video files remained, but lgtm all the same.

[jiayl2, 2015-09-16 21:56:59]

The CQ bit was checked by jiayl@webrtc.org

[jiayl2, 2015-09-16 21:57:00]

The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org,
glaznev@webrtc.org, magjed@webrtc.org, tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1335103004/#ps120001 (title:
"sync")

[commit-bot: I haz the power, 2015-09-16 21:57:12]

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

[jiayl2, 2015-09-16 23:20:53]

Committed patchset #5 (id:120001) manually as
7754285f7c2651c564a48de978c41b141ecfcb02 (presubmit successful).

[commit-bot: I haz the power, 2015-09-16 23:20:58]

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

[Henrik Grunell WebRTC, 2015-09-17 07:04:39]

lgtm

[phoglund, 2015-09-28 13:03:34]

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

[phoglund, 2015-09-28 13:03:34]

https://codereview.webrtc.org/1335103004/diff/120001/webrtc/base/java/src/org...
File webrtc/base/java/src/org/webrtc/Logging.java (right):

https://codereview.webrtc.org/1335103004/diff/120001/webrtc/base/java/src/org...
webrtc/base/java/src/org/webrtc/Logging.java:22: } catch (Throwable t) {
Umm, no. No no no no. Don't do this.

First, you should never ever catch Throwable. I suspect what you want to really
do here is to catch UnsatisfiedLinkError. Throwable includes OutOfMemoryError,
ClassNotFoundError and a lot of other stuff you don't want to catch.

Second, you need to do at better job of falling back. I just spent an hour
wondering why nativeEnableLogThreads wasn't linked in, but the problem was
actually that the .so didn't load. Only, this code hid that fact. 

I see you made some improvements to this code recently, but enableLogThreads
will still blow up with a confusing error if the so doesn't load. At the very
least you need to do something in enableLogThreads (like assert that the so load
succeeded, and if it didn't, say something like "you're trying to use
enableLogThreads but the .so didn't load!"). That would separate "the .so didn't
load" from "enableLogThreads is not defined".

What are you trying to support with this catch? What valid use case is there if
libjingle isn't around?