Looks good, a few comments below: ======================================================================== http://mondrian.corp.google.com/file/8529424///depot/googleclient/gears/opensource/gears/geolocation/gps_location_provider.cc?a=2 File //depot/googleclient/gears/opensource/gears/geolocation/gps_location_provider.cc (snapshot 2) ------------------------------------ Line 83: LocationProviderBase::RegisterListener(listener, request_address); I'm confused: you call RegisterListener here, ------------------------------------ Line 102: LocationProviderBase::RegisterListener(listener, is_address_requested_); ...and then here, again. I guess the first call is the wrong one? ------------------------------------ Line 119: is_listener_update_needed_ = true; A small suggestion: I wonder if this decision needs to be made here. Rather, could we could just stuff the listener into a queue of temporary listeners. The queue would then be consumed up by the worker thread in its normal event loop. This way we would avoid locking 2 mutexes (and its classical deadlock danger) and all of the above logic. ------------------------------------ Line 179: if (is_listener_update_needed_) { Here, we'd just empty the queue and properly register each listener, also deciding if any other action is needed. ------------------------------------ Line 239: MutexLock position_lock(&position_mutex_); Similarly, maybe we can just record the new position here, signal the event, and let the worker do the checking. It would get around having to lock status_mutex_. ========================================================================
-- To respond, reply to this email or visit http://mondrian.corp.google.com/8529424
