Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Description • 12 years ago

(I forgot to file this bug.. only filed 772747)

The API was added in bug 772747. Use this instead of hardcoding the apn settings in preferences.

	Shian-Yow Wu [:swu]
	Comment 1 • 12 years ago

Patrick, would you like to take this bug?

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 4 • 12 years ago

Comment on attachment 673112 [details] [diff] [review]
Patch: Use setupDataCallByType to build data connection

Summarize this patch: In RequestDataConnection, we check if data connection has already been setup. If the data connection has not been setup, we will call SetupDataCallByType() to setup data connection.

When we realize that the connection for AGPS has already been setup, we will get the APN that we are using for AGPS, then call mAGpsInterface->data_conn_open() to configure AGPS in the callback of settingservice.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 5 • 12 years ago

Update error log tag.

	Kan-Ru Chen [:kanru] (UTC+9) Reporter
	Comment 6 • 12 years ago

Comment on attachment 673132 [details] [diff] [review]
Patch: Use setupDataCallByType to build data connection

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +36,5 @@
>  #define DEBUG_GPS 0
>  
> +#undef LOG
> +#define ERR(args...)  __android_log_print(ANDROID_LOG_ERROR, "GonkGPSGeolocationProvider" , ## args)
> +

you could use printf_stderr

@@ +52,1 @@
>  

you don't have to escape the newline

@@ +341,5 @@
> +#else
> +    mAGpsInterface->data_conn_open(apn.get());
> +#endif
> +  } else if (GetDataConnectionState() ==
> +             nsINetworkInterface::NETWORK_STATE_DISCONNECTED) {

Get the state once and cache the result. Otherwise you might get different result.

@@ +645,3 @@
>    }
> +  settingsService->CreateLock(getter_AddRefs(lock));
> +  lock->Get("ril.supl.apn", this);

If the data connection state is disconnected then we could save one settings query.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 7 • 12 years ago

Summarize this patch:
1. In RequestDataConnection, we test if supl connection is already set up. If supl haven't been set up, we will call SetupDataConnection by type.
2. When we find that the connection state is changed, we query setting service to get the APN of supl. Then change data connection state of AGPS in callback of setting service.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 8 • 12 years ago

Remove unnecessary escape characters.

	Doug Turner (:dougt)
	Comment 9 • 12 years ago

