Use Chromium's Logging instead of Android's Logging for media files

Use Chromium's Logging instead of Android's Logging for media files

This patch modifies all the media/base/android/java files to use
org.chromium.base.Log instead of android.util.Log and changes the
log messages as per chromium guidelines.

BUG=489318, 472152
Committed: https://crrev.com/9d09b77f46a52e685f390b705202a7ba0326ce08
Cr-Commit-Position: refs/heads/master@{#334124}

[amogh.bihani, 2015-06-09 11:46:18]

Patchset #1 (id:1) has been deleted

[amogh.bihani, 2015-06-09 11:48:06]

amogh.bihani@samsung.com changed reviewers:
+ dgn@chromium.org, mcasas@chromium.org, xhwang@chromium.org

[amogh.bihani, 2015-06-09 11:48:17]

PTAL.

[dgn, 2015-06-09 13:10:53]

A debug log will be printed as something like

D/cr.media.AudioManagerAndroid ( 999): [AudioManagerAndroid.java:42] my message,
foo = bar

Putting the file name again in the tag is redundant. Depending on how you would
want to display the logs, a simple "media" tag everywhere can do. Or maybe
"media.NativeBridges", "media.Video", etc. groups?

Also, there is a length limit, set at 23 characters on a possible tag. Because
of "cr.", that's 20 usable chars. I'll add a presubmit check to verify that
asap.

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:376:
Log.w(TAG, "Requires MODIFY_AUDIO_SETTINGS and RECORD_AUDIO");
Any specific reason why it is in two instructions here?, also, same in line 420

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: " + ex);
How about that error? currently the stack will not be displayed. is that on
purpose?

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureTango.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureTango.java:184:
Log.e(TAG, "Unknown camera, #id: " + mTangoCameraId);
%d ?

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:23:
static final String LOG_TAG = Log.makeTag("media.WebAudioMediaCodec");
I think presubmit will keep complaining if the variable name is not TAG

[amogh.bihani, 2015-06-10 04:44:20]

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:376:
Log.w(TAG, "Requires MODIFY_AUDIO_SETTINGS and RECORD_AUDIO");
On 2015/06/09 13:10:53, dgn wrote:
> Any specific reason why it is in two instructions here?, also, same in line
420

Don't know. But it makes sense to combine them.

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: " + ex);
On 2015/06/09 13:10:53, dgn wrote:
> How about that error? currently the stack will not be displayed. is that on
> purpose?

I'm sorry. I could not understand why the stack trace will not come. Is it
because it should be with a ',' and not '+'?
I have changed that. If there is something else please let me know.

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureTango.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureTango.java:184:
Log.e(TAG, "Unknown camera, #id: " + mTangoCameraId);
On 2015/06/09 13:10:53, dgn wrote:
> %d ?

Done.

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:23:
static final String LOG_TAG = Log.makeTag("media.WebAudioMediaCodec");
On 2015/06/09 13:10:53, dgn wrote:
> I think presubmit will keep complaining if the variable name is not TAG

Done.

[dgn, 2015-06-10 11:01:13]

Thanks it's looking good. I landed a patch to add the presubmit checks I
mentioned in earlier comments. Could you please tell me if it all runs properly
for you?

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: " + ex);
On 2015/06/10 04:44:20, amogh.bihani wrote:
> On 2015/06/09 13:10:53, dgn wrote:
> > How about that error? currently the stack will not be displayed. is that on
> > purpose?
> 
> I'm sorry. I could not understand why the stack trace will not come. Is it
> because it should be with a ',' and not '+'?
> I have changed that. If there is something else please let me know.

Yes, if you concatenate, I think it will just use the exception's toString(),
thus printing the exception's error message, but not the full stack. Maybe
that's what you want here.

Your updated code will print something like:

E/cr.media ( 999): Camera.open:
E/cr.media ( 999): RuntimeException: <exception message>
E/cr.media ( 999):              at blah blah .. the stack


So the log message itself will not have the exception message. If you want it,
then you should use:

Log.e(TAG, "Camera.open: %s", ex);

%s will make sure the argument's toString is used to complete the log message,
and because the last argument provided to the Log.e() is an exception, it will
also be used to dump the stack trace.

So if you don't want to dump the stack trace, concatenation with + is fine.

https://codereview.chromium.org/1174523002/diff/40001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:38:
private static final String TAG = Log.makeTag("media");
Sorry about that, but I just submitted a patch
(https://codereview.chromium.org/1131903007/#) to deprecate Log#makeTag. (For
more context, please the the bug linked in the review)

The replacement is to directly use TAG = "cr.<your tag>"

Could you please update this patch to reflect that?

https://codereview.chromium.org/1174523002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:376:
Log.w(TAG, "Requires MODIFY_AUDIO_SETTINGS and RECORD_AUDIO\n"
nit: Because of the \n here, grep likely will not get the second line since that
second one will not have the "W/cr.media" start

[amogh.bihani, 2015-06-11 06:22:01]

Thanks for the review :)

Yes the new presubmit script is passing.

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: " + ex);
On 2015/06/10 11:01:12, dgn wrote:
> On 2015/06/10 04:44:20, amogh.bihani wrote:
> > On 2015/06/09 13:10:53, dgn wrote:
> > > How about that error? currently the stack will not be displayed. is that
on
> > > purpose?
> > 
> > I'm sorry. I could not understand why the stack trace will not come. Is it
> > because it should be with a ',' and not '+'?
> > I have changed that. If there is something else please let me know.
> 
> Yes, if you concatenate, I think it will just use the exception's toString(),
> thus printing the exception's error message, but not the full stack. Maybe
> that's what you want here.
> 
> Your updated code will print something like:
> 
> E/cr.media ( 999): Camera.open:
> E/cr.media ( 999): RuntimeException: <exception message>
> E/cr.media ( 999):              at blah blah .. the stack
> 
> 
> So the log message itself will not have the exception message. If you want it,
> then you should use:
> 
> Log.e(TAG, "Camera.open: %s", ex);
> 
> %s will make sure the argument's toString is used to complete the log message,
> and because the last argument provided to the Log.e() is an exception, it will
> also be used to dump the stack trace.
> 
> So if you don't want to dump the stack trace, concatenation with + is fine.

Acknowledged.

Thanks for the explanation :)

