bluetooth: Refactor repeated code used to find a Bluetooth object from its ID.

bluetooth: Refactor repeated code used to find a Bluetooth object from its ID.

getCharacteristic, getCharacteristics, getService, getServices, readValue,
writeValue, startNotifications, etc use the same code to find a Bluetooth Object
based on the object's instance ID. We refactor bluetooth_dispatcher_host so that
we don't have to repeat so much code.

BUG=545153
Committed: https://crrev.com/20550da3be779194f3d7eb2475cd5bb07e4894aa
Cr-Commit-Position: refs/heads/master@{#355199}

[ortuno, 2015-10-13 01:41:22]

Patchset #4 (id:60001) has been deleted

[ortuno, 2015-10-19 20:59:48]

Description was changed from

==========
bluetooth: Refactor repeated code that queries the bluetooth cache.

BUG=
==========

to

==========
bluetooth: Refactor repeated code used to find a Bluetooth object from its ID.

getCharacteristic, getCharacteristics, getService, getServices, readValue, 
writeValue, startNotifications, etc use the same code to find a Bluetooth Object
based on the object's instance ID. We refactor bluetooth_dispatcher_host so that
we don't have to repeat so much code.

BUG=545153
==========

[ortuno, 2015-10-19 21:00:00]

ortuno@chromium.org changed reviewers:
+ scheib@chromium.org

[ortuno, 2015-10-19 21:00:01]

scheib: PTAL

[scheib, 2015-10-19 23:15:54]

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:308:
blink::WebBluetoothError error;
Instead of storing 'error', just have an accessor that can get a
WebBluetoothError from the 'outcome' member.

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:599: if
(QueryCacheForService(service_instance_id, query_result)) {
Just feels like it should be
if(!QueryCache...)
  return; 
Because, we're returning because we failed to get the object out of cache.

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:604:
RecordOutcomeWithCacheQueryResult(
We already have to list the specific function (e.g. 'GetChacteristic'), so why
not make a call directly to a function named that way.

I propose RecordGetCharacteristicOutcome have an overload that takes the
query_result.outcome. That breaks up RecordOutcomeWithCacheQueryResult into many
functions that are more directly named, and don't have a switch of switches.

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:76: NOTIMPLEMENTED();
I suggest not using 'default' in switch statements. Without it the compiler will
inform developers when adding a new enum what they have to fix. List cases
explicitly that are not handled.

[ortuno, 2015-10-20 17:44:04]

Thanks!

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:308:
blink::WebBluetoothError error;
On 2015/10/19 23:15:54, scheib wrote:
> Instead of storing 'error', just have an accessor that can get a
> WebBluetoothError from the 'outcome' member.

Done.

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:599: if
(QueryCacheForService(service_instance_id, query_result)) {
On 2015/10/19 23:15:54, scheib wrote:
> Just feels like it should be
> if(!QueryCache...)
>   return; 
> Because, we're returning because we failed to get the object out of cache.

Removed the confusing return value and added an enum for the Renderer Crashed
outcome.

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:604:
RecordOutcomeWithCacheQueryResult(
On 2015/10/19 23:15:54, scheib wrote:
> We already have to list the specific function (e.g. 'GetChacteristic'), so why
> not make a call directly to a function named that way.
> 
> I propose RecordGetCharacteristicOutcome have an overload that takes the
> query_result.outcome. That breaks up RecordOutcomeWithCacheQueryResult into
many
> functions that are more directly named, and don't have a switch of switches.
> 

Done.

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/1397983004/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:76: NOTIMPLEMENTED();
On 2015/10/19 23:15:54, scheib wrote:
> I suggest not using 'default' in switch statements. Without it the compiler
will
> inform developers when adding a new enum what they have to fix. List cases
> explicitly that are not handled.

Done.

[scheib, 2015-10-20 17:55:33]

LGTM

https://codereview.chromium.org/1397983004/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1397983004/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:45: SUCCESS = 0,
FYI it's fine to make these explicit, but it's a bit overkill since they are
never serialized. The values only have meaning within the single executable
build, and if the values change in the next build nothing breaks.

[ortuno, 2015-10-20 17:57:07]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 17:57:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1397983004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1397983004/140001

[ortuno, 2015-10-20 17:59:11]

https://codereview.chromium.org/1397983004/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1397983004/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:45: SUCCESS = 0,
On 2015/10/20 at 17:55:33, scheib wrote:
> FYI it's fine to make these explicit, but it's a bit overkill since they are
never serialized. The values only have meaning within the single executable
build, and if the values change in the next build nothing breaks.

Ack.

[commit-bot: I haz the power, 2015-10-20 18:15:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-20 18:15:25]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ortuno, 2015-10-20 23:13:28]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 23:14:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1397983004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1397983004/160001

[commit-bot: I haz the power, 2015-10-21 00:09:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-21 00:09:47]

Dry run: This issue passed the CQ dry run.

[ortuno, 2015-10-21 00:10:03]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-10-21 00:10:03]

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/1397983004/#ps160001
(title: "Fix compile errors on bots")

[commit-bot: I haz the power, 2015-10-21 00:11:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1397983004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1397983004/160001

[commit-bot: I haz the power, 2015-10-21 00:20:20]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-10-21 00:21:53]

Patchset 8 (id:??) landed as
https://crrev.com/20550da3be779194f3d7eb2475cd5bb07e4894aa
Cr-Commit-Position: refs/heads/master@{#355199}