843958 - Unused enum value in BluetoothUnixSocketConnector::SetUp

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't know if this is important or not but we should fix this warning in any case.

/home/bent/src/mozilla/firefox/dom/bluetooth/BluetoothUnixSocketConnector.cpp:78:10: warning: enumeration value ‘SCO’ not handled in switch [-Wswitch]

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Add warning about SCO not handling LM

Comment 2

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 718645 [details] [diff] [review]
Patch 1 (v1) - Add warning about SCO not handling LM

Review of attachment 718645 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Kyle, please take a look of my comment. Thank you.

::: dom/bluetooth/BluetoothUnixSocketConnector.cpp
@@ +86,5 @@
>      break;
> +  case BluetoothSocketType::SCO:
> +    if (mAuth || mEncrypt) {
> +      NS_WARNING("LM not yet supported for SCO!");
> +      return false;

I think no need to check mAuth & mEncrypt here. Just return true should be fine.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok! I was being overly paranoid, didn't know if it mattered or not. Will scale that back and resubmit. :)

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Fix compiler warning on SCO not handling LM

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 719155 [details] [diff] [review]
Patch 1 (v2) - Fix compiler warning on SCO not handling LM

Shouldn't the 'default' branch at least issue a warning? I'd hope it could be MOZ_NOT_REACHED actually.

Comment 6

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 719155 [details] [diff] [review]
Patch 1 (v2) - Fix compiler warning on SCO not handling LM

Review of attachment 719155 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks! Also, according to Ben's comment, please add MOZ_NOT_REACHED for condition 'default'.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3817c9dc83d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3817c9dc83d

Comment 9

[Eric Chou [:ericchou] [:echou]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f3817c9dc83d

Hey Kyle, I think you may misunderstand what I meant in comment 6. For BluetoothSocketType::SCO, we should return true; for other cases (default), call MOZ_NOT_REACHED(). It looks like you miss the "return true", so it would crash once SCO connection is established.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Support SCO in switch statement

This fixes a crash that was introduced by the first patch.

Comment 11

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 723897 [details] [diff] [review]
[02] Support SCO in switch statement

Review of attachment 723897 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good to me, but I'll let Kyle decide if we could return earlier in the SCO case. (which was the basic idea of Kyle's patch)

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Oh, I thought the attached patch was the one that is already committed to m-c. I should have had a look instead. I really don't care which patch goes in as long as it fixes the crash. But thanks for your review.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, return early is fine.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I'll go ahead and land this now, since it's breaking pretty much everything. :)

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Or at least will when m-i reopens or I can hang out to babysit an m-c merge. :|

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b15e0890b1f

(Yes m-i just came back up :( )

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8b15e0890b1f