history_api -> UIThreadExtensionFunction.

chrome/browser/extensions/api/history/history_api.cc

Index: chrome/browser/extensions/api/history/history_api.cc
diff --git a/chrome/browser/extensions/api/history/history_api.cc b/chrome/browser/extensions/api/history/history_api.cc
index 9d0441493caa1d026374d3d41021284ef7b17d7f..d450d9ae1ad78abcd6c7f5466606d6b697c4091f 100644
--- a/chrome/browser/extensions/api/history/history_api.cc
+++ b/chrome/browser/extensions/api/history/history_api.cc
@@ -219,20 +219,22 @@ void HistoryAPI::OnListenerAdded(const EventListenerInfo& details) {
   EventRouter::Get(browser_context_)->UnregisterObserver(this);
 }
 
-bool HistoryFunction::ValidateUrl(const std::string& url_string, GURL* url) {
+bool HistoryFunction::ValidateUrl(const std::string& url_string,
+                                  GURL* url,
+                                  std::string* error) {
   GURL temp_url(url_string);
   if (!temp_url.is_valid()) {
-    error_ = kInvalidUrlError;
+    *error = kInvalidUrlError;
     return false;
   }
   url->Swap(&temp_url);
   return true;
 }
 
-bool HistoryFunction::VerifyDeleteAllowed() {
+bool HistoryFunction::VerifyDeleteAllowed(std::string* error) {
   PrefService* prefs = GetProfile()->GetPrefs();
   if (!prefs->GetBoolean(prefs::kAllowDeletingBrowserHistory)) {
-    error_ = kDeleteProhibitedError;
+    *error = kDeleteProhibitedError;
     return false;
   }
   return true;
@@ -248,38 +250,36 @@ base::Time HistoryFunction::GetTime(double ms_from_epoch) {
       base::Time::UnixEpoch() : base::Time::FromDoubleT(seconds_from_epoch);
 }
 
-HistoryFunctionWithCallback::HistoryFunctionWithCallback() {
+Profile* HistoryFunction::GetProfile() const {
+  return Profile::FromBrowserContext(browser_context());
 }
 
-HistoryFunctionWithCallback::~HistoryFunctionWithCallback() {
-}
+HistoryFunctionWithCallback::HistoryFunctionWithCallback() {}
 
-bool HistoryFunctionWithCallback::RunAsync() {
-  AddRef();  // Balanced in SendAysncRepose() and below.
-  bool retval = RunAsyncImpl();
-  if (false == retval)
-    Release();
-  return retval;
-}
+HistoryFunctionWithCallback::~HistoryFunctionWithCallback() {}
 
-void HistoryFunctionWithCallback::SendAsyncResponse() {
+void HistoryFunctionWithCallback::SendAsyncResponse(

[Devlin, 2017/02/03 22:30:53]

Do we have to SendAsyncResponse in order to prevent accidentally calling back
synchronously?  It looks like most of these are chained from callbacks - are
some of those (possibly) called synchronously?

[lazyboy, 2017/02/03 23:49:06]

HistoryService apis run on its own TaskRunner, e.g.:
https://cs.chromium.org/chromium/src/components/history/core/browser/history_...

All of these apis
QueryURL/QueryHistory/AddPage/DeleteURL/ExpireHistoryBetween
will call PostTask, so the callbacks shouldn't ever be called synchronously.

I discovered one early out in AddPage from HistoryAddUrlFunction:
https://cs.chromium.org/chromium/src/components/history/core/browser/history_...
However, since we don't wait for the task to finish before responding, that
shouldn't be an issue either.

[Devlin, 2017/02/06 16:09:26]

So if that's the case (that all these methods on history service run async), do
we need SendAsyncResponse at all?  Or could we just use Respond() in each place?

[lazyboy, 2017/02/14 00:55:30]

Done.

+    ResponseValue response_value) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
-      base::Bind(&HistoryFunctionWithCallback::SendResponseToCallback, this));
+      base::Bind(&HistoryFunctionWithCallback::SendResponseToCallback, this,
+                 base::Passed(&response_value)));
 }
 
-void HistoryFunctionWithCallback::SendResponseToCallback() {
-  SendResponse(true);
-  Release();  // Balanced in RunAsync().
+void HistoryFunctionWithCallback::SendResponseToCallback(
+    ResponseValue response_value) {
+  Respond(std::move(response_value));
+  Release();  // Balanced in Run().
 }
 
-bool HistoryGetVisitsFunction::RunAsyncImpl() {
+ExtensionFunction::ResponseAction HistoryGetVisitsFunction::Run() {
   std::unique_ptr<GetVisits::Params> params(GetVisits::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   GURL url;
-  if (!ValidateUrl(params->details.url, &url))
-    return false;
+  std::string error;
+  if (!ValidateUrl(params->details.url, &url, &error))
+    return RespondNow(Error(error));
 
   history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
       GetProfile(), ServiceAccessType::EXPLICIT_ACCESS);
@@ -288,7 +288,8 @@ bool HistoryGetVisitsFunction::RunAsyncImpl() {
                base::Bind(&HistoryGetVisitsFunction::QueryComplete,
                           base::Unretained(this)),
                &task_tracker_);
-  return true;
+  AddRef();               // Balanced in SendAsyncResponse().
+  return RespondLater();  // QueryComplete() will be called asynchronously.
 }
 
 void HistoryGetVisitsFunction::QueryComplete(
@@ -301,11 +302,10 @@ void HistoryGetVisitsFunction::QueryComplete(
       visit_item_vec.push_back(GetVisitItem(visit));
   }
 
-  results_ = GetVisits::Results::Create(visit_item_vec);
-  SendAsyncResponse();
+  SendAsyncResponse(ArgumentList(GetVisits::Results::Create(visit_item_vec)));

[Devlin, 2017/02/06 16:09:26]

e.g. this becomes just:
Respond(ArgumentList(...));
Release();  // Balanced in Run().

[lazyboy, 2017/02/14 00:55:30]

Done.

 }
 
-bool HistorySearchFunction::RunAsyncImpl() {
+ExtensionFunction::ResponseAction HistorySearchFunction::Run() {
   std::unique_ptr<Search::Params> params(Search::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
@@ -330,7 +330,8 @@ bool HistorySearchFunction::RunAsyncImpl() {
                               base::Unretained(this)),
                    &task_tracker_);
 
-  return true;
+  AddRef();               // Balanced in SendAsyncResponse().
+  return RespondLater();  // SearchComplete() will be called asynchronously.
 }
 
 void HistorySearchFunction::SearchComplete(history::QueryResults* results) {
@@ -339,36 +340,36 @@ void HistorySearchFunction::SearchComplete(history::QueryResults* results) {
     for (const history::URLResult* item : *results)
       history_item_vec.push_back(GetHistoryItem(*item));
   }
-  results_ = Search::Results::Create(history_item_vec);
-  SendAsyncResponse();
+  SendAsyncResponse(ArgumentList(Search::Results::Create(history_item_vec)));
 }
 
-bool HistoryAddUrlFunction::RunAsync() {
+ExtensionFunction::ResponseAction HistoryAddUrlFunction::Run() {
   std::unique_ptr<AddUrl::Params> params(AddUrl::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   GURL url;
-  if (!ValidateUrl(params->details.url, &url))
-    return false;
+  std::string error;
+  if (!ValidateUrl(params->details.url, &url, &error))
+    return RespondNow(Error(error));
 
   history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
       GetProfile(), ServiceAccessType::EXPLICIT_ACCESS);
   hs->AddPage(url, base::Time::Now(), history::SOURCE_EXTENSION);
 
-  SendResponse(true);
-  return true;
+  return RespondNow(NoArguments());
 }
 
-bool HistoryDeleteUrlFunction::RunAsync() {
+ExtensionFunction::ResponseAction HistoryDeleteUrlFunction::Run() {
   std::unique_ptr<DeleteUrl::Params> params(DeleteUrl::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
-  if (!VerifyDeleteAllowed())
-    return false;
+  std::string error;
+  if (!VerifyDeleteAllowed(&error))
+    return RespondNow(Error(error));
 
   GURL url;
-  if (!ValidateUrl(params->details.url, &url))
-    return false;
+  if (!ValidateUrl(params->details.url, &url, &error))
+    return RespondNow(Error(error));
 
   history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
       GetProfile(), ServiceAccessType::EXPLICIT_ACCESS);
@@ -384,17 +385,17 @@ bool HistoryDeleteUrlFunction::RunAsync() {
     activity_log->RemoveURL(url);
   }
 
-  SendResponse(true);
-  return true;
+  return RespondNow(NoArguments());
 }
 
-bool HistoryDeleteRangeFunction::RunAsyncImpl() {
+ExtensionFunction::ResponseAction HistoryDeleteRangeFunction::Run() {
   std::unique_ptr<DeleteRange::Params> params(
       DeleteRange::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
-  if (!VerifyDeleteAllowed())
-    return false;
+  std::string error;
+  if (!VerifyDeleteAllowed(&error))
+    return RespondNow(Error(error));
 
   base::Time start_time = GetTime(params->range.start_time);
   base::Time end_time = GetTime(params->range.end_time);
@@ -418,16 +419,18 @@ bool HistoryDeleteRangeFunction::RunAsyncImpl() {
     activity_log->RemoveURLs(restrict_urls);
   }
 
-  return true;
+  AddRef();               // Balanced in SendAsyncResponse().
+  return RespondLater();  // DeleteComplete() will be called asynchronously.
 }
 
 void HistoryDeleteRangeFunction::DeleteComplete() {
-  SendAsyncResponse();
+  SendAsyncResponse(NoArguments());
 }
 
-bool HistoryDeleteAllFunction::RunAsyncImpl() {
-  if (!VerifyDeleteAllowed())
-    return false;
+ExtensionFunction::ResponseAction HistoryDeleteAllFunction::Run() {
+  std::string error;
+  if (!VerifyDeleteAllowed(&error))
+    return RespondNow(Error(error));
 
   std::set<GURL> restrict_urls;
   history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
@@ -448,11 +451,12 @@ bool HistoryDeleteAllFunction::RunAsyncImpl() {
     activity_log->RemoveURLs(restrict_urls);
   }
 
-  return true;
+  AddRef();               // Balanced in SendAsyncResponse().
+  return RespondLater();  // DeleteComplete() will be called asynchronously.
 }
 
 void HistoryDeleteAllFunction::DeleteComplete() {
-  SendAsyncResponse();
+  SendAsyncResponse(NoArguments());
 }
 
 }  // namespace extensions