863051 - no singleshot information in GonkGpsGeoLocationProvider layer

Status: RESOLVED WONTFIX

(Core :: DOM: Geolocation, defect)

Description

[Stephen Li]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130409194949

Steps to reproduce:

Application requests for getCurrentPosition()


Actual results:

If Application requests for getCurrentPosition(), it will be translated into singleshot, but it is converted into tracking, and the "singleshot" information does not reach GonkGpsGeoLocationProvider. Location engine needs to have the "singleshot" information to do better job on Time to First FIX for concurrent use case along with tracking sessions


Expected results:

The "singleshot" information should be passed down to GonkGpsGeoLocationProvider layer

Comment 1

[Josh Matthews [:jdm]]

We will probably need to change the API of nsIGeolocationProvider to provide a hint argument to Startup.

Comment 2

[Andreas Gal :gal]

Same here, doug has the details, needed for 1.1 geolocation service use

Comment 3

[Doug Turner (:dougt)]

jdm,

I think we should do the following:


1) we should remove all of the mProviders support. there is only *one* provider every supported in the system.  We never use two providers and this adds a bunch of unneeded complexity (like IsBetterPosition).


2) we should change the nsIGeolocationProvider interface be something like:

[scriptable, uuid(FCBFA39D-F49C-4345-A7C5-EBDE39C467F8)]
interface nsIGeolocationProvider : nsISupports {

/** 
 * register for location updates. The privacy argument
 * informs the provider whether the initiating request
 * came from a private context; it is up to the provider
 * to use that information in a sensible manner.
 */ 
void register(in nsIGeolocationUpdate callback, 
in bool highAccuarcy, 
in unsigned long timeout, 
in unsigned long maximumAge,
in bool requestPrivate);

/** 
 * unregister 
 */ 
void unregister(in nsIGeolocationUpdate callback);

};

So, the provider now, if it wanted to, can figure out what kinds of location requests the dom level has currently got going.


On IRC it sounded like you could hack this together this week.  If not, toss it my way as soon as possible.

Comment 4

[Doug Turner (:dougt)]

Attachment: Remove multiple geolocation providers patch v.1

Comment 5

[Josh Matthews [:jdm]]

How does the removal of multiple providers interact with the work to add a cellid provider in bug 837987? Relatedly, we have work there to inject the position obtained into the GPS provider to achieve better accuracy.

Comment 6

[Josh Matthews [:jdm]]

In any case, I like the direction of comment 3 and I'm starting on implementing it now.

Comment 7

[Doug Turner (:dougt)]

regarding comment 5, removal of the multiple providers makes things *alot* easier in the code.  We do not actively support multiple providers really well (read over the code in IsBetterPosition).  If I am not mistaken, all of the injection stuff happens only in one provider -- the gonk provider.  Commercial replacements (like what a oem or chip can do) will continue to work.

If you think you don't need to remove support for multiple providers, awesome... but I tend to think the win is small by keeping them around.

Comment 8

[Josh Matthews [:jdm]]

Ok, but I don't understand how this ties in with the CellId provider work that's going on right now. That's another geolocation provider; what's the plan there?

Comment 9

[Doug Turner (:dougt)]

that should merge with the gonk provider, it think.  kchen, do you agree?

Comment 10

[Josh Matthews [:jdm]]

Attachment: WIP

wip to show my line of effort.

Comment 11

[Stephen Li]

Why is a new registerRequest() added for WiFigeolocation provider? At GonkGpsLocationProvider layer,

Comment 12

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

How close are we to the "final" nsIGeolocationProvider interface changes with WIP patches?  If we can get confidence in the new API quickly (today/tomorrow), then that'll decouple us and we can proceed optimistically with fixing up our implementation.

Comment 13

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

