Hello cprince,
I'd like you to do a code review. Please execute
g4 diff -c 8148621
or point your web browser to
http://mondrian/8148621
to review the following code:
Change 8148621 by [EMAIL PROTECTED] on 2008/09/01 13:09:23 *pending*
Updates Position altitude, accuracy and altitude_accuracy fields to use
double rather than int internally.
R=cprince
[EMAIL PROTECTED]
DELTA=88 (12 added, 16 deleted, 60 changed)
OCL=8148621
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/position_table.cc#2
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#45
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#19
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc#21
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/gps_location_provider_wince.cc#9
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#21
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#31
edit
88 delta lines: 12 added, 16 deleted, 60 changed
Also consider running:
g4 lint -c 8148621
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 8148621 by [EMAIL PROTECTED] on 2008/09/01 13:09:23 *pending*
Updates Position altitude, accuracy and altitude_accuracy fields to use
double rather than int internally.
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/position_table.cc#2
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#45
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#19
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc#2
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc#21
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/gps_location_provider_wince.cc#9
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#21
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#31
edit
====
//depot/googleclient/gears/opensource/gears/base/common/position_table.cc#2 -
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/base/common/position_table.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/position_table.cc
2008-09-01 13:10:54.000000000 +0100
+++ googleclient/gears/opensource/gears/base/common/position_table.cc
2008-09-01 13:14:58.000000000 +0100
@@ -94,15 +94,18 @@
LOG_BIND_ERROR("longitude");
return false;
}
- if (SQLITE_OK != statement.bind_int(3, position.altitude)) {
+ // The Altitude, Accuracy and AltitudeAccuracy columns have type INTEGER, but
+ // we can safely store and extract floating point values because SQLite uses
+ // type affinity. See http://www.sqlite.org/datatype3.html#affinity.
+ if (SQLITE_OK != statement.bind_double(3, position.altitude)) {
LOG_BIND_ERROR("altitude");
return false;
}
- if (SQLITE_OK != statement.bind_int(4, position.accuracy)) {
+ if (SQLITE_OK != statement.bind_double(4, position.accuracy)) {
LOG_BIND_ERROR("horizontal accuracy");
return false;
}
- if (SQLITE_OK != statement.bind_int(5, position.altitude_accuracy)) {
+ if (SQLITE_OK != statement.bind_double(5, position.altitude_accuracy)) {
LOG_BIND_ERROR("vertical accuracy");
return false;
}
@@ -203,9 +206,12 @@
// Skip Name at index 0.
position->latitude = statement.column_double(1);
position->longitude = statement.column_double(2);
- position->altitude = statement.column_int(3);
- position->accuracy = statement.column_int(4);
- position->altitude_accuracy = statement.column_int(5);
+ // The Altitude, Accuracy and AltitudeAccuracy columns have type INTEGER, but
+ // we can safely store and extract floating point values because SQLite uses
+ // type affinity. See http://www.sqlite.org/datatype3.html#affinity.
+ position->altitude = statement.column_double(3);
+ position->accuracy = statement.column_double(4);
+ position->altitude_accuracy = statement.column_double(5);
position->timestamp = statement.column_int64(6);
position->address.street_number = statement.column_text16_safe(7);
position->address.street = statement.column_text16_safe(8);
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#45
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-09-01 13:10:55.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-08-29 17:28:10.000000000 +0100
@@ -160,11 +160,6 @@
const std::string16 &name,
JsScopedToken *token);
-// Sets an object integer property if the input value is valid.
-static bool SetObjectPropertyIfValidInt(const std::string16 &property_name,
- int value,
- JsObject *object);
-
// Sets an object string property if the input value is valid.
static bool SetObjectPropertyIfValidString(const std::string16 &property_name,
const std::string16 &value,
@@ -918,8 +913,8 @@
position.latitude);
result &= position_object->SetPropertyDouble(STRING16(L"longitude"),
position.longitude);
- result &= position_object->SetPropertyInt(STRING16(L"accuracy"),
- position.accuracy);
+ result &= position_object->SetPropertyDouble(STRING16(L"accuracy"),
+ position.accuracy);
scoped_ptr<JsObject> date_object(js_runner->NewDate(position.timestamp));
result &= NULL != date_object.get();
if (date_object.get()) {
@@ -928,12 +923,14 @@
}
// Other properties may not be valid.
- result &= SetObjectPropertyIfValidInt(STRING16(L"altitude"),
- position.altitude,
- position_object);
- result &= SetObjectPropertyIfValidInt(STRING16(L"altitudeAccuracy"),
- position.altitude_accuracy,
- position_object);
+ if (position.altitude > kBadAltitude) {
+ result &= position_object->SetPropertyDouble(STRING16(L"altitude"),
+ position.altitude);
+ }
+ if (position.altitude_accuracy >= 0.0) {
+ result &= position_object->SetPropertyDouble(STRING16(L"altitudeAccuracy"),
+ position.altitude_accuracy);
+ }
// Address
if (use_address) {
@@ -1143,16 +1140,6 @@
return false;
}
*token = token_local;
- return true;
-}
-
-static bool SetObjectPropertyIfValidInt(const std::string16 &property_name,
- int value,
- JsObject *object) {
- assert(object);
- if (kint32min != value) {
- return object->SetPropertyInt(property_name, value);
- }
return true;
}
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#19 -
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/geolocation.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-09-01 13:10:55.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-08-29 17:04:44.000000000 +0100
@@ -44,7 +44,11 @@
#include "gears/geolocation/timed_callback.h"
#include "third_party/linked_ptr/linked_ptr.h"
-static const int kBadLatLng = 200;
+static const double kBadLatLng = 200;
+// Lowest point on land is at approximately -400 metres.
+static const int kBadAltitude = -1000;
+static const int kBadAccuracy = -1; // Accuracy must be non-negative.
+
// Error codes for returning to JavaScript.
const int kGeolocationLocationAcquisitionErrorCode = 2;
@@ -63,24 +67,23 @@
std::string16 postal_code; // postal code
};
-// The internal representation of a position. We use kint32min to represent
-// unknown values for integer fields. Some properties use different types when
-// passed to JavaScript.
+// The internal representation of a position. Some properties use different
+// types when passed to JavaScript.
struct Position {
public:
Position()
: latitude(kBadLatLng),
longitude(kBadLatLng),
- altitude(kint32min),
- accuracy(kint32min),
- altitude_accuracy(kint32min),
+ altitude(kBadAltitude),
+ accuracy(kBadAccuracy),
+ altitude_accuracy(kBadAccuracy),
timestamp(-1),
error_code(kint32min) {}
bool IsGoodFix() const {
// A good fix has a valid latitude, longitude, accuracy and timestamp.
return latitude >= -90.0 && latitude <= 90.0 &&
longitude >= -180.0 && longitude <= 180.0 &&
- accuracy != kint32min &&
+ accuracy >= 0.0 &&
timestamp != -1;
}
bool IsInitialized() const {
@@ -90,9 +93,9 @@
// These properties are returned to JavaScript as a Position object.
double latitude; // In degrees
double longitude; // In degrees
- int altitude; // In metres
- int accuracy; // In metres
- int altitude_accuracy; // In metres
+ double altitude; // In metres
+ double accuracy; // In metres
+ double altitude_accuracy; // In metres
int64 timestamp; // Milliseconds since 1st Jan 1970
// Note that the corresponding JavaScript Position property is
'gearsAddress'.
Address address;
====
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc#2
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc
2008-09-01 13:46:53.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc
2008-09-01 13:16:58.000000000 +0100
@@ -45,11 +45,11 @@
}
Position position;
- position.latitude = 1.0;
- position.longitude = 2.0;
- position.altitude = 3;
- position.accuracy = 4;
- position.altitude_accuracy = 5;
+ position.latitude = 1.1;
+ position.longitude = 2.1;
+ position.altitude = 3.1;
+ position.accuracy = 4.1;
+ position.altitude_accuracy = 5.1;
position.timestamp = 6;
position.address.street_number = STRING16(L"street number");
position.address.street = STRING16(L"street");
@@ -64,11 +64,11 @@
position.error_message = STRING16(L"error");
Position updated_position;
- updated_position.latitude = 11.0;
- updated_position.longitude = 12.0;
- updated_position.altitude = 13;
- updated_position.accuracy = 14;
- updated_position.altitude_accuracy = 15;
+ updated_position.latitude = 11.1;
+ updated_position.longitude = 12.1;
+ updated_position.altitude = 13.1;
+ updated_position.accuracy = 14.1;
+ updated_position.altitude_accuracy = 15.1;
updated_position.timestamp = 16;
updated_position.address.street_number = STRING16(L"updated street number");
updated_position.address.street = STRING16(L"updated street");
====
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc#21
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation_test.cc
2008-09-01 13:10:55.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation_test.cc
2008-08-29 17:12:22.000000000 +0100
@@ -517,12 +517,12 @@
&position.latitude);
GetDoublePropertyIfDefined(context, object, STRING16(L"longitude"),
&position.longitude);
- GetIntegerPropertyIfDefined(context, object, STRING16(L"altitude"),
- &position.altitude);
- GetIntegerPropertyIfDefined(context, object, STRING16(L"accuracy"),
- &position.accuracy);
- GetIntegerPropertyIfDefined(context, object, STRING16(L"altitudeAccuracy"),
- &position.altitude_accuracy);
+ GetDoublePropertyIfDefined(context, object, STRING16(L"altitude"),
+ &position.altitude);
+ GetDoublePropertyIfDefined(context, object, STRING16(L"accuracy"),
+ &position.accuracy);
+ GetDoublePropertyIfDefined(context, object, STRING16(L"altitudeAccuracy"),
+ &position.altitude_accuracy);
GetIntegerPropertyIfDefined(context, object, STRING16(L"errorCode"),
&position.error_code);
GetStringPropertyIfDefined(context, object, STRING16(L"errorMessage"),
====
//depot/googleclient/gears/opensource/gears/geolocation/gps_location_provider_wince.cc#9
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/gps_location_provider_wince.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/geolocation/gps_location_provider_wince.cc
2008-09-01 13:10:55.000000000 +0100
+++
googleclient/gears/opensource/gears/geolocation/gps_location_provider_wince.cc
2008-08-29 17:16:06.000000000 +0100
@@ -233,7 +233,7 @@
}
if (gps_position.dwValidFields & GPS_VALID_ALTITUDE_WRT_ELLIPSOID) {
- int altitude = static_cast<int>(gps_position.flAltitudeWRTEllipsoid);
+ double altitude = gps_position.flAltitudeWRTEllipsoid;
if (position_.altitude != altitude) {
position_.altitude = altitude;
update_available = true;
====
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#21
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/network_location_request.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/network_location_request.cc
2008-09-01 13:10:55.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/network_location_request.cc
2008-09-01 13:40:39.000000000 +0100
@@ -369,15 +369,15 @@
// 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.
-// Gets an integer if it's present.
-static bool GetAsInt(const Json::Value &object,
- const std::string &property_name,
- int *out) {
+// Gets a double if it's present.
+static bool GetAsDouble(const Json::Value &object,
+ const std::string &property_name,
+ double *out) {
assert(out);
- if (!object[property_name].isInt()) {
- return false;
- }
- *out = object[property_name].asInt();
+ if (!object[property_name].isDouble()) {
+ return false;
+ }
+ *out = object[property_name].asDouble();
return true;
}
@@ -427,16 +427,16 @@
// latitude, longitude and accuracy fields are required.
if (!location[kLatitudeString].isDouble() ||
!location[kLongitudeString].isDouble() ||
- !location[kAccuracyString].isInt()) {
+ !location[kAccuracyString].isDouble()) {
return false;
}
position->latitude = location[kLatitudeString].asDouble();
position->longitude = location[kLongitudeString].asDouble();
- position->accuracy = location[kAccuracyString].asInt();
+ position->accuracy = location[kAccuracyString].asDouble();
// Other fields are optional.
- GetAsInt(location, kAltitudeString, &position->altitude);
- GetAsInt(location, kAltitudeAccuracyString, &position->altitude_accuracy);
+ GetAsDouble(location, kAltitudeString, &position->altitude);
+ GetAsDouble(location, kAltitudeAccuracyString, &position->altitude_accuracy);
Json::Value address = location[kAddressString];
if (address.isObject()) {
GetAsString(address, kStreetNumberString,
&position->address.street_number);
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#31
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/test/testcases/internal_tests.js
====
# action=edit type=text
--- googleclient/gears/opensource/gears/test/testcases/internal_tests.js
2008-09-01 13:46:54.000000000 +0100
+++ googleclient/gears/opensource/gears/test/testcases/internal_tests.js
2008-09-01 13:35:42.000000000 +0100
@@ -423,9 +423,9 @@
'"location" : { ' +
'"latitude" : 53.1, ' +
'"longitude" : -0.1, ' +
- '"altitude" : 30, ' +
- '"horizontal_accuracy" : 1200, ' +
- '"vertical_accuracy" : 10, ' +
+ '"altitude" : 30.1, ' +
+ '"horizontal_accuracy" : 1200.1, ' +
+ '"vertical_accuracy" : 10.1, ' +
'"address" : { ' +
'"street_number": "100", ' +
'"street": "Amphibian Walkway", ' +
@@ -447,9 +447,9 @@
correctPosition = new Object();
correctPosition.latitude = 53.1;
correctPosition.longitude = -0.1;
- correctPosition.altitude = 30;
- correctPosition.accuracy = 1200;
- correctPosition.altitudeAccuracy = 10;
+ correctPosition.altitude = 30.1;
+ correctPosition.accuracy = 1200.1;
+ correctPosition.altitudeAccuracy = 10.1;
correctPosition.gearsAddress = new Object();
correctPosition.gearsAddress.streetNumber = '100';
correctPosition.gearsAddress.street = 'Amphibian Walkway';
@@ -576,7 +576,7 @@
var mockPosition = {
latitude: 51.0,
longitude: -0.1,
- accuracy: 100
+ accuracy: 100.1
};
internalTests.configureGeolocationMockLocationProviderForTest(mockPosition);
var locationAvailable2 = function(position) {
