Clean up WebResourceService:

* Use base::Bind instead of NewRunnableMethod.
* Inline WebResourceFetcher into WebResourceService, as they have the same lifetime.
* Move stuff specific to PromoResourceService into that class.


BUG=none
TEST=none

Review URL: http://codereview.chromium.org/8905022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@114306 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 92 insertions, 158 deletions

diff --git a/chrome/browser/web_resource/web_resource_service.cc b/chrome/browser/web_resource/web_resource_service.cc
index aa44b95..712142e 100644
--- a/chrome/browser/web_resource/web_resource_service.cc
+++ b/chrome/browser/web_resource/web_resource_service.cc
@@ -4,122 +4,39 @@
 
 #include "chrome/browser/web_resource/web_resource_service.h"
 
+#include "base/bind.h"
 #include "base/command_line.h"
-#include "base/file_path.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prefs/pref_service.h"
-#include "chrome/browser/sync/sync_ui_util.h"
-#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_utility_messages.h"
-#include "chrome/common/extensions/extension.h"
 #include "chrome/common/web_resource/web_resource_unpacker.h"
+#include "content/browser/utility_process_host.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/common/url_fetcher.h"
-#include "content/public/common/url_fetcher_delegate.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_request_status.h"
 
 using content::BrowserThread;
 
