Use IntentsQuery as the WDS consumer. By unencumbering WebIntentsRegistry of this responsibility we…

chrome/browser/intents/web_intents_registry.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/intents/web_intents_registry.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 133 matching lines @@
       (lhs.action != rhs.action) ||
       (lhs.title != rhs.title) ||
       (lhs.disposition != rhs.disposition));
 }
 
 }  // namespace
 
 using webkit_glue::WebIntentServiceData;
 
 // Internal object representing all data associated with a single query.
-struct WebIntentsRegistry::IntentsQuery {
+struct WebIntentsRegistry::IntentsQuery : public WebDataServiceConsumer {
+
+  WebIntentsRegistry* registry_;

[groby-ooo-7-16, 2012/07/27 00:11:26]

Please document variable and ownership. (I.e. I assume this is weak?)

[Steve McKay, 2012/07/27 00:54:46]

Done.

+
   // Underlying data query.
-  WebDataService::Handle pending_query_;
+  WebDataService::Handle query_handle_;
 
   // The callback for this particular query.
   QueryCallback callback_;
 
   // Callback for a query for defaults.
   DefaultQueryCallback default_callback_;
 
   // The particular action to filter for while searching through extensions.
   // If |action_| is empty, return all extension-provided services.
   string16 action_;
 
   // The MIME type that was requested for this service query.
   // Suppports wild cards.
   string16 type_;
 
   // The url of the invoking page.
   GURL url_;
 
   // Create a new IntentsQuery for services with the specified action/type.
-  IntentsQuery(const QueryCallback& callback,
+  IntentsQuery(WebIntentsRegistry* registry,
+               const QueryCallback& callback,
                const string16& action, const string16& type)
-      : callback_(callback), action_(action), type_(type) {}
+      : registry_(registry), callback_(callback), action_(action),
+        type_(type) {}
 
   // Create a new IntentsQuery for all intent services or for existence checks.
-  explicit IntentsQuery(const QueryCallback callback)
-      : callback_(callback), type_(ASCIIToUTF16("*")) {}
+  IntentsQuery(WebIntentsRegistry* registry,
+               const QueryCallback callback)
+      : registry_(registry), callback_(callback), type_(ASCIIToUTF16("*")) {}
 
   // Create a new IntentsQuery for default services.
-  IntentsQuery(const DefaultQueryCallback& callback,
+  IntentsQuery(WebIntentsRegistry* registry,
+               const DefaultQueryCallback& callback,
                const string16& action, const string16& type, const GURL& url)
-      : default_callback_(callback), action_(action), type_(type), url_(url) {}
+      : registry_(registry), default_callback_(callback), action_(action),
+        type_(type), url_(url) {}
+
+  void OnWebDataServiceRequestDone(
+      WebDataService::Handle h,
+      const WDTypedResult* result) OVERRIDE {
+
+    // dispatch the request
+    if (result->GetType() == WEB_INTENTS_RESULT) {

[groby-ooo-7-16, 2012/07/27 00:11:26]

Why is this not a callback? This way we can save ourselves a lot of type-based
dispatching? (And the back-link to the registry, too)

[Greg Billock, 2012/07/27 00:41:29]

We discussed doing that. With that design, we'd need a separate trampoline class
anyway, so this is a simpler intermediate position. Ultimately, the WDS methods
should take callbacks, making all this query object crap vanish.

[Steve McKay, 2012/07/27 00:54:46]

IntentsQuery will be decomposed into sub-classes in a future CL. Since the team
asked me to keep my CLs small, this seemed like a good first step in the change.

+      registry_->OnWebIntentsResultReceived(this, h, result);
+    } else if (result->GetType() == WEB_INTENTS_DEFAULTS_RESULT) {
+      registry_->OnWebIntentsDefaultsResultReceived(this, h, result);
+    } else {
+      NOTREACHED();
+    }
+  }
 };
 
 WebIntentsRegistry::WebIntentsRegistry() {}
 
 WebIntentsRegistry::~WebIntentsRegistry() {
+
   // Cancel all pending queries, since we can't handle them any more.
-  for (QueryMap::iterator it(queries_.begin()); it != queries_.end(); ++it) {
-    wds_->CancelRequest(it->first);
-    delete it->second;
+  for (QueryVector::iterator it = pending_queries_.begin();
+      it != pending_queries_.end(); ++it) {
+    IntentsQuery* query = *it;
+    wds_->CancelRequest(query->query_handle_);
+    delete query;
   }
 }
 
 void WebIntentsRegistry::Initialize(
     scoped_refptr<WebDataService> wds,
     ExtensionServiceInterface* extension_service) {
   wds_ = wds;
   extension_service_ = extension_service;
 }
 
-void WebIntentsRegistry::OnWebDataServiceRequestDone(
+void WebIntentsRegistry::OnWebIntentsResultReceived(
+    IntentsQuery* query,
     WebDataService::Handle h,

[Greg Billock, 2012/07/27 00:41:29]

Can get rid of handle param

[Steve McKay, 2012/07/27 00:54:46]

Done.

     const WDTypedResult* result) {
+  DCHECK(query);
   DCHECK(result);
-  if (result->GetType() == WEB_INTENTS_DEFAULTS_RESULT) {
-    OnWebDataServiceDefaultsRequestDone(h, result);
-    return;
-  }
   DCHECK(result->GetType() == WEB_INTENTS_RESULT);
 
-  QueryMap::iterator it = queries_.find(h);
-  DCHECK(it != queries_.end());
-
-  IntentsQuery* query(it->second);
-  DCHECK(query);
-  queries_.erase(it);
+  bool query_found = ClaimQuery(query);
+  DCHECK(query_found);
 
   IntentServiceList matching_services = static_cast<
       const WDResult<IntentServiceList>*>(result)->GetValue();
 
   // Loop over all services in all extensions, collect ones
   // matching the query.
   if (extension_service_) {
     const ExtensionSet* extensions = extension_service_->extensions();
     if (extensions) {
       for (ExtensionSet::const_iterator i(extensions->begin());
            i != extensions->end(); ++i) {
         AddMatchingServicesForExtension(**i, query->action_,
                                         &matching_services);
       }
     }
   }
 
   // Filter out all services not matching the query type.
   FilterServicesByMimetype(query->type_, &matching_services);
 
   // Collapse intents that are equivalent for all but |type|.
   CollapseIntents(&matching_services);
 
   query->callback_.Run(matching_services);
   delete query;
 }
 
-const Extension* WebIntentsRegistry::ExtensionForURL(const std::string& url) {
-  const ExtensionSet* extensions = extension_service_->extensions();
-  if (!extensions)
-    return NULL;
-
-  // Use the unsafe ExtensionURLInfo constructor: we don't care if the extension
-  // is running or not.
-  GURL gurl(url);
-  ExtensionURLInfo info(gurl);
-  return extensions->GetExtensionOrAppByURL(info);
-}
-
-void WebIntentsRegistry::OnWebDataServiceDefaultsRequestDone(
+void WebIntentsRegistry::OnWebIntentsDefaultsResultReceived(
+    IntentsQuery* query,
     WebDataService::Handle h,

[Greg Billock, 2012/07/27 00:41:29]

We don't need the handle anymore, right?

[Steve McKay, 2012/07/27 00:54:46]

Done.

     const WDTypedResult* result) {
-  QueryMap::iterator it = queries_.find(h);
-  DCHECK(it != queries_.end());
+  DCHECK(query);
+  DCHECK(result);
+  DCHECK(result->GetType() == WEB_INTENTS_DEFAULTS_RESULT);
 
-  IntentsQuery* query(it->second);
-  DCHECK(query);
-  queries_.erase(it);
+  bool query_found = ClaimQuery(query);
+  DCHECK(query_found);
 
   std::vector<DefaultWebIntentService> services = static_cast<
       const WDResult<std::vector<DefaultWebIntentService> >*>(result)->
           GetValue();
 
   DefaultWebIntentService default_service;
   std::vector<DefaultWebIntentService>::iterator iter(services.begin());
   for (; iter != services.end(); ++iter) {
     if (!WebIntentsTypesMatch(iter->type, query->type_)) {
       continue;
@@ skipping 37 matching lines @@
         break;
       }
     }
   }
 
   query->default_callback_.Run(default_service);
   delete query;
 }
 
 void WebIntentsRegistry::GetIntentServices(
-    const string16& action, const string16& mimetype,
+    const string16& action, const string16& type,
     const QueryCallback& callback) {
   DCHECK(wds_.get());
   DCHECK(!callback.is_null());
 
-  IntentsQuery* query = new IntentsQuery(callback, action, mimetype);
-  query->pending_query_ = wds_->GetWebIntentServices(action, this);
-  queries_[query->pending_query_] = query;
+  IntentsQuery* query = new IntentsQuery(this, callback, action, type);
+  query->query_handle_ = wds_->GetWebIntentServices(action, query);
+  TrackQuery(query);
 }
 
 void WebIntentsRegistry::GetAllIntentServices(
     const QueryCallback& callback) {
   DCHECK(wds_.get());
   DCHECK(!callback.is_null());
 
-  IntentsQuery* query = new IntentsQuery(callback);
-  query->pending_query_ = wds_->GetAllWebIntentServices(this);
-  queries_[query->pending_query_] = query;
+  IntentsQuery* query = new IntentsQuery(this, callback);
+  query->query_handle_ = wds_->GetAllWebIntentServices(query);
+  TrackQuery(query);
 }
 
 void WebIntentsRegistry::IntentServiceExists(
     const WebIntentServiceData& service,
     const base::Callback<void(bool)>& callback) {
   DCHECK(!callback.is_null());
 
   IntentsQuery* query = new IntentsQuery(
-      base::Bind(&ExistenceCallback, service, callback));
-  query->pending_query_ = wds_->GetWebIntentServicesForURL(
-      UTF8ToUTF16(service.service_url.spec()), this);
-  queries_[query->pending_query_] = query;
+      this, base::Bind(&ExistenceCallback, service, callback));
+  query->query_handle_ = wds_->GetWebIntentServicesForURL(
+      UTF8ToUTF16(service.service_url.spec()), query);
+  TrackQuery(query);
 }
 
 void WebIntentsRegistry::GetIntentServicesForExtensionFilter(
         const string16& action,
         const string16& mimetype,
         const std::string& extension_id,
         const QueryCallback& callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(!callback.is_null());
 
+  // This isn't a WDS query, so we don't track it,
+  // or claim the query later.
   scoped_ptr<IntentsQuery> query(
-      new IntentsQuery(callback, action, mimetype));
+      new IntentsQuery(this, callback, action, mimetype));
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
       base::Bind(&WebIntentsRegistry::DoGetIntentServicesForExtensionFilter,
                  base::Unretained(this),
                  base::Passed(&query), extension_id));
 }
 
 void WebIntentsRegistry::DoGetIntentServicesForExtensionFilter(
     scoped_ptr<IntentsQuery> query,
@@ skipping 25 matching lines @@
 }
 
 void WebIntentsRegistry::GetDefaultIntentService(
     const string16& action,
     const string16& type,
     const GURL& invoking_url,
     const DefaultQueryCallback& callback) {
   DCHECK(!callback.is_null());
 
   IntentsQuery* query =
-      new IntentsQuery(callback, action, type, invoking_url);
-  query->pending_query_ =
-      wds_->GetDefaultWebIntentServicesForAction(action, this);
-  queries_[query->pending_query_] = query;
+      new IntentsQuery(this, callback, action, type, invoking_url);
+  query->query_handle_ =
+      wds_->GetDefaultWebIntentServicesForAction(action, query);
+  TrackQuery(query);
 }
 
 void WebIntentsRegistry::RegisterIntentService(
     const WebIntentServiceData& service) {
   DCHECK(wds_.get());
   wds_->AddWebIntentService(service);
 }
 
 void WebIntentsRegistry::UnregisterIntentService(
     const WebIntentServiceData& service) {
@@ skipping 24 matching lines @@
       if (write_iter != read_iter)
         *write_iter = *read_iter;
     }
     ++read_iter;
   }
 
   // Cut off everything after the last intent copied to the list.
   if (++write_iter != services->end())
     services->erase(write_iter, services->end());
 }
