Remove subclasses and outstanding_requests_ from QuotaDispatcherHost

Remove outstanding_requests_ from QuotaDispatcherHost and associated
subclasses to address TODO. These subclasses existed to carry the
request_id around. Now that the request_id has been removed from the
QuotaDispatcherHost system, these subclasses are no longer needed.

Also change the design of QuotaDispatcherHost slightly; instead of
creating a separate object to hold all the request parameters, curry
the parameters into the various async callbacks.

Bug: 781643
Change-Id: Ia85ed5a8f6e75a424767faa632e0f1f831a32ddf
Reviewed-on: https://chromium-review.googlesource.com/799573Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Sasha Morrissey <sashab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521919}

content/browser/quota_dispatcher_host.cc

@@ -12,8 +12,6 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/numerics/safe_conversions.h"
-#include "base/trace_event/trace_event.h"
-#include "content/public/browser/quota_permission_context.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "net/base/url_util.h"
 #include "storage/browser/quota/quota_manager.h"
@@ -26,221 +24,15 @@ using storage::StorageType;
 
 namespace content {
 
-// Created one per request to carry the request's request_id around.
-// Dispatches requests from renderer/worker to the QuotaManager and
-// sends back the response to the renderer/worker.
-// TODO(sashab): Remove, since request_id is no longer needed.
-class QuotaDispatcherHost::RequestDispatcher {
- public:
-  RequestDispatcher(base::WeakPtr<QuotaDispatcherHost> dispatcher_host)
-      : dispatcher_host_(dispatcher_host),
-        render_process_id_(dispatcher_host->process_id_) {
-    request_id_ =
-        dispatcher_host_->outstanding_requests_.Add(base::WrapUnique(this));
-  }
-  virtual ~RequestDispatcher() {}
-
- protected:
-  // Subclass must call this when it's done with the request.
-  void Completed() {
-    if (dispatcher_host_)
-      dispatcher_host_->outstanding_requests_.Remove(request_id_);
-  }
-
-  QuotaDispatcherHost* dispatcher_host() const {
-    return dispatcher_host_.get();
-  }
-  storage::QuotaManager* quota_manager() const {
-    return dispatcher_host_ ? dispatcher_host_->quota_manager_ : nullptr;
-  }
-  QuotaPermissionContext* permission_context() const {
-    return dispatcher_host_ ? dispatcher_host_->permission_context_.get()
-                            : nullptr;
-  }
-  int render_process_id() const { return render_process_id_; }
-
- private:
-  base::WeakPtr<QuotaDispatcherHost> dispatcher_host_;
-  int render_process_id_;
-  int request_id_;
-};
-
-class QuotaDispatcherHost::QueryUsageAndQuotaDispatcher
-    : public RequestDispatcher {
- public:
-  QueryUsageAndQuotaDispatcher(
-      base::WeakPtr<QuotaDispatcherHost> dispatcher_host,
-      QueryStorageUsageAndQuotaCallback callback)
-      : RequestDispatcher(dispatcher_host),
-        callback_(std::move(callback)),
-        weak_factory_(this) {}
-  ~QueryUsageAndQuotaDispatcher() override {}
-
-  void QueryStorageUsageAndQuota(const GURL& origin, StorageType type) {
-    // crbug.com/349708
-    TRACE_EVENT0("io",
-                 "QuotaDispatcherHost::QueryUsageAndQuotaDispatcher"
-                 "::QueryStorageUsageAndQuota");
-
-    quota_manager()->GetUsageAndQuotaForWebApps(
-        origin, type,
-        base::Bind(&QueryUsageAndQuotaDispatcher::DidQueryStorageUsageAndQuota,
-                   weak_factory_.GetWeakPtr()));
-  }
-
- private:
-  void DidQueryStorageUsageAndQuota(QuotaStatusCode status,
-                                    int64_t usage,
-                                    int64_t quota) {
-    if (!dispatcher_host())
-      return;
-    // crbug.com/349708
-    TRACE_EVENT0("io", "QuotaDispatcherHost::RequestQuotaDispatcher"
-                 "::DidQueryStorageUsageAndQuota");
-    std::move(callback_).Run(status, usage, quota);
-    Completed();
-  }
-
-  QueryStorageUsageAndQuotaCallback callback_;
-  base::WeakPtrFactory<QueryUsageAndQuotaDispatcher> weak_factory_;
-};
-
-class QuotaDispatcherHost::RequestQuotaDispatcher
-    : public RequestDispatcher {
- public:
-  typedef RequestQuotaDispatcher self_type;
-
-  RequestQuotaDispatcher(
-      base::WeakPtr<QuotaDispatcherHost> dispatcher_host,
-      int64_t render_frame_id,
-      const GURL& origin_url,
-      storage::StorageType storage_type,
-      uint64_t requested_size,
-      mojom::QuotaDispatcherHost::RequestStorageQuotaCallback callback)
-      : RequestDispatcher(dispatcher_host),
-        render_frame_id_(render_frame_id),
-        origin_url_(origin_url),
-        storage_type_(storage_type),
-        requested_size_(requested_size),
-        callback_(std::move(callback)),
-        current_usage_(0),
-        current_quota_(0),
-        requested_quota_(0),
-        weak_factory_(this) {
-    // Convert the requested size from uint64_t to int64_t since the quota
-    // backend requires int64_t values.
-    // TODO(nhiroki): The backend should accept uint64_t values.
-    requested_quota_ = base::saturated_cast<int64_t>(requested_size);
-  }
-  ~RequestQuotaDispatcher() override {}
-
-  void Start() {
-    DCHECK(dispatcher_host());
-    // crbug.com/349708
-    TRACE_EVENT0("io", "QuotaDispatcherHost::RequestQuotaDispatcher::Start");
-
-    DCHECK(storage_type_ == storage::kStorageTypeTemporary ||
-           storage_type_ == storage::kStorageTypePersistent);
-    if (storage_type_ == storage::kStorageTypePersistent) {
-      quota_manager()->GetUsageAndQuotaForWebApps(
-          origin_url_, storage_type_,
-          base::Bind(&self_type::DidGetPersistentUsageAndQuota,
-                     weak_factory_.GetWeakPtr()));
-    } else {
-      quota_manager()->GetUsageAndQuotaForWebApps(
-          origin_url_, storage_type_,
-          base::Bind(&self_type::DidGetTemporaryUsageAndQuota,
-                     weak_factory_.GetWeakPtr()));
-    }
-  }
-
- private:
-  void DidGetPersistentUsageAndQuota(QuotaStatusCode status,
-                                     int64_t usage,
-                                     int64_t quota) {
-    if (!dispatcher_host())
-      return;
-    if (status != storage::kQuotaStatusOk) {
-      DidFinish(status, 0, 0);
-      return;
-    }
-
-    if (quota_manager()->IsStorageUnlimited(origin_url_, storage_type_) ||
-        requested_quota_ <= quota) {
-      // Seems like we can just let it go.
-      DidFinish(storage::kQuotaStatusOk, usage, requested_size_);
-      return;
-    }
-    current_usage_ = usage;
-    current_quota_ = quota;
-
-    // Otherwise we need to consult with the permission context and
-    // possibly show a prompt.
-    DCHECK(permission_context());
-
-    StorageQuotaParams params;
-    params.render_frame_id = render_frame_id_;
-    params.origin_url = origin_url_;
-    params.storage_type = storage_type_;
-    params.requested_size = requested_size_;
-
-    permission_context()->RequestQuotaPermission(
-        params, render_process_id(),
-        base::Bind(&self_type::DidGetPermissionResponse,
-                   weak_factory_.GetWeakPtr()));
-  }
-
-  void DidGetTemporaryUsageAndQuota(QuotaStatusCode status,
-                                    int64_t usage,
-                                    int64_t quota) {
-    DidFinish(status, usage, std::min(requested_quota_, quota));
-  }
-
-  void DidGetPermissionResponse(
-      QuotaPermissionContext::QuotaPermissionResponse response) {
-    if (!dispatcher_host())
-      return;
-    if (response != QuotaPermissionContext::QUOTA_PERMISSION_RESPONSE_ALLOW) {
-      // User didn't allow the new quota.  Just returning the current quota.
-      DidFinish(storage::kQuotaStatusOk, current_usage_, current_quota_);
-      return;
-    }
-    // Now we're allowed to set the new quota.
-    quota_manager()->SetPersistentHostQuota(
-        net::GetHostOrSpecFromURL(origin_url_), requested_size_,
-        base::Bind(&self_type::DidSetHostQuota, weak_factory_.GetWeakPtr()));
-  }
-
-  void DidSetHostQuota(QuotaStatusCode status, int64_t new_quota) {
-    DidFinish(status, current_usage_, new_quota);
-  }
-
-  void DidFinish(QuotaStatusCode status, int64_t usage, int64_t granted_quota) {
-    if (!dispatcher_host())
-      return;
-    DCHECK(dispatcher_host());
-    std::move(callback_).Run(status, usage, granted_quota);
-    Completed();
-  }
-
-  const int64_t render_frame_id_;
-  const GURL origin_url_;
-  const storage::StorageType storage_type_;
-  const uint64_t requested_size_;
-  mojom::QuotaDispatcherHost::RequestStorageQuotaCallback callback_;
-
-  int64_t current_usage_;
-  int64_t current_quota_;
-  int64_t requested_quota_;
-  base::WeakPtrFactory<self_type> weak_factory_;
-};
-
 // static
 void QuotaDispatcherHost::Create(
     int process_id,
     QuotaManager* quota_manager,
     scoped_refptr<QuotaPermissionContext> permission_context,
     mojom::QuotaDispatcherHostRequest request) {
+  DCHECK(quota_manager);
+  // TODO(sashab): Work out the conditions for permission_context to be set and
+  // add a DCHECK for it here.
   mojo::MakeStrongBinding(
       base::MakeUnique<QuotaDispatcherHost>(process_id, quota_manager,
                                             std::move(permission_context)),
@@ -253,16 +45,18 @@ QuotaDispatcherHost::QuotaDispatcherHost(
     scoped_refptr<QuotaPermissionContext> permission_context)
     : process_id_(process_id),
       quota_manager_(quota_manager),
-      permission_context_(permission_context),
+      permission_context_(std::move(permission_context)),
       weak_factory_(this) {}
 
 void QuotaDispatcherHost::QueryStorageUsageAndQuota(
     const GURL& origin_url,
     storage::StorageType storage_type,
     QueryStorageUsageAndQuotaCallback callback) {
-  QueryUsageAndQuotaDispatcher* dispatcher = new QueryUsageAndQuotaDispatcher(
-      weak_factory_.GetWeakPtr(), std::move(callback));
-  dispatcher->QueryStorageUsageAndQuota(origin_url, storage_type);
+  quota_manager_->GetUsageAndQuotaForWebApps(
+      origin_url, storage_type,
+      base::Bind(&QuotaDispatcherHost::DidQueryStorageUsageAndQuota,
+                 weak_factory_.GetWeakPtr(),
+                 base::Passed(std::move(callback))));
 }
 
 void QuotaDispatcherHost::RequestStorageQuota(
@@ -278,10 +72,112 @@ void QuotaDispatcherHost::RequestStorageQuota(
     return;
   }
 
-  RequestQuotaDispatcher* dispatcher = new RequestQuotaDispatcher(
-      weak_factory_.GetWeakPtr(), render_frame_id, origin_url, storage_type,
-      requested_size, std::move(callback));
-  dispatcher->Start();
+  DCHECK(storage_type == storage::kStorageTypeTemporary ||
+         storage_type == storage::kStorageTypePersistent);
+  if (storage_type == storage::kStorageTypePersistent) {
+    quota_manager_->GetUsageAndQuotaForWebApps(
+        origin_url, storage_type,
+        base::Bind(&QuotaDispatcherHost::DidGetPersistentUsageAndQuota,
+                   weak_factory_.GetWeakPtr(), render_frame_id, origin_url,
+                   storage_type, requested_size,
+                   base::Passed(std::move(callback))));
+  } else {
+    quota_manager_->GetUsageAndQuotaForWebApps(
+        origin_url, storage_type,
+        base::Bind(&QuotaDispatcherHost::DidGetTemporaryUsageAndQuota,
+                   weak_factory_.GetWeakPtr(), requested_size,
+                   base::Passed(std::move(callback))));
+  }
+}
+
+void QuotaDispatcherHost::DidQueryStorageUsageAndQuota(
+    RequestStorageQuotaCallback callback,
+    QuotaStatusCode status,
+    int64_t usage,
+    int64_t quota) {
+  std::move(callback).Run(status, usage, quota);
+}
+
+void QuotaDispatcherHost::DidGetPersistentUsageAndQuota(
+    int64_t render_frame_id,
+    const GURL& origin_url,
+    storage::StorageType storage_type,
+    uint64_t requested_quota,
+    RequestStorageQuotaCallback callback,
+    QuotaStatusCode status,
+    int64_t current_usage,
+    int64_t current_quota) {
+  if (status != storage::kQuotaStatusOk) {
+    std::move(callback).Run(status, 0, 0);
+    return;
+  }
+
+  // If we have enough quota for the requested storage, we can just let it go.
+  // Convert the requested size from uint64_t to int64_t since the quota backend
+  // requires int64_t values.
+  // TODO(nhiroki): The backend should accept uint64_t values.
+  int64_t requested_quota_signed =
+      base::saturated_cast<int64_t>(requested_quota);
+  if (quota_manager_->IsStorageUnlimited(origin_url, storage_type) ||
+      requested_quota_signed <= current_quota) {
+    std::move(callback).Run(storage::kQuotaStatusOk, current_usage,
+                            requested_quota);
+    return;
+  }
+
+  // Otherwise we need to consult with the permission context and possibly show
+  // a prompt.
+  DCHECK(permission_context_);
+  StorageQuotaParams params;
+  params.render_frame_id = render_frame_id;
+  params.origin_url = origin_url;
+  params.storage_type = storage_type;
+  params.requested_size = requested_quota;
+
+  permission_context_->RequestQuotaPermission(
+      params, process_id_,
+      base::Bind(&QuotaDispatcherHost::DidGetPermissionResponse,
+                 weak_factory_.GetWeakPtr(), origin_url, requested_quota,
+                 current_usage, current_quota,
+                 base::Passed(std::move(callback))));
+}
+
+void QuotaDispatcherHost::DidGetPermissionResponse(
+    const GURL& origin_url,
+    uint64_t requested_quota,
+    int64_t current_usage,
+    int64_t current_quota,
+    RequestStorageQuotaCallback callback,
+    QuotaPermissionContext::QuotaPermissionResponse response) {
+  // If user didn't allow the new quota, just return the current quota.
+  if (response != QuotaPermissionContext::QUOTA_PERMISSION_RESPONSE_ALLOW) {
+    std::move(callback).Run(storage::kQuotaStatusOk, current_usage,
+                            current_quota);
+    return;
+  }
+
+  // Otherwise, return the new quota.
+  quota_manager_->SetPersistentHostQuota(
+      net::GetHostOrSpecFromURL(origin_url), requested_quota,
+      base::Bind(&QuotaDispatcherHost::DidSetHostQuota,
+                 weak_factory_.GetWeakPtr(), current_usage,
+                 base::Passed(std::move(callback))));
+}
+
+void QuotaDispatcherHost::DidSetHostQuota(int64_t current_usage,
+                                          RequestStorageQuotaCallback callback,
+                                          QuotaStatusCode status,
+                                          int64_t new_quota) {
+  std::move(callback).Run(status, current_usage, new_quota);
+}
+
+void QuotaDispatcherHost::DidGetTemporaryUsageAndQuota(
+    int64_t requested_quota,
+    RequestStorageQuotaCallback callback,
+    QuotaStatusCode status,
+    int64_t usage,
+    int64_t quota) {
+  std::move(callback).Run(status, usage, std::min(requested_quota, quota));
 }
 
 QuotaDispatcherHost::~QuotaDispatcherHost() = default;

content/browser/quota_dispatcher_host.h

@@ -5,10 +5,10 @@
 #ifndef CONTENT_BROWSER_QUOTA_DISPATCHER_HOST_H_
 #define CONTENT_BROWSER_QUOTA_DISPATCHER_HOST_H_
 
-#include "base/containers/id_map.h"
 #include "base/macros.h"
 #include "content/common/quota_dispatcher_host.mojom.h"
-#include "content/public/browser/browser_message_filter.h"
+#include "content/public/browser/quota_permission_context.h"
+#include "storage/common/quota/quota_status_code.h"
 
 class GURL;
 
@@ -44,9 +44,34 @@ class QuotaDispatcherHost : public mojom::QuotaDispatcherHost {
                            RequestStorageQuotaCallback callback) override;
 
  private:
-  class RequestDispatcher;
-  class QueryUsageAndQuotaDispatcher;
-  class RequestQuotaDispatcher;
+  void DidQueryStorageUsageAndQuota(RequestStorageQuotaCallback callback,
+                                    storage::QuotaStatusCode status,
+                                    int64_t usage,
+                                    int64_t quota);
+  void DidGetPersistentUsageAndQuota(int64_t render_frame_id,
+                                     const GURL& origin_url,
+                                     storage::StorageType storage_type,
+                                     uint64_t requested_quota,
+                                     RequestStorageQuotaCallback callback,
+                                     storage::QuotaStatusCode status,
+                                     int64_t usage,
+                                     int64_t quota);
+  void DidGetPermissionResponse(
+      const GURL& origin_url,
+      uint64_t requested_quota,
+      int64_t current_usage,
+      int64_t current_quota,
+      RequestStorageQuotaCallback callback,
+      QuotaPermissionContext::QuotaPermissionResponse response);
+  void DidSetHostQuota(int64_t current_usage,
+                       RequestStorageQuotaCallback callback,
+                       storage::QuotaStatusCode status,
+                       int64_t new_quota);
+  void DidGetTemporaryUsageAndQuota(int64_t requested_quota,
+                                    RequestStorageQuotaCallback callback,
+                                    storage::QuotaStatusCode status,
+                                    int64_t usage,
+                                    int64_t quota);
 
   // The ID of this process.
   int process_id_;
@@ -54,8 +79,6 @@ class QuotaDispatcherHost : public mojom::QuotaDispatcherHost {
   storage::QuotaManager* quota_manager_;
   scoped_refptr<QuotaPermissionContext> permission_context_;
 
-  base::IDMap<std::unique_ptr<RequestDispatcher>> outstanding_requests_;
-
   base::WeakPtrFactory<QuotaDispatcherHost> weak_factory_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(QuotaDispatcherHost);