Drive extension functions from ExtensionFunction::Run. The

extensions/browser/extension_function.h

 // Copyright 2013 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.
 
 #ifndef EXTENSIONS_BROWSER_EXTENSION_FUNCTION_H_
 #define EXTENSIONS_BROWSER_EXTENSION_FUNCTION_H_
 
 #include <list>
 #include <string>
 
@@ skipping 100 matching lines @@
 
   // Returns true if the function has permission to run.
   //
   // The default implementation is to check the Extension's permissions against
   // what this function requires to run, but some APIs may require finer
   // grained control, such as tabs.executeScript being allowed for active tabs.
   //
   // This will be run after the function has been set up but before Run().
   virtual bool HasPermission();
 
-  // Execute the API. Clients should initialize the ExtensionFunction using
-  // SetArgs(), set_request_id(), and the other setters before calling this
-  // method.
+  // The result of a function call.
   //
-  // Note that once Run() returns, dispatcher() can be NULL, so be sure to
-  // NULL-check.
-  void Run();
+  // Use NoArguments(), SingleArgument(), MultipleArguments(), or Error()
+  // rather than this class directly.
+  class ResponseValueObject {
+   public:
+    virtual ~ResponseValueObject() {}
+
+    // Returns true for success, false for failure.
+    virtual bool Apply() = 0;
+  };
+  typedef scoped_ptr<ResponseValueObject> ResponseValue;
+
+  // The action to use when returning from RunAsync.
+  //
+  // Use RespondNow() or RespondLater() rather than this class directly.
+  class ResponseActionObject {
+   public:
+    virtual ~ResponseActionObject() {}
+
+    virtual void Execute() = 0;
+  };
+  typedef scoped_ptr<ResponseActionObject> ResponseAction;
+
+  // Runs the function and returns the action to take when the caller is ready
+  // to respond.
+  //
+  // Callers must call Execute() on the return ResponseAction at some point,
+  // exactly once.
+  //
+  // SyncExtensionFunction and AsyncExtensionFunction implement this in terms
+  // of SyncExtensionFunction::RunSync and AsyncExtensionFunction::RunAsync,
+  // but this is deprecated. ExtensionFunction implementations are encouraged
+  // to just implement Run.
+  virtual ResponseAction Run() WARN_UNUSED_RESULT = 0;
 
   // Gets whether quota should be applied to this individual function
   // invocation. This is different to GetQuotaLimitHeuristics which is only
   // invoked once and then cached.
   //
   // Returns false by default.
   virtual bool ShouldSkipQuotaLimiting() const;
 
   // Optionally adds one or multiple QuotaLimitHeuristic instances suitable for
   // this function to |heuristics|. The ownership of the new QuotaLimitHeuristic
@@ skipping 63 matching lines @@
   extensions::functions::HistogramValue histogram_value() const {
     return histogram_value_; }
 
   void set_response_callback(const ResponseCallback& callback) {
     response_callback_ = callback;
   }
 
   void set_source_tab_id(int source_tab_id) { source_tab_id_ = source_tab_id; }
   int source_tab_id() const { return source_tab_id_; }
 
-  // The result of a function call.
-  //
-  // Use NoArguments(), SingleArgument(), MultipleArguments(), or Error()
-  // rather than this class directly.
-  class ResponseValueObject {
-   public:
-    virtual ~ResponseValueObject() {}
-
-    // Returns true for success, false for failure.
-    virtual bool Apply() = 0;
-  };
-  typedef scoped_ptr<ResponseValueObject> ResponseValue;
-
-  // The action to use when returning from RunImpl.
-  //
-  // Use RespondNow() or RespondLater() rather than this class directly.
-  class ResponseActionObject {
-   public:
-    virtual ~ResponseActionObject() {}
-
-    virtual void Execute() = 0;
-  };
-  typedef scoped_ptr<ResponseActionObject> ResponseAction;
-
  protected:
   friend struct ExtensionFunctionDeleteTraits;
 
   // ResponseValues.
   //
   // Success, no arguments to pass to caller
   ResponseValue NoArguments();
   // Success, a single argument |result| to pass to caller. TAKES OWNERSHIP.
   ResponseValue SingleArgument(base::Value* result);
   // Success, a list of arguments |results| to pass to caller. TAKES OWNERSHIP.
   ResponseValue MultipleArguments(base::ListValue* results);
   // Error. chrome.runtime.lastError.message will be set to |error|.
   ResponseValue Error(const std::string& error);
   // Bad message. A ResponseValue equivalent to EXTENSION_FUNCTION_VALIDATE().
   ResponseValue BadMessage();
 
   // ResponseActions.
   //
   // Respond to the extension immediately with |result|.
   ResponseAction RespondNow(ResponseValue result);
