Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9513209
or point your web browser to
http://mondrian/9513209
to review the following code:
Change 9513209 by stevebl...@steveblock-gears3 on 2008/12/23 13:33:13 *pending*
Adds Geolocation position.coords property.
R=andreip
[email protected]
DELTA=197 (99 added, 1 deleted, 97 changed)
OCL=9513209
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#70
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#15
edit
197 delta lines: 99 added, 1 deleted, 97 changed
Also consider running:
g4 lint -c 9513209
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 9513209 by stevebl...@steveblock-gears3 on 2008/12/23 13:33:13 *pending*
Adds Geolocation position.coords property.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#70
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#15
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#70
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-12-23 13:03:41.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-12-23 14:24:03.000000000 +0000
@@ -82,6 +82,13 @@
static const int kLastRepeatingRequestId = kint32max; // Repeating IDs
positive
static const int kLastSingleRequestId = kint32min; // Single IDs negative
+// Position object constants.
+static const char16 *kLatitudeString = STRING16(L"latitude");
+static const char16 *kLongitudeString = STRING16(L"longitude");
+static const char16 *kAccuracyString = STRING16(L"accuracy");
+static const char16 *kAltitudeString = STRING16(L"altitude");
+static const char16 *kAltitudeAccuracyString = STRING16(L"altitudeAccuracy");
+
TimedMessage::TimedMessage(int timeout_milliseconds,
const std::string16 &message_type,
@@ -1299,45 +1306,73 @@
assert(position.IsGoodFix());
bool result = true;
- // latitude, longitude, accuracy and timestamp should always be valid.
- result &= position_object->SetPropertyDouble(STRING16(L"latitude"),
+
+ // The timestamp is a property of the position object. It should always be
+ // valid.
+ scoped_ptr<JsObject> date_object(js_runner->NewDate(position.timestamp));
+ if (!date_object.get()) {
+ assert(false);
+ return false;
+ }
+ result &= position_object->SetPropertyObject(STRING16(L"timestamp"),
+ date_object.get());
+
+ // The W3C spec stipulates that the latitude, longitude etc should be members
+ // of the coords object. In addition, we add these properties to the position
+ // object directly to maintain backwards compatibility with the pre-exisiting
+ // Gears API.
+ scoped_ptr<JsObject> coords_object(js_runner->NewObject());
+ if (!coords_object.get()) {
+ assert(false);
+ return false;
+ }
+
+ // latitude, longitude and accuracy should always be valid.
+ result &= position_object->SetPropertyDouble(kLatitudeString,
position.latitude);
- result &= position_object->SetPropertyDouble(STRING16(L"longitude"),
+ result &= position_object->SetPropertyDouble(kLongitudeString,
position.longitude);
- result &= position_object->SetPropertyDouble(STRING16(L"accuracy"),
+ result &= position_object->SetPropertyDouble(kAccuracyString,
position.accuracy);
- scoped_ptr<JsObject> date_object(js_runner->NewDate(position.timestamp));
- result &= NULL != date_object.get();
- if (date_object.get()) {
- result &= position_object->SetPropertyObject(STRING16(L"timestamp"),
- date_object.get());
- }
+ result &= coords_object->SetPropertyDouble(kLatitudeString,
+ position.latitude);
+ result &= coords_object->SetPropertyDouble(kLongitudeString,
+ position.longitude);
+ result &= coords_object->SetPropertyDouble(kAccuracyString,
+ position.accuracy);
// Other properties may not be valid.
if (position.altitude > kBadAltitude) {
- result &= position_object->SetPropertyDouble(STRING16(L"altitude"),
+ result &= position_object->SetPropertyDouble(kAltitudeString,
position.altitude);
+ result &= coords_object->SetPropertyDouble(kAltitudeString,
+ position.altitude);
}
if (position.altitude_accuracy >= 0.0) {
- result &= position_object->SetPropertyDouble(STRING16(L"altitudeAccuracy"),
+ result &= position_object->SetPropertyDouble(kAltitudeAccuracyString,
position.altitude_accuracy);
- }
+ result &= coords_object->SetPropertyDouble(kAltitudeAccuracyString,
+ position.altitude_accuracy);
+ }
+
+ result &= position_object->SetPropertyObject(STRING16(L"coords"),
+ coords_object.get());
// Address
if (use_address) {
scoped_ptr<JsObject> address_object(js_runner->NewObject());
- if (address_object.get()) {
- result &= CreateJavaScriptAddressObject(position.address,
- address_object.get());
- // Only add the address object if it has some properties.
- std::vector<std::string16> properties;
- if (address_object.get()->GetPropertyNames(&properties) &&
- !properties.empty()) {
- result &= position_object->SetPropertyObject(STRING16(L"gearsAddress"),
- address_object.get());
- }
- } else {
- result = false;
+ if (!address_object.get()) {
+ assert(false);
+ return false;
+ }
+ result &= CreateJavaScriptAddressObject(position.address,
+ address_object.get());
+ // Only add the address object if it has some properties.
+ std::vector<std::string16> properties;
+ if (address_object.get()->GetPropertyNames(&properties) &&
+ !properties.empty()) {
+ result &= position_object->SetPropertyObject(STRING16(L"gearsAddress"),
+ address_object.get());
}
}
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#15
-
c:\MyDocs\Gears3/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-12-23 13:35:34.000000000 +0000
+++
googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js
2008-12-23 14:44:12.000000000 +0000
@@ -157,23 +157,23 @@
// Test good response with valid position.
var responseBody = '{ ' +
- '"location" : { ' +
- '"latitude" : 53.1, ' +
- '"longitude" : -0.1, ' +
- '"altitude" : 30.1, ' +
- '"accuracy" : 1200.1, ' +
- '"altitude_accuracy" : 10.1, ' +
- '"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" ' +
- '} ' +
- '} ' +
+ ' "location" : { ' +
+ ' "latitude" : 53.1, ' +
+ ' "longitude" : -0.1, ' +
+ ' "altitude" : 30.1, ' +
+ ' "accuracy" : 1200.1, ' +
+ ' "altitude_accuracy" : 10.1, ' +
+ ' "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
@@ -181,43 +181,52 @@
responseBody,
42, // timestamp
''); // server URL
- correctPosition = new Object();
- correctPosition.latitude = 53.1;
- correctPosition.longitude = -0.1;
- 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';
- correctPosition.gearsAddress.city = 'Mountain View';
- correctPosition.gearsAddress.county = 'Mountain View County';
- correctPosition.gearsAddress.region = 'California';
- correctPosition.gearsAddress.country = 'United States of America';
- correctPosition.gearsAddress.countryCode = 'US';
- correctPosition.gearsAddress.postalCode = '94043';
- correctPosition.timestamp = new Date(42);
+ correctPosition = {
+ latitude: 53.1,
+ longitude: -0.1,
+ altitude: 30.1,
+ accuracy: 1200.1,
+ altitudeAccuracy: 10.1,
+ coords: {
+ latitude: 53.1,
+ longitude: -0.1,
+ altitude: 30.1,
+ accuracy: 1200.1,
+ altitudeAccuracy: 10.1
+ },
+ gearsAddress: {
+ streetNumber: '100',
+ street: 'Amphibian Walkway',
+ city: 'Mountain View',
+ county: 'Mountain View County',
+ region: 'California',
+ country: 'United States of America',
+ countryCode: 'US',
+ postalCode: '94043'
+ },
+ 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, ' +
- '"accuracy" : 1200, ' +
- '"altitude_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" ' +
- '} ' +
- '} ' +
+ ' "location" : { ' +
+ ' "latitude" : 53, ' +
+ ' "longitude" : 0, ' +
+ ' "altitude" : 30, ' +
+ ' "accuracy" : 1200, ' +
+ ' "altitude_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
@@ -230,6 +239,13 @@
correctPosition.altitude = 30;
correctPosition.accuracy = 1200;
correctPosition.altitudeAccuracy = 10;
+ correctPosition.coords = {
+ latitude: 53,
+ longitude: 0,
+ altitude: 30,
+ accuracy: 1200,
+ altitudeAccuracy: 10
+ };
assertObjectEqual(correctPosition, position);
// Test no response.
@@ -356,6 +372,13 @@
altitude: 30,
accuracy: 1200,
altitudeAccuracy: 10,
+ coords: {
+ latitude: 51.59,
+ longitude: -1.49,
+ altitude: 30,
+ accuracy: 1200,
+ altitudeAccuracy: 10
+ },
gearsAddress: {
streetNumber: '76',
street: 'Buckingham Palace Road',
@@ -367,7 +390,7 @@
countryCode: 'uk'
}
};
- position.timestamp = undefined;
+ delete position.timestamp;
assertObjectEqual(correctPosition, position);
makeUnsuccessfulRequest();
};
@@ -397,13 +420,17 @@
var mockPosition = {
latitude: 51.0,
longitude: -0.1,
- accuracy: 100.1
+ accuracy: 100.1,
+ coords: {
+ latitude: 51.0,
+ longitude: -0.1,
+ accuracy: 100.1
+ }
};
internalTests.configureGeolocationMockLocationProviderForTest(mockPosition);
function locationAvailable(position) {
- assertEqualAnyType(mockPosition.latitude, position.latitude);
- assertEqualAnyType(mockPosition.longitude, position.longitude);
- assertEqualAnyType(mockPosition.accuracy, position.accuracy);
+ delete position.timestamp;
+ assertObjectEqual(mockPosition, position);
internalTests.removeGeolocationMockLocationProvider();
completeAsync();
};
@@ -448,14 +475,19 @@
var mockPositionOne = {
latitude: -33.0,
longitude: 137.0,
- accuracy: 100.0
+ accuracy: 100.0,
+ coords: {
+ latitude: -33.0,
+ longitude: 137.0,
+ accuracy: 100.0
+ }
};
function setPositionOne() {
internalTests.configureGeolocationMockGpsDeviceForTest(mockPositionOne);
geolocation.getCurrentPosition(callbackOne, null, options);
}
function callbackOne(position) {
- position.timestamp = undefined;
+ delete position.timestamp;
assertObjectEqual(mockPositionOne, position);
setPositionTwo();
}
@@ -463,14 +495,19 @@
var mockPositionTwo = {
latitude: 45.0,
longitude: -100.0,
- accuracy: 100.0
+ accuracy: 100.0,
+ coords: {
+ latitude: 45.0,
+ longitude: -100.0,
+ accuracy: 100.0
+ }
};
function setPositionTwo() {
internalTests.configureGeolocationMockGpsDeviceForTest(mockPositionTwo);
geolocation.getCurrentPosition(callbackTwo, null, options);
}
function callbackTwo(position) {
- position.timestamp = undefined;
+ delete position.timestamp;
assertObjectEqual(mockPositionTwo, position);
completeAsync();
}
@@ -505,18 +542,21 @@
gearsLocationProviderUrls: [reverseGeocoder]
};
+ // The address is hard-coded in reverse_geocoder.py.
var mockGpsPosition = {
latitude: 0.1,
longitude: 2.3,
altitude: 4,
accuracy: 5,
- altitudeAccuracy: 6
- };
-
- function successCallback(position) {
- var correctPosition = mockGpsPosition;
- // This is hard-coded in reverse_geocoder.py.
- correctPosition.gearsAddress = {
+ altitudeAccuracy: 6,
+ coords: {
+ latitude: 0.1,
+ longitude: 2.3,
+ altitude: 4,
+ accuracy: 5,
+ altitudeAccuracy: 6
+ },
+ gearsAddress: {
streetNumber: '76',
street: 'Buckingham Palace Road',
postalCode: 'SW1W 9TQ',
@@ -525,9 +565,12 @@
region: 'London',
country: 'United Kingdom',
countryCode: 'uk'
- };
- position.timestamp = undefined;
- assertObjectEqual(correctPosition, position);
+ }
+ };
+
+ function successCallback(position) {
+ delete position.timestamp;
+ assertObjectEqual(mockGpsPosition, position);
completeAsync();
}
@@ -554,7 +597,12 @@
var mockPosition = {
latitude: 51.0,
longitude: -0.1,
- accuracy: 100.1
+ accuracy: 100.1,
+ coords: {
+ latitude: 51.0,
+ longitude: -0.1,
+ accuracy: 100.1
+ }
};
internalTests.configureGeolocationMockLocationProviderForTest(mockPosition);
function locationAvailable(position) {
@@ -581,12 +629,22 @@
var mockPosition1 = {
latitude: 51.0,
longitude: -0.1,
- accuracy: 100.1
+ accuracy: 100.1,
+ coords: {
+ latitude: 51.0,
+ longitude: -0.1,
+ accuracy: 100.1
+ }
};
var mockPosition2 = {
latitude: 52.0,
longitude: -1.1,
- accuracy: 101.1
+ accuracy: 101.1,
+ coords: {
+ latitude: 52.0,
+ longitude: -1.1,
+ accuracy: 101.1
+ }
};
var state = 0;
function successCallback(position) {
@@ -683,7 +741,12 @@
var positionToCache = {
latitude: 51.0,
longitude: -0.1,
- accuracy: 1.0
+ accuracy: 1.0,
+ coords: {
+ latitude: 51.0,
+ longitude: -0.1,
+ accuracy: 1.0
+ }
};
// Helper function for testCachedPositionXXX tests
