455327 - geolocation - support timeout position option

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

see: 

1) http://lists.w3.org/Archives/Public/public-geolocation/2008Aug/0013.html
2) http://dev.w3.org/geo/api/spec-source.html

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Doug Turner (:dougt)]

smaug, do you think you can take a look at this?

Comment 3

[Doug Turner (:dougt)]

i will switch the lists over to use nsTArray<nsRefPtr<nsSomeClass> > instead of casting.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 342944 [details] [diff] [review]
patch v.1

>+  nsCOMPtr<nsIJSContextStack> stack(do_GetService("@mozilla.org/js/xpc/ContextStack;1"));
>+  if (!stack || NS_FAILED(stack->Push(nsnull)))
>+    return;
>+  
>+  callbackProxy->HandleEvent(this);
>+  
>+  // remove the stack
>+  JSContext* cx;
>+  stack->Pop(&cx);
Perhaps you could make a simple cxpusher helper class to be used on stack.
Or maybe even just modify nsCxPusher so that it works also in this case.
Anyway, since you're just moving this code, no need to make the change in this bug.

>+      nsRefPtr<nsDOMGeoPositionError> positionError = new nsDOMGeoPositionError(4, NS_LITERAL_STRING("Timeout"));
Please #define this magical 4. Same thing elsewhere. 

From l10n point of few, I'm not sure if using literals is the right thing to do here.
The spec doesn't say if the message is possibly localized.

>+      // remove ourselves from the locators callback lists.
Should it be locator's, not locators?


>     for (PRUint32 i = 0; i< mGeolocators.Length(); i++)
>-      mGeolocators[i]->Shutdown();
>+      ((nsGeolocationRequest*)mGeolocators[i])->Shutdown();
Should use c++ casting here and elsewhere, but I guess you don't need
any casting if you use nsTArray

>   nsGeolocation* mLocator; // The locator exists alonger than this object.
alonger?

>-  nsCOMArray<nsGeolocationRequest> mPendingCallbacks;
>-  nsCOMArray<nsGeolocationRequest> mWatchingCallbacks;
>+  // We use nsCOMArray's here because we want the reference counting magic,
>+  // but sadly we have to cast because we want to access the implementation class.
>+  nsCOMArray<nsIGeolocationRequest> mPendingCallbacks;
>+  nsCOMArray<nsIGeolocationRequest> mWatchingCallbacks;
These will change...

>+++ b/dom/tests/mochitest/geolocation/Makefile.in
>@@ -53,6 +53,7 @@
> 		test_cancelWatch.html \
> 		test_clearWatch.html \
> 		test_geoPrompt.html \
>+       test_timeoutWatch.html \
Using spaces when you should use tabs

I'd like to see clarification to that l10n/.message question.

Comment 5

[Doug Turner (:dougt)]

thanks for the feedback.

i sent a message to the w3c geolocation wg regarding the l10n issue you raised.  as I recall, this was suppose to be for debugging purposed only -- not for something that is displayed to the end user.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.2

regarding the l10n issue on the PositionError message.  This is suppose to be a "debug" message for "application developers" and not to be displayed to the end user.  The spec is going to be updated to reflect this.  For now, i have simply removed the text message as they are really just a duplication of the numeric value.  If/when things get more complex, we can pass more interesting strings back.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Did you diff somehow comparing to the earlier version of the patch or what?
The patch looks a bit strange.

Some comments, including some nits:
- I should have caught this earlier, but I think using nsITimerCallback might
  be better than nsIObserver
- To empty a string, use .Truncate() (not NS_LITERAL_STRING(""), and btw, if 
  you need an empty string, use EmptyString())
- parameter names should start with 'a' and then a capital letter
- Use C++ casting; static_cast<nsSomeClass*>(expr)
- Add OOM checks after |new|
- I think the GEO defines should have NS_ -prefix
- nsGeolocation::HasActiveCallbacks() should either return 0 or 1, so
  return mWatchingCallbacks.Length() != 0;

With those r=me, even though you didn't ask me to re-review ;)

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.3

with all of smaug's nits.

This is just the patch that is in my patch queue.  seems to apply to a clean tree, more or less.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343135 [details] [diff] [review]
patch v.3

>+++ b/dom/src/geolocation/nsGeolocation.cpp
>+  void Send(nsIDOMGeoPositionErrorCallback* callback);

I'd call this NotifyCallback() perhaps, but see below.

>+nsDOMGeoPositionError::Send(nsIDOMGeoPositionErrorCallback* aCallback)

As discussed on IRC, this shouldn't need the proxy.  If it needs to be async, just use a runnable.

>+nsGeolocationRequest::nsGeolocationRequest(nsGeolocation* aLocator, nsIDOMGeoPositionCallback* aCallback, nsIDOMGeoPositionErrorCallback* aErrorCallback, nsIDOMGeoPositionOptions* aOptions)
>+  : mAllowed(PR_FALSE), mCleared(PR_FALSE), mFuzzLocation(PR_FALSE), mHasSentData(PR_FALSE), mCallback(aCallback), mErrorCallback(aErrorCallback), mOptions(aOptions), mLocator(aLocator)

80 columns?

> nsGeolocationRequest::Cancel()

This is basically copy/pasting code from Notify() that differs only in the error code passed to the nsDOMGeoPositionError constructor.  Perhaps better to have a ReportError method that takes said code and then share it between the two callsites?  That method could even do the JSContext stack munging, instead of putting that on the error object itself.

>+nsGeolocation::RemoveRequest(nsGeolocationRequest* aRequest)
>+  nsIGeolocationRequest* r = static_cast<nsIGeolocationRequest*>(aRequest);

That cast doesn't seem to be needed.  Why are we doing it?

The rest looks OK, though you might want to also drop the ref to the timer when it fires.

Oh, one more thing.  I don't see how the part about watchPosition triggering the timeout on position changes can possibly work with this code.  Is it meant to?

Also, a -p -U 8 patch would be much easier to review..

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.4

w/ bz's comments.

i didn't combine the Send stuff, but did rename it.

Also, as I mentioned on IRC, this patch deals with timing out the request due to acquisition timeout failures. 

I do not think if location changes and we don't make the corresponding successCallback call within the timeout, we need to report an error.  I will send a email to the wg about this case.  I don't want to support this as it seem not very useful and overly complex.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So why not merge the NotifyCallback callers?

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.5

bz -  no reason.  here i merged them.

Comment 13

[Doug Turner (:dougt)]

a087bc8963b3

thanks bz+olli for your comments.