B2G RIL: Add lastKnownMcc into nsIDOMMobileConnectionInfo

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: allstars.chh, Unassigned)

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
(Assignee)

Updated

6 years ago
Blocks: 833714
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)

Updated

6 years ago
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
Created attachment 705789 [details] [diff] [review]
Part 1: Add lastKnownMcc into nsIDOMMobileConnectionInfo.
Created attachment 705790 [details] [diff] [review]
Part 2: Add lastKnownMcc in RIL.
Created attachment 705791 [details] [diff] [review]
Part 4: marionette for lastKnownMcc.
Created attachment 705796 [details] [diff] [review]
Part 2: Move lastKnownMcc into voice connection. v2

Merged Part 5 patch from Bug 830164.
Attachment #705790 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #705789 - Flags: review?(vyang)
(Assignee)

Updated

6 years ago
Attachment #705796 - Flags: review?(vyang)
(Assignee)

Updated

6 years ago
Attachment #705791 - Flags: review?(vyang)
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)
Created attachment 706209 [details] [diff] [review]
Part 2: Move lastKnownMcc into voice connection. v3

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.
Created attachment 706326 [details] [diff] [review]
Part 2: Move lastKnownMcc into voice connection. v4
Attachment #706209 - Attachment is obsolete: true
Created attachment 706334 [details] [diff] [review]
Part 2: Move lastKnownMcc into voice connection. v4
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+
Created attachment 706942 [details] [diff] [review]
Part 2 (new) :  Revise handleoperatorchange in RadioInterfaceLayer and updateInfo in RILContentHelper.

As Vicamo suggests to split code into another patch.
This patch is already r+
Created attachment 706944 [details] [diff] [review]
Part 3: Move lastKnownMcc into voice connection. v4 (Former Part 2)
Attachment #706334 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #705791 - Attachment description: Part 3: marionette for lastKnownMcc. → Part 4: marionette for lastKnownMcc.
Created attachment 706971 [details] [diff] [review]
Part 1: Add lastKnownMcc into nsIDOMMobileConnectionInfo. v2

rebase
Attachment #705789 - Attachment is obsolete: true

Comment 24

6 years ago
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

Comment 26

6 years ago
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.