Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12974004:
Add speaker on/off control on Android for WebRTC (Closed)
|
Created:
7 years, 9 months ago by leozwang1 Modified:
7 years, 8 months ago Reviewers:
tommi (sloooow) - chröme, nilesh, Ami GONE FROM CHROMIUM, no longer working on chromium, qinmin, Yaron, wjia(left Chromium) CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd speaker on/off control on Android for WebRTC
Description:
Application listens to the headset plug intent, turns on or off speaker when
webrtc is running. It will help devices with dual mics to improve audio
quality.
BUG=222394
Landed in r192371, close it.
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : remember speaker status #
Total comments: 22
Patch Set 4 : address comments #
Total comments: 19
Patch Set 5 : #Patch Set 6 : check mReceiver #
Total comments: 49
Patch Set 7 : anddress comments #
Total comments: 21
Patch Set 8 : #Patch Set 9 : change order #Patch Set 10 : remove log #
Messages
Total messages: 23 (0 generated)
please take a look.
didn't take a look at the java code. https://chromiumcodereview.appspot.com/12974004/diff/11001/media/audio/audio_... File media/audio/audio_manager_base.h (right): https://chromiumcodereview.appspot.com/12974004/diff/11001/media/audio/audio_... media/audio/audio_manager_base.h:141: base::android::ScopedJavaGlobalRef<jobject> j_am_; full_descriptive_variable_names_please_ :) https://chromiumcodereview.appspot.com/12974004/diff/11001/media/audio/audio_... media/audio/audio_manager_base.h:151: void UnRegisterHeadsetReceiver(); Unregister is one word I believe
Drive by. https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:41: public AudioManagerAndroid(Context context) { You can make the constructor private. https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:50: Log.d(TAG, "AudioManager is null"); Please remove these debug and verbose logs. Too many of these make the logcat useless. For debugging use a static final variable to protect these log statements. More info here: https://sites.google.com/a/google.com/clank/engineering/best-practices/java-l... https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:90: if ( mReceiver == null ) { Extra spaces here. https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:108: return tm.getPhoneType() != TelephonyManager.PHONE_TYPE_NONE; Is there a reason to not check for tm == null?
https://chromiumcodereview.appspot.com/12974004/diff/11001/media/audio/audio_... File media/audio/audio_manager_base.cc (right): https://chromiumcodereview.appspot.com/12974004/diff/11001/media/audio/audio_... media/audio/audio_manager_base.cc:407: jobject context = base::android::GetApplicationContext(); You don't use the context here. Remove it https://chromiumcodereview.appspot.com/12974004/diff/11001/media/audio/audio_... File media/audio/audio_manager_base.h (right): https://chromiumcodereview.appspot.com/12974004/diff/11001/media/audio/audio_... media/audio/audio_manager_base.h:141: base::android::ScopedJavaGlobalRef<jobject> j_am_; Make this private. Also this object (AudioManagerBase) will get destroyed at some point? Want to make sure we don't keep the java object from get gc'd https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:20: private static final String TAG = "AMAndroid"; TAG = AudioManagerAndroid.class.getSimpleName() https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:37: public static AudioManagerAndroid createAudioManagerAndroid(Context context) { This can be private too. Access modifiers are checked at compile time but the native code still has access https://chromiumcodereview.appspot.com/12974004/diff/11001/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:61: if (intent.getAction().compareTo(Intent.ACTION_HEADSET_PLUG) == 0 ) { Why compareTo? It's a string, just do .equals. Also, use the constant to guard against null intents (we've seen it): Intent.ACTION_HEADSET_PLUG.equals(..)
please take a look https://codereview.chromium.org/12974004/diff/11001/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/12974004/diff/11001/media/audio/audio_manager... media/audio/audio_manager_base.cc:407: jobject context = base::android::GetApplicationContext(); On 2013/03/21 16:51:31, Yaron wrote: > You don't use the context here. Remove it Done. https://codereview.chromium.org/12974004/diff/11001/media/audio/audio_manager... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/12974004/diff/11001/media/audio/audio_manager... media/audio/audio_manager_base.h:141: base::android::ScopedJavaGlobalRef<jobject> j_am_; On 2013/03/21 15:31:27, tommi wrote: > full_descriptive_variable_names_please_ :) Done. https://codereview.chromium.org/12974004/diff/11001/media/audio/audio_manager... media/audio/audio_manager_base.h:141: base::android::ScopedJavaGlobalRef<jobject> j_am_; scoped_ptr<media::AudioManager> audio_manager_ is defined in broswer_main_loop.h https://codereview.chromium.org/12974004/diff/11001/media/audio/audio_manager... media/audio/audio_manager_base.h:151: void UnRegisterHeadsetReceiver(); On 2013/03/21 15:31:27, tommi wrote: > Unregister is one word I believe Done. https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:20: private static final String TAG = "AMAndroid"; On 2013/03/21 16:51:31, Yaron wrote: > TAG = AudioManagerAndroid.class.getSimpleName() Done. https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:37: public static AudioManagerAndroid createAudioManagerAndroid(Context context) { On 2013/03/21 16:51:31, Yaron wrote: > This can be private too. Access modifiers are checked at compile time but the > native code still has access Done. https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:41: public AudioManagerAndroid(Context context) { On 2013/03/21 16:42:33, nilesh wrote: > You can make the constructor private. Done. https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:50: Log.d(TAG, "AudioManager is null"); thanks, I removed most of log, but still keep one log below that states final speaker state, I think it could be helpful even in production. https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:61: if (intent.getAction().compareTo(Intent.ACTION_HEADSET_PLUG) == 0 ) { On 2013/03/21 16:51:31, Yaron wrote: > Why compareTo? It's a string, just do .equals. Also, use the constant to guard > against null intents (we've seen it): > Intent.ACTION_HEADSET_PLUG.equals(..) Done. https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:90: if ( mReceiver == null ) { On 2013/03/21 16:42:33, nilesh wrote: > Extra spaces here. Done. https://codereview.chromium.org/12974004/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:108: return tm.getPhoneType() != TelephonyManager.PHONE_TYPE_NONE; On 2013/03/21 16:42:33, nilesh wrote: > Is there a reason to not check for tm == null? Done.
https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:48: (AudioManager)context.getSystemService(Context.AUDIO_SERVICE); fix indent： 8 spaces https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:58: @Override fix indent： 4 spaces https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:63: (AudioManager)context.getSystemService(Context.AUDIO_SERVICE); fix indent： 8 spaces https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:97: (AudioManager)mContext.getSystemService(Context.AUDIO_SERVICE); indent https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:104: private int hasEarpiece() { Use enum, or constants, instead of int to make it clear what the meaning of those ints are. Something like USING_SPEAKER, USING_EARPICE, ERROR, etc
More nits. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:46: public void registerHeadsetReceiver(Context context) { Remove context. You can use mContext https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:47: AudioManager audioManager = assert mReceiver == null or return You have a similar check in unregister https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:64: if (null == audioManager) { nit: if (audioManager == null || AudioManager.MODE_IN_COMMUNICATION != audioManager.getMode())... https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:76: Log.d(TAG, "Turn off speaker, use earpiece now"); Is this log useful? https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:97: (AudioManager)mContext.getSystemService(Context.AUDIO_SERVICE); Nit: You have 4 calls like this. I think creating a private method to do this will be a good idea. private AudioManager getAudioManageService() { return (AudioManager)mContext.getSystemService(Context.AUDIO_SERVICE); }
PTAL https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:46: public void registerHeadsetReceiver(Context context) { On 2013/03/21 20:08:54, nilesh wrote: > Remove context. > You can use mContext Done. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:48: (AudioManager)context.getSystemService(Context.AUDIO_SERVICE); On 2013/03/21 19:56:25, qinmin wrote: > fix indent： 8 spaces Done. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:58: @Override On 2013/03/21 19:56:25, qinmin wrote: > fix indent： 4 spaces Done. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:63: (AudioManager)context.getSystemService(Context.AUDIO_SERVICE); On 2013/03/21 19:56:25, qinmin wrote: > fix indent： 8 spaces Done. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:64: if (null == audioManager) { On 2013/03/21 20:08:54, nilesh wrote: > nit: if (audioManager == null || AudioManager.MODE_IN_COMMUNICATION != > audioManager.getMode())... Done. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:76: Log.d(TAG, "Turn off speaker, use earpiece now"); On 2013/03/21 20:08:54, nilesh wrote: > Is this log useful? Done. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:97: (AudioManager)mContext.getSystemService(Context.AUDIO_SERVICE); On 2013/03/21 20:08:54, nilesh wrote: > Nit: You have 4 calls like this. I think creating a private method to do this > will be a good idea. > > private AudioManager getAudioManageService() { > return (AudioManager)mContext.getSystemService(Context.AUDIO_SERVICE); > } Done. https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:104: private int hasEarpiece() { On 2013/03/21 19:56:25, qinmin wrote: > Use enum, or constants, instead of int to make it clear what the meaning of > those ints are. > Something like USING_SPEAKER, USING_EARPICE, ERROR, etc Done.
lgtm https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:140: if (1 == num_output_streams_) To be on the safe side: if (stream && 1 == num_output_streams) https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:186: if (1 == num_input_streams_) same here
lgtm
https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:85: if (null == mReceiver) { Why would mReceiver be null here. You assign it on the line above. What I meant was to do some kind of checking if mReceiver is not null and registerHeadsetReceiver is called.
https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:58: @Override On 2013/03/21 20:50:25, leozwang1 wrote: > On 2013/03/21 19:56:25, qinmin wrote: > > fix indent： 4 spaces > > Done. I think this was meant for the whole block. Not just @Override
https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:27: private static final int kPhoneTypeError = -1; java don't use this style. constants should be defined as PHONE_TYPE_ERROR. same for everything below. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:111: return -1; return kPhoneTypeError
https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:70: DCHECK(context); unnecessary- GAC() does this https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:140: if (1 == num_output_streams_) On 2013/03/21 20:58:16, tommi wrote: > To be on the safe side: > > if (stream && 1 == num_output_streams) better yet, move this into the if at l.136 above? https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.h:179: // Java AudioManagerAndroid instance. s/Android// - the Java type doesn't have "Android" in its name. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:23: private Context mContext = null; The Java language does an implicit =null for you; these aren't necessary. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:29: private static final int kPhoneTypePhone = 1; "phonetypephone"? Really? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:43: private static AudioManagerAndroid createAudioManagerAndroid(Context context) { Why not just use the ctor? Are there auto-binding implications? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:53: AudioManager audioManager = getAudioManager(); If this was final you could drop the copy/pasta at l.67. (unless it's possible for getAudioManager()'s return value to change between calls; is that possible?) https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:54: if (null == audioManager) { please reverse these comparisons to have the constant on the right. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:55: return; When can this be true? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:57: mIsSpeakerOn = audioManager.isSpeakerphoneOn(); mOriginalSpeakerStatus ? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:66: int state = intent.getIntExtra("state", 0); s/0/kUnplugged/ https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:69: AudioManager.MODE_IN_COMMUNICATION != audioManager.getMode()) { How can this fail to be true? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:73: if (kPhoneTypeError == ep) { why isn't this using a Java enum with a switch statement? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:80: audioManager.setSpeakerphoneOn(true); l.72-81 can be summarized as: audioManager.setSpeakerphoneOn(state != kPlugged || ep != kPhoneTypePhone); https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:93: if (mReceiver == null) { how can this be true? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:100: (AudioManager)mContext.getSystemService(Context.AUDIO_SERVICE); use getAudioManager() https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:110: if (null == tm) { how can this fail? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:111: return -1; kPhoneTypeError? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:114: TelephonyManager.PHONE_TYPE_NONE ? kPhoneTypePhone : kPhoneTypeNone; What does SIP/GSM/CDMA have to do with having an earpiece or not? Why does "earpiece" enter into it? Isn't the real concern here whether a "headset" is plugged in? Put another way: if ACTION_HEADSET_PLUG is fired with a state of 1, why would you *not* silence the external speaker? https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:118: return null == mContext ? how can this be false?
https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:70: DCHECK(context); On 2013/03/21 21:26:21, Ami Fischman wrote: > unnecessary- GAC() does this Done. https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:140: if (1 == num_output_streams_) On 2013/03/21 20:58:16, tommi wrote: > To be on the safe side: > > if (stream && 1 == num_output_streams) Done. https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:140: if (1 == num_output_streams_) On 2013/03/21 20:58:16, tommi wrote: > To be on the safe side: > > if (stream && 1 == num_output_streams) Done. https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:140: if (1 == num_output_streams_) On 2013/03/21 21:26:21, Ami Fischman wrote: > On 2013/03/21 20:58:16, tommi wrote: > > To be on the safe side: > > > > if (stream && 1 == num_output_streams) > > better yet, move this into the if at l.136 above? Done. https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.cc:186: if (1 == num_input_streams_) On 2013/03/21 20:58:16, tommi wrote: > same here Done. https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/12974004/diff/33001/media/audio/audio_manager... media/audio/audio_manager_base.h:179: // Java AudioManagerAndroid instance. On 2013/03/21 21:26:21, Ami Fischman wrote: > s/Android// - the Java type doesn't have "Android" in its name. Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:23: private Context mContext = null; On 2013/03/21 21:26:21, Ami Fischman wrote: > The Java language does an implicit =null for you; these aren't necessary. Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:27: private static final int kPhoneTypeError = -1; On 2013/03/21 21:19:38, qinmin wrote: > java don't use this style. constants should be defined as PHONE_TYPE_ERROR. same > for everything below. Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:29: private static final int kPhoneTypePhone = 1; On 2013/03/21 21:26:21, Ami Fischman wrote: > "phonetypephone"? Really? Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:29: private static final int kPhoneTypePhone = 1; these are removed. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:43: private static AudioManagerAndroid createAudioManagerAndroid(Context context) { ctor binding doesn't exist in auto-generated jni file. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:53: AudioManager audioManager = getAudioManager(); On 2013/03/21 21:26:21, Ami Fischman wrote: > If this was final you could drop the copy/pasta at l.67. > (unless it's possible for getAudioManager()'s return value to change between > calls; is that possible?) Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:54: if (null == audioManager) { On 2013/03/21 21:26:21, Ami Fischman wrote: > please reverse these comparisons to have the constant on the right. Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:57: mIsSpeakerOn = audioManager.isSpeakerphoneOn(); On 2013/03/21 21:26:21, Ami Fischman wrote: > mOriginalSpeakerStatus ? Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:66: int state = intent.getIntExtra("state", 0); On 2013/03/21 21:26:21, Ami Fischman wrote: > s/0/kUnplugged/ Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:66: int state = intent.getIntExtra("state", 0); On 2013/03/21 21:26:21, Ami Fischman wrote: > s/0/kUnplugged/ Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:73: if (kPhoneTypeError == ep) { On 2013/03/21 21:26:21, Ami Fischman wrote: > why isn't this using a Java enum with a switch statement? Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:85: if (null == mReceiver) { On 2013/03/21 21:02:48, nilesh wrote: > Why would mReceiver be null here. You assign it on the line above. > What I meant was to do some kind of checking if mReceiver is not null and > registerHeadsetReceiver is called. Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:93: if (mReceiver == null) { On 2013/03/21 21:26:21, Ami Fischman wrote: > how can this be true? Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:100: (AudioManager)mContext.getSystemService(Context.AUDIO_SERVICE); On 2013/03/21 21:26:21, Ami Fischman wrote: > use getAudioManager() Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:110: if (null == tm) { On 2013/03/21 21:26:21, Ami Fischman wrote: > how can this fail? Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:111: return -1; On 2013/03/21 21:26:21, Ami Fischman wrote: > kPhoneTypeError? Done. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:114: TelephonyManager.PHONE_TYPE_NONE ? kPhoneTypePhone : kPhoneTypeNone; This function is wrong, thanks for pointing it out. https://codereview.chromium.org/12974004/diff/33001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:118: return null == mContext ? On 2013/03/21 21:26:21, Ami Fischman wrote: > how can this be false? Done.
LGTM % nits https://codereview.chromium.org/12974004/diff/43002/media/audio/audio_manager... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/12974004/diff/43002/media/audio/audio_manager... media/audio/audio_manager_base.cc:138: if (1 == num_output_streams_) please review your CL and reverse the comparisons that don't conform to the order: variable operator constant https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:27: private static final int STATE_UNPLUGGED = 0; nit My point in an earlier comment was: Why aren't these an enum like: private static enum State { UNPLUGGED, PLUGGED }; But all these are used for now is to give an int a name and immediately switch on the name, which seems silly. l.61-70 are just saying: audioManager.setSpeakerphoneOn(intent.getIntExtra("state", 0) == 0); I would simply drop these constants. https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:33: audioManager.setMode(mode); nit You sure do like temporary variables :) getAudioManager().setMode(mode); https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:55: filter.addAction(Intent.ACTION_HEADSET_PLUG); Do these two lines have an effect different to: new IntentFilter(Intent.ACTION_HEADSET_PLUG); ? https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:62: // state 0 for unplugged, 1 for plugged. comment is silly now? https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:83: audioManager.setSpeakerphoneOn(mOriginalSpeakerStatus); Since you seem to be saying that getAudioManager() can never return null, you can store a mAudioManager in the ctor and use it directly in the subsequent call sites.
more nits https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:12: import android.telephony.TelephonyManager; Remove unused imports https://codereview.chromium.org/12974004/diff/43002/media/base/android/java/s... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:23: private Context mContext; this can be final now. Also group final and non-final variables together,
Thanks Ami, Nilesh, please take another look. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/audio/audio_... File media/audio/audio_manager_base.cc (right): https://chromiumcodereview.appspot.com/12974004/diff/43002/media/audio/audio_... media/audio/audio_manager_base.cc:138: if (1 == num_output_streams_) On 2013/03/21 23:52:58, Ami Fischman wrote: > please review your CL and reverse the comparisons that don't conform to the > order: > variable operator constant Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:12: import android.telephony.TelephonyManager; On 2013/03/22 00:43:27, nilesh wrote: > Remove unused imports Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:23: private Context mContext; On 2013/03/22 00:43:27, nilesh wrote: > this can be final now. > Also group final and non-final variables together, Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:27: private static final int STATE_UNPLUGGED = 0; On 2013/03/21 23:52:58, Ami Fischman wrote: > nit > > My point in an earlier comment was: > Why aren't these an enum like: > private static enum State { UNPLUGGED, PLUGGED }; > > But all these are used for now is to give an int a name and immediately switch > on the name, which seems silly. > l.61-70 are just saying: > > audioManager.setSpeakerphoneOn(intent.getIntExtra("state", 0) == 0); > > I would simply drop these constants. Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:33: audioManager.setMode(mode); On 2013/03/21 23:52:58, Ami Fischman wrote: > nit > You sure do like temporary variables :) > getAudioManager().setMode(mode); Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:33: audioManager.setMode(mode); On 2013/03/21 23:52:58, Ami Fischman wrote: > nit > You sure do like temporary variables :) > getAudioManager().setMode(mode); Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:55: filter.addAction(Intent.ACTION_HEADSET_PLUG); On 2013/03/21 23:52:58, Ami Fischman wrote: > Do these two lines have an effect different to: > new IntentFilter(Intent.ACTION_HEADSET_PLUG); > ? Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:55: filter.addAction(Intent.ACTION_HEADSET_PLUG); On 2013/03/21 23:52:58, Ami Fischman wrote: > Do these two lines have an effect different to: > new IntentFilter(Intent.ACTION_HEADSET_PLUG); > ? Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:62: // state 0 for unplugged, 1 for plugged. On 2013/03/21 23:52:58, Ami Fischman wrote: > comment is silly now? Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:62: // state 0 for unplugged, 1 for plugged. On 2013/03/21 23:52:58, Ami Fischman wrote: > comment is silly now? Done. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:83: audioManager.setSpeakerphoneOn(mOriginalSpeakerStatus); On 2013/03/21 23:52:58, Ami Fischman wrote: > Since you seem to be saying that getAudioManager() can never return null, you > can store a mAudioManager in the ctor and use it directly in the subsequent call > sites. Done.
LGTM with one more comment. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:12: import android.telephony.TelephonyManager; On 2013/03/22 01:33:13, leozwang1 wrote: > On 2013/03/22 00:43:27, nilesh wrote: > > Remove unused imports > > Done. remove android.util.Log too
Thanks Nilesh so much! Friendly ping other reviewers, if there is no more comments, I will commit it today. https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/12974004/diff/43002/media/base/android... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:12: import android.telephony.TelephonyManager; On 2013/03/22 01:43:43, nilesh wrote: > On 2013/03/22 01:33:13, leozwang1 wrote: > > On 2013/03/22 00:43:27, nilesh wrote: > > > Remove unused imports > > > > Done. > > remove android.util.Log too Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leozwang@chromium.org/12974004/50009
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leozwang@chromium.org/12974004/50009
Failed to apply patch for media/audio/audio_manager_base.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
patching file media/audio/audio_manager_base.cc
Hunk #1 FAILED at 63.
Hunk #2 FAILED at 125.
Hunk #3 FAILED at 166.
Hunk #4 FAILED at 256.
Hunk #5 FAILED at 371.
5 out of 5 hunks FAILED -- saving rejects to file
media/audio/audio_manager_base.cc.rej
Patch: media/audio/audio_manager_base.cc
Index: media/audio/audio_manager_base.cc
===================================================================
--- media/audio/audio_manager_base.cc (revision 189667)
+++ media/audio/audio_manager_base.cc (working copy)
@@ -63,6 +63,13 @@
CHECK(audio_thread_->Start());
#endif
message_loop_ = audio_thread_->message_loop_proxy();
+
+#if defined(OS_ANDROID)
+ JNIEnv* env = base::android::AttachCurrentThread();
+ jobject context = base::android::GetApplicationContext();
+ j_audio_manager_.Reset(
+ Java_AudioManagerAndroid_createAudioManagerAndroid(env, context));
+#endif
}
AudioManagerBase::~AudioManagerBase() {
@@ -125,8 +132,13 @@
break;
}
- if (stream)
+ if (stream) {
++num_output_streams_;
+#if defined(OS_ANDROID)
+ if (num_output_streams_ == 1)
+ RegisterHeadsetReceiver();
+#endif
+ }
return stream;
}
@@ -166,13 +178,13 @@
break;
}
- if (stream)
+ if (stream) {
++num_input_streams_;
-
#if defined(OS_ANDROID)
- if (num_input_streams_ == 1)
- SetAudioMode(kAudioModeInCommunication);
+ if (num_input_streams_ == 1)
+ SetAudioMode(kAudioModeInCommunication);
#endif
+ }
return stream;
}
@@ -256,6 +268,10 @@
// streams.
--num_output_streams_;
delete stream;
+#if defined(OS_ANDROID)
+ if (!num_output_streams_)
+ UnregisterHeadsetReceiver();
+#endif
}
void AudioManagerBase::ReleaseInputStream(AudioInputStream* stream) {
@@ -371,12 +387,22 @@
#if defined(OS_ANDROID)
void AudioManagerBase::SetAudioMode(int mode) {
- JNIEnv* env = base::android::AttachCurrentThread();
- jobject context = base::android::GetApplicationContext();
- DCHECK(context);
+ Java_AudioManagerAndroid_setMode(
+ base::android::AttachCurrentThread(),
+ j_audio_manager_.obj(), mode);
+}
- Java_AudioManagerAndroid_setMode(env, context, mode);
+void AudioManagerBase::RegisterHeadsetReceiver() {
+ Java_AudioManagerAndroid_registerHeadsetReceiver(
+ base::android::AttachCurrentThread(),
+ j_audio_manager_.obj());
}
-#endif
+void AudioManagerBase::UnregisterHeadsetReceiver() {
+ Java_AudioManagerAndroid_unregisterHeadsetReceiver(
+ base::android::AttachCurrentThread(),
+ j_audio_manager_.obj());
+}
+#endif // defined(OS_ANDROID)
+
} // namespace media
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
