878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

GonkGPSGeolocationProvider requests data connection setup, info for current cell location, etc. to complete geolocation functions.  Within MultiSIM configuration, we have multiple radio interfaces (modem) and need a method to determine the appropriate target instance.

Gonk GPS now relies on Gecko to setup data connection for AGPS data stream.  That GPS hardware module is highly platform dependent, and by Android's design, there is unlikely to have multiple GPS hardware instances even in MultiSIM supported devices.  That's the first thing we have to figure out.

On the other hand, cell location based AGPS can be available from each voice network registered radio interface.  Whether or not do you need such multiplicity and complexity forms the second question that we need an answer.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Our cell location server could use multiple cell tower id to find the best approximation of the user location. Once the API to query multiple radio interface is exposed, it's natural to hook this up.

Regarding the GPS hardware, it also depends on whether the GPS module could talk to multiple modem.

Comment 2

[:garvan]

Doug/Garvan note: as code is about to get updated, reminder to add iterator for radio interfaces as we did in NetworkGeolocationProvider.js 1.3t (instead of getRadioInterface(0)).

http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#653

Comment 3

[Ken Chang[:kenkai]]

Hi Garvan, I wonder if you would like to take this issue? Because Bug 1024309 is opened for this same problem, I plan to duplicate bug 1024309 of this bug.

Comment 4

[:garvan]

It already on my "do it if no one else gets to it first" list :).
That said, if it is needed ASAP, please advise and I'll jump on it immediately.

Comment 5

[:garvan]

Attachment: bug-878748-fix.patch

Iterate the radio interfaces to find one that has valid iccInfo, and therefore a valid SIM card.

Comment 6

[shawn ku [:sku]]

Sorry to jump in.

mRadioInterface is not only used for iccInfo, but also used for requesting setup data connection regarding supl.

In your case, we will always get the mRadioInterface instance for slot 0 if both SIMs are inserted no matter what setting (SIM manager -> Data) user picked. In other word, the result should be the same as the case w/o the patch.

My idea is
1. check what current data setting is (which SIM) at SetupAGPS.
2. add observer to listen if user change any setting.
 
Please correct me if anything is wrong.
Thanks!!
Shawn

Comment 7

[:garvan]