Comment on attachment 674170 [details] [diff] [review]
Patch: Use setupDataCallByType to build data connection

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +358,5 @@
>  
> +  if (GetDataConnectionState() == nsINetworkInterface::NETWORK_STATE_CONNECTED) {
> +    // Connection is already established, we don't need to setup again.
> +    // We just get supl APN and make AGPS data connection state updated.
> +    nsCOMPtr<nsISettingsServiceLock> lock;

move this to where it's used.

@@ +360,5 @@
> +    // Connection is already established, we don't need to setup again.
> +    // We just get supl APN and make AGPS data connection state updated.
> +    nsCOMPtr<nsISettingsServiceLock> lock;
> +    nsCOMPtr<nsISettingsService> settingsService =
> +      do_GetService("@mozilla.org/settingsService;1");

Nit: Did you really need to wrap this?  settingsService == ss?

@@ +636,5 @@
> +  // We call Setting Service before we get the state of supl data connection
> +  // since it is possible that state of supl data connection haven't been
> +  // update and will be update after we finished this function (code that
> +  // updates the state is in another dataCallStateChanged callback).
> +  nsCOMPtr<nsISettingsServiceLock> lock;

move this to where it is used.

@@ +640,5 @@
> +  nsCOMPtr<nsISettingsServiceLock> lock;
> +  nsCOMPtr<nsISettingsService> settingsService =
> +    do_GetService("@mozilla.org/settingsService;1");
> +  if (!settingsService) {
> +    printf_stderr("Failed to get settingsLock service!");

you don't want printf's in production code.

@@ +664,5 @@
> +                                   JSContext* cx)
> +{
> +  if (aName.EqualsLiteral("ril.supl.apn")) {
> +    // When we get the APN, we attempt to call data_call_open of AGPS.
> +    JSString *apnStr = aResult.toString();

Is aResult always a string?

@@ +669,5 @@
> +    if (apnStr) {
> +      nsDependentJSString apn;
> +      apn.init(cx, apnStr);
> +      SetAGpsDataConn(apn);
> +    }

Maybe :

if (aResult.isString()) {
  nsDependentJSString apn;
  apn.init(cx, aResult.toString());
  SetAGpsDataConn(apn);  // can this be a empty string?
}

@@ +679,5 @@
> +GonkGPSGeolocationProvider::HandleError(const nsAString& aErrorMessage,
> +                                        JSContext* cx)
> +{
> +  printf_stderr("GonkGPSGeolocationProvider::HandleError: %s\n",
> +                NS_LossyConvertUTF16toASCII(aErrorMessage).get());

you'll want to remove this, right?

	Shian-Yow Wu [:swu]
	Comment 10 • 12 years ago

Kaze, we also need Gaia to write APN settings for SUPL.  I know you are working on switching to the Android APN database.  Please make sure this has been taken into consideration.  Thank you!

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 11 • 12 years ago

Kan-Ru and I seemed to be unable to get this patch working with T-Mobile EDGE (epc.tmobile.com).

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 12 • 12 years ago

(In reply to Shian-Yow Wu from comment #10)
> Kaze, we also need Gaia to write APN settings for SUPL.  I know you are
> working on switching to the Android APN database.  Please make sure this has
> been taken into consideration.  Thank you!

If we found that SUPL APN is not set, maybe we could just set SUPL APN to default APN so user can still access SUPL server through the Internet?

	Shian-Yow Wu [:swu]
	Comment 13 • 12 years ago

(In reply to Patrick Wang [:kk1fff] from comment #12)
> If we found that SUPL APN is not set, maybe we could just set SUPL APN to
> default APN so user can still access SUPL server through the Internet?

How about this: If SUPL APN is set, connect to SUPL APN, otherwise connect to default APN.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 14 • 12 years ago

After discussed with Shian-Yow, we think it would be better if we don't setup AGPS when SUPL isn't set. Therefore, AGPS uses the SUPL APN that is got from settings. If the SUPL APN was not a string or was an empty string, AGPS would not be set up.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 15 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> Kan-Ru and I seemed to be unable to get this patch working with T-Mobile
> EDGE (epc.tmobile.com).

I just tried to use AGPS with EDGE. It works in my test when SUPL APN is the same as default data APN (I can see the log in supl proxy, and the phone can get its location).

But if SUPL APN is different from default APN, AGPS might fail because RIL fires disconnect event right after we connected to SUPL. In my test, I also applied patch of bug 804500 to fix this problem.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 16 • 12 years ago

(In reply to Patrick Wang [:kk1fff] from comment #15)
> But if SUPL APN is different from default APN, AGPS might fail because RIL
> fires disconnect event right after we connected to SUPL. In my test, I also
> applied patch of bug 804500 to fix this problem.

Sorry, I mean that the problem can be fixed by applying patch of bug 804500. So I think if we test with two different APNs, we might also need to apply patch of bug 804500.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 17 • 12 years ago

Fix previous patch according to comment 9.

	Doug Turner (:dougt)
	Comment 18 • 12 years ago

Comment on attachment 675504 [details] [diff] [review]
Patch: Use setupDataCallByType to build data connection

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +352,5 @@
> +GonkGPSGeolocationProvider::RequestSettingValue(char* aKey)
> +{
> +  MOZ_ASSERT(aKey);
> +  nsCOMPtr<nsISettingsService> ss = do_GetService("@mozilla.org/settingsService;1");
> +  if (!ss) {

you might as well MOZ_ASSERT(ss);

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 19 • 12 years ago

Fix according to comment 18.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/dffca1d334ae

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/dffca1d334ae

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4a27e4d5da5