Closed Bug 1026727 Opened 6 years ago Closed 6 years ago

Unknown state is displayed instead of Connected during manual network selection

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.4 unaffected, b2g-v2.0 verified, b2g-v2.1 verified)

RESOLVED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: anshulj, Assigned: edgar)

References

()

Details

(Keywords: regression, Whiteboard: [p=1])

Attachments

(3 files)

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
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.
Blocks: 898445
Keywords: regression
I will check this, thank you.
Assignee: nobody → echen
No longer blocks: 898445
Depends on: 898445
blocking-b2g: 2.0? → 2.0+
Whiteboard: [ETA:6/27]
Target Milestone: --- → 2.0 S5 (4july)
Summary: Unknown state is displayed instead of current during manual network selection → Unknown state is displayed instead of Connected during manual network selection
This fixes the selected network shows "Unknown" instead of "Connected".
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.
Attachment #8442583 - Flags: review?(htsai)
Attachment #8442583 - Flags: review?(bugs)
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
Attachment #8442628 - Flags: review?(htsai)
Attachment #8442583 - Flags: review?(htsai) → review+
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!
Attachment #8442628 - Flags: review?(htsai) → review+
(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
Attachment #8442583 - Flags: review?(bugs) → review+
(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.
Whiteboard: [ETA:6/27] → [ETA:6/27][p=1]
(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.
Flags: in-moztrap?(bzumwalt)
Test case exists that addresses this bug: https://moztrap.mozilla.org/manage/case/3539/

Expected results may need to be slightly altered.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Test case updated in moztrap:

https://moztrap.mozilla.org/manage/case/3539/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap+
Attached video 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
You need to log in before you can comment on or make changes to this bug.