Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Refactor geolocation to prepare actor for uplifting #805

merged 1 commit into from Aug 23, 2013


None yet
3 participants

jryans commented Aug 21, 2013

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 into dbg-geolocation-actors and dbg-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).

@mykmelez mykmelez commented on the diff Aug 21, 2013

+ },
+ /**
+ * Dump a debug message to stdout. This is defined as a method to avoid
+ * polluting the global namespace of the debugger server, and it always dumps
+ * because the Add-on SDK automatically determines whether to log the message.
+ */
+ debug: function debug() {
+ dump(Array.slice(arguments).join(" ") + "\n");
+ },
+ onAttach: function(aRequest) {
+ this.debug("Geolocation UI actor received an 'attach' command");
+ return {};
+ }

mykmelez Aug 21, 2013


Nit: it'd be worth noting that the purpose of this function is simply to trigger evaluation of this actor script. Earlier in the diff, I noticed the comment "Actors are initialized lazily, so tell them to attach", but by the time I got here, I had forgotten that and was confused by the "attach" request type, which doesn't seem to do anything, so I had to go back and read through the patch until I reread that comment and realized what was going on here. Future developers (including future versions of us, if we haven't looked at the code in a while!) may be similarly confused.


mykmelez commented Aug 21, 2013

I just have that one minor nit. Otherwise, this looks great!


jryans commented Aug 22, 2013

Good call, I have added comments to reflect that.

Should I squash the patches, or do you typically do that during merging?


ochameau commented Aug 22, 2013

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.


jryans commented Aug 22, 2013

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.


mykmelez commented Aug 23, 2013

Looks good, thanks @jryans!

mykmelez added a commit that referenced this pull request Aug 23, 2013

Merge pull request #805 from jryans/refactor-geolocation
Refactor geolocation to prepare actor for uplifting

@mykmelez mykmelez merged commit c78423d into mozilla:master Aug 23, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment