Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(256)

Issue 1174523002: Use Chromium's Logging instead of Android's Logging for media files (Closed)

Created:
5 years, 6 months ago by amogh.bihani
Modified:
5 years, 6 months ago
CC:
avayvod+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 : #

Total comments: 10

Patch Set 2 : addressing comments #

Total comments: 4

Patch Set 3 : hardcoding cr. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -88 lines) Patch
M media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java View 1 2 4 chunks +6 lines, -6 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/AudioRecordInput.java View 1 2 7 chunks +9 lines, -11 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/MediaCodecBridge.java View 1 2 3 chunks +5 lines, -5 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/MediaDrmBridge.java View 1 2 13 chunks +15 lines, -15 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java View 1 2 3 chunks +4 lines, -3 lines 4 comments Download
M media/base/android/java/src/org/chromium/media/VideoCaptureCamera.java View 1 2 7 chunks +9 lines, -11 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/VideoCaptureCamera2.java View 1 2 4 chunks +5 lines, -5 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/VideoCaptureFactory.java View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/VideoCaptureTango.java View 1 2 3 chunks +4 lines, -3 lines 0 comments Download
M media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java View 1 2 10 chunks +17 lines, -24 lines 0 comments Download

Messages

Total messages: 18 (4 generated)
amogh.bihani
PTAL.
5 years, 6 months ago (2015-06-09 11:48:17 UTC) #3
dgn
A debug log will be printed as something like D/cr.media.AudioManagerAndroid ( 999): [AudioManagerAndroid.java:42] my message, ...
5 years, 6 months ago (2015-06-09 13:10:53 UTC) #4
amogh.bihani
https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java#newcode376 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 ...
5 years, 6 months ago (2015-06-10 04:44:20 UTC) #5
dgn
Thanks it's looking good. I landed a patch to add the presubmit checks I mentioned ...
5 years, 6 months ago (2015-06-10 11:01:13 UTC) #6
amogh.bihani
Thanks for the review :) Yes the new presubmit script is passing. https://codereview.chromium.org/1174523002/diff/20001/media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java ...
5 years, 6 months ago (2015-06-11 06:22:01 UTC) #7
dgn
non owner lgtm, thanks!
5 years, 6 months ago (2015-06-11 15:45:26 UTC) #8
xhwang
lgtm
5 years, 6 months ago (2015-06-11 16:04:02 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1174523002/60001
5 years, 6 months ago (2015-06-12 02:48:32 UTC) #11
commit-bot: I haz the power
Committed patchset #3 (id:60001)
5 years, 6 months ago (2015-06-12 03:58:18 UTC) #12
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/9d09b77f46a52e685f390b705202a7ba0326ce08 Cr-Commit-Position: refs/heads/master@{#334124}
5 years, 6 months ago (2015-06-12 03:59:21 UTC) #13
jdduke (slow)
https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java (right): https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java#newcode71 media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java:71: Log.e(TAG, "Camera.open: ", ex); Are we missing a %s ...
5 years, 6 months ago (2015-06-25 16:34:03 UTC) #15
dgn
https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java (right): https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java#newcode71 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 ...
5 years, 6 months ago (2015-06-25 16:46:10 UTC) #16
jdduke (slow)
https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java File media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java (right): https://codereview.chromium.org/1174523002/diff/60001/media/base/android/java/src/org/chromium/media/VideoCaptureAndroid.java#newcode71 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: ...
5 years, 6 months ago (2015-06-25 16:53:12 UTC) #17
dgn
5 years, 6 months ago (2015-06-25 17:02:56 UTC) #18
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698