Currently, if an exception occurs when invoking the user-supplied success callback, the error callback (if supplied) is invoked. See Geolocation::sendPositionToOneShots() and Geolocation::sendPositionToWatchers(). The error callback is intended to report errors in the Geolocation location acquisition process itself, not errors in the user's script. See http://www.w3.org/TR/geolocation-API/#geolocation. For this reason, the error callback should not be invoked in the case of exceptions in the success callback.
Created attachment 33531 [details] Patch
Your ChangeLog doesn't look right: you need the bug URL and some brief explanation of the changes. Please see the "ChangeLog files" section at http://webkit.org/coding/contributing.html Andrei
Created attachment 33532 [details] Patch for bug 27255 Fixed ChangeLog to add bug URL and brief explanation of changes.
Oh, the reason I did that was this message: http://lists.w3.org/Archives/Public/public-geolocation/2008Oct/0088.html I'm open to this suggestion, however. Andrei?
Hi, The spec currently says: "UNKNOWN_ERROR (numeric value 0) The location acquisition process failed due to an error not covered by the definition of any other error code in this interface." http://dev.w3.org/geo/api/spec-source.html#unknown_error So UNKNOWN denotes an error in acquiring a position (it is a catch-all value for any situation not covered by the other error codes). If the UA is executing the success callback, then it means that the position was successfully acquired so, as far as the UA is concerned, the job is done. It can, of course, happen that the success callback throws an exception but I think this is a problem with the user code and not the Geolocation API implementation. As such, error callback should not be invoked. Instead, I think an error event should be raised and window.onerror should be invoked (if defined) or the error should be logged to the JS console. Andrei
(In reply to comment #5) > It can, of course, happen that the success callback throws an exception but I > think this is a problem with the user code and not the Geolocation API > implementation. As such, error callback should not be invoked. Instead, I think > an error event should be raised and window.onerror should be invoked (if > defined) or the error should be logged to the JS console. Great. I think the code already will report the exception in that way, so this patch implements what you describe above. I'm not a reviewer, however. I'm going to move the bug to "NEW", in hope it gets reviewed.
Comment on attachment 33532 [details] Patch for bug 27255 OK. That information/justification from the spec really should go in the ChangeLog. Please update it when landing.
Comment on attachment 33532 [details] Patch for bug 27255 Oh. You can't land your own yet, can you. Please post one final patch with the justification information from the spec. Something like "these are marked [NoInterfaceObject] in the spec:" with the links you provided Thanks!
> Something like "these are marked [NoInterfaceObject] in the spec:" with the > links you provided This appears to refer to Bug 27250. To which does bug does your previous comment refer? Do you have any comments for this bug?
Comment on attachment 33532 [details] Patch for bug 27255 This needs a layout test. Otherwise it looks fine.
> This needs a layout test. Adding a layout test for this change requires a mock implementation of GeolocationService such that we can guarantee that the success callback is called. I plan to add such a mock implementation (and hooks for DumpRenderTree) as part of Bug 27716, but it might take a while to get the whole framework in place. Would it be OK to submit this patch now, but leave the bug open to add the layout tests once the mock GeolocationService is in place?
Created attachment 39257 [details] Patch 3 for bug 27255 Adds a LayoutTest for the fix.
Comment on attachment 39257 [details] Patch 3 for bug 27255 > if (exec->hadException()) { > reportCurrentException(exec); > - raisedException = true; > } Just noticed that braces should go now that this is only one line. > - virtual void handleEvent(Geoposition*, bool& raisedException); > + virtual void handleEvent(Geoposition*); This should be private, not public. > + virtual void handleEvent(Geoposition* position) = 0; The argument name "position" should be omitted.
Created attachment 39270 [details] Patch 4 for bug 27255 > Just noticed that braces should go now that this is only one line. Fixed This should be private, not public. Fixed, and also in JSCustomPositionErrorCallback.h > The argument name "position" should be omitted. Fixed Could you commit on my behalf please - I don't have committer rights?
Comment on attachment 39270 [details] Patch 4 for bug 27255 Once Darin reviews this updated patch, a committer can mark it commit-queue+ and it will be committed by the bot.
Comment on attachment 39270 [details] Patch 4 for bug 27255 Actually, in this case, I can just forward Darin's r+ of your previous patch.
Comment on attachment 39270 [details] Patch 4 for bug 27255 Clearing flags on attachment: 39270 Committed r48210: <http://trac.webkit.org/changeset/48210>
All reviewed patches have been landed. Closing bug.