Hello jripley,
I'd like you to do a code review. Please execute
g4 diff -c 8167137
or point your web browser to
http://mondrian/8167137
to review the following code:
Change 8167137 by [EMAIL PROTECTED] on 2008/09/03 19:02:19 *pending*
Makes network location provider robust to integer values for floating
point fields in server response.
R=jripley
[EMAIL PROTECTED]
DELTA=49 (45 added, 0 deleted, 4 changed)
OCL=8167137
Affected files ...
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#23
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#33
edit
49 delta lines: 45 added, 0 deleted, 4 changed
Also consider running:
g4 lint -c 8167137
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 8167137 by [EMAIL PROTECTED] on 2008/09/03 19:02:19 *pending*
Makes network location provider robust to integer values for floating
point fields in server response.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#23
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#33
edit
====
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#23
-
c:\MyDocs\Gears7/googleclient/gears/opensource/gears/geolocation/network_location_request.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/network_location_request.cc
2008-09-04 00:11:20.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/network_location_request.cc
2008-09-03 23:16:16.000000000 +0100
@@ -366,6 +366,15 @@
return false;
}
+// Numeric values without a decimal point have type integer and IsDouble() will
+// return false. This is convenience function for detecting integer or floating
+// point numeric values. Note that isIntegral() includes boolean values, which
+// is not what we want.
+static bool IsDoubleOrInt(const Json::Value &object,
+ const std::string &property_name) {
+ return object[property_name].isDouble() || object[property_name].isInt();
+}
+
// The JsValue::asXXX() methods return zero if a property isn't specified. For
// our purposes, zero is a valid value, so we have to test for existence.
@@ -374,7 +383,7 @@
const std::string &property_name,
double *out) {
assert(out);
- if (!object[property_name].isDouble()) {
+ if (!IsDoubleOrInt(object, property_name)) {
return false;
}
*out = object[property_name].asDouble();
@@ -401,6 +410,9 @@
LOG(("ParseServerResponse() : Response was empty.\n"));
return false;
}
+ LOG(("ParseServerResponse() : Parsing response %s.\n",
+ response_body.c_str()));
+
// Parse the response, ignoring comments.
Json::Reader reader;
Json::Value response_object;
@@ -425,9 +437,9 @@
}
// latitude, longitude and accuracy fields are required.
- if (!location[kLatitudeString].isDouble() ||
- !location[kLongitudeString].isDouble() ||
- !location[kAccuracyString].isDouble()) {
+ if (!IsDoubleOrInt(location, kLatitudeString) ||
+ !IsDoubleOrInt(location, kLongitudeString) ||
+ !IsDoubleOrInt(location, kAccuracyString)) {
return false;
}
position->latitude = location[kLatitudeString].asDouble();
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#33
-
c:\MyDocs\Gears7/googleclient/gears/opensource/gears/test/testcases/internal_tests.js
====
# action=edit type=text
--- googleclient/gears/opensource/gears/test/testcases/internal_tests.js
2008-09-04 00:11:20.000000000 +0100
+++ googleclient/gears/opensource/gears/test/testcases/internal_tests.js
2008-09-03 23:16:53.000000000 +0100
@@ -462,6 +462,39 @@
correctPosition.timestamp = new Date(42);
assertObjectEqual(correctPosition, position);
+ // We should also accept integer values for floating point fields.
+ var responseBody = '{ ' +
+ '"location" : { ' +
+ '"latitude" : 53, ' +
+ '"longitude" : 0, ' +
+ '"altitude" : 30, ' +
+ '"horizontal_accuracy" : 1200, ' +
+ '"vertical_accuracy" : 10, ' +
+ '"address" : { ' +
+ '"street_number": "100", ' +
+ '"street": "Amphibian Walkway", ' +
+ '"city": "Mountain View", ' +
+ '"county": "Mountain View County", ' +
+ '"region": "California", ' +
+ '"country": "United States of America", ' +
+ '"country_code": "US", ' +
+ '"postal_code": "94043" ' +
+ '} ' +
+ '} ' +
+ '}';
+ position = internalTests.testGeolocationGetLocationFromResponse(
+ true, // HttpPost result
+ 200, // status code
+ responseBody,
+ 42, // timestamp
+ ''); // server URL
+ correctPosition.latitude = 53;
+ correctPosition.longitude = 0;
+ correctPosition.altitude = 30;
+ correctPosition.accuracy = 1200;
+ correctPosition.altitudeAccuracy = 10;
+ assertObjectEqual(correctPosition, position);
+
// Test no response.
position = internalTests.testGeolocationGetLocationFromResponse(
false, // HttpPost result
