Closed Bug 1222899 Opened 4 years ago Closed 4 years ago

[GONK-GPSGeolocation] Show/Hide location tracking icon when there is StatusCallback from HAL

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Tracking Status
firefox46 --- fixed

People

(Reporter: alchen, Assigned: ywu)

References

Details

Attachments

(1 file, 5 obsolete files)

Quote from bug 1201778 comment 66

The event to indicate that tracking has started or stopped is this callback from GPS HAL: (StatusCallback) https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/dom/system/gonk/GonkGPSGeolocationProvider.cpp?h=mozilla/v2.2#n129, where the GPSStatus indicates the state of GPS engine.
Once GonkGPSGeolocationProvider knows this, it needs a way to tell Gecko/Gaia to show / hide the location icon.
Hi Bhavna,
There are five value for GpsStatusValue[1].
Do we need to take care about all of them?

[1] https://source.android.com/devices/halref/gps_8h_source.html#l00070
Flags: needinfo?(sbhavna)
Besides, the current design of location tracking icon is depended on the state nsGeolocationService.
So we may also need to consider about the combinations. 
("the state nsGeolocationService" X  "GpsStatusValue")


/** GPS status event values. */
typedef uint16_t GpsStatusValue;

/** GPS status unknown. */
#define GPS_STATUS_NONE             0

/** GPS has begun navigating. */
#define GPS_STATUS_SESSION_BEGIN    1

/** GPS has stopped navigating. */
#define GPS_STATUS_SESSION_END      2

/** GPS has powered on but is not navigating. */
#define GPS_STATUS_ENGINE_ON        3

