Closed Bug 995109 Opened 10 years ago Closed 10 years ago

B2G RIL: Don't handle |dataInfo.connected| in DataConnectionHandler.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S6 (25apr)
tracking-b2g backlog

People

(Reporter: edgar, Assigned: edgar)

References

Details

(Whiteboard: [p=2])

Attachments

(1 file, 2 obsolete files)

The value of |dataInfo.connected| means the "default" connection is established or not. And right now we will update |dataInfo.connected| in DataConnectionHandler in some case. This makes bug 939046 hard to move on. So I would like to clean up it in this bug.
Whiteboard: [p=2]
Target Milestone: --- → 1.4 S6 (25apr)
blocking-b2g: --- → backlog
Attached patch WIP, Patch, v1 (obsolete) — Splinter Review
We can update |dataInfo.connected| by monitoring 'network-active-changed' and checking |NetworkManager.active| [1].

But right now, if active becomes to null, there is no 'network-active-changed' propagated. We need to fix this first.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsINetworkManager.idl?from=nsINetworkManager.idl#147-153
Depends on: 996409
(In reply to Edgar Chen [:edgar][:echen] from comment #1)
> Created attachment 8406088 [details] [diff] [review]
> WIP, Patch, v1
> 
> We can update |dataInfo.connected| by monitoring 'network-active-changed'
> and checking |NetworkManager.active| [1].
> 
> But right now, if active becomes to null, there is no
> 'network-active-changed' propagated. We need to fix this first.
Filed a bug for this, bug 996409.

> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> nsINetworkManager.idl?from=nsINetworkManager.idl#147-153
Comment on attachment 8406088 [details] [diff] [review]
WIP, Patch, v1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2425,4 @@
>      dataInfo.connected = false;
> +    if (gNetworkManager.active &&
> +        gNetworkManager.active.type ===
> +          Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE) {

We need to consider |serviceId| as well, will address it in next version.

@@ +3341,5 @@
> +        let dataInfo = this.rilContext.data;
> +        let connected = false;
> +        if (gNetworkManager.active &&
> +            gNetworkManager.active.type ===
> +              Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE) {

ditto
Attached patch Patch, v2 (obsolete) — Splinter Review
Address comment #3.
Attachment #8406088 - Attachment is obsolete: true
Attachment #8408089 - Flags: review?(htsai)
Comment on attachment 8408089 [details] [diff] [review]
Patch, v2

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

Thank you :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3346,5 @@
> +          connected = true;
> +        }
> +        if (dataInfo.connected !== connected) {
> +          dataInfo.connected = connected;
> +          gMessageManager.sendMobileConnectionMessage("RIL:DataInfoChanged",

Eventually, RadioInterface won't need to observe this topic, and rilContext.data won't care. Let's do the cleanup in bug 843452, then the patch looks fine now.
Attachment #8408089 - Flags: review?(htsai) → review+
Blocks: 843452
Comment on attachment 8408089 [details] [diff] [review]
Patch, v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3349,5 @@
> +          dataInfo.connected = connected;
> +          gMessageManager.sendMobileConnectionMessage("RIL:DataInfoChanged",
> +                                                      this.clientId, dataInfo);
> +        }
> +        break

Oh, missing a semicolon here, my bad. :(
Address comment #6.
Attachment #8408089 - Attachment is obsolete: true
Attachment #8408867 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e7e1f3c312ab
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.