Closed jryans closed 11 years ago
I just have that one minor nit. Otherwise, this looks great!
Good call, I have added comments to reflect that.
Should I squash the patches, or do you typically do that during merging?
I think it is easier if you do the squash and then we can just click the big green github button ;)
Otherwise, I think we should iterate on this in order to get rid of the always registered FakeGeolocationProvider.js
xpcom component. I don't think it is reasonable to always load this xpcom when we are going to ship this actor on m-c. We would like to register this xpcom dynamically.
That may be handy to do that in the addon before uplifting the patch to m-c, but feel free to do that during the uplift.
Okay, squashed!
@ochameau: I agree, I was already thinking that! The provider component does need to be loaded dynamically. I'll probably handle that in the addon with a separate PR to minimize differences with m-c after the uplift.
Looks good, thanks @jryans!
I am working towards moving the geolocation simulating functionality up to m-c, so that it will already be on-device in the future.
In that vein, I've extracted several actors from the general purpose
dbg-simulator-actors
intodbg-geolocation-actors
anddbg-geolocation-ui-actors
.dbg-geolocation-actor
is what I would want to get on-device.dbg-geolocation-ui-actors
is very much dependent on the simulator's UI flow for controlling the geolocation settings, and since that may change significantly with the App Manager, I will leave this here in the simulator add-on for now.I will likely extend
dbg-geolocation-actor
before moving it on device (as part of issue #804 and perhaps others).