Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9087840
or point your web browser to
http://mondrian/9087840
to review the following code:
Change 9087840 by [EMAIL PROTECTED] on 2008/11/20 12:54:50 *pending*
When the timeout for a pending watch callback expires, makes sure that
the fix request still exists before making the callback.
R=andreip
[EMAIL PROTECTED]
DELTA=48 (14 added, 10 deleted, 24 changed)
OCL=9087840
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#62
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#28
edit
48 delta lines: 14 added, 10 deleted, 24 changed
Also consider running:
g4 lint -c 9087840
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 9087840 by [EMAIL PROTECTED] on 2008/11/20 12:54:50 *pending*
When the timeout for a pending watch callback expires, makes sure that
the fix request still exists before making the callback.
Affected files ...
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#62
edit
... //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#28
edit
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.cc#62
-
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/geolocation/geolocation.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-20 13:01:04.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.cc
2008-11-20 12:56:27.000000000 +0000
@@ -137,6 +137,7 @@
DISALLOW_EVIL_CONSTRUCTORS(NotificationDataGeoBase);
};
+// Used for messages of type kLocationAvailableObserverTopic.
class LocationAvailableNotificationData : public NotificationDataGeoBase {
public:
LocationAvailableNotificationData(GearsGeolocation *object_in,
@@ -153,19 +154,19 @@
DISALLOW_EVIL_CONSTRUCTORS(LocationAvailableNotificationData);
};
+// Used for messages of type kCallbackRequiredObserverTopic.
struct CallbackRequiredNotificationData : public NotificationDataGeoBase {
public:
- CallbackRequiredNotificationData(
- GearsGeolocation *object_in,
- GearsGeolocation::FixRequestInfo *fix_info_in)
+ CallbackRequiredNotificationData(GearsGeolocation *object_in,
+ int fix_request_id_in)
: NotificationDataGeoBase(object_in),
- fix_info(fix_info_in) {}
+ fix_request_id(fix_request_id_in) {}
virtual ~CallbackRequiredNotificationData() {}
friend class GearsGeolocation;
private:
- GearsGeolocation::FixRequestInfo *fix_info;
+ int fix_request_id;
DISALLOW_EVIL_CONSTRUCTORS(CallbackRequiredNotificationData);
};
@@ -399,25 +400,24 @@
return;
}
- if (char16_wmemcmp(kLocationAvailableObserverTopic,
- topic,
- char16_wcslen(topic)) == 0) {
+ std::string16 topic_string(topic);
+ if (topic_string == kLocationAvailableObserverTopic) {
+ // A location provider reported a new position.
const LocationAvailableNotificationData *location_available_data =
reinterpret_cast<const LocationAvailableNotificationData*>(data);
-
- // Invoke the implementation.
LocationUpdateAvailableImpl(location_available_data->provider);
- } else if (char16_wmemcmp(kCallbackRequiredObserverTopic,
- topic,
- char16_wcslen(topic)) == 0) {
+ } else if (topic_string == kCallbackRequiredObserverTopic) {
+ // The timeout for a watch callback expired.
const CallbackRequiredNotificationData *callback_required_data =
reinterpret_cast<const CallbackRequiredNotificationData*>(data);
-
- // Delete this timer.
- FixRequestInfo *fix_info = callback_required_data->fix_info;
- assert(fix_info->success_callback_timer.get());
- fix_info->success_callback_timer.reset();
- MakeSuccessCallback(fix_info, fix_info->pending_position);
+ // Check that the fix request still exists
+ FixRequestInfo *fix_info =
+ MaybeGetFixRequest(callback_required_data->fix_request_id);
+ if (fix_info) {
+ assert(fix_info->success_callback_timer.get());
+ fix_info->success_callback_timer.reset();
+ MakeSuccessCallback(fix_info, fix_info->pending_position);
+ }
}
}
@@ -561,11 +561,10 @@
bool GearsGeolocation::CancelWatch(const int &watch_id) {
ASSERT_SINGLE_THREAD();
- FixRequestInfoMap::iterator watch_iter = fix_requests_.find(watch_id);
- if (watch_iter == fix_requests_.end()) {
+ FixRequestInfo *info = MaybeGetFixRequest(watch_id);
+ if (info == NULL) {
return false;
}
- FixRequestInfo *info = watch_iter->second;
if (!info->repeats) {
return false;
}
@@ -618,9 +617,7 @@
} else {
// Start an asynchronous timer which will post a message back to this
// thread once the minimum time period has elapsed.
- MakeFutureSuccessCallback(static_cast<int>(time_remaining),
- fix_info,
- position);
+ MakeFutureSuccessCallback(static_cast<int>(time_remaining), id,
position);
}
}
}
@@ -720,8 +717,7 @@
int id = ids.back();
ids.pop_back();
if (id < 0) {
- FixRequestInfoMap::const_iterator iter = fix_requests_.find(id);
- if (iter != fix_requests_.end()) {
+ if (MaybeGetFixRequest(id)) {
HandleSingleRequestUpdate(provider, id, position);
}
}
@@ -745,8 +741,7 @@
while (!watch_ids.empty()) {
int watch_id = watch_ids.back();
watch_ids.pop_back();
- FixRequestInfoMap::const_iterator iter = fix_requests_.find(watch_id);
- if (iter != fix_requests_.end()) {
+ if (MaybeGetFixRequest(watch_id)) {
HandleRepeatingRequestUpdate(watch_id, position);
}
}
@@ -1045,9 +1040,14 @@
}
GearsGeolocation::FixRequestInfo *GearsGeolocation::GetFixRequest(int id) {
+ FixRequestInfo *fix_info = MaybeGetFixRequest(id);
+ assert(fix_info);
+ return fix_info;
+}
+
+GearsGeolocation::FixRequestInfo *GearsGeolocation::MaybeGetFixRequest(int id)
{
FixRequestInfoMap::const_iterator iter = fix_requests_.find(id);
- assert(iter != fix_requests_.end());
- return iter->second;
+ return iter == fix_requests_.end() ? NULL : iter->second;
}
bool GearsGeolocation::RecordNewFixRequest(FixRequestInfo *fix_request) {
@@ -1184,9 +1184,11 @@
}
void GearsGeolocation::MakeFutureSuccessCallback(int timeout_milliseconds,
- FixRequestInfo *fix_info,
+ int fix_request_id,
const Position &position) {
ASSERT_SINGLE_THREAD();
+
+ FixRequestInfo *fix_info = GetFixRequest(fix_request_id);
// Check that there isn't already a timer running for this request.
assert(!fix_info->success_callback_timer.get());
@@ -1194,7 +1196,7 @@
fix_info->success_callback_timer.reset(new TimedMessage(
timeout_milliseconds,
kCallbackRequiredObserverTopic,
- new CallbackRequiredNotificationData(this, fix_info)));
+ new CallbackRequiredNotificationData(this, fix_request_id)));
}
// Local functions
==== //depot/googleclient/gears/opensource/gears/geolocation/geolocation.h#28 -
c:\MyDocs\Gears4/googleclient/gears/opensource/gears/geolocation/geolocation.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-20 13:01:04.000000000 +0000
+++ googleclient/gears/opensource/gears/geolocation/geolocation.h
2008-11-20 12:53:00.000000000 +0000
@@ -308,6 +308,8 @@
// Gets the fix request for a given ID. The supplied ID must be valid.
FixRequestInfo *GetFixRequest(int id);
+ // Gets the fix request for a given ID if valid.
+ FixRequestInfo *MaybeGetFixRequest(int id);
// Takes a pointer to a new fix request and records it in our map. Returns
// false if the maximum number of fix requests has been reached. Otherwise
@@ -328,7 +330,7 @@
// Causes a callback to JavaScript to be made at the specified number of
// milliseconds in the future.
void MakeFutureSuccessCallback(int timeout_milliseconds,
- FixRequestInfo *fix_info,
+ int fix_request_id,
const Position &position);
// TODO(steveblock): Refactor the logic used to maintain the fix request maps