(In reply to shawn ku [:sku] from comment #6)
> In your case, we will always get the mRadioInterface instance for slot 0 if
> both SIMs are inserted no matter what setting (SIM manager -> Data) user
> picked. In other word, the result should be the same as the case w/o the
> patch.

I agree that it fails to handle the dual-sim case correctly.
If there is a SIM only in slot #2, wouldn't this code handle that case? 

> 
> My idea is
> 1. check what current data setting is (which SIM) at SetupAGPS.
> 2. add observer to listen if user change any setting.
> 
Good idea.
It looks like the data setting is available here:
http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/interfaces/nsIMobileConnectionInfo.idl#28

Comment 8

[shawn ku [:sku]]

(In reply to Garvan Keeley [:garvank] from comment #7)
> (In reply to shawn ku [:sku] from comment #6)
> > In your case, we will always get the mRadioInterface instance for slot 0 if
> > both SIMs are inserted no matter what setting (SIM manager -> Data) user
> > picked. In other word, the result should be the same as the case w/o the
> > patch.
> 
> I agree that it fails to handle the dual-sim case correctly.
> If there is a SIM only in slot #2, wouldn't this code handle that case?

For single SIM inserted case, there is no problem with ur patch.
However, we can not expect user only(always) use one SIM in device. Isn't it? :)

> 
> > 
> > My idea is
> > 1. check what current data setting is (which SIM) at SetupAGPS.
> > 2. add observer to listen if user change any setting.
> > 
> Good idea.
> It looks like the data setting is available here:
> http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/
> interfaces/nsIMobileConnectionInfo.idl#28

Not really, my thought is similar to [1]

[1] - http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#960

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8442364 [details] [diff] [review]
bug-878748-fix.patch

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

Should we use the same SIM that we select for data or make it configurable?

We still can't answer the questions in comment 0

1. What if the hardware has multiple GPS hardware? I think it's unlikely.
2. Could the GPS hardware talk to multiple modem? Maybe it could only use the first SIM. Maybe not all SIM providers has AGPS plan.

Defer review per comment 7.

Comment 10

[shawn ku [:sku]]

Attachment: 878748.patch

Comment 11

[shawn ku [:sku]]

I upload my patch with preliminary idea which might contains bug.

Comment 12

[:garvan]

I have only superficial knowledge of the RIL, I'll leave this bug to those who understand more of its inner workings.

Comment 13

[Ken Chang[:kenkai]]

Shawn,do you want to take this bug?

Comment 14

[shawn ku [:sku]]

Sure, I can take this bug.

Comment 16

[Ken Chang[:kenkai]]

Not sure if we can uplift this bug in 1.4.

Comment 17

[Preeti Raghunath(:Preeti)]

GPS not in Dolphin and hence backlog.

Comment 18

[Wayne Chang [:wchang]]

(In reply to Preeti Raghunath(:Preeti) from comment #17)
> GPS not in Dolphin and hence backlog.

Sorry to have misinformed you, but only the first dolphin device will be without GPS. Potential subsequent devices and Shark will have A/GPS.

Nominating again

Comment 19

[Ivan Tsay (:ITsay)]

Triage: Need GPS to work on multiSIM device. Blocker for 1.4

Comment 20

[Kai-Chih Hu [:khu]]

Add dolphin keyword in the summary field.

Comment 21

[Chris Peterson [:cpeterson]]

Shawn: do we need to uplift this fix to 2.0 branch? Does this bug only affect devices using MozRIL? Will devices using QC RIL have this bug?

Comment 22

[shawn ku [:sku]]

(In reply to Chris Peterson (:cpeterson) from comment #21)
> Shawn: do we need to uplift this fix to 2.0 branch? Does this bug only
> affect devices using MozRIL? Will devices using QC RIL have this bug?

IMO, 2.0 is needed. Besides, I have no idea how QC implement this part, hence, I cannot comment on QC's scope.

Comment 23

[Michael Vines [:m1] [:evilmachines]]

This is not required for v2.0

Comment 24

[Chris Peterson [:cpeterson]]

If partners shipping non-QC chipsets plan to ship 2.0, then we will need this fix for 2.0.

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

1.4 had been marked as fixed, but I didn't see any of above patches landed. Please land to b2g-inbound, and m-c first.

Comment 26

[shawn ku [:sku]]

Attachment: Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration.

Comment 27

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8450063 [details] [diff] [review]
Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration.

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +648,5 @@
>      NS_WARNING("Cannot get SUPL server settings");
>      return;
>    }
>  
> +  // Request client ID for correct RadioInterface object first

Write more comments about this is for MultiSIM support and we will get the radio interface once we know the correct client ID (or service ID).

@@ +649,5 @@
>      return;
>    }
>  
> +  // Request client ID for correct RadioInterface object first
> +  RequestSettingValue(RIL_DEFAULT_SERVICE_ID);

Why is the ID sometimes called "client ID" sometimes "service ID"? Could you unify the naming?

@@ +794,5 @@
>    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
>    if (obs) {
>      obs->RemoveObserver(this, kNetworkConnStateChangedTopic);
> +    if (mSupportsMSA || mSupportsMSB) {
> +      obs->RemoveObserver(this, MOZ_SETTINGS_CHANGE_ID);

We could remove the observer unconditionally here.

@@ +857,5 @@
> +    if (!jsKey) {
> +      return NS_OK;
> +    }
> +    nsDependentJSString keyStr;
> +    if (!keyStr.init(cx, jsKey) || !keyStr.EqualsLiteral(RIL_DEFAULT_SERVICE_ID)) {

Use JS_StringEqualsAscii to compare literal string.

@@ +867,5 @@
> +      return NS_OK;
> +    }
> +
> +    mRilClientId = value.toInt32();
> +    UpdateRadioInterface();

Check if the value is valid or fallback to zero as below.

@@ +908,5 @@
> +    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +    NS_ENSURE_TRUE(obs, NS_OK);
> +    if (NS_FAILED(obs->AddObserver(this, kNetworkConnStateChangedTopic, false))) {
> +      NS_WARNING("Failed to add network state changed observer!");
> +    } else if (NS_FAILED(obs->AddObserver(this, MOZ_SETTINGS_CHANGE_ID, false))) {

Use just "if".

::: dom/system/gonk/GonkGPSGeolocationProvider.h
@@ +19,5 @@
>  
>  #include <hardware/gps.h> // for GpsInterface
> +#ifdef MOZ_B2G_RIL
> +#include "mozilla/Observer.h"
> +#endif

Unused?

@@ +109,5 @@
>    bool mSupportsTimeInjection;
>  
>    const GpsInterface* mGpsInterface;
>  #ifdef MOZ_B2G_RIL
> +  int32_t mRilClientId;

uint32_t? It can't be negative right?

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Chris Peterson (:cpeterson) from comment #24)
> If partners shipping non-QC chipsets plan to ship 2.0, then we will need
> this fix for 2.0.

For now, its a wontfix for the same reason listed in https://bugzilla.mozilla.org/show_bug.cgi?id=1033274#c28. If we have partners picking shipping non-CAF chipsets, we will have to address this accordingly. Please continue to land on central/master so the fix is on 2.1.

Comment 29

[shawn ku [:sku]]

Due to biz trip, I may have slow reponse on this bug, if my slots are all full, will request help then.
Please keep ni? me if anything missed/needed.

Thanks!!
Shawn

Comment 31

[shawn ku [:sku]]

Attachment: Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v2.

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8460781 [details] [diff] [review]
Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v2.

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

Looks good to me.

bz, could you help review the JS API usage?

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +881,5 @@
> +      if (!value.isInt32()) {
> +        // Fallback to slot 0 if we got an invalid RIL data service ID
> +        mRilDataServiceId = 0;
> +        return NS_OK;
> +      }

Check value.isInt32() >= 0

@@ +920,5 @@
> +      mRilDataServiceId = aResult.toInt32();
> +    } else {
> +      // Fallback to slot 0 if we got an invalid RIL data service ID
> +      mRilDataServiceId = 0;
> +    }

Ditto.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460781 [details] [diff] [review]
Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v2.

I think the code in Observe is wrong.  It's assuming things about number values ending up as isInt32 that are just not guaranteed.

I would strongly recommend using a WebIDL dictionary here (inited from the JSON string; they even have a convenience method for it) and avoiding jsapi entirely.

For Handle, is there a reason to not just use JS::ToInt32 instead of once again making assumptions about what callers will pass you, especially because callers can't actually control whether their numeric values end up isInt32() or isDouble()?

Comment 34

[shawn ku [:sku]]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Comment on attachment 8460781 [details] [diff] [review]
> Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in
> MultiSIM configuration. v2.
> 
> I think the code in Observe is wrong.  It's assuming things about number
> values ending up as isInt32 that are just not guaranteed.
> 
> I would strongly recommend using a WebIDL dictionary here (inited from the
> JSON string; they even have a convenience method for it) and avoiding jsapi
> entirely.
> 
> For Handle, is there a reason to not just use JS::ToInt32 instead of once
> again making assumptions about what callers will pass you, especially
> because callers can't actually control whether their numeric values end up
> isInt32() or isDouble()?

Thank you very much, Boris.
It looks like some module(s) did the same mistake as I did.

And, JS::ToInt32(...) at Observe is not that smart as your reply in Comment 33.
Could you please kindly show me some document about how to introduce WebIDL dictionary into the patch?

Thanks again!!
Shawn

Comment 35

[Boris Zbarsky [:bzbarsky]]

> It looks like some module(s) did the same mistake as I did.

Sure.  People keep wanting to use the JSAPI.  They should stop.  :(

> And, JS::ToInt32(...) at Observe is not that smart as your reply in Comment 33.

I'm not sure what you mean.  I suggested using JS::ToInt32 in Handle, not in Observe.

> Could you please kindly show me some document about how to introduce WebIDL dictionary 

First you define a dictionary type in Web IDL that represents your expected object structure.

Then in the C++ you do:

  MyDictionaryType dict;  // pick a better variable name
  if (!dict.Init(myJSONString)) {
    // return NS_ERROR_FAILURE;
  }

and then work with the resulting struct.

In your case, myJSONString would be nsDependentString(aData).

The dictionary would be defined something like this:

  dictionary GinkGPSGeolocationWhatever { // Pick a better name
    DOMString key = "";
    long value = 0;
  };

(with the default values picked in such a way that your input will be treated as invalid if not passed, in this case). And then after the Init() call you would do:

  if (!dict.mKey.Value().EqualsAscii(RIL_DATA_DEFAULT_SERVICE_ID) {
    // Doesn't match
    return NS_OK;
  }

  mRilDataServiceId = dict.mValue; // Will be 0 if not set in the JSON.

See also http://heycam.github.io/webidl/#idl-dictionaries and https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types

Comment 36

[shawn ku [:sku]]

(In reply to Boris Zbarsky [:bz] from comment #35)
> > It looks like some module(s) did the same mistake as I did.
> 
> Sure.  People keep wanting to use the JSAPI.  They should stop.  :(
> 
> > And, JS::ToInt32(...) at Observe is not that smart as your reply in Comment 33.
> 
> I'm not sure what you mean.  I suggested using JS::ToInt32 in Handle, not in
> Observe.
> 
> > Could you please kindly show me some document about how to introduce WebIDL dictionary 
> 
> First you define a dictionary type in Web IDL that represents your expected
> object structure.
> 
> Then in the C++ you do:
> 
>   MyDictionaryType dict;  // pick a better variable name
>   if (!dict.Init(myJSONString)) {
>     // return NS_ERROR_FAILURE;
>   }
> 
> and then work with the resulting struct.
> 
> In your case, myJSONString would be nsDependentString(aData).
> 
> The dictionary would be defined something like this:
> 
>   dictionary GinkGPSGeolocationWhatever { // Pick a better name
>     DOMString key = "";
>     long value = 0;
>   };
> 
> (with the default values picked in such a way that your input will be
> treated as invalid if not passed, in this case). And then after the Init()
> call you would do:
> 
>   if (!dict.mKey.Value().EqualsAscii(RIL_DATA_DEFAULT_SERVICE_ID) {
>     // Doesn't match
>     return NS_OK;
>   }
> 
>   mRilDataServiceId = dict.mValue; // Will be 0 if not set in the JSON.
> 
> See also http://heycam.github.io/webidl/#idl-dictionaries and
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Dictionary_types

Thanks bz, I will check how to adopt this after study:)
If there is a MDN to instuctucing this, it will be perfect.

Comment 37

[shawn ku [:sku]]

Hi Olli:
 Per comment 33, bz suggest me to add a dictionary to complete the task.

Could you please give me some suggestion/comment if I add a new dictionary as below since I would like to have a more generic way?

dictionary MozSettingsValue
{
  DOMString settingName = "";
  (boolean or long or DOMString or object) settingValue = false;
};

Thanks!!
Shawn

Comment 38

[Boris Zbarsky [:bzbarsky]]

Why do you need that union type?  In the C++ you know you're looking for an int value, no?

Comment 39

[shawn ku [:sku]]

Yes, the type I need is only int for this case. However, there are some modules (see [1] as an example) could re-use this dictionary as well.

Meanwhile, we can only see the ambiguous for int case. I double if we need to create an union type for that?
That's why I need your suggestion here.


[1] - http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothService.cpp#615

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Reuse dictionary in which way? Adding new webidl dictionaries when needed is cheap and makes the code simpler.
Do you have some concrete example how the dictionary would be used?

And in any case, perhaps better to have simpler dictionary here, and change it later if reusing
would make other code simpler.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Olli is right: the point of this dictionary is to be convenient to use.  Make it as simple as you can.

Comment 42

[shawn ku [:sku]]

(In reply to Olli Pettay [:smaug] from comment #40)
> Reuse dictionary in which way? Adding new webidl dictionaries when needed is
> cheap and makes the code simpler.
> Do you have some concrete example how the dictionary would be used?
> 
> And in any case, perhaps better to have simpler dictionary here, and change
> it later if reusing
> would make other code simpler.

Thanks for your response.
I will follow this way to finish the job.

Comment 43

[shawn ku [:sku]]

Attachment: Bug 878748 Part 1 - webIDL patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration.

Comment 44

[shawn ku [:sku]]

Attachment: Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v3.

Comment 45

[shawn ku [:sku]]

(In reply to shawn ku [:sku] from comment #44)
> Created attachment 8465953 [details] [diff] [review]
> Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct
> RadioInterface instance in MultiSIM configuration. v3.

Address comment 33.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465953 [details] [diff] [review]
Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v3.

?+    mRilDataServiceId = static_cast<uint32_t>(dict.mValue);

Make the dictionary member unsigned long, and drop this cast.

>+    if(JS::ToInt32(cx, aResult, &v)) {

If this returns false, you need to immediately "return NS_ERROR_FAILURE", not do the fallback to 0 thing, I'd think.

Also, space between "if" and "(".

>+      mRilDataServiceId = static_cast<uint32_t>(v);

So if the caller passes -1 you want to end up with 0xffffffff?  Why?

Can Handle be called multiple times with name set to RIL_DATA_DEFAULT_SERVICE_ID?  If so, doing the AddObserver calls on each call is broken, no?

Comment 47

[shawn ku [:sku]]

Comment on attachment 8465953 [details] [diff] [review]
Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v3.

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

Clear r? due to something needs to be address.

Comment 48

[shawn ku [:sku]]

Thanks bz!!

Patch needs revision per offline irc chat.

Comment 49

[Chris Peterson [:cpeterson]]

Shawn: are you planning to fix this bug in 1.4 (for Dolphin) or 2.0?

Comment 50

[shawn ku [:sku]]

(In reply to Chris Peterson (:cpeterson) from comment #49)
> Shawn: are you planning to fix this bug in 1.4 (for Dolphin) or 2.0?

Hi Chris:
 We should go to m-c first after r+, then up-lift to necessary branch.
So, master is the major branch I concern now.
Thanks!!
Shawn

Comment 51

[thomas tsai]

(In reply to shawn ku [:sku] from comment #50)
> (In reply to Chris Peterson (:cpeterson) from comment #49)
> > Shawn: are you planning to fix this bug in 1.4 (for Dolphin) or 2.0?
> 
> Hi Chris:
>  We should go to m-c first after r+, then up-lift to necessary branch.
> So, master is the major branch I concern now.
> Thanks!!
> Shawn

so remove 1.4+

Comment 52

[shawn ku [:sku]]

(In reply to On vacation Aug 5-18.  Please do not request review. from comment #46)
> Comment on attachment 8465953 [details] [diff] [review]
> Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct
> RadioInterface instance in MultiSIM configuration. v3.
> 
> ?+    mRilDataServiceId = static_cast<uint32_t>(dict.mValue);
> 
> Make the dictionary member unsigned long, and drop this cast.
> 
> >+    if(JS::ToInt32(cx, aResult, &v)) {
> 
> If this returns false, you need to immediately "return NS_ERROR_FAILURE",
> not do the fallback to 0 thing, I'd think.
> 
> Also, space between "if" and "(".
> 
> >+      mRilDataServiceId = static_cast<uint32_t>(v);
> 
> So if the caller passes -1 you want to end up with 0xffffffff?  Why?
> 
> Can Handle be called multiple times with name set to
> RIL_DATA_DEFAULT_SERVICE_ID?  If so, doing the AddObserver calls on each
> call is broken, no?

After checking, RIL_DATA_DEFAULT_SERVICE_ID@Handle will only be called once 
(ie: Init() -> StartGPS() -> SetupAGPS()) unless caller invoke Init many times.

Comment 53

[shawn ku [:sku]]

Attachment: Bug 878748 Part 1 - webIDL patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration.

Comment 54

[shawn ku [:sku]]

Attachment: Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

Comment 55

[shawn ku [:sku]]

Comment on attachment 8472154 [details] [diff] [review]
Bug 878748 Part 1 - webIDL patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration.

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

Hi Olli:
 Per comment 33, a corresponding dictionary is created.
Could you please help review it?
Thanks!!
Shawn

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8472154 [details] [diff] [review]
Bug 878748 Part 1 - webIDL patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration.

>exporting patch:
><fdopen>

Something odd looking here at the end of the patch.

Comment 57

[shawn ku [:sku]]

Attachment: Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

Comment 58

[shawn ku [:sku]]

Attachment: Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

Comment 59

[shawn ku [:sku]]

Since BZ is OOO, will request his review on " Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4." when he comes back.

Comment 60

[shawn ku [:sku]]

Comment on attachment 8473460 [details] [diff] [review]
Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

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

Hi Bz:
 Could you please help check comment 52, and do the review for me?
Thanks!!
Shawn

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8473460 [details] [diff] [review]
Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

> RIL_DATA_DEFAULT_SERVICE_ID@Handle will only be called once

Mind adding a debug-only assert to that effect?

>+#ifdef MOZ_B2G_RIL
>+using namespace mozilla::dom;

Please make that unconditional.

>+  , mNumOfRilService(1)

What is this member supposed to represent?  It's not obvious from the name.  Why is it based on a pref?  Who is responsible for setting that pref?

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8473460 [details] [diff] [review]
Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

Oh, and r=me with the changes I asked for and a better name (and perhaps even a comment!) for the member, assuming the pref thing makes sense.

Comment 63

[shawn ku [:sku]]

(In reply to Boriz Zbarsky [:bz] from comment #61)
> Comment on attachment 8473460 [details] [diff] [review]
> Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct
> RadioInterface instance in MultiSIM configuration. v4.
> 
> > RIL_DATA_DEFAULT_SERVICE_ID@Handle will only be called once
> 
> Mind adding a debug-only assert to that effect?

I will check how to address it in final patch.

> 
> >+#ifdef MOZ_B2G_RIL
> >+using namespace mozilla::dom;
> 
> Please make that unconditional.

Roger that!!

> 
> >+  , mNumOfRilService(1)
> 
> What is this member supposed to represent?  It's not obvious from the name. 
> Why is it based on a pref?  Who is responsible for setting that pref?

mNumOfRilService represents how many SIM slot(s) we support for device.
1. partner will set property - ro.moz.ril.numclients to proper value if device support multi-SIM at build time. Definitely, 1 will be read if partner did not set that property.
2. radioInterfaceLayer.js (see [1]) will take responsibility to set preference.

Probably, adding a comment to describe above might erase ambiguity.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#1000

Comment 64

[Boris Zbarsky [:bzbarsky]]

> mNumOfRilService represents how many SIM slot(s) we support for device.

How about mNumberOfSIMSlots or mNumberOfRilServices (if there are actually multiple "RIL services" involved) then?

Comment 65

[shawn ku [:sku]]

(In reply to Boriz Zbarsky [:bz] from comment #64)
> > mNumOfRilService represents how many SIM slot(s) we support for device.
> 
> How about mNumberOfSIMSlots or mNumberOfRilServices (if there are actually
> multiple "RIL services" involved) then?

Thanks BZ,
I prefer "mNumberOfRilServices", :-)
Comment and naming will be address in final patch too.

Comment 66

[shawn ku [:sku]]

Attachment: Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

Hi BZ:
 Could you help help check if the final version is ready to go (per Comment 61)?
Thanks!!

Comment 67

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8483192 [details] [diff] [review]
Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4.

>mObservedNetworkConnStateChange
>mObservedSettingsChange

How about "observing" instead of "observed"?

The rest looks great.

Comment 68

[shawn ku [:sku]]

Attachment: Bug 878748 Part 1 - webIDL patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. r=smaug.

Comment 69

[shawn ku [:sku]]

Attachment: Bug 878748 Part 2 - AGPS patch : [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v4. r=KanRu, BZ.

Comment 70

[shawn ku [:sku]]

try result - https://tbpl.mozilla.org/?tree=Try&rev=103e10f5ee0d

Comment 71

[Ryan VanderMeulen [:RyanVM]]

This needs serious rebasing.

Do you realize that you pushed this to Try on top of an over month-old qbase? Even if the patches did apply, I'd almost consider the run in comment 70 to be invalid given how old the parent rev is. *Please* make sure you're generating patches on a current tree before pushing to Try, requesting checkin, etc.

Comment 72

[shawn ku [:sku]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #71)
> This needs serious rebasing.
> 
> Do you realize that you pushed this to Try on top of an over month-old
> qbase? Even if the patches did apply, I'd almost consider the run in comment
> 70 to be invalid given how old the parent rev is. *Please* make sure you're
> generating patches on a current tree before pushing to Try, requesting
> checkin, etc.

Ooops, sorry to run into this.
Will rebase the patch, run try, and request check-in then.

Thanks for your reminding.

Comment 73

[Kan-Ru Chen [:kanru] (UTC+9)]

Is anyone working this?

Comment 74

[shawn ku [:sku]]

Hi Kn-Ru:
 Thanks for reminding, I am kind of busy recently.
Will try to integrate the code into the latest one next week.

Thanks!!
Shawn

Comment 75

[shawn ku [:sku]]

Attachment: Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v5.

Hi KanRu:
 May I have you review again on this patch?

Due to arch. change (see [1]), dictionary is not enough to handle kSettingDebugGpsIgnored/kSettingDebugEnabled, I do some change to fit to current design.

[1] - https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#1025

Comment 76

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8547926 [details] [diff] [review]
Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v5.

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +61,5 @@
>  static const char* kMozSettingsChangedTopic = "mozsettings-changed";
> +#ifdef MOZ_B2G_RIL
> +static const char* kPrefRilNumRadioInterfaces = "ril.numRadioInterfaces";
> +static const char* kRilDataDefaultServiceId = "ril.data.defaultServiceId";
> +#endif

kSettingRilDefaultServiceId or kSettingRilDataDefaultServiceId

I prefer the shorter one.

@@ +1137,5 @@
> +    if (NS_FAILED(obs->AddObserver(this, MOZ_SETTINGS_CHANGE_ID, false))) {
> +      NS_WARNING("Failed to add mozsettings changed observer!");
> +    } else {
> +      mObservingSettingsChange = true;
> +    }

We are always observing settings change now so this is not needed. Also MOZ_SETTINGS_CHANGE_ID has been changed to kMozSettingsChangedTopic.

Comment 77

[shawn ku [:sku]]

Attachment: Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v6.

Comment 78

[shawn ku [:sku]]

Comment on attachment 8548673 [details] [diff] [review]
Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. v6.

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

Hi KanRu,
 Thanks for catching the wrong part for me, please kindly reserve your time for review again.

Thanks again!!
Shawn

Comment 79

[shawn ku [:sku]]

try looks weird - https://treeherder.mozilla.org/#/jobs?repo=try&revision=33d8b481b694.


KeyError: 'influxdb_credentials'
Return code: 1
../../../../gecko/dom/system/gonk/GonkGPSGeolocationProvider.h:121:8: error: 'GonkGPSGeolocationProvider::mObservingSettingsChange' will be initialized after [-Werror=reorder]
../../../../gecko/dom/system/gonk/GonkGPSGeolocationProvider.h:107:8: error: 'bool GonkGPSGeolocationProvider::mSupportsSingleShot' [-Werror=reorder]
../../../../gecko/dom/system/gonk/GonkGPSGeolocationProvider.cpp:290:1: error: when initialized here [-Werror=reorder]

Comment 80

[shawn ku [:sku]]

Attachment: Bug 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration. r=KanRu.

Comment 81

[shawn ku [:sku]]

Try result - https://tbpl.mozilla.org/?tree=Try&rev=7b5aaf0cccae

Comment 82

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/856679a135f2

Comment 83

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/856679a135f2