Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 10662153
or point your web browser to
http://mondrian/10662153
to review the following code:
Change 10662153 by stevebl...@steveblock-gears3 on 2009/03/31 16:34:45 *pending*
Fixes Geolocation testZeroTimeout, which is flaky on Chrome in workers.
This is due to a race condition between callbacks due to a zero
timeout, and callbacks from location providers.
Also adds some additional tests.
R=andreip
[email protected]
DELTA=171 (81 added, 17 deleted, 73 changed)
OCL=10662153
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#74
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#32
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js#7
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#19
edit
171 delta lines: 81 added, 17 deleted, 73 changed
Also consider running:
g4 lint -c 10662153
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 10662153 by stevebl...@steveblock-gears3 on 2009/03/31 16:34:45 *pending*
Fixes Geolocation testZeroTimeout, which is flaky on Chrome in workers.
This is due to a race condition between callbacks due to a zero
timeout, and callbacks from location providers.
Also adds some additional tests.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#74
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#32
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js#7
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#19
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#74
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-03-31 16:57:37.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-03-31 16:29:05.000000000 +0100
@@ -493,8 +493,10 @@
bool GearsGeolocation::IsCachedPositionSuitable(int maximum_age) {
ASSERT_SINGLE_THREAD();
- assert(maximum_age != 0);
-
+
+ if(maximum_age == 0) {
+ return false;
+ }
if (!last_position_.IsGoodFix()) {
return false;
}
@@ -609,62 +611,47 @@
}
info->repeats = repeats;
- // This is purely an optimisation.
- bool is_useful_to_add_location_providers = true;
- // If the fix request is a one-shot request and will call back immediately,
- // there's no point in adding any providers.
- if (!info->repeats &&
- info->maximum_age != 0 &&
- IsCachedPositionSuitable(info->maximum_age)) {
- is_useful_to_add_location_providers = false;
- }
-
- // Add the location providers.
- if (is_useful_to_add_location_providers) {
- AddProvidersToRequest(urls, info.get());
-
- // If this is a non-repeating request, hint to all providers that new data
- // is required ASAP.
- if (!info->repeats) {
- for (ProviderVector::iterator iter = info->providers.begin();
- iter != info->providers.end();
- ++iter) {
- (*iter)->UpdatePosition();
- }
- }
- }
-
- // Check whether we need to call back immediately.
- bool should_call_back_immediately = false;
+ // Add the location providers. Note that we can't determine whether any
+ // providers were specified in the request without actually adding them to
the
+ // request, and hence starting them. This is because ...
+ // - Network provider URLs my be invalid
+ // - The platform may not support GPS
+ // - The mock provided may not be enabled
+ AddProvidersToRequest(urls, info.get());
+
+ // If this is a non-repeating request, hint to all providers that new data
+ // is required ASAP.
+ if (!info->repeats) {
+ for (ProviderVector::iterator iter = info->providers.begin();
+ iter != info->providers.end();
+ ++iter) {
+ (*iter)->UpdatePosition();
+ }
+ }
+
+ // If this fix has maximumAge zero and no providers, throw an exception and
+ // quit.
+ if (info->maximum_age == 0 && info->providers.empty()) {
+ context->SetException(STRING16(L"Fix request has no location providers."));
+ return;
+ }
+
+ // See whether we need to call back immediately with a cached position.
Position position_to_call_back;
- if (info->maximum_age == 0) {
- // If this fix has no providers, throw an exception and quit.
- if (info->providers.empty()) {
- context->SetException(
- STRING16(L"Fix request has no location providers."));
- return;
- }
- } else {
- // Non-zero maximumAge
- if (info->providers.empty()) {
- should_call_back_immediately = true;
- if (IsCachedPositionSuitable(info->maximum_age)) {
- position_to_call_back = last_position_;
- } else {
- position_to_call_back.error_code =
- Position::ERROR_CODE_POSITION_UNAVAILABLE;
- position_to_call_back.error_message =
- STRING16(L"No suitable cached position available.");
- }
- } else {
- if (IsCachedPositionSuitable(info->maximum_age)) {
- should_call_back_immediately = true;
- position_to_call_back = last_position_;
- }
- }
- }
- assert(is_useful_to_add_location_providers || should_call_back_immediately);
-
+ if (IsCachedPositionSuitable(info->maximum_age)) {
+ position_to_call_back = last_position_;
+ }
+
+ // See whether we need to call back immediately with a cached position error.
+ if (info->providers.empty() &&
+ info->timeout != 0 &&
+ !IsCachedPositionSuitable(info->maximum_age)) {
+ position_to_call_back.error_code =
+ Position::ERROR_CODE_POSITION_UNAVAILABLE;
+ position_to_call_back.error_message =
+ STRING16(L"No suitable cached position available.");
+ }
+
// Record this fix. This updates the map of providers for this fix request
// and provides a fix request ID. The map takes ownership of the info
// structure on success.
@@ -676,9 +663,17 @@
}
assert(fix_request_id != 0);
- // Invoke the callback. We use a message to do so asynchronously.
- if (should_call_back_immediately) {
- assert(position_to_call_back.IsInitialized());
+ // A timeout of zero is a special case which means that we never use the
+ // providers to obtain a position fix. In this case, we can and must cancel
+ // them.
+ if (info->timeout == 0) {
+ RemoveAllProviders(fix_request_id);
+ }
+
+ // Invoke the callback for the cached position or error. We use a message to
+ // do so asynchronously. This must be done before we start the timeout timer.
+ // Note that the callback handler cancels any active providers.
+ if (position_to_call_back.IsInitialized()) {
assert(!info->success_callback_timer.get());
info->pending_position = position_to_call_back;
@@ -688,10 +683,9 @@
new FixRequestIdNotificationData(this, fix_request_id)));
}
- // If we have providers, start the timeout timer for this fix request.
- if (!info->providers.empty()) {
- StartTimeoutTimer(fix_request_id);
- }
+ // Start the timeout timer for this fix request. Note that this handles all
+ // timeouts, include immediate timeout callbacks when timeout is zero.
+ StartTimeoutTimer(fix_request_id);
// Set the return value if this fix repeats.
if (info->repeats) {
@@ -1480,16 +1474,22 @@
void GearsGeolocation::RemoveFixRequest(int fix_request_id) {
ASSERT_SINGLE_THREAD();
+ RemoveAllProviders(fix_request_id);
+ fix_requests_.erase(fix_request_id);
+}
+
+void GearsGeolocation::RemoveAllProviders(int fix_request_id) {
+ ASSERT_SINGLE_THREAD();
+
FixRequestInfo *fix_request = GetFixRequest(fix_request_id);
- fix_requests_.erase(fix_request_id);
// For each location provider used by this request, update the provider's
// list of fix requests in the map.
ProviderVector *member_providers = &fix_request->providers;
- for (ProviderVector::iterator iter = member_providers->begin();
- iter != member_providers->end();
- ++iter) {
- LocationProviderBase *provider = *iter;
+ while (!member_providers->empty()) {
+ LocationProviderBase *provider = member_providers->back();
+ member_providers->pop_back();
+ assert(provider);
// Check that we have an entry in the map for this provider.
ProviderMap::iterator provider_iter = providers_.find(provider);
@@ -1500,7 +1500,7 @@
IdList::iterator id_iterator =
std::find(ids->begin(), ids->end(), fix_request_id);
- // If we can't find this request the list, something has gone wrong.
+ // If we can't find this request in the list, something has gone wrong.
assert(id_iterator != ids->end());
// Remove this fix request from the list of fix requests for this provider.
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#32 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/geolocation/geolocation.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.h
2009-03-31 16:57:37.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/geolocation.h
2009-03-31 15:59:05.000000000 +0100
@@ -346,6 +346,10 @@
// object.
void RemoveFixRequest(int fix_request_id);
+ // Removes all providers from a fix request. Cancels any pending requests to
+ // the location providers.
+ void RemoveAllProviders(int fix_request_id);
+
// Deletes a fix request and decrements our ref count.
void DeleteFixRequest(FixRequestInfo *fix_request);
void RemoveAndDeleteFixRequest(int fix_request_id);
====
//depot/googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js#7
-
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
2009-03-31 16:34:41.000000000 +0100
+++ googleclient/gears/opensource/gears/test/testcases/geolocation_tests.js
2009-03-31 16:46:03.000000000 +0100
@@ -147,8 +147,9 @@
}, gearsLocationProviderUrlsTypeError);
}
-function testNoProviders() {
- // A request with no providers should assert unless maximumAge is non-zero.
+function testNoProvidersZeroTimout() {
+ // A request with no providers should throw an exception if maximumAge is
+ // zero.
assertError(
function() {
geolocation.getCurrentPosition(
@@ -157,23 +158,42 @@
{gearsLocationProviderUrls: []});
},
'Fix request has no location providers.',
- 'Calling getCurrentPosition() should fail if no location providers are '
+
- 'specified unless maximumAge is non-zero.');
+ 'Calling getCurrentPosition() should throw an exception if no location '
+
+ 'providers are specified when maximumAge is zero.');
geolocation.getCurrentPosition(
dummyFunction,
dummyFunction,
{maximumAge: 1, gearsLocationProviderUrls: []});
}
-// TODO(steveblock): Fix and uncomment this test.
-// Test is flakey when run in worker after CL 10410384.
-//function testZeroTimeout() {
-// // A request with a zero timeout should call the error callback
immediately.
-// function errorCallback(error) {
-// assertEqual(error.TIMEOUT, error.code,
-// 'Error callback should be called with code TIMEOUT.');
-// completeAsync();
-// }
-// startAsync();
-// geolocation.getCurrentPosition(dummyFunction, errorCallback, {timeout: 0});
-//}
+function testNoProvidersZeroTimout() {
+ // A request with no providers should throw an exception if maximumAge is
+ // zero, even when timeout is zero.
+ assertError(
+ function() {
+ geolocation.getCurrentPosition(
+ dummyFunction,
+ dummyFunction,
+ {gearsLocationProviderUrls: [], timeout: 0});
+ },
+ 'Fix request has no location providers.',
+ 'Calling getCurrentPosition() should throw an exception if no location '
+
+ 'providers are specified when maximumAge is zero and timeout is zero.');
+ geolocation.getCurrentPosition(
+ dummyFunction,
+ dummyFunction,
+ {maximumAge: 1, gearsLocationProviderUrls: [], timeout: 0});
+}
+
+function testZeroTimeout() {
+ // A request with a zero timeout should call the error callback immediately
+ // when maximumAge is zero.
+ function errorCallback(error) {
+ assertEqual(error.TIMEOUT, error.code,
+ 'Error callback should be called with code TIMEOUT.');
+ completeAsync();
+ }
+ startAsync();
+ geolocation.getCurrentPosition(dummyFunction, errorCallback, {timeout: 0});
+}
+
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#19
-
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
2009-03-31 16:34:41.000000000 +0100
+++
googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js
2009-03-31 16:34:49.000000000 +0100
@@ -43,8 +43,8 @@
'Error message has incorrect value.');
}
-// Tests the parsing of the options passed to Geolocation.GetCurrentPosition
and
-// Geolocation.WatchPosition.
+// Tests the parsing of the options passed to Geolocation.getCurrentPosition
and
+// Geolocation.watchPosition.
function testParseOptions() {
if (isUsingCCTests) {
var dummyFunction = function() {};
@@ -772,7 +772,7 @@
function testCachedPositionWithinMaximumAge() {
if (isUsingCCTests) {
// Test that a request with a non-zero maximumAge immediately calls the
- // success callback with the cached position if we have a suitable one.
+ // success callback with the cached position when we have a suitable one.
startAsync();
PopulateCacheAndMakeRequest(
0, // Wait for less than maximumAge.
@@ -790,7 +790,7 @@
function testCachedPositionOutsideMaximumAgeNoProviders() {
if (isUsingCCTests) {
// Test that a request with a non-zero maximumAge and no location providers
- // immediately calls the error callback if we don't have a suitable cached
+ // immediately calls the error callback when we don't have a suitable
cached
// position.
startAsync();
PopulateCacheAndMakeRequest(
@@ -809,7 +809,7 @@
function testCachedPositionOutsideMaximumAge() {
if (isUsingCCTests) {
// Test that a request with a non-zero maximumAge and location providers
- // does not immediately call the error callback if we don't have a suitable
+ // does not immediately call the error callback when we don't have a
suitable
// cached position.
// We use a non-existant URL for the network location provider to force an
// error and check that the error is due to the failure of the network
@@ -827,3 +827,43 @@
{maximumAge: 1, gearsLocationProviderUrls: ['non_existant_url']});
}
}
+
+function testCachedPositionWithinMaximumAgeZeroTimeout() {
+ if (isUsingCCTests) {
+ // Test that a request with a non-zero maximumAge immediately calls the
+ // success callback with the cached position when we have a suitable one,
even
+ // when timeout is zero.
+ startAsync();
+ PopulateCacheAndMakeRequest(
+ 0, // Wait for less than maximumAge.
+ function(position) {
+ delete position.timestamp;
+ assertObjectEqual(positionToCache, position,
+ 'Position incorrect in TestMaximumAge.');
+ completeAsync();
+ },
+ null,
+ {maximumAge: 1000, timeout: 0});
+ }
+}
+
+function testCachedPositionOutsideMaximumAgeZeroTimeout() {
+ if (isUsingCCTests) {
+ // Test that a request with a non-zero maximumAge and zero timeout
+ // immediately calls the error callback with a timeout error when we don't
+ // have a suitable cached position.
+ startAsync();
+ PopulateCacheAndMakeRequest(
+ 20, // Wait for longer than maximumAge.
+ function() {},
+ function(error) {
+ assertErrorEqual(error.TIMEOUT,
+ 'A position fix was not obtained within the ' +
+ 'specified time limit.',
+ error);
+ completeAsync();
+ },
+ {maximumAge: 1, timeout: 0});
+ }
+}
+