https://codereview.chromium.org/1174523002/diff/40001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:38:
private static final String TAG = Log.makeTag("media");
On 2015/06/10 11:01:13, dgn wrote:
> Sorry about that, but I just submitted a patch
> (https://codereview.chromium.org/1131903007/) to deprecate Log#makeTag. (For
> more context, please the the bug linked in the review)
> 
> The replacement is to directly use TAG = "cr.<your tag>"
> 
> Could you please update this patch to reflect that?

Done.

https://codereview.chromium.org/1174523002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:376:
Log.w(TAG, "Requires MODIFY_AUDIO_SETTINGS and RECORD_AUDIO\n"
On 2015/06/10 11:01:13, dgn wrote:
> nit: Because of the \n here, grep likely will not get the second line since
that
> second one will not have the "W/cr.media" start

Done.

[dgn, 2015-06-11 15:45:26]

non owner lgtm, thanks!

[xhwang, 2015-06-11 16:04:02]

lgtm

[amogh.bihani, 2015-06-12 02:48:11]

The CQ bit was checked by amogh.bihani@samsung.com

[commit-bot: I haz the power, 2015-06-12 02:48:32]

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

[commit-bot: I haz the power, 2015-06-12 03:58:18]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-06-12 03:59:21]

Patchset 3 (id:??) landed as
https://crrev.com/9d09b77f46a52e685f390b705202a7ba0326ce08
Cr-Commit-Position: refs/heads/master@{#334124}

[jdduke (slow), 2015-06-25 16:34:02]

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

[jdduke (slow), 2015-06-25 16:34:03]

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: ", ex);
Are we missing a %s here?

[dgn, 2015-06-25 16:46:10]

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: ", ex);
On 2015/06/25 at 16:34:03, jdduke wrote:
> Are we missing a %s here?

Depends on what we want to do. Currently it should output something like

E/TAG: Camera.open:
E/TAG: Exception: exception message
E/TAG:    at ...trace...

So the exception is not lost here.

Adding %s should get us

E/TAG: Camera.open: exception message
E/TAG: Exception: exception message
E/TAG:    at ...trace...

Using ex.getMessage() would remove the stack trace.

If you noticed a different behaviour then I have a bug somewhere...

[jdduke (slow), 2015-06-25 16:53:12]

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: ", ex);
On 2015/06/25 16:46:09, dgn wrote:
> On 2015/06/25 at 16:34:03, jdduke wrote:
> > Are we missing a %s here?
> 
> Depends on what we want to do. Currently it should output something like
> 
> E/TAG: Camera.open:
> E/TAG: Exception: exception message
> E/TAG:    at ...trace...
> 
> So the exception is not lost here.
> 
> Adding %s should get us
> 
> E/TAG: Camera.open: exception message
> E/TAG: Exception: exception message
> E/TAG:    at ...trace...
> 
> Using ex.getMessage() would remove the stack trace.
> 
> If you noticed a different behaviour then I have a bug somewhere...

Oh nevermind, I guess format just appends the stringified exception?

[dgn, 2015-06-25 17:02:56]

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java
(right):

https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71:
Log.e(TAG, "Camera.open: ", ex);
> Oh nevermind, I guess format just appends the stringified exception?

Well, Throwables have a special handling when they are the last argument of the
call. They are always forwarded to the variant of the framework's log function
that prints the stack trace. So they can't be lost.

Then for the formatting of the log message itself, it takes as many objects from
the arguments as there are placeholders. Since there is none here, message
formatting ignores the exception. With a %s added, it would call
String.valueOf(ex) or something similar, yes.