WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
29080
Geolocation Coordinates::toString() prints bogus values for unspecified properties.
https://bugs.webkit.org/show_bug.cgi?id=29080
Summary
Geolocation Coordinates::toString() prints bogus values for unspecified prope...
Steve Block
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Steve Block
Comment 1
2009-09-09 05:11:16 PDT
Created
attachment 39264
[details]
Patch 1 for
bug 29080
Fixes bug and adds LayoutTest.
Darin Adler
Comment 2
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?
Steve Block
Comment 3
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.
Eric Seidel (no email)
Comment 4
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.
Steve Block
Comment 5
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.
Sam Weinig
Comment 6
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.
Steve Block
Comment 7
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?
Greg Bolsinga
Comment 8
2009-09-09 14:52:34 PDT
Sounds good to me; I has assumed things had to implement ::toString()!
Sam Weinig
Comment 9
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.
Steve Block
Comment 10
2009-09-09 17:01:45 PDT
Created
attachment 39312
[details]
Patch 4 for
bug 29080
Removes Geoposition.toString and Coordinates.toString methods.
Maciej Stachowiak
Comment 11
2009-09-10 05:12:00 PDT
Comment on
attachment 39312
[details]
Patch 4 for
bug 29080
r=me
WebKit Commit Bot
Comment 12
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
.
Ben Murdoch
Comment 13
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)
Ben Murdoch
Comment 14
2009-09-10 06:27:34 PDT
landed as
r48252
.
Eric Seidel (no email)
Comment 15
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug