Closed
Bug 460063
Opened 17 years ago
Closed 17 years ago
Geolocation prompt should not appear if there are no providers installed
Categories
(Core :: DOM: Geolocation, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: bzbarsky, Assigned: dougt)
References
()
Details
(Keywords: fixed1.9.1)
Attachments
(2 files, 3 obsolete files)
8.09 KB,
patch
|
smaug
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
5.18 KB,
patch
|
Details | Diff | Splinter Review |
BUILD: Current trunk build
STEPS TO REPRODUCE:
1) Run the javascript: URI in the URL field
EXPECTED RESULTS: User is not prompted to allow or deny, since there is no location information to report anyway. Just return the right error to the page.
ACTUAL RESULTS: User is prompted as to whether a location should be provided to the web page. This makes it seem like the webpage _could_ conceivably get the location, when in fact it can't. This is a serious problem in terms of user perception of the privacy status of our product.
Flags: blocking1.9.1?
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → doug.turner
Assignee | ||
Updated•17 years ago
|
Component: DOM → Geolocation
Updated•17 years ago
|
QA Contact: general → geolocation
Assignee | ||
Comment 1•17 years ago
|
||
I expose a Init() method off of the nsGeolocationRequest object that is called before the user sees any dialog. This method is responsible for doing any precheck to see if a dialog should be displayed. Currently, it simply looks to see if we have a geolocation provider available. If it doesn't, we send an error callback and bail.
Attachment #344092 -
Flags: superreview?
Attachment #344092 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #344092 -
Flags: superreview?(bzbarsky)
Attachment #344092 -
Flags: superreview?
Attachment #344092 -
Flags: review?(Olli.Pettay)
Attachment #344092 -
Flags: review?
![]() |
Reporter | |
Comment 2•17 years ago
|
||
Is synchronously notifying the callback OK per spec?
Also, we call StartDevice() on every geolocation get attempt, right? Is it really OK to call Startup() on an already-started provider? And tell it to watch |this| again?
Assignee | ||
Comment 3•17 years ago
|
||
@bz Nothing is the spec prevents a synchronous callback. but now that you point that out, i wonder if it is good form. Thoughts bz?
It is fine to call start device multiple times:
http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/geolocation/nsIGeolocationProvider.idl#108
![]() |
Reporter | |
Comment 4•17 years ago
|
||
OK. Is calling watch() with the same callback multiple times good too? Clearly allowed, but will it have the behavior we want?
No opinion on the sync-vs-async issue, really; just worth checking with the WG.
Assignee | ||
Comment 5•17 years ago
|
||
it is allowed and the behavior isn't that terrible. I would there may be embeders also consuming from the geolocation provider's interface as well. More documentation on how to write a geolocation provider is needed. Part of this will be fleshed out in bug 459073.
I will talk with the WG wrt the functionality here. I do not think we need to block for it. we can do it sync for now, then go the other way if we decide that an asynchronous response is desired.
![]() |
Reporter | |
Comment 6•17 years ago
|
||
> and the behavior isn't that terrible
So we won't get tons of callbacks on every single location change (one per past location request)?
Assignee | ||
Comment 7•17 years ago
|
||
well, the sync callback we are talking about only happens when someone initially calls watchPosition or getCurrentPosition. This only happens one in most cases.
So, if you call watchPosition with a positionErrorCallback, and there isn't a geolocation provider installed, you will get a call on positionErrorCallback before watchPosition returns.
Other errors (for example, the GPS not acquiring, or the user presses cancel) will happen well after watchPosition returns.
![]() |
Reporter | |
Comment 8•17 years ago
|
||
I'm not talking about the sync callback. I'm talking about the
mProvider->Watch(this);
call which is going from being called once to being called once per getCurrentPosition/watchPosition. Say a page calls getCurrentPosition 1000 times and then the position changes. Are we supposed to get 1000 calls to nsGeolocationService::Update? What's preventing that from happening?
Assignee | ||
Comment 9•17 years ago
|
||
Boris, this should work fine, but as you point out, it is not ideal. The |this| passed to the mProvider will never change between the multiple calls - it will always be "the geolocation service". To reduce this unnecessary startup+watch calls, the service could set a flag that indicates that this has already been done. Would that work for you? I could do this here, or in the overhaul of this code in bug 454490.
![]() |
Reporter | |
Comment 10•17 years ago
|
||
Setting the flag sounds good to me.
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #344092 -
Attachment is obsolete: true
Attachment #344935 -
Flags: superreview?
Attachment #344935 -
Flags: review?
Attachment #344092 -
Flags: superreview?(bzbarsky)
Attachment #344092 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•17 years ago
|
Attachment #344935 -
Flags: superreview?(bzbarsky)
Attachment #344935 -
Flags: superreview?
Attachment #344935 -
Flags: review?(Olli.Pettay)
Attachment #344935 -
Flags: review?
Comment 12•17 years ago
|
||
Comment on attachment 344935 [details] [diff] [review]
patch v.2 (with the flag)
This patch isn't generated against trunk, right? r-
>@@ -434,16 +450,25 @@ nsGeolocationService::nsGeolocationServi
> // point in meters, and the direction in degrees
> mDisplacementMagnitude = rand() % DISPLACEMENT_MAX_DISTANCE_IN_METERS;
> mDisplacementDirection = rand() % 360;
>
> nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
> pref->SetIntPref("geo.displacement.magnitude", mDisplacementMagnitude);
> pref->SetIntPref("geo.displacement.direction", mDisplacementDirection);
> }
>+
>+ mProvider = do_GetService(NS_GEOLOCATION_PROVIDER_CONTRACTID);
>+
>+ // if NS_MAEMO_LOCATION, see if we should try the MAEMO location provider
>+#ifdef NS_MAEMO_LOCATION
>+ if (!mProvider)
>+ mProvider = new MaemoLocationProvider();
>+#endif
>+
Are you moving these to ctor? Why? IMO better to keep any object
creationg in Init()/Startup() or some such so that it is easier to handle OOM.
> nsGeolocationService::StartDevice()
I think this method would be easier to read if you
return early if provider has been started.
> // if we have one, start it up.
> nsresult rv = mProvider->Startup();
> if (NS_FAILED(rv))
> return NS_ERROR_NOT_AVAILABLE;
>-
>+
No need for this whitespace change.
> // lets monitor it for any changes.
> mProvider->Watch(this);
>-
>+
>+ // remember that we are started up
>+ mProviderStarted = PR_TRUE;
>+
> // we do not want to keep the geolocation devices online
> // indefinitely. Close them down after a reasonable period of
> // inactivivity
> SetDisconnectTimer();
>+
nor this. (Or is this patch -w ?)
Attachment #344935 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 13•17 years ago
|
||
Cleaned up patch. I will follow up with another bug/patch that moves this code out of the constructor and puts it into an ::Init method (see bug 462192).
Attachment #344935 -
Attachment is obsolete: true
Attachment #345323 -
Flags: review?
Attachment #344935 -
Flags: superreview?(bzbarsky)
Assignee | ||
Updated•17 years ago
|
Attachment #345323 -
Flags: review? → review?(Olli.Pettay)
Comment 14•17 years ago
|
||
Comment on attachment 345323 [details] [diff] [review]
patch v.3
>+
>+ mProvider = do_GetService(NS_GEOLOCATION_PROVIDER_CONTRACTID);
>+
>+ // if NS_MAEMO_LOCATION, see if we should try the MAEMO location provider
>+#ifdef NS_MAEMO_LOCATION
>+ if (!mProvider)
>+ mProvider = new MaemoLocationProvider();
>+#endif
>+
Why you need to move these from StartDevice to ctor?
Shouldn't it be ok to create mProvider when StartDevice
is first time called?
Assignee | ||
Comment 15•17 years ago
|
||
StartDevice occurs after the user give permission. This is wanted because we do not want to turn on any geolocation device (think GPS) without the user's expressed permission. I needed to move looking up the mProvider into the service's ctor because I want to immediately post from getCurrentPosition and watchPosition if no provider is available.
Comment 16•17 years ago
|
||
Comment on attachment 345323 [details] [diff] [review]
patch v.3
Per IRC, I'm expecting a new patch. (Would be really great to have tests to check that everything works after disconnect timer)
Attachment #345323 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 17•17 years ago
|
||
this allows up to restart the provider after a disconnect timeout. nice find smaug!
Attachment #345323 -
Attachment is obsolete: true
Attachment #345538 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #345538 -
Flags: review? → review?(Olli.Pettay)
Updated•17 years ago
|
Attachment #345538 -
Flags: review?(Olli.Pettay) → review+
Comment 18•17 years ago
|
||
Comment on attachment 345538 [details] [diff] [review]
patch v.4
But I really would like to see some tests for the disconnect timeout.
Assignee | ||
Comment 19•17 years ago
|
||
Comment on attachment 345538 [details] [diff] [review]
patch v.4
me too. let me build some.
Attachment #345538 -
Flags: superreview?(bzbarsky)
Assignee | ||
Comment 20•17 years ago
|
||
test case in 462564.
![]() |
Reporter | |
Comment 21•17 years ago
|
||
Comment on attachment 345538 [details] [diff] [review]
patch v.4
Fix the whitespace weirdness that seems to be present in some places, and looks good.
Attachment #345538 -
Flags: superreview?(bzbarsky) → superreview+
Updated•17 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Updated•17 years ago
|
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
Assignee | ||
Comment 22•17 years ago
|
||
Assignee | ||
Comment 23•17 years ago
|
||
pushed a1364547a182.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: fixed1.9.1
Updated•16 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•