Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2129763002:
bluetooth: Only unregister the receiver if dialog hasn't been closed. (Closed)
Descriptionbluetooth: Only unregister the receiver if dialog hasn't been closed.
Also modifies nativeOnDialogFinished to more closely resemble the
non-test nativeOnDialogFinished function. This change would have
allowed us to catch the problem with the receiver.
BUG=625398
Committed: https://crrev.com/d04c4837088288ebe2d6348e29018f754a1f91d7
Cr-Commit-Position: refs/heads/master@{#404170}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use a boolean to keep track of the receiver #
Total comments: 2
Messages
Total messages: 24 (9 generated)
ortuno@chromium.org changed reviewers: + scheib@chromium.org
scheib: PTAL
LGTM
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ortuno@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara: I usually send android bluetooth patches to tedchoc for owners review, but he's OOO. Would you mind taking a look?
lgtm % question https://codereview.chromium.org/2129763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2129763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:160: mActivity.unregisterReceiver(mLocationModeBroadcastReceiver); Is there any way that the dialog can be created but not have Show() called? The current code path seems to prevent that from happening. Just wondering if it makes more sense to keep an boolean that explicitly tracks the registration state instead of implicitly using the pointer to track that.
Thanks! https://codereview.chromium.org/2129763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2129763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:160: mActivity.unregisterReceiver(mLocationModeBroadcastReceiver); On 2016/07/06 at 21:30:11, dfalcantara wrote: > Is there any way that the dialog can be created but not have Show() called? The current code path seems to prevent that from happening. Just wondering if it makes more sense to keep an boolean that explicitly tracks the registration state instead of implicitly using the pointer to track that. Hmm yeah I think that will be safer. Done.
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm https://codereview.chromium.org/2129763002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2129763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:70: boolean mIsLocationModeChangedReceiverRegistered = false; Default value for booleans in Java is false, so adding "= false" isn't necessary unless you really want to be explicit about it.
https://codereview.chromium.org/2129763002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java (right): https://codereview.chromium.org/2129763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:70: boolean mIsLocationModeChangedReceiverRegistered = false; On 2016/07/06 at 22:07:10, dfalcantara wrote: > Default value for booleans in Java is false, so adding "= false" isn't necessary unless you really want to be explicit about it. In C++ the value is undefined so I developed the habit of always initializing booleans. I rather have it have an explicit value but I can change it if non-explicit is how it's usually done java code :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2129763002/#ps20001 (title: "Use a boolean to keep track of the receiver")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Only unregister the receiver if dialog hasn't been closed. Also modifies nativeOnDialogFinished to more closely resemble the non-test nativeOnDialogFinished function. This change would have allowed us to catch the problem with the receiver. BUG=625398 ========== to ========== bluetooth: Only unregister the receiver if dialog hasn't been closed. Also modifies nativeOnDialogFinished to more closely resemble the non-test nativeOnDialogFinished function. This change would have allowed us to catch the problem with the receiver. BUG=625398 Committed: https://crrev.com/d04c4837088288ebe2d6348e29018f754a1f91d7 Cr-Commit-Position: refs/heads/master@{#404170} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d04c4837088288ebe2d6348e29018f754a1f91d7 Cr-Commit-Position: refs/heads/master@{#404170} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
