Bug 29080 - Geolocation Coordinates::toString() prints bogus values for unspecified properties.
Summary: Geolocation Coordinates::toString() prints bogus values for unspecified prope...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-09 05:02 PDT by Steve Block
Modified: 2009-09-10 12:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch 1 for bug 29080 (7.21 KB, patch)
2009-09-09 05:11 PDT, Steve Block
darin: review+
Details | Formatted Diff | Diff
Patch 2 for bug 29080 (7.57 KB, patch)
2009-09-09 09:09 PDT, Steve Block
eric: review-
Details | Formatted Diff | Diff
Patch 3 for bug 29080 (deleted)
2009-09-09 12:13 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch 4 for bug 29080 (24.38 KB, patch)
2009-09-09 17:01 PDT, Steve Block
mjs: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-09-09 05:02:41 PDT
Geolocation Coordinates::toString() prints the values of all properties, even for optional properties which have not been specified. This means that it is not possible to determine which properties are valid.

Note that this affects the toString method in both C++ and JS.
Comment 1 Steve Block 2009-09-09 05:11:16 PDT
Created attachment 39264 [details]
Patch 1 for bug 29080

Fixes bug and adds LayoutTest.
Comment 2 Darin Adler 2009-09-09 07:33:39 PDT
Comment on attachment 39264 [details]
Patch 1 for bug 29080

Coordinates::toString now does String appending, which requires new memory allocation for every single operation and is hence quite inefficient. It's a better idiom to use StringBuilder or Vector<UChar>. Same comment about the existing code in Geoposition::toString.

Does this textual version of the object match anyone else's Geolocation implementation? Is the behavior specified anywhere?
Comment 3 Steve Block 2009-09-09 09:09:11 PDT
Created attachment 39271 [details]
Patch 2 for bug 29080

> Coordinates::toString now does String appending, which requires new memory
> allocation for every single operation and is hence quite inefficient.
Fixed

> Does this textual version of the object match anyone else's Geolocation
> implementation?
Neither Gears nor Firefox provide custom toString() methods for their JS position or coordinates objects.

> Is the behavior specified anywhere?
No, the spec does not mention this.
Comment 4 Eric Seidel (no email) 2009-09-09 10:51:17 PDT
Comment on attachment 39271 [details]
Patch 2 for bug 29080

These results assume that the mock makes a synchronous callback.  Probably warrants a comment.

Also:
+    window.layoutTestController.notifyDone();
is not needed if this is sync.

Nit 4space indent:
+function shouldContainString(expression, string)
+{
+  var expressionResult = eval(expression);
+
+  if (expressionResult.indexOf(string) != -1)
+    testPassed(expression + ' contains "' + string + '".');
+  else
+    testFailed(expression + ' should contain "' + string + '". Was "' + stringify(expressionResult) + '".');
+}

window. are not technically needed here, but certainly don't hurt:
+window.layoutTestController.setGeolocationPermission(true);
+window.layoutTestController.setMockGeolocationPosition(mockLatitude,

Probably needs a comment:
+    position = p;  // shouldBe can't use local variables yet.

Oh, I see.  You modified the template to not include js-post.  In that case, you really should add this script to the exclusion list for make-script-test-wrappers.  And possibly ignore my comments above about this being sync.

In general this looks good, but could use one more round.
Comment 5 Steve Block 2009-09-09 12:13:31 PDT
Created attachment 39292 [details]
Patch 3 for bug 29080

> Nit 4space indent:
Fixed

>Probably needs a comment:
> +    position = p;  // shouldBe can't use local variables yet.
Fixed

> Oh, I see.  You modified the template to not include js-post.  In that case,
> you really should add this script to the exclusion list for
> make-script-test-wrappers.
Fixed, and also for asynchronous other Geolocation tests.

>  And possibly ignore my comments above about this being sync.
Yes, it's asynchronous.
Comment 6 Sam Weinig 2009-09-09 14:21:29 PDT
> > Is the behavior specified anywhere?
> No, the spec does not mention this.

If the spec does not mention this and we are the only ones who support it, we should just remove it.
Comment 7 Steve Block 2009-09-09 14:50:36 PDT
> If the spec does not mention this and we are the only ones who support it, we
> should just remove it.
Sounds reasonable to me. Presumably we should remove the C++ side too, as it's only useful for logging?
Comment 8 Greg Bolsinga 2009-09-09 14:52:34 PDT
Sounds good to me; I has assumed things had to implement ::toString()!
Comment 9 Sam Weinig 2009-09-09 15:13:01 PDT
(In reply to comment #7)
> > If the spec does not mention this and we are the only ones who support it, we
> > should just remove it.
> Sounds reasonable to me. Presumably we should remove the C++ side too, as it's
> only useful for logging?

Yeah, no reason to keep it around.
Comment 10 Steve Block 2009-09-09 17:01:45 PDT
Created attachment 39312 [details]
Patch 4 for bug 29080

Removes Geoposition.toString and Coordinates.toString methods.
Comment 11 Maciej Stachowiak 2009-09-10 05:12:00 PDT
Comment on attachment 39312 [details]
Patch 4 for bug 29080

r=me
Comment 12 WebKit Commit Bot 2009-09-10 05:28:54 PDT
Comment on attachment 39312 [details]
Patch 4 for bug 29080

Rejecting patch 39312 from commit-queue.

benm@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
Comment 13 Ben Murdoch 2009-09-10 05:33:45 PDT
(In reply to comment #12)
> (From update of attachment 39312 [details])
> Rejecting patch 39312 from commit-queue.
> 
> benm@google.com does not have committer permissions according to
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

Will land manually and ask Eric why the commit queue thinks I'm not a committer (I'm on line 50 of committers.py)
Comment 14 Ben Murdoch 2009-09-10 06:27:34 PDT
landed as r48252.
Comment 15 Eric Seidel (no email) 2009-09-10 12:39:15 PDT
(In reply to comment #12)
> (From update of attachment 39312 [details])
> Rejecting patch 39312 from commit-queue.
> 
> benm@google.com does not have committer permissions according to
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

Probably we hit bug 29113.  It's possible the commit-queue was still running from an old copy of committers.py.  I'll check.