Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9151059
or point your web browser to
http://mondrian/9151059
to review the following code:
Change 9151059 by [EMAIL PROTECTED] on 2008/11/25 12:19:15 *pending*
Adds Geolocation error code constants to JavaScript PositionError
object.
R=andreip
[EMAIL PROTECTED]
DELTA=92 (25 added, 29 deleted, 38 changed)
OCL=9151059
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#64
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#30
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js#4
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#11
edit
92 delta lines: 25 added, 29 deleted, 38 changed
Also consider running:
g4 lint -c 9151059
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 9151059 by [EMAIL PROTECTED] on 2008/11/25 12:19:15 *pending*
Adds Geolocation error code constants to JavaScript PositionError
object.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#64
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#30
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js#4
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#11
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#64
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-25 12:19:11.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-25 12:24:43.000000000 +0000
@@ -1192,19 +1192,28 @@
// static
bool GearsGeolocation::CreateJavaScriptPositionErrorObject(
const Position &position,
- JsObject *error_object) {
- assert(error_object);
+ JsObject *error) {
+ assert(error);
assert(position.IsInitialized());
assert(!position.IsGoodFix());
bool result = true;
+ // We provide the values of all W3C error constants.
+ result &= error->SetPropertyInt(STRING16(L"UNKNOWN_ERROR"),
+ Position::ERROR_CODE_UNKNOWN_ERROR);
+ result &= error->SetPropertyInt(STRING16(L"PERMISSION_DENIED"),
+ Position::ERROR_CODE_PERMISSION_DENIED);
+ result &= error->SetPropertyInt(STRING16(L"POSITION_UNAVAILABLE"),
+ Position::ERROR_CODE_POSITION_UNAVAILABLE);
+ result &= error->SetPropertyInt(STRING16(L"TIMEOUT"),
+ Position::ERROR_CODE_TIMEOUT);
// error_code should always be valid.
- result &= error_object->SetPropertyInt(STRING16(L"code"),
- position.error_code);
+ result &= error->SetPropertyInt(STRING16(L"code"),
+ position.error_code);
// Other properties may not be valid.
result &= SetObjectPropertyIfValidString(STRING16(L"message"),
position.error_message,
- error_object);
+ error);
return result;
}
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#30 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/geolocation/geolocation.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-25 12:19:11.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-25 12:24:38.000000000 +0000
@@ -81,13 +81,15 @@
struct Position {
public:
// Error codes for returning to JavaScript. These values are defined by the
- // W3C spec. The following codes are not used by Gears.
- //
- // UNKNOWN_ERROR = 0
- // PERMISSION_DENIED = 1 - Geolocation methods throw an exception if
- // permission has not been granted.
+ // W3C spec. Note that Gears does not use all of these codes, but we need
+ // values for all of them to allow us to provide the constants on the error
+ // object.
enum ErrorCode {
- ERROR_CODE_NONE = -1, // Gears addition
+ ERROR_CODE_NONE = -1, // Gears addition
+ ERROR_CODE_UNKNOWN_ERROR = 0, // Not used by Gears
+ ERROR_CODE_PERMISSION_DENIED = 1, // Not used by Gears - Geolocation
+ // methods throw an exception if
+ // permission has not been granted.
ERROR_CODE_POSITION_UNAVAILABLE = 2,
ERROR_CODE_TIMEOUT = 3,
};
@@ -315,9 +317,9 @@
static bool CreateJavaScriptPositionObject(const Position &position,
bool use_address,
JsRunnerInterface *js_runner,
- JsObject *js_object);
+ JsObject *position_object);
static bool CreateJavaScriptPositionErrorObject(const Position &position,
- JsObject *js_object);
+ JsObject *error);
// Gets the fix request for a given ID. The supplied ID must be valid.
FixRequestInfo *GetFixRequest(int id);
====
//depot/googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js#4
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js
====
# action=edit type=text
--- googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js
2008-11-25 13:01:27.000000000 +0000
+++ googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js
2008-11-25 12:42:37.000000000 +0000
@@ -22,9 +22,6 @@
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
// OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
// ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-// Error code values. These values must match those in geolocation.h.
-var ERROR_CODE_TIMEOUT = 3;
var geolocation = google.gears.factory.create('beta.geolocation');
var dummyFunction = function() {};
@@ -136,10 +133,8 @@
function testZeroTimeout() {
// A request with a zero timeout should call the error callback immediately.
function errorCallback(error) {
- assertEqual(
- ERROR_CODE_TIMEOUT,
- error.code,
- 'Error callback should be called with code ERROR_CODE_TIMEOUT (3).');
+ assertEqual(error.TIMEOUT, error.code,
+ 'Error callback should be called with code TIMEOUT.');
completeAsync();
}
startAsync();
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#11
-
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-11-25 12:35:17.000000000 +0000
+++
googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js
2008-11-25 13:00:41.000000000 +0000
@@ -22,9 +22,6 @@
// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
// OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
// ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-// Error code values. These values must match those in geolocation.h.
-var ERROR_CODE_POSITION_UNAVAILABLE = 2;
if (isUsingCCTests) {
var internalTests = google.gears.factory.create('beta.test');
@@ -136,6 +133,18 @@
// location provider.
function testGetLocationFromResponse() {
if (isUsingCCTests) {
+ function assertErrorEqual(expectedCode, expectedMessage, error) {
+ assertEqual(0, error.UNKNOWN_ERROR, 'UKNOWN_ERROR has incorrect value.');
+ assertEqual(1, error.PERMISSION_DENIED,
+ 'PERMISSION_DENIED has incorrect value.');
+ assertEqual(2, error.POSITION_UNAVAILABLE,
+ 'POSITION_UNAVAILABLE has incorrect value.');
+ assertEqual(3, error.TIMEOUT, 'TIMEOUT has incorrect value.');
+ assertEqual(expectedCode, error.code, 'Error code has incorrect value.');
+ assertEqual(expectedMessage, error.message,
+ 'Error message has incorrect value.');
+ }
+
var dummy_server = 'http://test.server.com';
var malformedResponseError = 'Response from network provider at ' +
dummy_server +
@@ -145,9 +154,7 @@
' did not provide a good position fix.'
var position;
var correctPosition;
-
- // Error code values. These values must match those in geolocation.h.
- var ERROR_CODE_POSITION_UNAVAILABLE = 2;
+ var error;
// Test good response with valid position.
var responseBody = '{ ' +
@@ -227,92 +234,75 @@
assertObjectEqual(correctPosition, position);
// Test no response.
- position = internalTests.testGeolocationGetLocationFromResponse(
+ error = internalTests.testGeolocationGetLocationFromResponse(
false, // HttpPost result
0, // status code
'', // response body
0, // timestamp
dummy_server);
- correctPosition = new Object();
- correctPosition.code = ERROR_CODE_POSITION_UNAVAILABLE;
- correctPosition.message = 'No response from network provider at ' +
- dummy_server +
- '.';
- assertObjectEqual(correctPosition, position);
+ assertErrorEqual(error.POSITION_UNAVAILABLE,
+ 'No response from network provider at ' +
+ dummy_server +
+ '.',
+ error);
// Test bad response.
- position = internalTests.testGeolocationGetLocationFromResponse(
+ error = internalTests.testGeolocationGetLocationFromResponse(
true, // HttpPost result
400, // status code
'', // response body
0, // timestamp
dummy_server);
- correctPosition = new Object();
- correctPosition.code = ERROR_CODE_POSITION_UNAVAILABLE;
- correctPosition.message = 'Network provider at ' +
- dummy_server +
- ' returned error code 400.';
- assertObjectEqual(correctPosition, position);
+ assertErrorEqual(error.POSITION_UNAVAILABLE,
+ 'Network provider at ' +
+ dummy_server +
+ ' returned error code 400.',
+ error);
// Test good response with malformed body.
- position = internalTests.testGeolocationGetLocationFromResponse(
+ error = internalTests.testGeolocationGetLocationFromResponse(
true, // HttpPost result
200, // status code
'malformed response body',
0, // timestamp
dummy_server);
- correctPosition = new Object();
- correctPosition.code = ERROR_CODE_POSITION_UNAVAILABLE;
- correctPosition.message = malformedResponseError;
- assertObjectEqual(correctPosition, position);
+ assertErrorEqual(error.POSITION_UNAVAILABLE, malformedResponseError,
error);
// Test good response with empty body.
- position = internalTests.testGeolocationGetLocationFromResponse(
+ error = internalTests.testGeolocationGetLocationFromResponse(
true, // HttpPost result
200, // status code
'', // response body
0, // timestamp
dummy_server);
- correctPosition = new Object();
- correctPosition.code = ERROR_CODE_POSITION_UNAVAILABLE;
- correctPosition.message = malformedResponseError;
- assertObjectEqual(correctPosition, position);
+ assertErrorEqual(error.POSITION_UNAVAILABLE, malformedResponseError,
error);
// Test good response where body is not an object.
- position = internalTests.testGeolocationGetLocationFromResponse(
+ error = internalTests.testGeolocationGetLocationFromResponse(
true, // HttpPost result
200, // status code
'"a string"',
0, // timestamp
dummy_server);
- correctPosition = new Object();
- correctPosition.code = ERROR_CODE_POSITION_UNAVAILABLE;
- correctPosition.message = malformedResponseError;
- assertObjectEqual(correctPosition, position);
+ assertErrorEqual(error.POSITION_UNAVAILABLE, malformedResponseError,
error);
// Test good response with unknown position.
- position = internalTests.testGeolocationGetLocationFromResponse(
+ error = internalTests.testGeolocationGetLocationFromResponse(
true, // HttpPost result
200, // status code
'{}', // response body
0, // timestamp
dummy_server);
- correctPosition = new Object();
- correctPosition.code = ERROR_CODE_POSITION_UNAVAILABLE;
- correctPosition.message = noGoodFixError;
- assertObjectEqual(correctPosition, position);
+ assertErrorEqual(error.POSITION_UNAVAILABLE, noGoodFixError, error);
// Test good response with explicit unknown position.
- position = internalTests.testGeolocationGetLocationFromResponse(
+ error = internalTests.testGeolocationGetLocationFromResponse(
true, // HttpPost result
200, // status code
'{"position": null}',
0, // timestamp
dummy_server);
- correctPosition = new Object();
- correctPosition.code = ERROR_CODE_POSITION_UNAVAILABLE;
- correctPosition.message = noGoodFixError;
- assertObjectEqual(correctPosition, position);
+ assertErrorEqual(error.POSITION_UNAVAILABLE, noGoodFixError, error);
}
}
@@ -382,13 +372,13 @@
};
function noPositionErrorCallback(error) {
- assertEqual(ERROR_CODE_POSITION_UNAVAILABLE, error.code);
+ assertEqual(error.POSITION_UNAVAILABLE, error.code);
assert(error.message.search('did not provide a good position fix') > 0);
makeMalformedRequest();
};
function malformedRequestErrorCallback(error) {
- assertEqual(ERROR_CODE_POSITION_UNAVAILABLE, error.code);
+ assertEqual(error.POSITION_UNAVAILABLE, error.code);
assert(error.message.search('returned error code 400') > 0);
completeAsync();
};