-  // Don't respond now, but promise to call SendResponse later.
+  // Don't respond now, but promise to call Done() later.
   ResponseAction RespondLater();
 
+  // If RespondLater() was used, functions must at some point call Done() with
+  // |result| as their result.
+  void Done(ResponseValue result);

[not at google - send to devlin, 2014/04/30 15:25:46]

This is basically a rename of SendResponseTypesafe to Done. I'm not especially
happy about it.

- Done() may be overly cute. It doesn't imply the completion of a RespondLater
call, but I can't find a good name for that. RespondWith() perhaps, or just
Respond()?

- My struggle in finding a name may imply that this "I'm done" callback should
really be a property that gets populated by RespondLater somehow. I experimented
with making RespondLater take a base::Callback*, so that the extension function
flow would look something like:

class SomeExtensionFunction : ExtensionFunction {
 protected:
  ResponseValue Run() {
    ...
    DoAsyncThing(base::Bind(this, &SomeAsyncThingHappened));
    return RespondLater(&done_);
  }

 private:
  void SomeAsyncThingHappened() {
    done_.Run(SingleArgument(42));
  }

  base::Callback<void(ResponseValue)> done_;
};

but then forcing every ExtensionFunction which uses RunLater to define a member
variable called "done_" didn't seem any better than just defining a Done()
function. Less magical I suppose? The other issue with the base::Callback
approach is slightly subtle lifetime / circular refcounting.

Another pattern that could work with the RespondLater(&done_) thing is writing
the callback to a *local* variable and currying that around in the base::Bind
calls that the function implementation probably uses. Like:

class SomeExtensionFunction : ExtensionFunction {
 protected:
  ResponseValue Run() {
    ...
    
    base::Callback<void(ResponseValue)> done;
    ResponseValue respond_later = RespondLater(&done);
    DoAsyncThing(base::Bind(this, &SomeAsyncThingHappened), done);
    return respond_later;
  }

 private:
  void SomeAsyncThingHappened(base::Callback<void(ResponseValue)> done) {
    done.Run(SingleArgument(42));
  }
};

but again that seemed overly boilerplatey.

Do you have any ideas? A better pattern than RespondLater taking a Callback?

[Ken Rockot(use gerrit already), 2014/04/30 23:38:08]

Just throwing this out there before I head out for real: Why not have RespondNow
serve both purposes (i.e. respond synchronously when called directly, or
complete an async response when called after RespondLater)? I haven't thought it
through -at all- but couldn't ExtensionFunction manage the appropriate state to
make that magic happen?

[not at google - send to devlin, 2014/04/30 23:54:39]

Hm. Good point. I had dismissed that option as too magical but maybe it could
work:

class SomeExtensionFunction : ExtensionFunction {
 protected:
  ResponseValue Run() {
    ... 
    return RespondLater();
  }

 private:
  void SomeAsyncThingHappened() {
    RespondNow(SingleArgument(42))->Execute();
  }
};

nice actually. (the question is then what to call "Execute").

+
   virtual ~ExtensionFunction();
 
   // Helper method for ExtensionFunctionDeleteTraits. Deletes this object.
   virtual void Destruct() const = 0;
 
-  // Derived classes should implement one of these methods to do their work.
+  // Do not call this function directly, return the appropriate ResponseAction
+  // from Run() instead. If using RespondLater then call Done().
   //
