Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2172063002:
arc: bluetooth: Don't unset bluetooth_adapter_ (Closed)
|
Created:
4 years, 5 months ago by puthik_chromium Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Remove AdapterPresentChanged override
There is no reason to unset bluetooth_adapter_. If the adapter is
not present, making calls to it will simply invoke the error callback.
Just removing this should fix the crash.
BUG=630069
TEST=build
Committed: https://crrev.com/f1cfa2249cf447ad0b257fad881eb948d24483cb
Cr-Commit-Position: refs/heads/master@{#407590}
Patch Set 1 #
Total comments: 3
Patch Set 2 : arc: bluetooth: Remove AdapterPresentChanged override #Patch Set 3 : rebase #
Messages
Total messages: 26 (15 generated)
puthik@chromium.org changed reviewers: + lhchavez@chromium.org, rkc@chromium.org
From suggestion in http://crbug.com/630069#c6
https://codereview.chromium.org/2172063002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (left): https://codereview.chromium.org/2172063002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:110: bluetooth_adapter_ = nullptr; is it guaranteed that the adapter (reference) will never change? if so, nit: if (present || adapter != bluetooth_adapter_) return; adapter->RemoveObserver(this);
https://codereview.chromium.org/2172063002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (left): https://codereview.chromium.org/2172063002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:105: void ArcBluetoothBridge::AdapterPresentChanged(BluetoothAdapter* adapter, Why is this override necessary at all? If the adapter becomes available again, we won't get that notification at all with this code.
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Description was changed from ========== arc: bluetooth: Don't unset bluetooth_adapter_ There is no reason to unset bluetooth_adapter_. If the adapter is not present, making calls to it will simply invoke the error callback. Just removing this should fix the crash. BUG=630069 TEST=build ========== to ========== arc: bluetooth: Remove AdapterPresentChanged override There is no reason to unset bluetooth_adapter_. If the adapter is not present, making calls to it will simply invoke the error callback. Just removing this should fix the crash. BUG=630069 TEST=build ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2172063002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (left): https://codereview.chromium.org/2172063002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:105: void ArcBluetoothBridge::AdapterPresentChanged(BluetoothAdapter* adapter, On 2016/07/22 19:59:48, rkc wrote: > Why is this override necessary at all? > If the adapter becomes available again, we won't get that notification at all > with this code. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
rs-lgtm since you're just removing code and rkc@ already reviewed.
The CQ bit was checked by puthik@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by puthik@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...
The CQ bit was unchecked by puthik@chromium.org
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2172063002/#ps40001 (title: "rebase")
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.
Description was changed from ========== arc: bluetooth: Remove AdapterPresentChanged override There is no reason to unset bluetooth_adapter_. If the adapter is not present, making calls to it will simply invoke the error callback. Just removing this should fix the crash. BUG=630069 TEST=build ========== to ========== arc: bluetooth: Remove AdapterPresentChanged override There is no reason to unset bluetooth_adapter_. If the adapter is not present, making calls to it will simply invoke the error callback. Just removing this should fix the crash. BUG=630069 TEST=build ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Remove AdapterPresentChanged override There is no reason to unset bluetooth_adapter_. If the adapter is not present, making calls to it will simply invoke the error callback. Just removing this should fix the crash. BUG=630069 TEST=build ========== to ========== arc: bluetooth: Remove AdapterPresentChanged override There is no reason to unset bluetooth_adapter_. If the adapter is not present, making calls to it will simply invoke the error callback. Just removing this should fix the crash. BUG=630069 TEST=build Committed: https://crrev.com/f1cfa2249cf447ad0b257fad881eb948d24483cb Cr-Commit-Position: refs/heads/master@{#407590} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f1cfa2249cf447ad0b257fad881eb948d24483cb Cr-Commit-Position: refs/heads/master@{#407590} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
