[gears-eng] a medium-size code review (8654410) Fixes bug in NetworkLocationProvider/Request

[Steve Block]

Hello andreip,

I'd like you to do a code review.  Please execute
        g4 diff -c 8654410

or point your web browser to
        http://mondrian/8654410

to review the following code:

Change 8654410 by [EMAIL PROTECTED] on 2008/10/20 12:15:07 *pending*

        Fixes bug in NetworkLocationProvider/Request where attempt to re-start 
AsyncTask can cause assertion failure.
        
        R=andreip
        [EMAIL PROTECTED]
        DELTA=37  (29 added, 2 deleted, 6 changed)
        OCL=8654410

Affected files ...

... 
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#30
 edit
... 
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.h#8
 edit

37 delta lines: 29 added, 2 deleted, 6 changed

Also consider running:
        g4 lint -c 8654410

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 8654410 by [EMAIL PROTECTED] on 2008/10/20 12:15:07 *pending*

        Fixes bug in NetworkLocationProvider/Request where attempt to re-start 
AsyncTask can cause assertion failure.

Affected files ...

... 
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#30
 edit
... 
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.h#8
 edit

==== 
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.cc#30
 - 
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/network_location_request.cc
 ====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/network_location_request.cc 
2008-10-20 13:47:47.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/network_location_request.cc 
2008-10-20 12:38:21.000000000 +0100
@@ -74,11 +74,20 @@
 static void AddWifiData(const WifiData &wifi_data, Json::Value *body_object);
 
 // static
-NetworkLocationRequest* NetworkLocationRequest::Create(
+NetworkLocationRequest *NetworkLocationRequest::Create(
     const std::string16 &url,
     const std::string16 &host_name,
     ListenerInterface *listener) {
-  return new NetworkLocationRequest(url, host_name, listener);
+  NetworkLocationRequest *request =
+      new NetworkLocationRequest(url, host_name, listener);
+  if (!request) {
+    return NULL;
+  }
+  if (!request->Init() || !request->Start()) {
+    delete request;
+    return NULL;
+  }
+  return request;
 }
 
 NetworkLocationRequest::NetworkLocationRequest(const std::string16 &url,
@@ -87,10 +96,8 @@
     : AsyncTask(NULL),
       listener_(listener),
       url_(url),
-      host_name_(host_name) {
-  if (!Init()) {
-    assert(false);
-  }
+      host_name_(host_name),
+      is_shutting_down_(false) {
 }
 
 bool NetworkLocationRequest::MakeRequest(const std::string16 &access_token,
@@ -110,12 +117,23 @@
     return false;
   }
   timestamp_ = timestamp;
-  // This will fail if there's a request currently in progress.
-  return Start();
+
+  thread_event_.Signal();
+  return true;
 }
 
 // AsyncTask implementation.
 void NetworkLocationRequest::Run() {
+  while (true) {
+    thread_event_.Wait();
+    if (is_shutting_down_) {
+      break;
+    }
+    MakeRequestImpl();
+  }
+}
+
+void NetworkLocationRequest::MakeRequestImpl() {
   WebCacheDB::PayloadInfo payload;
   // TODO(andreip): remove this once WebCacheDB::PayloadInfo.data is a Blob.
   scoped_refptr<BlobInterface> payload_data;
@@ -277,6 +295,8 @@
   // particular, we can't call Abort() then block here waiting for HttpPost to
   // return.
   is_processing_response_mutex_.Lock();
+  is_shutting_down_ = true;
+  thread_event_.Signal();
   Abort();
   is_processing_response_mutex_.Unlock();
   DeleteWhenDone();
==== 
//depot/googleclient/gears/opensource/gears/geolocation/network_location_request.h#8
 - 
c:\MyDocs\Gears1/googleclient/gears/opensource/gears/geolocation/network_location_request.h
 ====
# action=edit type=text
--- googleclient/gears/opensource/gears/geolocation/network_location_request.h  
2008-10-20 13:47:47.000000000 +0100
+++ googleclient/gears/opensource/gears/geolocation/network_location_request.h  
2008-10-20 12:36:20.000000000 +0100
@@ -51,6 +51,8 @@
         const std::string16 &access_token) = 0;
   };
 
+  // Creates the object and starts its worker thread running. Returns NULL if
+  // creation or initialisation fails.
   static NetworkLocationRequest* Create(const std::string16 &url,
                                         const std::string16 &host_name,
                                         ListenerInterface *listener);
@@ -76,6 +78,8 @@
                          const std::string16 &host_name,
                          ListenerInterface *listener);
   virtual ~NetworkLocationRequest() {}
+
+  void MakeRequestImpl();
 
   // AsyncTask implementation.
   virtual void Run();
@@ -110,6 +114,9 @@
 
   bool is_reverse_geocode_;
 
+  Event thread_event_;
+  bool is_shutting_down_;
+
 #ifdef USING_CCTESTS
   // Uses FormRequestBody for testing.
   friend void TestGeolocationFormRequestBody(JsCallContext *context);