/** GPS is powered off. */
#define GPS_STATUS_ENGINE_OFF       4
(In reply to Alphan Chen [:alchen] from comment #1)
> Hi Bhavna,
> There are five value for GpsStatusValue[1].
> Do we need to take care about all of them?
> 
> [1] https://source.android.com/devices/halref/gps_8h_source.html#l00070

Taking care of only GPS_STATUS_ENGINE_ON and GPS_STATUS_ENGINE_OFF should be good enough.
Yes we will have to take care of cases where nsGeolocationService might be already showing an icon as well.

Sorry for the delay in reply though.
Flags: needinfo?(sbhavna)
Here is my proposal of this bug.
There are three states(0x0, 0x1, 0x10) for different combinations.
0x1 and 0x10 will show location tracking icon.

(1) geolocation is active (state : 0x1)
 -> the location tracking icon exists all the time (state : 0x1)

(2) geolocation is in-active without location tracking icon (state : 0x0)
 -> got callback with GPS_STATUS_ENGINE_ON : show location tracking icon (state : 0x10)
 -> got callback with GPS_STATUS_ENGINE_OFF : No change in this case. (state : 0x0)

(3) geolocation is in-active with location tracking icon (state : 0x10)
 -> got callback with GPS_STATUS_ENGINE_ON : keep location tracking icon (state : 0x10)
 -> got callback with GPS_STATUS_ENGINE_OFF : remove the tracking icon. (state : 0x0)
Assignee: nobody → alchen
You mean the GonkGPSGeolocationProvider would either send state 0x0 (ENGINE_OFF) or 0x10 (ENGNIE_ON) to Gecko/Gaia.
(In reply to Bhavna Sharma from comment #5)
> You mean the GonkGPSGeolocationProvider would either send state 0x0
> (ENGINE_OFF) or 0x10 (ENGNIE_ON) to Gecko/Gaia.

Actually I want to maintain the state in gecko.

Here is what we do in current design.
We use chrome event with type "geolocation-status" to communicate with gaia.
https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1145
Yeah I would agree maintaining the state in Gecko will be the best way to go.
Hi Alphan,

How much effort do you think it is for this task ? i.e. we should be able to send a icon turn on/off request to Gaia from GonkGPSGeolocationProvider. Can you give me an ETA for it ?
(In reply to Bhavna Sharma from comment #8)
> Hi Alphan,
> 
> How much effort do you think it is for this task ? i.e. we should be able to
> send a icon turn on/off request to Gaia from GonkGPSGeolocationProvider. Can
> you give me an ETA for it ?

I can release a patch next week.
Thanks, I still do not have a confirmation on the requirement of this test case. Just wanted an estimate of work incase it is. Will keep you updated.
Ya-Chieh will help on this patch.
Assignee: alchen → ywu
In this patch, we tried to handle the scenario that GPS engine is turned ON directly by modem. This is not the usual way that GPS engine is triggered.
By using this patch, we can handle the location tracking icon better by cover the case when GPS is ON by modem but not showing the icon to the user.

We handle the location tracking icon as the description below.
During this GPS engine is turned ON by the modem, we make the location tracking icon visible to user.
Once GPS engine is turned OFF, the location icon will disappear.
If GPS engine is not turned ON by the modem or GPS location service is triggered,
we let GPS service take over the control of showing or removing the location tracking icon.
Attached patch 0001-Bug-1222899.patch (obsolete) — Splinter Review
Attachment #8692823 - Attachment is obsolete: true
Attachment #8698767 - Attachment is obsolete: true
Attachment #8699352 - Flags: review?(kchen)
The behavior that the GPS engine is turned ON in the modem is different from the legacy behavior where the users know about when location tracking is started and stopped. That's why it will be better if we prompt an warning to notify the users that the GPS is turned ON in the modem.

Dear Luke, could you help to prompt a warning in Gaia to notify the users that GPS is turned ON in the modem which is not a regular behavior? As this patch that we will pass "prompt: true" to Gaia.
Attachment #8699352 - Attachment is obsolete: true
Attachment #8699352 - Flags: review?(kchen)
Flags: needinfo?(lchang)
Hi Ya-Chieh,

Sure. I'll file a corresponding Gaia bug for this request.
Flags: needinfo?(lchang)
This case is a special scenario that the GPS is not turned ON either by any launched applications or by our OS. According to bug 1201778 comment 66 mentions, GPS engine is ON directly by the modem. This context goes with the event of an emergency to locate mobile phones. When the users use their phone to call the emergency number, the GPS is able to be turned ON by the modem.
 

This patch will show the location icon to the users and then there is a following Blocks: 1233684 that will prompt a warning to users.
Attachment #8699411 - Attachment is obsolete: true
Attachment #8700923 - Flags: review?(kchen)
Comment on attachment 8700923 [details] [diff] [review]
During this GPS engine is turned ON by the modem, we make the location tracking icon visible to user.

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

Looks good. There are some formatting nits.

::: b2g/chrome/content/shell.js
@@ +1137,5 @@
> +//The "GPSChipOn" is to indicate that GPS engine is turned ON by the modem.
> +//During this GPS engine is turned ON by the modem, we make the location tracking icon visible to user.
> +//Once GPS engine is turned OFF, the location icon will disappear.
> +//If GPS engine is not turned ON by the modem or GPS location service is triggered,
> +//we let GPS service take over the control of showing the location tracking icon.

nit: one space after //

Explain the possible sequence of the geolocation-device-events in the comment.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +152,5 @@
>    MozStumble(somewhere);
>  #endif
>  }
>  
> +class NotifyObserversGPSTask final : public nsRunnable {

nit: { should use a newline

@@ +159,5 @@
> +    : mData(aData)
> +  {}
> +  NS_IMETHOD Run() override {
> +      RefPtr<nsIGeolocationProvider> provider =
> +      GonkGPSGeolocationProvider::GetSingleton();

nit: indent this line and the whole function should use 2 space indent level

@@ +161,5 @@
> +  NS_IMETHOD Run() override {
> +      RefPtr<nsIGeolocationProvider> provider =
> +      GonkGPSGeolocationProvider::GetSingleton();
> +      nsCOMPtr<nsIObserverService> obsService = services::GetObserverService();
> +      obsService->NotifyObservers(provider,"geolocation-device-events", mData);

nit: space after ,
Attachment #8700923 - Flags: review?(kchen) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3196d7dae075
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.