Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 10228283
or point your web browser to
http://mondrian/10228283
to review the following code:
Change 10228283 by stevebl...@steveblock-gears1 on 2009/02/20 15:44:58 *pending*
Fixes flakiness in testCachedPositionXXX internal Geolocation tests.
R=andreip
[email protected]
DELTA=26 (11 added, 3 deleted, 12 changed)
OCL=10228283
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#72
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#17
edit
26 delta lines: 11 added, 3 deleted, 12 changed
Also consider running:
g4 lint -c 10228283
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 10228283 by stevebl...@steveblock-gears1 on 2009/02/20 15:44:58 *pending*
Fixes flakiness in testCachedPositionXXX internal Geolocation tests.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#72
edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#17
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#72
-
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-02-20 16:13:51.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2009-02-20 15:47:28.000000000 +0000
@@ -458,7 +458,7 @@
reinterpret_cast<const FixRequestIdNotificationData*>(data);
TimeoutExpiredImpl(timeout_expired_data->fix_request_id);
} else if (topic_string == kCallbackRequiredObserverTopic) {
- // The timeout for a callback expired.
+ // We need to make a callback.
const FixRequestIdNotificationData *callback_required_data =
reinterpret_cast<const FixRequestIdNotificationData*>(data);
CallbackRequiredImpl(callback_required_data->fix_request_id);
@@ -502,11 +502,9 @@
return true;
}
assert(maximum_age > 0);
- // maximum_age is limited to 2^31 milliseconds. The current (November 2008)
- // timestamp is around 2^40, so there's no danger of underflow.
- int64 oldest_timestamp = GetCurrentTimeMillis() - maximum_age;
- assert(oldest_timestamp > 0);
- return last_position_.timestamp > oldest_timestamp;
+ int64 age = GetCurrentTimeMillis() - last_position_.timestamp;
+ assert(age >= 0);
+ return age <= maximum_age;
}
void GearsGeolocation::AddProvidersToRequest(std::vector<std::string16> &urls,
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js#17
-
c:\MyDocs\Gears1/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-02-20 16:13:51.000000000 +0000
+++
googleclient/gears/opensource/gears/test/testcases/internal_geolocation_tests.js
2009-02-20 15:57:36.000000000 +0000
@@ -22,6 +22,9 @@
// 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.
+
+// Timing discrepancy between C++ side and JS side can be up to 20ms.
+var TIMING_DISCREPANCY = 20;
if (isUsingCCTests) {
var internalTests = google.gears.factory.create('beta.test');
@@ -711,7 +714,12 @@
internalTests.configureGeolocationMockLocationProviderForTest(
intermediatePositionToCache);
geolocation.getCurrentPosition(
- function(position) { SetPosition(); },
+ function() {
+ assert(geolocation.lastPosition.timestamp >=
+ startTime - TIMING_DISCREPANCY,
+ 'Last position too old in SetIntermediatePosition');
+ SetPosition();
+ },
null,
{gearsLocationProviderUrls: null});
}
@@ -719,10 +727,10 @@
internalTests.configureGeolocationMockLocationProviderForTest(
positionToCache);
geolocation.getCurrentPosition(
- function(position) {
- assert(geolocation.lastPosition.timestamp >= Date.parse(startTime),
+ function() {
+ var lastPosition = geolocation.lastPosition;
+ assert(lastPosition.timestamp >= startTime - TIMING_DISCREPANCY,
'Last position too old in PopulateCachedPosition.');
- var lastPosition = geolocation.lastPosition;
delete lastPosition.timestamp;
assertObjectEqual(
positionToCache, lastPosition,
@@ -733,7 +741,7 @@
null,
{gearsLocationProviderUrls: null});
}
- var startTime = Date();
+ var startTime = new Date();
SetIntermediatePosition();
}
@@ -786,7 +794,7 @@
// position.
startAsync();
PopulateCacheAndMakeRequest(
- 10, // Wait for longer than maximumAge.
+ 20, // Wait for longer than maximumAge.
function() {},
function(error) {
assertErrorEqual(error.POSITION_UNAVAILABLE,
@@ -802,11 +810,13 @@
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
- // cached position. We use a non-existant URL for the location provider to
- // force an error.
+ // 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
+ // location provider, not the lack of a cached position.
startAsync();
PopulateCacheAndMakeRequest(
- 10, // Wait for longer than maximumAge.
+ 20, // Wait for longer than maximumAge.
function() {},
function(error) {
assert(error.message.search('returned error code 404.') != -1);
