B2G MMS: Configure MMS proxy settings through SettingsService

RESOLVED FIXED in Firefox 20

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: vicamo, Assigned: Gene Lian (I already quit Mozilla))

Tracking

unspecified
mozilla20
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox20 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
See also bug 776294.
(Reporter)

Comment 1

6 years ago
Bug 772747 is going to introduce new settings "ril.mms.apn", "ril.mms.xxx". We should use these new settings names.
Depends on: 772747
(Assignee)

Comment 2

6 years ago
I used to have some experiences on the settings. If no one is looking into this one, I'm glad to take it. Please let me know.
(Reporter)

Comment 3

6 years ago
(In reply to Gene Lian [:gene] from comment #2)
> I used to have some experiences on the settings. If no one is looking into
> this one, I'm glad to take it. Please let me know.

Thank you a lot, but please wait until bug 772747 is settled down.
(Assignee)

Updated

6 years ago
Assignee: nobody → clian
(Assignee)

Comment 4

6 years ago
Is this a V1 feature or basecamp-blocker?

Comment 5

6 years ago
It's not a V1 feature, but since we already have part of MMS function working, and MMS is really an important feature, supporting this earlier will benefit on MMS development & testing.

Updated

6 years ago
Depends on: 805781

Updated

6 years ago
Blocks: 804679
(Assignee)

Comment 6

6 years ago
Since we're going to support some initiative testings recently, I'll start working on this to hook up the MMS back-end functions to Gaia-end.
Take a look at https://github.com/mozilla-b2g/gaia/pull/6483. That code is part of the operator variant logic.
(Assignee)

Comment 9

6 years ago
Created attachment 686031 [details] [diff] [review]
WIP Patch V2

Hi Vicamo,

Could you please return some feedback when you're available? Thanks! The following summarizes what I've done in this patch:

1. I used the following structure to cache the current MMS settings, which will be updated whenever the settings are changed or initialized when the MMS service starts up.

  MMSProxyInfoSettings: { "mmsc": null,
                          "mmsproxy": null,
                          "mmsport": null },

2. Another local variable |MMSNetworkConnected| is used to specify whether the MMS network is currently connected. Note that the |MMSProxyInfoSettings| is valid only when the |MMSNetworkConnected| is true.

3. An |MMSProxyInfoSettingsToRead| is used to specify whether the |MMSProxyInfoSettings| is completely read during initialization.

4. I understand there could be an edge case that |MMSProxyInfoSettings| is not completely updated during run-time but I prefer firing another bug to address that since it needs lots of changes in our current settings service mechanism.
Attachment #684673 - Attachment is obsolete: true
Attachment #686031 - Flags: feedback?(vyang)
(Assignee)

Comment 10

6 years ago
Created attachment 686036 [details] [diff] [review]
WIP Patch V2.1

Just some fine tunes. Please see the main summary in the comment #9.
Attachment #686031 - Attachment is obsolete: true
Attachment #686031 - Flags: feedback?(vyang)
Attachment #686036 - Flags: feedback?(vyang)
(Reporter)

Comment 11

6 years ago
Comment on attachment 686036 [details] [diff] [review]
WIP Patch V2.1

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

It's nice! Let's do it this way and hopefully we can integrate all the mms related setting entries into one someday.
Attachment #686036 - Flags: feedback?(vyang) → feedback+
(Assignee)

Comment 12

6 years ago
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> Comment on attachment 686036 [details] [diff] [review]
> WIP Patch V2.1
> 
> Review of attachment 686036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's nice! Let's do it this way and hopefully we can integrate all the mms
> related setting entries into one someday.

Thanks for the review! I'll come back with a formal patch after more testings to make sure everything is feasible.

Btw, the Bug 779381 addresses the issue of multiple settings in a single set.
(Assignee)

Comment 13

6 years ago
Created attachment 686504 [details] [diff] [review]
Patch V3

This patch perfectly synchronizes both settings and preferences by taking the settings.js into consideration. Also, the preference fortunately has an observer mechanism to listen to the preference changes, so we can listen to the "nsPref:changed" observer topic to update the cached proxy info at run-time.

Could you please take a chance to review this patch? Thanks a lot!
Attachment #686036 - Attachment is obsolete: true
Attachment #686504 - Flags: review?(vyang)
(Assignee)

Comment 14

6 years ago
Created attachment 686945 [details] [diff] [review]
Patch V3.1

Rename some variable/function names to make them more readable. Please see comment #13 for the main summary. Thanks!
Attachment #686504 - Attachment is obsolete: true
Attachment #686504 - Flags: review?(vyang)
Attachment #686945 - Flags: review?(vyang)
(Assignee)

Comment 15

6 years ago
Created attachment 686951 [details] [diff] [review]
Patch V3.2

Sorry. Fix a spot.
Attachment #686945 - Attachment is obsolete: true
Attachment #686945 - Flags: review?(vyang)
Attachment #686951 - Flags: review?(vyang)
(Assignee)

Comment 16

6 years ago
Created attachment 686953 [details] [diff] [review]
Patch V3.3

Damn... wrong patch. Sorry for the noise!
Attachment #686951 - Attachment is obsolete: true
Attachment #686951 - Flags: review?(vyang)
Attachment #686953 - Flags: review?(vyang)
(Assignee)

Comment 17

6 years ago
Created attachment 687014 [details] [diff] [review]
Patch V3.4

Fix some nits based on the review in person.
Attachment #686953 - Attachment is obsolete: true
Attachment #686953 - Flags: review?(vyang)
Attachment #687014 - Flags: review?(vyang)
(Reporter)

Comment 18

6 years ago
Comment on attachment 687014 [details] [diff] [review]
Patch V3.4

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

Thank you :)
Attachment #687014 - Flags: review?(vyang) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5b542c05974
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Updated

6 years ago
Depends on: 817945
(Reporter)

Updated

6 years ago
No longer blocks: 804679
(Assignee)

Comment 21

6 years ago
This bug relates to MMS features and needs to be tagged as leo+ so that we can uplift it into the b2g-18 branch.
blocking-b2g: --- → leo?
blocking-b2g: leo? → leo+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/29e37785c631
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox20: --- → fixed
The entire set of clian's pushes was backed out for multiple reasons.
https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=a0b06192f882

1.) The tree rules are clear that you are not to land on top of bustage. At the time you pushed, both B2G Mn and B2G xpcshell had bustage from prior commits that hadn't been backed out yet.
2.) The tree rules are also clear that you are to watch your pushes for any bustage and handle them accordingly. mozilla-inbound is the ONLY tree where this rule does not apply.
3.) Even after the earlier bustage was backed out, something in one of your many pushes was causing further B2G Mn failures as shown in the log below.
https://tbpl.mozilla.org/php/getParsedLog.php?id=20424173&tree=Mozilla-B2g18
4.) This isn't cause for backout by itself, but it is also strongly preferred to not push each commit individually as our build and testing resources are limited and doing so stretches them even thinner. Please limit your number of pushes as much as possible unless you have good reason for keeping them separate.
status-b2g18: fixed → affected

Updated

5 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.