Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9051842
or point your web browser to
http://mondrian/9051842
to review the following code:
Change 9051842 by [EMAIL PROTECTED] on 2008/11/18 14:24:27 *pending*
Updates Geolocation API error codes to match W3C spec.
Note that this change involves collapsing two previous error codes (
kGeolocationLocationAcquisitionErrorCode = 2 and
kGeolocationLocationNotFoundErrorCode = 3) to one
(ERROR_CODE_POSITION_UNAVAILABLE = 2). Strictly speaking, this is a breaking
change with regard to the storage of Position objects in the Geolocation
database used to cache the last known position. However, we don't need to
update the database schema beacuse we only ever store good position fixes (ie
no error) in the database (see geolocation.cc, line 245).
R=andreip
[EMAIL PROTECTED]
DELTA=61 (29 added, 4 deleted, 28 changed)
OCL=9051842
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/position_table.cc#5
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#60
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#26
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc#32
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/gps_device_android.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/gps_device_wince.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#32
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#7
edit
61 delta lines: 29 added, 4 deleted, 28 changed
Also consider running:
g4 lint -c 9051842
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 9051842 by [EMAIL PROTECTED] on 2008/11/18 14:24:27 *pending*
Updates Geolocation API error codes to match W3C spec.
Note that this change involves collapsing two previous error codes (
kGeolocationLocationAcquisitionErrorCode = 2 and
kGeolocationLocationNotFoundErrorCode = 3) to one
(ERROR_CODE_POSITION_UNAVAILABLE = 2). Strictly speaking, this is a breaking
change with regard to the storage of Position objects in the Geolocation
database used to cache the last known position. However, we don't need to
update the database schema beacuse we only ever store good position fixes (ie
no error) in the database (see geolocation.cc, line 245).
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/position_table.cc#5
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#60
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#26
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc#32
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/gps_device_android.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/gps_device_wince.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#32
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#7
edit
====
//depot/googleclient/gears/opensource/gears/base/common/position_table.cc#5 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/base/common/position_table.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/position_table.cc
2008-11-18 18:07:13.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/position_table.cc
2008-11-18 16:46:58.000000000 +0000
@@ -222,8 +222,17 @@
position->address.country = statement.column_text16_safe(13);
position->address.country_code = statement.column_text16_safe(14);
position->address.postal_code = statement.column_text16_safe(15);
- position->error_code = statement.column_int(16);
+ int error_code = statement.column_int(16);
position->error_message = statement.column_text16_safe(17);
+
+ switch (error_code) {
+ case Position::ERROR_CODE_POSITION_UNAVAILABLE:
+ position->error_code = Position::ERROR_CODE_POSITION_UNAVAILABLE;
+ default:
+ // Note that in previous versions, error_code was set to kint32min to
+ // signify no error.
+ position->error_code = Position::ERROR_CODE_NONE;
+ }
return true;
}
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#60
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-11 11:16:49.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-18 16:49:35.000000000 +0000
@@ -209,7 +209,13 @@
// Retrieve the cached last known position, if available.
GeolocationDB *db = GeolocationDB::GetDB();
if (db) {
- db->RetrievePosition(kLastPositionName, &last_position_);
+ // Only update last_position if the stored position is good. This should
+ // always be the case.
+ Position stored_position;
+ if (db->RetrievePosition(kLastPositionName, &stored_position) &&
+ stored_position.IsGoodFix()) {
+ last_position_ = stored_position;
+ }
}
}
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#26 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/geolocation/geolocation.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-18 18:07:13.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-18 16:10:16.000000000 +0000
@@ -49,10 +49,6 @@
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;
-const int kGeolocationLocationNotFoundErrorCode = 3;
// The internal representation of an address.
struct Address {
@@ -84,6 +80,13 @@
// types when passed to JavaScript.
struct Position {
public:
+ // Error codes for returning to JavaScript. These values are defined by the
+ // W3C spec.
+ enum ErrorCode {
+ ERROR_CODE_NONE = -1,
+ ERROR_CODE_POSITION_UNAVAILABLE = 2,
+ };
+
Position()
: latitude(kBadLatLng),
longitude(kBadLatLng),
@@ -91,7 +94,7 @@
accuracy(kBadAccuracy),
altitude_accuracy(kBadAccuracy),
timestamp(-1),
- error_code(kint32min) {}
+ error_code(ERROR_CODE_NONE) {}
bool IsGoodFix() const {
// A good fix has a valid latitude, longitude, accuracy and timestamp.
return latitude >= -90.0 && latitude <= 90.0 &&
@@ -100,7 +103,7 @@
timestamp != -1;
}
bool IsInitialized() const {
- return IsGoodFix() || error_code != kint32min;
+ return IsGoodFix() || error_code != ERROR_CODE_NONE;
}
bool IncludesAddress() const {
return address.IsPopulated();
@@ -117,7 +120,7 @@
Address address;
// These properties are returned to JavaScript as a PositionError object.
- int error_code;
+ ErrorCode error_code;
std::string16 error_message; // Human-readable error message
};
====
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc#3
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc
2008-10-07 21:44:01.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation_db_test.cc
2008-11-18 16:37:25.000000000 +0000
@@ -60,7 +60,7 @@
position.address.country = STRING16(L"country");
position.address.country_code = STRING16(L"country code");
position.address.postal_code = STRING16(L"postal code");
- position.error_code = 7;
+ position.error_code = Position::ERROR_CODE_NONE;
position.error_message = STRING16(L"error");
Position updated_position;
@@ -79,7 +79,8 @@
updated_position.address.country = STRING16(L"updated country");
updated_position.address.country_code = STRING16(L"updated country code");
updated_position.address.postal_code = STRING16(L"updated postal code");
- updated_position.error_code = 17;
+ updated_position.error_code =
+ Position::ERROR_CODE_POSITION_UNAVAILABLE;
updated_position.error_message = STRING16(L"updated error");
Position retrieved_position;
====
//depot/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc#32
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/geolocation/geolocation_test.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation_test.cc
2008-11-18 18:07:13.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation_test.cc
2008-11-18 16:51:43.000000000 +0000
@@ -557,10 +557,16 @@
GetDoublePropertyIfDefined(context, object.get(),
STRING16(L"altitudeAccuracy"),
&position->altitude_accuracy);
+ int error_code = Position::ERROR_CODE_NONE;
GetIntegerPropertyIfDefined(context, object.get(), STRING16(L"errorCode"),
- &position->error_code);
+ &error_code);
GetStringPropertyIfDefined(context, object.get(), STRING16(L"errorMessage"),
&position->error_message);
+
+ switch (error_code) {
+ case Position::ERROR_CODE_POSITION_UNAVAILABLE:
+ position->error_code = Position::ERROR_CODE_POSITION_UNAVAILABLE;
+ }
}
void ConfigureGeolocationMockLocationProviderForTest(JsCallContext *context) {
====
//depot/googleclient/gears/opensource/gears/geolocation/gps_device_android.cc#1
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/geolocation/gps_device_android.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/gps_device_android.cc
2008-11-18 18:07:14.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/gps_device_android.cc
2008-11-18 17:35:55.000000000 +0000
@@ -123,7 +123,7 @@
if (scope.Occurred()) {
// Something went wrong when instantiating the Java object.
LOG(("Could not instantiate the Java object.\n"));
- position_.error_code = kGeolocationLocationAcquisitionErrorCode;
+ position_.error_code = Position::ERROR_CODE_POSITION_UNAVAILABLE;
position_.error_message = kGpsErrorMessage;
// No need to call 'scope.Clear();', as the exception
// will be cleared by the destructor of scope.
@@ -188,11 +188,11 @@
void AndroidGpsDevice::ProviderError(bool is_disabled) {
if (is_disabled) {
LOG(("GPS disabled, updating listener.\n"));
- listener_->GpsFatalError(kGeolocationLocationAcquisitionErrorCode,
+ listener_->GpsFatalError(Position::ERROR_CODE_POSITION_UNAVAILABLE,
kGpsDisabledMessage);
} else {
LOG(("GPS error, updating listener.\n"));
- listener_->GpsFatalError(kGeolocationLocationAcquisitionErrorCode,
+ listener_->GpsFatalError(Position::ERROR_CODE_POSITION_UNAVAILABLE,
kGpsErrorMessage);
}
}
====
//depot/googleclient/gears/opensource/gears/geolocation/gps_device_wince.cc#1 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/geolocation/gps_device_wince.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/gps_device_wince.cc
2008-11-18 18:07:14.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/gps_device_wince.cc
2008-11-18 17:37:19.000000000 +0000
@@ -64,7 +64,7 @@
// Local function
// This assumes that position_2 is a good fix.
static bool PositionsDiffer(const Position &position_1,
- const Position &position_2);
+ const Position &position_2);
// GpsLocationProvider factory method for platform-dependent GPS devices.
@@ -110,7 +110,7 @@
0); // Reserved
if (gps_handle_ == NULL) {
LOG(("WinceGpsDevice::Run() : Failed to open handle to GPS device.\n"));
- listener_->GpsFatalError(kGeolocationLocationAcquisitionErrorCode,
+ listener_->GpsFatalError(Position::ERROR_CODE_POSITION_UNAVAILABLE,
kFailedToConnectErrorMessage);
return;
}
@@ -169,11 +169,11 @@
assert(wait_milliseconds != INFINITE);
switch (state_) {
case STATE_CONNECTING:
- listener_->GpsFatalError(kGeolocationLocationAcquisitionErrorCode,
+ listener_->GpsFatalError(Position::ERROR_CODE_POSITION_UNAVAILABLE,
kFailedToConnectErrorMessage);
break;
case STATE_ACQUIRING_FIX:
- listener_->GpsFatalError(kGeolocationLocationNotFoundErrorCode,
+ listener_->GpsFatalError(Position::ERROR_CODE_POSITION_UNAVAILABLE,
kFailedToGetFixErrorMessage);
break;
default:
====
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#32
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/geolocation/network_location_request.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/network_location_request.cc
2008-11-18 18:07:14.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/network_location_request.cc
2008-11-18 16:57:25.000000000 +0000
@@ -247,7 +247,7 @@
if (!http_post_result) {
LOG(("NetworkLocationRequest::GetLocationFromResponse() : HttpPost request
"
"failed.\n"));
- position->error_code = kGeolocationLocationAcquisitionErrorCode;
+ position->error_code = Position::ERROR_CODE_POSITION_UNAVAILABLE;
position->error_message = STRING16(L"No response from network provider "
L"at ");
position->error_message += server_url.c_str();
@@ -260,7 +260,7 @@
// The response was successfully parsed, but it may not be a valid
// position fix.
if (!position->IsGoodFix()) {
- position->error_code = kGeolocationLocationNotFoundErrorCode;
+ position->error_code = Position::ERROR_CODE_POSITION_UNAVAILABLE;
position->error_message = STRING16(L"Network provider at ");
position->error_message += server_url.c_str();
position->error_message += STRING16(L" did not provide a good position
"
@@ -270,7 +270,7 @@
// We failed to parse the repsonse.
LOG(("NetworkLocationRequest::GetLocationFromResponse() : Response "
"malformed.\n"));
- position->error_code = kGeolocationLocationAcquisitionErrorCode;
+ position->error_code = Position::ERROR_CODE_POSITION_UNAVAILABLE;
position->error_message = STRING16(L"Response from network provider at
");
position->error_message += server_url.c_str();
position->error_message += STRING16(L" was malformed.");
@@ -279,7 +279,7 @@
// The response was bad.
LOG(("NetworkLocationRequest::GetLocationFromResponse() : HttpPost "
"response was bad.\n"));
- position->error_code = kGeolocationLocationAcquisitionErrorCode;
+ position->error_code = Position::ERROR_CODE_POSITION_UNAVAILABLE;
position->error_message = STRING16(L"Network provider at ");
position->error_message += server_url.c_str();
position->error_message += STRING16(L" returned error code ");
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#7
-
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js
====
# action=edit type=text
---
googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js
2008-11-18 18:07:14.000000000 +0000
+++
googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js
2008-11-18 18:05:57.000000000 +0000
@@ -153,8 +153,8 @@
var position;
var correctPosition;
- var locationAcquisitionErrorCode = 2;
- var locationNotFoundErrorCode = 3;
+ // Error code values. These must match those defined in geolocation.h.
+ var error_code_position_unavailable = 2;
// Test good response with valid position.
var responseBody = '{ ' +
@@ -241,7 +241,7 @@
0, // timestamp
dummy_server);
correctPosition = new Object();
- correctPosition.code = locationAcquisitionErrorCode;
+ correctPosition.code = error_code_position_unavailable;
correctPosition.message = 'No response from network provider at ' +
dummy_server +
'.';
@@ -255,7 +255,7 @@
0, // timestamp
dummy_server);
correctPosition = new Object();
- correctPosition.code = locationAcquisitionErrorCode;
+ correctPosition.code = error_code_position_unavailable;
correctPosition.message = 'Network provider at ' +
dummy_server +
' returned error code 400.';
@@ -269,7 +269,7 @@
0, // timestamp
dummy_server);
correctPosition = new Object();
- correctPosition.code = locationAcquisitionErrorCode;
+ correctPosition.code = error_code_position_unavailable;
correctPosition.message = malformedResponseError;
assertObjectEqual(correctPosition, position);
@@ -281,7 +281,7 @@
0, // timestamp
dummy_server);
correctPosition = new Object();
- correctPosition.code = locationAcquisitionErrorCode;
+ correctPosition.code = error_code_position_unavailable;
correctPosition.message = malformedResponseError;
assertObjectEqual(correctPosition, position);
@@ -293,7 +293,7 @@
0, // timestamp
dummy_server);
correctPosition = new Object();
- correctPosition.code = locationAcquisitionErrorCode;
+ correctPosition.code = error_code_position_unavailable;
correctPosition.message = malformedResponseError;
assertObjectEqual(correctPosition, position);
@@ -305,7 +305,7 @@
0, // timestamp
dummy_server);
correctPosition = new Object();
- correctPosition.code = locationNotFoundErrorCode;
+ correctPosition.code = error_code_position_unavailable;
correctPosition.message = noGoodFixError;
assertObjectEqual(correctPosition, position);
@@ -317,7 +317,7 @@
0, // timestamp
dummy_server);
correctPosition = new Object();
- correctPosition.code = locationNotFoundErrorCode;
+ correctPosition.code = error_code_position_unavailable;
correctPosition.message = noGoodFixError;
assertObjectEqual(correctPosition, position);
}