-  // Returns the action to take. DO NOT USE WITH SyncExtensionFunction.
-  virtual ResponseAction RunImplTypesafe();
-  // Deprecated. Returns true on success. SendResponse() must be called later.
-  // Return false to indicate an error and respond immediately.
-  virtual bool RunImpl();
-
-  // Sends the result back to the extension.
-  //
-  // Responds with |response|.
-  void SendResponseTypesafe(ResponseValue response);
-  // Deprecated. Call with true to indicate success, false to indicate failure,
-  // in which case please set |error_|.
+  // Call with true to indicate success, false to indicate failure, in which
+  // case please set |error_|.
   virtual void SendResponse(bool success) = 0;
 
   // Common implementation for SendResponse.
   void SendResponseImpl(bool success);
 
   // Return true if the argument to this function at |index| was provided and
   // is non-null.
   bool HasOptionalArgument(size_t index);
 
   // Id of this request, used to map the response back to the caller.
@@ skipping 43 matching lines @@
   // is invoked.
   extensions::functions::HistogramValue histogram_value_;
 
   // The callback to run once the function has done execution.
   ResponseCallback response_callback_;
 
   // The ID of the tab triggered this function call, or -1 if there is no tab.
   int source_tab_id_;
 
  private:
+  void OnRespondingLater(ResponseValue response);
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionFunction);
 };
 
 // Extension functions that run on the UI thread. Most functions fall into
 // 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.
@@ skipping 132 matching lines @@
 };
 
 // Base class for an extension function that runs asynchronously *relative to
 // the browser's UI thread*.
 class AsyncExtensionFunction : public UIThreadExtensionFunction {
  public:
   AsyncExtensionFunction();
 
  protected:
   virtual ~AsyncExtensionFunction();
+
+  // Deprecated: Override UIThreadExtensionFunction and implement Run() instead.
+  //
+  // AsyncExtensionFunctions implement this method. Return true to indicate that
+  // nothing has gone wrong yet; SendResponse must be called later. Return true
+  // to respond immediately with an error.
+  virtual bool RunAsync() = 0;
+
+ private:
+  virtual ResponseAction Run() OVERRIDE;
 };
 
 // A SyncExtensionFunction is an ExtensionFunction that runs synchronously
 // *relative to the browser's UI thread*. Note that this has nothing to do with
 // running synchronously relative to the extension process. From the extension
 // process's point of view, the function is still asynchronous.
 //
 // This kind of function is convenient for implementing simple APIs that just
 // need to interact with things on the browser UI thread.
 class SyncExtensionFunction : public UIThreadExtensionFunction {
  public:
   SyncExtensionFunction();
 
-  virtual bool RunImpl() OVERRIDE;
+ protected:
+  virtual ~SyncExtensionFunction();
 
- protected:
+  // Deprecated: Override UIThreadExtensionFunction and implement Run() instead.
+  //
+  // SyncExtensionFunctions implement this method. Return true to respond
+  // immediately with success, false to respond immediately with an error.
   virtual bool RunSync() = 0;
 
-  virtual ~SyncExtensionFunction();
+ private:
+  virtual ResponseAction Run() OVERRIDE;
 };
 
 class SyncIOThreadExtensionFunction : public IOThreadExtensionFunction {
  public:
   SyncIOThreadExtensionFunction();
 
-  virtual bool RunImpl() OVERRIDE;
+ protected:
+  virtual ~SyncIOThreadExtensionFunction();
 
- protected:
+  // Deprecated: Override IOThreadExtensionFunction and implement Run() instead.
+  //
+  // SyncIOThreadExtensionFunctions implement this method. Return true to
+  // respond immediately with success, false to respond immediately with an
+  // error.
   virtual bool RunSync() = 0;
 
-  virtual ~SyncIOThreadExtensionFunction();
+ private:
+  virtual ResponseAction Run() OVERRIDE;
 };
 
 #endif  // EXTENSIONS_BROWSER_EXTENSION_FUNCTION_H_