+
+const Extension* WebIntentsRegistry::ExtensionForURL(const std::string& url) {

[groby-ooo-7-16, 2012/07/27 00:11:26]

Technically, functions should be in order of declaration in the header. If you
move them, you might as well put them in the right order (or update the header)

[Greg Billock, 2012/07/27 00:41:29]

Yeah, update the header.

On 2012/07/27 00:11:26, groby wrote:

[Steve McKay, 2012/07/27 00:54:46]

Done.

+  const ExtensionSet* extensions = extension_service_->extensions();
+  if (!extensions)
+    return NULL;
+
+  // Use the unsafe ExtensionURLInfo constructor: we don't care if the extension
+  // is running or not.
+  GURL gurl(url);
+  ExtensionURLInfo info(gurl);
+  return extensions->GetExtensionOrAppByURL(info);
+}
+
+void WebIntentsRegistry::TrackQuery(IntentsQuery* query) {
+  DCHECK(query);
+  pending_queries_.push_back(query);
+}
+
+bool WebIntentsRegistry::ClaimQuery(IntentsQuery* query) {
+  DCHECK(query);
+  QueryVector::iterator it = std::find(
+      pending_queries_.begin(), pending_queries_.end(), query);
+  if (it != pending_queries_.end()) {
+    pending_queries_.erase(it);
+    return true;
+  } else {
+    return false;
+  }
+}