B2G RIL: Add lastKnownMcc into nsIDOMMobileConnectionInfo

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: allstars.chh, Assigned: allstars.chh)

Tracking

unspecified
mozilla21
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

1.23 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.92 KB, patch
Details | Diff | Splinter Review
6.64 KB, patch
Details | Diff | Splinter Review
2.51 KB, patch
Details | Diff | Splinter Review
Although Bug 819560 adds lastKnownMcc, but it seemed that field was added into wrong place, it was added in MobileIccInfo.

The lastKnownMcc is from voice connection, so it should be added into nsIDOMMobileNetworkInfo.
CCing Gregor and Vicamo
Gregor, as I have discussed with you during Work Week,
seems we don't have to modify the IDL, 
we could just write lastKnownMcc into settigs/pref, right?

Otherwise if the phone reboots it still cannot read lastKnownMcc from MobileConnection.
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #2)
> Gregor, as I have discussed with you during Work Week,
> seems we don't have to modify the IDL, 
> we could just write lastKnownMcc into settigs/pref, right?
> 
> Otherwise if the phone reboots it still cannot read lastKnownMcc from
> MobileConnection.

If you call navigator.mozMobileConnection you still call the constructor right? And in the constructor we read the lastKnownMcc from the pref. So it should be accessible. But I am open for another solution here.
As Bug 830164 will remove lastKnownMcc from IccInfo, I mark this bug 833711 depends on bug 830164.
Depends on: 830164
Assignee: nobody → allstars.chh
As Vicamo suggests lastKnownMcc should not be in nsIDOMMobileNetworkInfo, because it doesn't belong to current network operator, but previous available network operator.
So I'll move lastKnownMcc into nsIDOMMobileConnectionInfo for voice connection.
Summary: B2G RIL: Add lastKnownMcc into nsIDOMMobileNetworkInfo → B2G RIL: Add lastKnownMcc into nsIDOMMobileConnectionInfo
Merged Part 5 patch from Bug 830164.
Attachment #705790 - Attachment is obsolete: true
Attachment #705789 - Flags: review?(vyang) → review+
Attachment #705791 - Flags: review?(vyang) → review+
Comment on attachment 705789 [details] [diff] [review]
Part 1: Add lastKnownMcc into nsIDOMMobileConnectionInfo.

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

Hi, Jonas

This bug is filed for Bug 819560 adds lastKnownMcc to wrong place, it adds lastKnowMcc into nsIDOMMobileIccInfo
which is wrong, since SIM card should have only one MCC.

My first patch is to add it into nsIDOMMobileNetworkInfo.

But Vicamo suggests since lastKnownMcc is not from current network operator, we should not add it into nsIDOMMobileNetworkInfo.
Instead, we should add lastKnownMcc into voice connection,
that is, nsIDOMMobileConnectionInfo. Please also see Comment 5.

thanks
Attachment #705789 - Flags: superreview?(jonas)
I found many functions in RILContentHelper are just to copy object,
so I add an utility updateInfo for that.
Attachment #705796 - Attachment is obsolete: true
Attachment #705796 - Flags: review?(vyang)
Comment on attachment 705789 [details] [diff] [review]
Part 1: Add lastKnownMcc into nsIDOMMobileConnectionInfo.

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

How is this different from previousMcc? They seem identical?
Comment on attachment 705789 [details] [diff] [review]
Part 1: Add lastKnownMcc into nsIDOMMobileConnectionInfo.

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

Hmm... ok.. i see. I don't know the interfaces under .mobileconnection well enough so if vicamo thinks this is good then that's fine with me.
Attachment #705789 - Flags: superreview?(jonas) → superreview+
Hi, Jonas
Actually It's the same with previousMcc,
but you commented in Bug 819560 comment 7 [1] to rename it to lastKnownMcc.

But at that time Gregor adds to wrong place, he adds to MobileIccInfo, which is for SIM card. 
Recall that SIM card has only one MCC, wherever you go.

So this bug is the move lastKnownMcc to correct place, into voice connection.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=819560#c7
Hi, Jonas 
Also I'd like to remind that the patch in Bug 819560 is an older one, it still uses 'previousMcc', but you can check the actual commit in Bug 819560 comment 8, it has been modified to 'lastKnownMcc'.
Comment on attachment 706209 [details] [diff] [review]
Part 2: Move lastKnownMcc into voice connection. v3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +773,5 @@
>        this.updateDataConnection(dataMessage);
>      }
>  
> +    if (operatorMessage) {
> +      this.handleOperatorChange(operatorMessage);

Here could be wrong, 
it will send a VoiceInfoChanged event.
Will address that.
Attachment #706326 - Attachment is obsolete: true
Attachment #706334 - Flags: review?(vyang)
Comment on attachment 706334 [details] [diff] [review]
Part 2: Move lastKnownMcc into voice connection. v4

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +773,5 @@
>        this.updateDataConnection(dataMessage);
>      }
>  
> +    if (operatorMessage) {
> +      operatorMessage.batch = true;

Please move these operator message handling changes into another patch or file another bug :)
Attachment #706334 - Flags: review?(vyang) → review+
As Vicamo suggests to split code into another patch.
This patch is already r+
Attachment #705791 - Attachment description: Part 3: marionette for lastKnownMcc. → Part 4: marionette for lastKnownMcc.
Try run for 423ce09fd88a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=423ce09fd88a
Results (out of 334 total builds):
    exception: 1
    success: 318
    warnings: 13
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-423ce09fd88a
Try run for 508f73f7c51f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=508f73f7c51f
Results (out of 337 total builds):
    exception: 1
    success: 327
    warnings: 7
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-508f73f7c51f
You need to log in before you can comment on or make changes to this bug.