Chris Peterson [:cpeterson] Reporter
	Description • 10 years ago

Bhavna from CAF reports:

On FFOS v2.1, CAF testers have been reporting that the GPS icon on status bar remains grayed out for at least 30 secs-60 secs after a tracking session is started. The behavior is inconsistent. Sometimes we see the icon turning ON (enabled) immediately, at times it takes up to a minute. These are QC MSM 8226 devices, being used with QC OS builds with our geolocation provider.

	Chris Peterson [:cpeterson] Reporter
	Comment 1 • 10 years ago

This is likely a UI-related bug because Bhavna says the GPS icon change takes 30–60 seconds, but the location results do not.

	Chris Peterson [:cpeterson] Reporter
	Comment 2 • 10 years ago

[Blocking Requested - why for this release]:

CAF believes this UI bug is a regression from B2G 2.0.

	Bhavna Sharma
	Comment 3 • 10 years ago

My understanding is that the update of the GPS icon does not depend on location results.
Is that correct ?

The update is just triggered from nsGeolocation at: https://hg.mozilla.org/mozilla-central/file/eaa80e4597a2/dom/geolocation/nsGeolocation.cpp#l883
And, then stopped when location requests have stopped here: https://hg.mozilla.org/mozilla-central/file/eaa80e4597a2/dom/geolocation/nsGeolocation.cpp#l963

	Chris Peterson [:cpeterson] Reporter
	Comment 4 • 10 years ago

Bhavna reported in email:

"Strangely I see that if I use Mozilla Geolocation provider I do not see this issue. I am trying to understand why using QC Geolocation provider can cause this issue. I believe this event just flows between the geolocation service and Gaia.

	Bhavna Sharma
	Comment 5 • 10 years ago

Can someone help answer the question in comment 3 and comment 4 above ?

1. Is the gps icon dependent on location results ?
2. Is the GonkGPSGeolocationProvider involved in anyway in the event to update the gps icon. My understanding is that this event just flows between the geolocation service and gaia.

	Chris Peterson [:cpeterson] Reporter
	Comment 6 • 10 years ago

Tim: do you know of any GPS or status bar changes in B2G 2.1 that might cause the GPS icon to be delayed?

Why does sb_updateGeolocation() return without calling _updateIconVisibility() when this.geolocationActive is true?

https://github.com/mozilla-b2g/gaia/blob/74ba93a5df5a0b313223595f784f0725ea93e656/apps/system/js/statusbar.js#L1327

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 7 • 10 years ago

You probably needinfo me since I wrote that line 2 years ago (as shown on the blame). Unfortunately statusbar.js has grown bigger into 16xx lines without receiving proper support on splitting it's functionality -- I am no longer familiar with the inner working of the code.

I think we should ask the developer who added the _updateIconVisibility() function in bug 1042105.

	Guillaume Marty [:gmarty] Assignee
	Comment 8 • 10 years ago

I think this comes from the fact that the minimised status bar is not refreshed when the geolocation icon is set to hidden. So it only disappears when another icon change happens in the status bar (like the time getting updated).
This PR fixes this issue + the same pattern used in video recording.

That said, when using the Geoloc app of the eng builds, I have noticed a delay between the time I press the Stop button and the time the matching 'geolocation-status' event is fired. That accounts only for about 10-15 seconds of waiting time, but can add to the delay described in the bug report.

Vivien or Etienne, can you please review this PR?

	Etienne Segonzac (:etienne)
	Comment 9 • 10 years ago

Comment on attachment 8501767 [details]
Github PR

comment on github, same than this morning, covering with an "internal" unit test beats not covering with a test at all :)

	Guillaume Marty [:gmarty] Assignee
	Comment 10 • 10 years ago

Comment on attachment 8501767 [details]
Github PR

I added a test to make sure that icons are reprioritised after each call to geolocation or recording.
I also addressed your comment from Github.

	Etienne Segonzac (:etienne)
	Comment 11 • 10 years ago

Comment on attachment 8501767 [details]
Github PR

all good, thanks!

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 12 • 10 years ago

Landed, so that it winds up in the next QA build.

master: https://github.com/mozilla-b2g/gaia/commit/eed1c73e4a82f73d45f5d19bac778bdf7d3f61c7

Guillaume, please request uplift.

	:garvan
	Comment 13 • 10 years ago

(In reply to Guillaume Marty [:gmarty] from comment #8)
> That said, when using the Geoloc app of the eng builds, I have noticed a
> delay between the time I press the Stop button and the time the matching
> 'geolocation-status' event is fired. That accounts only for about 10-15
> seconds of waiting time, but can add to the delay described in the bug
> report.
> 

AFAIK, this is correct, the geolocation service first waits for any geolocation requests to complete, then there is a timer (6 secs i think) before shutting down:
http://lxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocation.cpp#899
This is to avoid the overhead of starting up the geolocation service/providers repeatedly if another request follows within a short period of time.

	Guillaume Marty [:gmarty] Assignee
	Comment 14 • 10 years ago

Comment on attachment 8501767 [details]
Github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Status bar icon prioritisation
[User impact] if declined: The geolocation icon stays in the status bar longer than it should
[Testing completed]: Part of this patch is covered by unit tests but manual testing is needed
[Risk to taking this patch] (and alternatives if risky): Very low risk as only 1 file is changed
[String changes made]: None

	bhavana bajaj [:bajaj]
	Comment 15 • 10 years ago

Hey Bhavna,

Can you please try the latest 2.1 once this patch lands, to confirm this fixes the issue at your end ? Thanks !

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/dfe06e8d0954fd41b2402a15b98472d4bf4b2c53

	Bhavna Sharma
	Comment 17 • 10 years ago

Verified, issue is resolved.

	Kevin Grandon :kgrandon
	Comment 18 • 10 years ago

Here is a follow-up patch to remove the suite.only. Locally only a single test gets run from the file when you run it, so we have to be super careful to not check-in with .only() in the future.

	Kevin Grandon :kgrandon
	Comment 19 • 10 years ago

Follow-up landed: https://github.com/mozilla-b2g/gaia/commit/dc26d4a1c28682137130c62058399b43a4ea86b4

	Guillaume Marty [:gmarty] Assignee
	Comment 20 • 10 years ago

Added Marionette test in this patch. Etienne can you please review as I made changes to the StatusBar helper class.

	Etienne Segonzac (:etienne)
	Comment 21 • 10 years ago

Comment on attachment 8512664 [details]
Github PR for Marionette tests

We should maybe investigate having helper methods to check statusbar + minimized status bar.

But that's definitely not blocking for now, let's see when we add more tests :)

	Guillaume Marty [:gmarty] Assignee
	Comment 22 • 10 years ago

Thanks Etienne

The Marionette test for this bug landed in master in 
https://github.com/mozilla-b2g/gaia/commit/fede7e11f3786f8944ec5e6b74d8c30f5790c326