1052842 - Use enums for NetworkSelectionMode in nsIMobileConnectionProvider.idl

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Anshul]

The bug 898445 introduced the  MobileNetworkSelectionMode enum in the MozMobileConnection.webidl but no corresponding constants were added in nsIMobileConnectionProvider.idl. 

Please consider adding enums for network selection mode for getNetworkSelectionMode API in nsIMobileConnectionProvider.idl to strictly define the IPDL interface.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Jessica, I guess you wanna take this bug instead of being a QA Contact for this ;)

Comment 2

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Jessica, I guess you wanna take this bug instead of being a QA Contact for
> this ;)

Yes! thank you Hsinyi :)

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v1.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom/ipc changes, v1.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: gonk changes, v1.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: bt changes, v1.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8505369 [details] [diff] [review]
Part 1: idl changes, v1.

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

Thank you, Jessica!

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8505370 [details] [diff] [review]
Part 2: dom/ipc changes, v1.

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

r=me given that the change here is trivial. Thank you.

::: dom/mobileconnection/Assertions.cpp
@@ +11,5 @@
> +#define ASSERT_NETWORK_SELECTION_MODE_EQUALITY(webidlState, xpidlState) \
> +  static_assert(static_cast<int32_t>(MobileNetworkSelectionMode::webidlState) == nsIMobileConnection::xpidlState, \
> +                "MobileNetworkSelectionMode::" #webidlState " should equal to nsIMobileConnection::" #xpidlState)
> +
> +

nit: one blank should be enough.

@@ +15,5 @@
> +
> + ASSERT_NETWORK_SELECTION_MODE_EQUALITY(Automatic, NETWORK_SELECTION_MODE_AUTOMATIC);
> + ASSERT_NETWORK_SELECTION_MODE_EQUALITY(Manual, NETWORK_SELECTION_MODE_MANUAL);
> +
> +

Ditto.

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8505372 [details] [diff] [review]
Part 3: gonk changes, v1.

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

Thank you.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8505963 [details] [diff] [review]
Part 4: bt changes, v1.

Ben, we are using integer values instead of strings for selection mode to match MobileNetworkSelectionMode enum in webidl. May I have your review? Thanks.

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8505963 [details] [diff] [review]
Part 4: bt changes, v1.

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

LGTM.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 1: idl changes. r=hsinyi

Rebase and add r=hsinyi

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 2: dom/ipc changes. r=echen

Rebase, address nits in comment 8 and add r=echen

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 3: gonk changes. r=echen

Rebase and add r=echen

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 4: bt changes. r=btian

Rebase and add r=btian

Thanks for the review!

Comment 16

[Jessica Jong [:jessica] (inactive)]

try looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a84a7e7de4e9

Comment 17

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/febb1c493787
https://hg.mozilla.org/integration/b2g-inbound/rev/31aa7c1afbaf
https://hg.mozilla.org/integration/b2g-inbound/rev/76b4ab718c94
https://hg.mozilla.org/integration/b2g-inbound/rev/fffcb91f4fd1

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/febb1c493787
https://hg.mozilla.org/mozilla-central/rev/31aa7c1afbaf
https://hg.mozilla.org/mozilla-central/rev/76b4ab718c94
https://hg.mozilla.org/mozilla-central/rev/fffcb91f4fd1

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: (follow-up): nsIGonkMobileConnectionService.idl changes.

I missed this. :(

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8510955 [details] [diff] [review]
(follow-up): nsIGonkMobileConnectionService.idl changes.

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

Thanks!

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Reopened for the follow-up part.

Comment 22

[Jessica Jong [:jessica] (inactive)]

try for follow-up part:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43a0498c4ea6

Comment 23

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/129bd767cbfa

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/129bd767cbfa