Closed Bug 843958 Opened 7 years ago Closed 7 years ago

Unused enum value in BluetoothUnixSocketConnector::SetUp

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bent.mozilla, Assigned: qdot)

References

Details

Attachments

(2 files, 1 obsolete file)

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]
Assignee: nobody → kyle
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.
Attachment #718645 - Flags: review?(echou)
Ok! I was being overly paranoid, didn't know if it mattered or not. Will scale that back and resubmit. :)
Attachment #719155 - Attachment description: Patch 1 (v2) - Add warning about SCO not handling LM → Patch 1 (v2) - Fix compiler warning on SCO not handling LM
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 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'.
Attachment #719155 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/f3817c9dc83d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(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.
Status: RESOLVED → REOPENED
Flags: needinfo?(kyle)
Resolution: FIXED → ---
This fixes a crash that was introduced by the first patch.
Attachment #723897 - Flags: review?(kyle)
Attachment #723897 - Flags: review?(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)
Attachment #723897 - Flags: review?(echou) → review+
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.
Duplicate of this bug: 849870
Attachment #723897 - Flags: review?(kyle) → review+
Yeah, return early is fine.
Flags: needinfo?(kyle)
I'll go ahead and land this now, since it's breaking pretty much everything. :)
Or at least will when m-i reopens or I can hang out to babysit an m-c merge. :|
https://hg.mozilla.org/mozilla-central/rev/8b15e0890b1f
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.