Closed
Bug 460063
Opened 16 years ago
Closed 16 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•16 years ago
|
Assignee: nobody → doug.turner
Assignee | ||
Updated•16 years ago
|
Component: DOM → Geolocation
Updated•16 years ago
|
QA Contact: general → geolocation
Assignee | ||
Comment 1•16 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•16 years ago
|
Attachment #344092 -
Flags: superreview?(bzbarsky)
Attachment #344092 -
Flags: superreview?
Attachment #344092 -
Flags: review?(Olli.Pettay)
Attachment #344092 -
Flags: review?
Reporter | ||
Comment 2•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 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•16 years ago
|
||
Setting the flag sounds good to me.
Assignee | ||
Comment 11•16 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•16 years ago
|
Attachment #344935 -
Flags: superreview?(bzbarsky)
Attachment #344935 -
Flags: superreview?
Attachment #344935 -
Flags: review?(Olli.Pettay)
Attachment #344935 -
Flags: review?
Comment 12•16 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•16 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•16 years ago
|
Attachment #345323 -
Flags: review? → review?(Olli.Pettay)
Comment 14•16 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•16 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•16 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•16 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•16 years ago
|
Attachment #345538 -
Flags: review? → review?(Olli.Pettay)
Updated•16 years ago
|
Attachment #345538 -
Flags: review?(Olli.Pettay) → review+
Comment 18•16 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•16 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•16 years ago
|
||
test case in 462564.
Reporter | ||
Comment 21•16 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•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Updated•16 years ago
|
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
Assignee | ||
Comment 22•16 years ago
|
||
Assignee | ||
Comment 23•16 years ago
|
||
pushed a1364547a182.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Keywords: fixed1.9.1
Updated•15 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•