-class WebResourceService::WebResourceFetcher
-    : public content::URLFetcherDelegate {
- public:
-  explicit WebResourceFetcher(WebResourceService* web_resource_service) :
-      ALLOW_THIS_IN_INITIALIZER_LIST(fetcher_factory_(this)),
-      web_resource_service_(web_resource_service) {
-  }
-
-  // Delay initial load of resource data into cache so as not to interfere
-  // with startup time.
-  void StartAfterDelay(int64 delay_ms) {
-    MessageLoop::current()->PostDelayedTask(FROM_HERE,
-        fetcher_factory_.NewRunnableMethod(&WebResourceFetcher::StartFetch),
-                                           delay_ms);
-  }
-
-  // Initializes the fetching of data from the resource server.  Data
-  // load calls OnURLFetchComplete.
-  void StartFetch() {
-    // Balanced in OnURLFetchComplete.
-    web_resource_service_->AddRef();
-    // First, put our next cache load on the MessageLoop.
-    MessageLoop::current()->PostDelayedTask(FROM_HERE,
-        fetcher_factory_.NewRunnableMethod(&WebResourceFetcher::StartFetch),
-            web_resource_service_->cache_update_delay_);
-    // If we are still fetching data, exit.
-    if (web_resource_service_->in_fetch_)
-      return;
-    else
-      web_resource_service_->in_fetch_ = true;
-
-    std::string web_resource_server =
-        web_resource_service_->web_resource_server_;
-    if (web_resource_service_->apply_locale_to_url_) {
-      std::string locale = g_browser_process->GetApplicationLocale();
-      web_resource_server.append(locale);
-    }
-
-    url_fetcher_.reset(content::URLFetcher::Create(
-        GURL(web_resource_server), content::URLFetcher::GET, this));
-    // Do not let url fetcher affect existing state in system context (by
-    // setting cookies, for example).
-    url_fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE |
-        net::LOAD_DO_NOT_SAVE_COOKIES);
-    net::URLRequestContextGetter* url_request_context_getter =
-        g_browser_process->system_request_context();
-    url_fetcher_->SetRequestContext(url_request_context_getter);
-    url_fetcher_->Start();
-  }
-
-  // From content::URLFetcherDelegate.
-  void OnURLFetchComplete(const content::URLFetcher* source) {
-    // Delete the URLFetcher when this function exits.
-    scoped_ptr<content::URLFetcher> clean_up_fetcher(url_fetcher_.release());
-
-    // Don't parse data if attempt to download was unsuccessful.
-    // Stop loading new web resource data, and silently exit.
-    if (!source->GetStatus().is_success() || (source->GetResponseCode() != 200))
-      return;
-
-    std::string data;
-    source->GetResponseAsString(&data);
-    web_resource_service_->UpdateResourceCache(data);
-    web_resource_service_->Release();
-  }
-
- private:
-  // So that we can delay our start so as not to affect start-up time; also,
-  // so that we can schedule future cache updates.
-  ScopedRunnableMethodFactory<WebResourceFetcher> fetcher_factory_;
-
-  // The tool that fetches the url data from the server.
-  scoped_ptr<content::URLFetcher> url_fetcher_;
-
-  // Our owner and creator. Ref counted.
-  WebResourceService* web_resource_service_;
-};
-
 // This class coordinates a web resource unpack and parse task which is run in
 // a separate process.  Results are sent back to this class and routed to
 // the WebResourceService.
 class WebResourceService::UnpackerClient : public UtilityProcessHost::Client {
  public:
-  UnpackerClient(WebResourceService* web_resource_service,
-                 const std::string& json_data)
+  explicit UnpackerClient(WebResourceService* web_resource_service)
     : web_resource_service_(web_resource_service),
-      json_data_(json_data), got_response_(false) {
+      resource_dispatcher_host_(g_browser_process->resource_dispatcher_host()),
+      got_response_(false) {
   }
 
-  void Start() {
+  void Start(const std::string& json_data) {
     AddRef();  // balanced in Cleanup.
 
     // TODO(willchan): Look for a better signal of whether we're in a unit test
@@ -127,17 +44,17 @@ class WebResourceService::UnpackerClient : public UtilityProcessHost::Client {
     // If we don't have a resource_dispatcher_host_, assume we're in
     // a test and run the unpacker directly in-process.
     bool use_utility_process =
-        web_resource_service_->resource_dispatcher_host_ != NULL &&
+        resource_dispatcher_host_ != NULL &&
         !CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess);
     if (use_utility_process) {
       BrowserThread::ID thread_id;
       CHECK(BrowserThread::GetCurrentThreadIdentifier(&thread_id));
       BrowserThread::PostTask(
           BrowserThread::IO, FROM_HERE,
-          NewRunnableMethod(this, &UnpackerClient::StartProcessOnIOThread,
-                            thread_id));
+          base::Bind(&UnpackerClient::StartProcessOnIOThread,
+                     this, thread_id, json_data));
     } else {
-      WebResourceUnpacker unpacker(json_data_);
+      WebResourceUnpacker unpacker(json_data);
       if (unpacker.Run()) {
         OnUnpackWebResourceSucceeded(*unpacker.parsed_json());
       } else {
@@ -151,10 +68,8 @@ class WebResourceService::UnpackerClient : public UtilityProcessHost::Client {
 
   // UtilityProcessHost::Client
   virtual bool OnMessageReceived(const IPC::Message& message) {
-    bool msg_is_ok = true;
     bool handled = true;
-    IPC_BEGIN_MESSAGE_MAP_EX(WebResourceService::UnpackerClient, message,
-                             msg_is_ok)
+    IPC_BEGIN_MESSAGE_MAP(WebResourceService::UnpackerClient, message)
       IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_UnpackWebResource_Succeeded,
                           OnUnpackWebResourceSucceeded)
       IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_UnpackWebResource_Failed,
@@ -169,41 +84,42 @@ class WebResourceService::UnpackerClient : public UtilityProcessHost::Client {
       return;
 
     OnUnpackWebResourceFailed(
-        "Chrome crashed while trying to retrieve web resources.");
+        "Utility process crashed while trying to retrieve web resources.");
   }
 
   void OnUnpackWebResourceSucceeded(
       const DictionaryValue& parsed_json) {
-    web_resource_service_->OnWebResourceUnpacked(parsed_json);
+    web_resource_service_->Unpack(parsed_json);
     Cleanup();
   }
 
   void OnUnpackWebResourceFailed(const std::string& error_message) {
-    web_resource_service_->EndFetch();
+    LOG(ERROR) << error_message;
     Cleanup();
   }
 
   // Release reference and set got_response_.
   void Cleanup() {
-    if (got_response_)
-      return;
-
+    DCHECK(!got_response_);
     got_response_ = true;
+
+    web_resource_service_->EndFetch();
     Release();
   }
 
-  void StartProcessOnIOThread(BrowserThread::ID thread_id) {
+  void StartProcessOnIOThread(BrowserThread::ID thread_id,
+                              const std::string& json_data) {
     UtilityProcessHost* host = new UtilityProcessHost(this, thread_id);
     host->set_use_linux_zygote(true);
     // TODO(mrc): get proper file path when we start using web resources
     // that need to be unpacked.
-    host->Send(new ChromeUtilityMsg_UnpackWebResource(json_data_));
+    host->Send(new ChromeUtilityMsg_UnpackWebResource(json_data));
   }
 
   scoped_refptr<WebResourceService> web_resource_service_;
 
-  // Holds raw JSON string.
-  std::string json_data_;
+  // Owned by the global browser process.
+  ResourceDispatcherHost* resource_dispatcher_host_;
 
   // True if we got a response from the utility process and have cleaned up
   // already.
@@ -214,66 +130,28 @@ WebResourceService::WebResourceService(
     PrefService* prefs,
     const char* web_resource_server,
     bool apply_locale_to_url,
-    int notification_type,
     const char* last_update_time_pref_name,
-    int start_fetch_delay,
-    int cache_update_delay)
+    int start_fetch_delay_ms,
+    int cache_update_delay_ms)
     : prefs_(prefs),
-      ALLOW_THIS_IN_INITIALIZER_LIST(service_factory_(this)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
       in_fetch_(false),
       web_resource_server_(web_resource_server),
       apply_locale_to_url_(apply_locale_to_url),
-      notification_type_(notification_type),
       last_update_time_pref_name_(last_update_time_pref_name),
-      start_fetch_delay_(start_fetch_delay),
-      cache_update_delay_(cache_update_delay),
-      web_resource_update_scheduled_(false) {
+      start_fetch_delay_ms_(start_fetch_delay_ms),
+      cache_update_delay_ms_(cache_update_delay_ms) {
   DCHECK(prefs);
-  prefs_->RegisterStringPref(last_update_time_pref_name,
-                             "0",
-                             PrefService::UNSYNCABLE_PREF);
-  resource_dispatcher_host_ = g_browser_process->resource_dispatcher_host();
-  web_resource_fetcher_.reset(new WebResourceFetcher(this));
 }
 
 WebResourceService::~WebResourceService() { }
 
-void WebResourceService::PostNotification(int64 delay_ms) {
-  if (web_resource_update_scheduled_)
-    return;
-  if (delay_ms > 0) {
-    web_resource_update_scheduled_ = true;
-    MessageLoop::current()->PostDelayedTask(FROM_HERE,
-        service_factory_.NewRunnableMethod(
-            &WebResourceService::WebResourceStateChange), delay_ms);
-  } else if (delay_ms == 0) {
-    WebResourceStateChange();
-  }
-}
-
 void WebResourceService::EndFetch() {
   in_fetch_ = false;
 }
 
-void WebResourceService::OnWebResourceUnpacked(
-  const DictionaryValue& parsed_json) {
-  Unpack(parsed_json);
-  EndFetch();
-}
-
-void WebResourceService::WebResourceStateChange() {
-  web_resource_update_scheduled_ = false;
-  if (notification_type_ == chrome::NOTIFICATION_CHROME_END)
-    return;
-  content::NotificationService* service =
-      content::NotificationService::current();
-  service->Notify(notification_type_,
-                  content::Source<WebResourceService>(this),
-                  content::NotificationService::NoDetails());
-}
-
 void WebResourceService::StartAfterDelay() {
-  int64 delay = start_fetch_delay_;
+  int64 delay = start_fetch_delay_ms_;
   // Check whether we have ever put a value in the web resource cache;
   // if so, pull it out and see if it's time to update again.
   if (prefs_->HasPrefPath(last_update_time_pref_name_)) {
@@ -282,23 +160,79 @@ void WebResourceService::StartAfterDelay() {
     if (!last_update_pref.empty()) {
       double last_update_value;
       base::StringToDouble(last_update_pref, &last_update_value);
-      int64 ms_until_update = cache_update_delay_ -
+      int64 ms_until_update = cache_update_delay_ms_ -
           static_cast<int64>((base::Time::Now() - base::Time::FromDoubleT(
           last_update_value)).InMilliseconds());
-      delay = ms_until_update > cache_update_delay_ ?
-          cache_update_delay_ : (ms_until_update < start_fetch_delay_ ?
-                                start_fetch_delay_ : ms_until_update);
+      // Wait at least |start_fetch_delay_ms_|.
+      if (ms_until_update > start_fetch_delay_ms_)
+        delay = ms_until_update;
     }
   }
   // Start fetch and wait for UpdateResourceCache.
-  web_resource_fetcher_->StartAfterDelay(delay);
+  ScheduleFetch(delay);
 }
 
-void WebResourceService::UpdateResourceCache(const std::string& json_data) {
-  UnpackerClient* client = new UnpackerClient(this, json_data);
-  client->Start();
+// Delay initial load of resource data into cache so as not to interfere
+// with startup time.
+void WebResourceService::ScheduleFetch(int64 delay_ms) {
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&WebResourceService::StartFetch,
+                 weak_ptr_factory_.GetWeakPtr()),
+      delay_ms);
+}
+
+// Initializes the fetching of data from the resource server.  Data
+// load calls OnURLFetchComplete.
+void WebResourceService::StartFetch() {
+  // First, put our next cache load on the MessageLoop.
+  ScheduleFetch(cache_update_delay_ms_);
 
   // Set cache update time in preferences.
   prefs_->SetString(last_update_time_pref_name_,
       base::DoubleToString(base::Time::Now().ToDoubleT()));
+
+  // If we are still fetching data, exit.
+  if (in_fetch_)
+    return;
+  in_fetch_ = true;
+
+  // Balanced in OnURLFetchComplete.
+  AddRef();
+
+  std::string web_resource_server = web_resource_server_;
+  if (apply_locale_to_url_) {
+    std::string locale = g_browser_process->GetApplicationLocale();
+    web_resource_server.append(locale);
+  }
+
+  url_fetcher_.reset(content::URLFetcher::Create(
+      GURL(web_resource_server), content::URLFetcher::GET, this));
+  // Do not let url fetcher affect existing state in system context
+  // (by setting cookies, for example).
+  url_fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE |
+                             net::LOAD_DO_NOT_SAVE_COOKIES);
+  net::URLRequestContextGetter* url_request_context_getter =
+      g_browser_process->system_request_context();
+  url_fetcher_->SetRequestContext(url_request_context_getter);
+  url_fetcher_->Start();
+}
+
+void WebResourceService::OnURLFetchComplete(const content::URLFetcher* source) {
+  // Delete the URLFetcher when this function exits.
+  scoped_ptr<content::URLFetcher> clean_up_fetcher(url_fetcher_.release());
+
+  // Don't parse data if attempt to download was unsuccessful.
+  // Stop loading new web resource data, and silently exit.
+  if (!source->GetStatus().is_success() || (source->GetResponseCode() != 200))
+    return;
+
+  std::string data;
+  source->GetResponseAsString(&data);
+
+  // UnpackerClient releases itself.
+  UnpackerClient* client = new UnpackerClient(this);
+  client->Start(data);
+
+  Release();
 }