1026727 - Unknown state is displayed instead of Connected during manual network selection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Anshul]

STR
1. Go to Settings -> Cellular & Data -> Network Operator and disable automatic selection
2. Wait for the list of networks to be displayed

I see the following two issues.

1) Observed: The currently selected network shows as "unknown state".
   Expected: The currently selected network should show as "Connected".
2) If the short name returned by RIL is null:
   Observed: Settings app shows the network name as "null"
   Expected: Settings app should show the long name instead

Comment 1

[Anshul]

Both the issues are regressions due to bug 898445. The reason is that now RILContentHelper.js at http://lxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#1905 calls "new MozMobileNetworkInfo" which at http://lxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileNetworkInfo.webidl#5 only supports "available", "connected", "forbidden" while as per ril.h the possible values are "unknown", "available", "current" and "forbidden".

Note that difference between current in ril.h and connected in MozMobileNetworkInfo.webidl. The same definition of "connected" was working fine earlier as there was no strong typing to enum as is done now.

For the issue #2, the null shortname is being translated to "null" string causing the short name check at http://lxr.mozilla.org/gaia/source/apps/settings/js/carrier.js#512 to find a non null shortname and use it. I have confirmed that RILContentHelper.js received shortname as null as expected and so I suspect that http://lxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileNetworkInfo.webidl#15 is causing the null to be converted to "null" string.

Comment 2

[Edgar Chen [:edgar]]

I will check this, thank you.

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 1: The attributes of MozMobileNetworkInfo could be null, so the parameters of constructor should be nullable, v1

This fixes the "null" string issue. (issue #2 of comment #1)

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 2: Fixing the "Unknown" state is displayed instead of "Connected" during manual network selection, v1

This fixes the selected network shows "Unknown" instead of "Connected".

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8442583 [details] [diff] [review]
Part 1: The attributes of MozMobileNetworkInfo could be null, so the parameters of constructor should be nullable, v1

The attributes of MozMobileNetworkInfo is nullable, so the parameters of constructor should be nullable as well.

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8442628 [details] [diff] [review]
Part 2: Fixing the "Unknown" state is displayed instead of "Connected" during manual network selection, v1

Per ril.h, the possible values are "unknown", "available", "current" and "forbidden" [1]. And the values defined in MozMobileNetworkInfo.webidl are null, "available", "connected", "forbidden" [2]. We should not use the values from ril parcel directly, but remap it to the gecko one.

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L2044-L2048
[2] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileNetworkInfo.webidl?from=MozMobileNetworkInfo.webidl&case=true#5,32-35

Comment 7

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

Comment on attachment 8442628 [details] [diff] [review]
Part 2: Fixing the "Unknown" state is displayed instead of "Connected" during manual network selection, v1

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

Looks chic!

Comment 8

[Anshul]

(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> Comment on attachment 8442628 [details] [diff] [review]
> Part 2: Fixing the "Unknown" state is displayed instead of "Connected"
> during manual network selection, v1
> 
> Per ril.h, the possible values are "unknown", "available", "current" and
> "forbidden" [1]. And the values defined in MozMobileNetworkInfo.webidl are
> null, "available", "connected", "forbidden" [2]. We should not use the
> values from ril parcel directly, but remap it to the gecko one.
Just curious, what's the reason for not using the values from RIL directly. If the value returned by RIL doesn't match the existing values then we could simply show unknown state. To me it seems unnecessary to maintain the valid state values in RIL, Telephony and DOM.

> 
> [1]
> https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/
> telephony/ril.h#L2044-L2048
> [2]
> http://dxr.mozilla.org/mozilla-central/source/dom/webidl/
> MozMobileNetworkInfo.webidl?from=MozMobileNetworkInfo.webidl&case=true#5,32-
> 35

Comment 9

[Edgar Chen [:edgar]]

(In reply to Anshul from comment #8)
> (In reply to Edgar Chen [:edgar][:echen] from comment #6)
> > Comment on attachment 8442628 [details] [diff] [review]
> > Part 2: Fixing the "Unknown" state is displayed instead of "Connected"
> > during manual network selection, v1
> > 
> > Per ril.h, the possible values are "unknown", "available", "current" and
> > "forbidden" [1]. And the values defined in MozMobileNetworkInfo.webidl are
> > null, "available", "connected", "forbidden" [2]. We should not use the
> > values from ril parcel directly, but remap it to the gecko one.
> Just curious, what's the reason for not using the values from RIL directly.
> If the value returned by RIL doesn't match the existing values then we could
> simply show unknown state. To me it seems unnecessary to maintain the valid
> state values in RIL, Telephony and DOM.

Hi Anshul:

It is because,
1. We already define 'connected' state in API level for a long time. I guess exposing 'current' was an
   accident, and no one noticed this because there is no strong type checking in enum before.
2. And IMO 'connected' is much clearer than 'current' from the API point of view.

Thank you.

Comment 10

[Edgar Chen [:edgar]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=fb341c684cc8

Comment 11

[Anshul]

(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> (In reply to Anshul from comment #8)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #6)
> > > Comment on attachment 8442628 [details] [diff] [review]
> > > Part 2: Fixing the "Unknown" state is displayed instead of "Connected"
> > > during manual network selection, v1
> > > 
> > > Per ril.h, the possible values are "unknown", "available", "current" and
> > > "forbidden" [1]. And the values defined in MozMobileNetworkInfo.webidl are
> > > null, "available", "connected", "forbidden" [2]. We should not use the
> > > values from ril parcel directly, but remap it to the gecko one.
> > Just curious, what's the reason for not using the values from RIL directly.
> > If the value returned by RIL doesn't match the existing values then we could
> > simply show unknown state. To me it seems unnecessary to maintain the valid
> > state values in RIL, Telephony and DOM.
> 
> Hi Anshul:
> 
> It is because,
> 1. We already define 'connected' state in API level for a long time. I guess
> exposing 'current' was an
>    accident, and no one noticed this because there is no strong type
> checking in enum before.
I am proposing to change the API to use 'current' instead of connected even though it was incorrectly present in the API since the long time. I understand your point of view completely but I am trying to reduce unnecessary conversion functions in telephony code when they can be avoided. Anyways I don't have a strong opinion about it so I leave the final decision to you.
> 2. And IMO 'connected' is much clearer than 'current' from the API point of
> view.
Sure. That can be taken care of by Gaia to print "Connected" when the state is current.
> Thank you.

Comment 12

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5830932fb40f
https://hg.mozilla.org/integration/b2g-inbound/rev/aed60908cd17

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5830932fb40f
https://hg.mozilla.org/mozilla-central/rev/aed60908cd17

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4f56f62327c
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a4bbb56e146

Comment 15

[Brogan Zumwalt [Inactive]]

Test case exists that addresses this bug: https://moztrap.mozilla.org/manage/case/3539/

Expected results may need to be slightly altered.

Comment 16

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case updated in moztrap:

https://moztrap.mozilla.org/manage/case/3539/

Comment 17

[Elie]

Attachment: VIDEO0157_Compress.MP4

This issue has been successfully verified on Flame 2.0:
Gaia-Rev        856863962362030174bae4e03d59c3ebbc182473
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1
Build-ID        20141207000206
Version         32.0
Device-Name     flame
FW-Release      4.4.2

This issue has been successfully verified on Flame 2.1:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2