(In reply to Doug Turner (:dougt) from comment #9)
> that should merge with the gonk provider, it think.  kchen, do you agree?

I don't think we should remove the multiple providers support. What if we want to use both wifi position and GPS position? CellId and GPS position work independently so I think they shouldn't be merged.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I think the CellId should not be an independent GeolocationProvider, but rather be a utility other providers can use to get a faster fix and also feed back information into. In that sense, the Inject() method is a good idea.

Comment 15

[Josh Matthews [:jdm]]

Attachment: WIP for providing gonk GPS provider with singleshot hinting.

Haven't tried building it, but this patch would provide the singleshot hint that was originally requested.

Comment 16

[Josh Matthews [:jdm]]

With this design (ie. registerRequest/unregisterCallback), Gecko will still return cached values when allowed. The difference is that the geo provider will still be notified of the request; I'm not really sure that this makes sense. We could potentially provide information about a cached position that we're planning to return and allow the provider to veto it, but I think I need to discuss with Doug (and other interested parties) about the actual requirements here.

Comment 17

[Doug Turner (:dougt)]

spoke on the phone. jdm and I have a shared understanding.

Comment 18

[Josh Matthews [:jdm]]

Attachment: WIP

Another WIP. Still leaking like a sieve on mochitests, the IPC story is not well thought out yet, and xpcshell test status is uncertain.

Comment 19

[Josh Matthews [:jdm]]

Interface changes:
* should probably be registerRequest and unregisterRequest (instead of unregisterCallback)

I think this patch is going in the right direction. Requests register with the provider (via StartDevice) and unregister themselves later, and the service mostly acts as an intermediary. The IPC story is still not clear, unfortunately. It doesn't make sense to have more than one lister sending updates, but that doesn't easily match the behaviour of the in-process setup (ie. multiple requests registering themselves).

I think we'll need dedicated IPDL actors for each request; they won't actually transmit any updates back and forth, but they'll be used to reflect the lifetime of the requests in the child so the geolocation provider doesn't keep working unnecessarily. What we'll need to do is create a stub object (implementing nsIGeolocationUpdate) on the parent per request. These can all feed updates back to ContentParent, which can keep a count of expected results (as it does in the current patch) and transmit when all expected results have been received.

Comment 20

[Stephen Li]

Does the change allow the location technology integration (Gps, Network Location) to happen in lower layer, so the best accuracy/coverage/TTFF can be achieved at lowest power? Does the new change allow this to be done in GonkGpsLocationProvider & drivers below?

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

How would the CellId implementation hook into the new system? Would every geolocation provider (presumably only the ones on Android and B2G) call the appropriate CellId API methods? I'd like some consensus on this before rebasing bug 837987 on this one.

Comment 23

[Josh Matthews [:jdm]]

Yes, any geolocation provider interested in CellId results would explicitly use its API in whatever way made sense. I guess there wouldn't be any need for injection any more, since the geo provider can do whatever it wants with the resulting position.

Comment 24

[Josh Matthews [:jdm]]

Attachment: Part 1: Redesign Geolocation architecture to give providers more control over how to reply to individual requests.

Checkpoint. xpcshell tests pass, as do plain mochitests, with the exception of a persistent leak in the latter.

Comment 25

[Josh Matthews [:jdm]]

Attachment: Part 1: Redesign Geolocation architecture to give providers more control over how to reply to individual requests.

Now with five missing files.

Comment 26

[Josh Matthews [:jdm]]

Attachment: Part 1: Redesign Geolocation architecture to give providers more control over how to reply to individual requests.

Now with extra leak fixes. All mochjtests and xpcshell tests pass on my local machine. Try push at https://tbpl.mozilla.org/?tree=Try&rev=1cd1c9422381 .

Comment 27

[Doug Turner (:dougt)]

patch queue:   ssh://hg.mozilla.org/users/dougt_mozilla.com/geolocation_redesign/

Comment 28

[Stephen Li]

The UnregisterRequest() will also need to pass in the criterias; or pass in the request itself for Gonk to query the criterias, so the corresponding criterias can be removed when UnregisterRequest happens. So the Gonk layer below does not have to handle the mapping to each App, since the layer above (nsGeoLocation) has to handle that anyway. All the driver layer needs are the criterias. It does not associate the criteria with a particular session/App.

Comment 29

[Doug Turner (:dougt)]

Why can't you use the nsIGeolocationUpdate that is passed?

Comment 30

[Stephen Li]

After reviewing the suggested patches on this bug, these are some changes we want to propose:

1. In the nsIGeolocationProvider.idl, this is what we would desire:

{ 
+ void registerRequest(in nsComPtr<nsGeolocationRequest> aRequest);
+ void unregisterCallback(in nsComPtr<nsGeolocationReqest> aRequest);

 // this is ok to remove
-  void startup(); 

// Need one callback from Gonk to nsGeolocationService Update, not per request.
+ void watch(in nsIGeolocationUpdate callback, in boolean requestPrivate);

// we need this. GonkGPSGeolocationProvider does notkeep track of how many requests are pending. After the last unregistercallback, GonkGPSGeolocationProvider, needs to know if the GpsInterface can be shut down.
+  void shutdown();

// this is ok to remove
-  void setHighAccuracy(in boolean enable);
}

2. Can we get validity masks set on the input criteria ?
For example: if (aRequest->get_validmask() & NSGEOLOCATION_HAS_ACCURACY) then read accuracy.

Comment 31

[Doug Turner (:dougt)]

1) 

We can not passing concrete classes in the idl.

we changed the API so that your provider can see *all* requests that are current.  This reason for this is that your provider may want to drop down to a lower energy state if the only remaining provider is not marked at high accuracy.


2) I do not know what this means.


ftr, stephen and I are going to discuss this in person tomorrow.  :)

Comment 32

[Josh Matthews [:jdm]]

Stephen wants some way to find out if a value was explicitly present in the options dictionary.

Comment 33

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

Removing leo+ as sounds like some off-line discussion has occurred and this bug is  no longer necessary to meet CS on 1.1, and we were the sole drivers for including us in v1.1

Comment 34

[Hanno Schlichting]

This is a possibly outdated cleanup / refactoring issue. It's not directly tied to the "Cell ID service" project.

Comment 35

[Bhavna Sharma]

Doug, is there any discussion happening on this change ?

Comment 36

[Chris Peterson [:cpeterson]]

Doug: is this Gonk singleshot bug still relevant?

Comment 37

[Doug Turner (:dougt)]

yes -- we need this at some point.

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Closing as we are not working on Firefox OS anymore.