Bug 29080 – Geolocation Coordinates::toString() prints bogus values for unspecified properties.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

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.

[Steve Block]

Created attachment 39264 [details]
Patch 1 for bug 29080

Fixes bug and adds LayoutTest.

[Darin Adler]

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?

[Steve Block]

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.

[Eric Seidel (no email)]

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.

[Steve Block]

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.

[Sam Weinig]

> > 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.

[Steve Block]

> 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?

[Greg Bolsinga]

Sounds good to me; I has assumed things had to implement ::toString()!

[Sam Weinig]

(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.

[Steve Block]

Created attachment 39312 [details]
Patch 4 for bug 29080

Removes Geoposition.toString and Coordinates.toString methods.

[Maciej Stachowiak]

Comment on attachment 39312 [details]
Patch 4 for bug 29080

r=me

[WebKit Commit Bot]

Comment on attachment 39312 [details]
Patch 4 for bug 29080

Rejecting patch 39312 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Ben Murdoch]

(In reply to comment #12)
> (From update of attachment 39312 [details])
> Rejecting patch 39312 from commit-queue.
> 
> [e-mail] 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)

[Ben Murdoch]

landed as r48252.

[Eric Seidel (no email)]

(In reply to comment #12)
> (From update of attachment 39312 [details])
> Rejecting patch 39312 from commit-queue.
> 
> [e-mail] 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.