[Extensions] Remove UIThreadExtensionFunction::DelegateForTests

The ExtensionFunction test delegate basically just served as a way of
getting the results from the function, but we already have that by simply
setting the response callback in the function. Remove the
DelegateForTests class and update uses of it to use the callback.

Also add a response() field on ExtensionFunction for use in determining
if the function succeeded or failed. Previously, code would rely on
whether or not results_ were set, but this was wrong for a few reasons:
- The absence of results doesn't always indicate failure
- The presence of results doesn't always indicate success
- In practice, we always send back an empty list value (even if
  results weren't previously set).
Update code to use the response() value instead.

BUG=648276
TBR=benwells@chromium.org (apps browsertest change)

Review-Url: https://codereview.chromium.org/2348723002
Cr-Commit-Position: refs/heads/master@{#419560}

chrome/browser/apps/app_browsertest_util.cc

@@ -179,7 +179,7 @@ bool PlatformAppBrowserTest::RunGetWindowFunctionForExtension(
           base::StringPrintf("[%u]", window_id),
           browser(),
           utils::NONE);
-  return function->GetResultList() != NULL;
+  return *function->response_type() == ExtensionFunction::SUCCEEDED;
 }
 
 size_t PlatformAppBrowserTest::GetAppWindowCount() {

chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api_unittest.cc

@@ -210,11 +210,8 @@ class EPKChallengeKeyTestBase : public BrowserWithTestWindowTest {
   std::string RunFunctionAndReturnError(UIThreadExtensionFunction* function,
                                         std::unique_ptr<base::ListValue> args,
                                         Browser* browser) {
-    scoped_refptr<ExtensionFunction> function_owner(function);
-    // Without a callback the function will not generate a result.
-    function->set_has_callback(true);
     utils::RunFunction(function, std::move(args), browser, utils::NONE);
-    EXPECT_FALSE(function->GetResultList()) << "Did not expect a result";
+    EXPECT_EQ(ExtensionFunction::FAILED, *function->response_type());
     return function->GetError();
   }
 

chrome/browser/extensions/api/identity/identity_apitest.cc

@@ -78,45 +78,6 @@ namespace utils = extension_function_test_utils;
 static const char kAccessToken[] = "auth_token";
 static const char kExtensionId[] = "ext_id";
 
-// This helps us be able to wait until an UIThreadExtensionFunction calls
-// SendResponse.
-class SendResponseDelegate
-    : public UIThreadExtensionFunction::DelegateForTests {
- public:
-  SendResponseDelegate() : should_post_quit_(false) {}
-
-  virtual ~SendResponseDelegate() {}
-
-  void set_should_post_quit(bool should_quit) {
-    should_post_quit_ = should_quit;
-  }
-
-  bool HasResponse() {
-    return response_.get() != NULL;
-  }
-
-  bool GetResponse() {
-    EXPECT_TRUE(HasResponse());
-    return *response_;
-  }
-
-  void OnSendResponse(UIThreadExtensionFunction* function,
-                      bool success,
-                      bool bad_message) override {
-    ASSERT_FALSE(bad_message);
-    ASSERT_FALSE(HasResponse());
-    response_.reset(new bool);
-    *response_ = success;
-    if (should_post_quit_) {
-      base::MessageLoopForUI::current()->QuitWhenIdle();
-    }
-  }
-
- private:
-  std::unique_ptr<bool> response_;
-  bool should_post_quit_;
-};
-
 class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
  protected:
   // Asynchronous function runner allows tests to manipulate the browser window
@@ -124,8 +85,7 @@ class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
   void RunFunctionAsync(
       UIThreadExtensionFunction* function,
       const std::string& args) {
-    response_delegate_.reset(new SendResponseDelegate);
-    function->set_test_delegate(response_delegate_.get());
+    response_delegate_.reset(new api_test_utils::SendResponseHelper(function));
     std::unique_ptr<base::ListValue> parsed_args(utils::ParseList(args));
     EXPECT_TRUE(parsed_args.get()) <<
         "Could not parse extension function arguments: " << args;
@@ -144,7 +104,8 @@ class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
 
   std::string WaitForError(UIThreadExtensionFunction* function) {
     RunMessageLoopUntilResponse();
-    EXPECT_FALSE(function->GetResultList()) << "Did not expect a result";
+    CHECK(function->response_type());
+    EXPECT_EQ(ExtensionFunction::FAILED, *function->response_type());
     return function->GetError();
   }
 
@@ -162,16 +123,11 @@ class AsyncExtensionBrowserTest : public ExtensionBrowserTest {
 
  private:
   void RunMessageLoopUntilResponse() {
-    // If the RunAsync of |function| didn't already call SendResponse, run the
-    // message loop until they do.
-    if (!response_delegate_->HasResponse()) {
-      response_delegate_->set_should_post_quit(true);
-      content::RunMessageLoop();
-    }
-    EXPECT_TRUE(response_delegate_->HasResponse());
+    response_delegate_->WaitForResponse();
+    EXPECT_TRUE(response_delegate_->has_response());
   }
 
-  std::unique_ptr<SendResponseDelegate> response_delegate_;
+  std::unique_ptr<api_test_utils::SendResponseHelper> response_delegate_;
 };
 
 class TestHangOAuth2MintTokenFlow : public OAuth2MintTokenFlow {

chrome/browser/extensions/extension_function_test_utils.cc

@@ -88,10 +88,14 @@ std::string RunFunctionAndReturnError(UIThreadExtensionFunction* function,
                                       Browser* browser,
                                       RunFunctionFlags flags) {
   scoped_refptr<ExtensionFunction> function_owner(function);
-  // Without a callback the function will not generate a result.
-  function->set_has_callback(true);
   RunFunction(function, args, browser, flags);
-  EXPECT_FALSE(function->GetResultList()) << "Did not expect a result";
+  // When sending a response, the function will set an empty list value if there
+  // is no specified result.
+  const base::ListValue* results = function->GetResultList();
+  CHECK(results);
+  EXPECT_TRUE(results->empty()) << "Did not expect a result";
+  CHECK(function->response_type());
+  EXPECT_EQ(ExtensionFunction::FAILED, *function->response_type());
   return function->GetError();
 }
 
@@ -107,8 +111,6 @@ base::Value* RunFunctionAndReturnSingleResult(
     Browser* browser,
     RunFunctionFlags flags) {
   scoped_refptr<ExtensionFunction> function_owner(function);
-  // Without a callback the function will not generate a result.
-  function->set_has_callback(true);
   RunFunction(function, args, browser, flags);
   EXPECT_TRUE(function->GetError().empty()) << "Unexpected error: "
       << function->GetError();
@@ -120,45 +122,6 @@ base::Value* RunFunctionAndReturnSingleResult(
   return NULL;
 }
 
-// This helps us be able to wait until an UIThreadExtensionFunction calls
-// SendResponse.
-class SendResponseDelegate
-    : public UIThreadExtensionFunction::DelegateForTests {
- public:
-  SendResponseDelegate() : should_post_quit_(false) {}
-
-  virtual ~SendResponseDelegate() {}
-
-  void set_should_post_quit(bool should_quit) {
-    should_post_quit_ = should_quit;
-  }
-
-  bool HasResponse() {
-    return response_.get() != NULL;
-  }
-
-  bool GetResponse() {
-    EXPECT_TRUE(HasResponse());
-    return *response_;
-  }
-
-  void OnSendResponse(UIThreadExtensionFunction* function,
-                      bool success,
-                      bool bad_message) override {
-    ASSERT_FALSE(bad_message);
-    ASSERT_FALSE(HasResponse());
-    response_.reset(new bool);
-    *response_ = success;
-    if (should_post_quit_) {
-      base::MessageLoopForUI::current()->QuitWhenIdle();
-    }
-  }
-
- private:
-  std::unique_ptr<bool> response_;
-  bool should_post_quit_;
-};
-
 bool RunFunction(UIThreadExtensionFunction* function,
                  const std::string& args,
                  Browser* browser,

extensions/browser/api/bluetooth/bluetooth_apitest.cc

@@ -197,8 +197,7 @@ IN_PROC_BROWSER_TEST_F(BluetoothApiTest, Discovery) {
       .WillOnce(
           testing::Invoke(this, &BluetoothApiTest::DiscoverySessionCallback));
   start_function = setupFunction(new api::BluetoothStartDiscoveryFunction);
-  (void)
-      utils::RunFunctionAndReturnError(start_function.get(), "[]", browser());
+  utils::RunFunction(start_function.get(), "[]", browser(), utils::NONE);
 
   // End the discovery session. The StopDiscovery function should succeed.
   testing::Mock::VerifyAndClearExpectations(mock_adapter_);

extensions/browser/api/cast_channel/cast_channel_apitest.cc

@@ -476,23 +476,16 @@ IN_PROC_BROWSER_TEST_F(CastChannelAPITest, TestSetAuthorityKeysInvalid) {
   // been removed. The API is deprecated and will trivially return
   // success. So this is just testing that it succeeds for all inputs
   // (even invalid ones).
-  std::string errorResult = "";
-
   cast_channel_set_authority_keys_function =
       CreateSetAuthorityKeysFunction(empty_extension);
-  std::string error = utils::RunFunctionAndReturnError(
-      cast_channel_set_authority_keys_function.get(),
-      "[\"\", \"signature\"]",
-      browser());
-  EXPECT_EQ(error, errorResult);
+  EXPECT_TRUE(utils::RunFunction(cast_channel_set_authority_keys_function.get(),
+                                 "[\"\", \"signature\"]", browser(),
+                                 utils::NONE));
 
   cast_channel_set_authority_keys_function =
       CreateSetAuthorityKeysFunction(empty_extension);
-  error = utils::RunFunctionAndReturnError(
-      cast_channel_set_authority_keys_function.get(),
-      "[\"keys\", \"\"]",
-      browser());
-  EXPECT_EQ(error, errorResult);
+  EXPECT_TRUE(utils::RunFunction(cast_channel_set_authority_keys_function.get(),
+                                 "[\"keys\", \"\"]", browser(), utils::NONE));
 
   std::string keys =
       "CrMCCiBSnZzWf+XraY5w3SbX2PEmWfHm5SNIv2pc9xbhP0EOcxKOAjCCAQoCggEBALwigL"
@@ -511,27 +504,21 @@ IN_PROC_BROWSER_TEST_F(CastChannelAPITest, TestSetAuthorityKeysInvalid) {
 
   cast_channel_set_authority_keys_function =
       CreateSetAuthorityKeysFunction(empty_extension);
-  error = utils::RunFunctionAndReturnError(
-      cast_channel_set_authority_keys_function.get(),
-      "[\"" + keys + "\", \"signature\"]",
-      browser());
-  EXPECT_EQ(error, errorResult);
+  EXPECT_TRUE(utils::RunFunction(cast_channel_set_authority_keys_function.get(),
+                                 "[\"" + keys + "\", \"signature\"]", browser(),
+                                 utils::NONE));
 
   cast_channel_set_authority_keys_function =
       CreateSetAuthorityKeysFunction(empty_extension);
-  error = utils::RunFunctionAndReturnError(
-      cast_channel_set_authority_keys_function.get(),
-      "[\"keys\", \"" + signature + "\"]",
-      browser());
-  EXPECT_EQ(error, errorResult);
+  EXPECT_TRUE(utils::RunFunction(cast_channel_set_authority_keys_function.get(),
+                                 "[\"keys\", \"" + signature + "\"]", browser(),
+                                 utils::NONE));
 
   cast_channel_set_authority_keys_function =
       CreateSetAuthorityKeysFunction(empty_extension);
-  error = utils::RunFunctionAndReturnError(
-      cast_channel_set_authority_keys_function.get(),
-      "[\"" + keys + "\", \"" + signature + "\"]",
-      browser());
-  EXPECT_EQ(error, errorResult);
+  EXPECT_TRUE(utils::RunFunction(cast_channel_set_authority_keys_function.get(),
+                                 "[\"" + keys + "\", \"" + signature + "\"]",
+                                 browser(), utils::NONE));
 }
 
 IN_PROC_BROWSER_TEST_F(CastChannelAPITest, TestSetAuthorityKeysValid) {
@@ -563,9 +550,8 @@ IN_PROC_BROWSER_TEST_F(CastChannelAPITest, TestSetAuthorityKeysValid) {
       "bzPtNRRlTqfv7Rxm5YXkZMLmJJMZiTs5+o8FMRMTQZT4hRR3DQ+A/jofViyTGA==";
 
   std::string args = "[\"" + keys + "\", \"" + signature + "\"]";
-  std::string error = utils::RunFunctionAndReturnError(
-      cast_channel_set_authority_keys_function.get(), args, browser());
-  EXPECT_EQ(error, std::string());
+  EXPECT_TRUE(utils::RunFunction(cast_channel_set_authority_keys_function.get(),
+                                 args, browser(), utils::NONE));
 }
 
 // TODO(vadimgo): Win Dbg has a workaround that makes RunExtensionSubtest

extensions/browser/api_test_utils.cc

@@ -31,49 +31,37 @@ std::unique_ptr<base::ListValue> ParseList(const std::string& data) {
   return base::ListValue::From(ParseJSON(data));
 }
 
-// This helps us be able to wait until an UIThreadExtensionFunction calls
-// SendResponse.
-class SendResponseDelegate
-    : public UIThreadExtensionFunction::DelegateForTests {
- public:
-  SendResponseDelegate() {}
-
-  virtual ~SendResponseDelegate() {}
-
-  bool HasResponse() { return response_.get() != NULL; }
+}  // namespace
 
-  bool GetResponse() {
-    EXPECT_TRUE(HasResponse());
-    return *response_;
-  }
+namespace extensions {
 
-  void OnSendResponse(UIThreadExtensionFunction* function,
-                      bool success,
-                      bool bad_message) override {
-    ASSERT_FALSE(bad_message);
-    ASSERT_FALSE(HasResponse());
-    response_.reset(new bool);
-    *response_ = success;
-    run_loop_.Quit();
-  }
+namespace api_test_utils {
 
-  void WaitForResponse() {
-    // If the RunAsync of UIThreadExtensionFunction already called SendResponse,
-    // this will finish immediately.
-    run_loop_.Run();
-  }
+SendResponseHelper::SendResponseHelper(UIThreadExtensionFunction* function) {
+  function->set_has_callback(true);
+  function->set_response_callback(
+      base::Bind(&SendResponseHelper::OnResponse, base::Unretained(this)));
+}
 
- private:
-  base::RunLoop run_loop_;
-  std::unique_ptr<bool> response_;
-  DISALLOW_COPY_AND_ASSIGN(SendResponseDelegate);
-};
+SendResponseHelper::~SendResponseHelper() {}
 
-}  // namespace
+bool SendResponseHelper::GetResponse() {
+  EXPECT_TRUE(has_response());
+  return *response_;
+}
 
-namespace extensions {
+void SendResponseHelper::OnResponse(ExtensionFunction::ResponseType response,
+                                    const base::ListValue& results,
+                                    const std::string& error,
+                                    functions::HistogramValue histogram_value) {
+  ASSERT_NE(ExtensionFunction::BAD_MESSAGE, response);
+  response_.reset(new bool(response == ExtensionFunction::SUCCEEDED));
+  run_loop_.Quit();
+}
 
-namespace api_test_utils {
+void SendResponseHelper::WaitForResponse() {
+  run_loop_.Run();
+}
 
 std::unique_ptr<base::DictionaryValue> ParseDictionary(
     const std::string& data) {
@@ -160,8 +148,6 @@ std::unique_ptr<base::Value> RunFunctionWithDelegateAndReturnSingleResult(
     content::BrowserContext* context,
     std::unique_ptr<extensions::ExtensionFunctionDispatcher> dispatcher,
     RunFunctionFlags flags) {
-  // Without a callback the function will not generate a result.
-  function->set_has_callback(true);
   RunFunction(function.get(), std::move(args), context, std::move(dispatcher),
               flags);
   EXPECT_TRUE(function->GetError().empty()) << "Unexpected error: "
@@ -207,9 +193,14 @@ std::string RunFunctionAndReturnError(UIThreadExtensionFunction* function,
       new ExtensionFunctionDispatcher(context));
   scoped_refptr<ExtensionFunction> function_owner(function);
   // Without a callback the function will not generate a result.
-  function->set_has_callback(true);
   RunFunction(function, args, context, std::move(dispatcher), flags);
-  EXPECT_FALSE(function->GetResultList()) << "Did not expect a result";
+  // When sending a response, the function will set an empty list value if there
+  // is no specified result.
+  const base::ListValue* results = function->GetResultList();
+  CHECK(results);
+  EXPECT_TRUE(results->empty()) << "Did not expect a result";
+  CHECK(function->response_type());
+  EXPECT_EQ(ExtensionFunction::FAILED, *function->response_type());
   return function->GetError();
 }
 
@@ -240,8 +231,7 @@ bool RunFunction(
     content::BrowserContext* context,
     std::unique_ptr<extensions::ExtensionFunctionDispatcher> dispatcher,
     RunFunctionFlags flags) {
-  SendResponseDelegate response_delegate;
-  function->set_test_delegate(&response_delegate);
+  SendResponseHelper response_helper(function);
   function->SetArgs(args.get());
 
   CHECK(dispatcher);
@@ -250,10 +240,10 @@ bool RunFunction(
   function->set_browser_context(context);
   function->set_include_incognito(flags & INCLUDE_INCOGNITO);
   function->RunWithValidation()->Execute();
-  response_delegate.WaitForResponse();
+  response_helper.WaitForResponse();
 
-  EXPECT_TRUE(response_delegate.HasResponse());
-  return response_delegate.GetResponse();
+  EXPECT_TRUE(response_helper.has_response());
+  return response_helper.GetResponse();
 }
 
 }  // namespace api_test_utils

extensions/browser/api_test_utils.h

@@ -9,10 +9,10 @@
 #include <string>
 
 #include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "extensions/browser/extension_function.h"
 #include "extensions/common/manifest.h"
 
-class UIThreadExtensionFunction;
-
 namespace base {
 class DictionaryValue;
 class ListValue;
@@ -34,6 +34,33 @@ class ExtensionFunctionDispatcher;
 // and migrate existing users to the new API.
 namespace api_test_utils {
 
+// A helper class to handle waiting for a function response.
+class SendResponseHelper {
+ public:
+  explicit SendResponseHelper(UIThreadExtensionFunction* function);
+  ~SendResponseHelper();
+
+  bool has_response() { return response_.get() != nullptr; }
+
+  // Asserts a response has been posted (has_response()) and returns the value.
+  bool GetResponse();
+
+  // Waits until a response is posted.
+  void WaitForResponse();
+
+ private:
+  // Response handler.
+  void OnResponse(ExtensionFunction::ResponseType response,
+                  const base::ListValue& results,
+                  const std::string& error,
+                  functions::HistogramValue histogram_value);
+
+  base::RunLoop run_loop_;
+  std::unique_ptr<bool> response_;
+
+  DISALLOW_COPY_AND_ASSIGN(SendResponseHelper);
+};
+
 enum RunFunctionFlags { NONE = 0, INCLUDE_INCOGNITO = 1 << 0 };
 
 // Parse JSON and return as the specified type, or NULL if the JSON is invalid

extensions/browser/extension_function.cc

@@ -449,17 +449,18 @@ bool ExtensionFunction::HasOptionalArgument(size_t index) {
 void ExtensionFunction::SendResponseImpl(bool success) {
   DCHECK(!response_callback_.is_null());
 
-  ResponseType type = success ? SUCCEEDED : FAILED;
+  ResponseType response = success ? SUCCEEDED : FAILED;
   if (bad_message_) {
-    type = BAD_MESSAGE;
+    response = BAD_MESSAGE;
     LOG(ERROR) << "Bad extension message " << name_;
   }
+  response_type_ = base::MakeUnique<ResponseType>(response);
 
   // If results were never set, we send an empty argument list.
   if (!results_)
     results_.reset(new base::ListValue());
 
-  response_callback_.Run(type, *results_, GetError(), histogram_value());
+  response_callback_.Run(response, *results_, GetError(), histogram_value());
   LogUma(success, timer_.Elapsed(), histogram_value_);
 }
 
@@ -470,8 +471,7 @@ void ExtensionFunction::OnRespondingLater(ResponseValue value) {
 UIThreadExtensionFunction::UIThreadExtensionFunction()
     : context_(nullptr),
       render_frame_host_(nullptr),
-      is_from_service_worker_(false),
-      delegate_(nullptr) {}
+      is_from_service_worker_(false) {}
 
 UIThreadExtensionFunction::~UIThreadExtensionFunction() {
   if (dispatcher() && render_frame_host())
@@ -551,13 +551,9 @@ content::WebContents* UIThreadExtensionFunction::GetSenderWebContents() {
 void UIThreadExtensionFunction::SendResponse(bool success) {
   DCHECK(!did_respond_) << name_;
   did_respond_ = true;
-  if (delegate_)
-    delegate_->OnSendResponse(this, success, bad_message_);
-  else
-    SendResponseImpl(success);
+  SendResponseImpl(success);
 
   if (!transferred_blob_uuids_.empty()) {
-    DCHECK(!delegate_) << "Blob transfer not supported with test delegate.";
     render_frame_host_->Send(
         new ExtensionMsg_TransferBlobs(transferred_blob_uuids_));
   }

extensions/browser/extension_function.h

@@ -315,6 +315,8 @@ class ExtensionFunction
     return source_process_id_;
   }
 
+  ResponseType* response_type() const { return response_type_.get(); }
+
   // Sets did_respond_ to true so that the function won't DCHECK if it never
   // sends a response. Typically, this shouldn't be used, even in testing. It's
   // only for when you want to test functionality that doesn't exercise the
@@ -475,6 +477,9 @@ class ExtensionFunction
  private:
   base::ElapsedTimer timer_;
 
+  // The response type of the function, if the response has been sent.
+  std::unique_ptr<ResponseType> response_type_;
+
   void OnRespondingLater(ResponseValue response);
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionFunction);
@@ -484,26 +489,12 @@ class ExtensionFunction
 // this category.
 class UIThreadExtensionFunction : public ExtensionFunction {
  public:
-  // TODO(yzshen): We should be able to remove this interface now that we
-  // support overriding the response callback.
-  // A delegate for use in testing, to intercept the call to SendResponse.
-  class DelegateForTests {
-   public:
-    virtual void OnSendResponse(UIThreadExtensionFunction* function,
-                                bool success,
-                                bool bad_message) = 0;
-  };
-
   UIThreadExtensionFunction();
 
   UIThreadExtensionFunction* AsUIThreadExtensionFunction() override;
 
   bool PreRunValidation(std::string* error) override;
 
-  void set_test_delegate(DelegateForTests* delegate) {
-    delegate_ = delegate;
-  }
-
   // Called when a message was received.
   // Should return true if it processed the message.
   virtual bool OnMessageReceived(const IPC::Message& message);
@@ -579,8 +570,6 @@ class UIThreadExtensionFunction : public ExtensionFunction {
 
   std::unique_ptr<RenderFrameHostTracker> tracker_;
 
-  DelegateForTests* delegate_;
-
   // The blobs transferred to the renderer process.
   std::vector<std::string> transferred_blob_uuids_;