Drive extension functions from ExtensionFunction::Run. The

Drive extension functions from ExtensionFunction::Run. The
SyncExtensionFunction and AsyncExtensionFunction derivates now expose RunSync
and RunAsync respectively. New extension function implementations should just
implement Run directly.


BUG=365732
R=rockot@chromium.org
TBR=dmazzoni@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268033

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

Here is that awesome patch I was talking about, where "awesome" is a local
maximum of extension function cleanup patches :)

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/storage/storage_api.h (right):

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/api/s...
extensions/browser/api/storage/storage_api.h:25: virtual ResponseAction Run()
OVERRIDE;
Once again the Storage API is the only one which uses the new function type.

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:264: void Done(ResponseValue result);
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?

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function_dispatcher.cc:277:
function->Run()->Execute();
I also find this a bit weird. At least Run() can have WARN_UNUSED_RESULT, and
there are a very small number of call sites, but still... weird.

It's also weird that "Run" and "Execute" are synonyms yet sort-of mean different
things here. Maybe "Execute" should be renamed "Respond" or "Reply" or "Send"?

I decided this was better than making those RespondNow() functions immediately
do their action, since they could declare those "ResponseAction" variables
locally and expect them not to do anything. This could be made to work by
changing Run() to return void again, and get rid of ResponseAction. An option I
didn't immediately consider since I like the "return" aspect, the point was
enforcing at the type level that only a single thing can happen.

Another option would be to define a
RunAndExecute()/RunAndRespond()/RunAndReply()/RunAndSend() function on
ExtensionFunction(), define Run() as protected, then the only option here would
be to call that.

[not at google - send to devlin, 2014-04-30 16:19:17]

https://codereview.chromium.org/257333002/diff/40001/apps/shell/browser/api/s...
File apps/shell/browser/api/shell/shell_api.cc (right):

https://codereview.chromium.org/257333002/diff/40001/apps/shell/browser/api/s...
apps/shell/browser/api/shell/shell_api.cc:35: ExtensionFunction::ResponseAction
ShellCreateWindowFunction::Run() {
app_shell didn't compile, so I fixed this up, and converted to ::Run while I was
at it.

[Ken Rockot(use gerrit already), 2014-04-30 22:37:15]

lgtm

https://codereview.chromium.org/257333002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.h
(right):

https://codereview.chromium.org/257333002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth_socket/bluetooth_socket_api.h:14: class
BluetoothSocketCreateFunction : public AsyncExtensionFunction {
nice

https://codereview.chromium.org/257333002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/257333002/diff/40001/extensions/browser/exten...
extensions/browser/extension_function.h:264: void Done(ResponseValue result);
nice

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

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:264: void Done(ResponseValue result);
On 2014/04/30 15:25:46, kalman wrote:
> 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?

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?

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function_dispatcher.cc:277:
function->Run()->Execute();
On 2014/04/30 15:25:46, kalman wrote:
> I also find this a bit weird. At least Run() can have WARN_UNUSED_RESULT, and
> there are a very small number of call sites, but still... weird.
> 
> It's also weird that "Run" and "Execute" are synonyms yet sort-of mean
different
> things here. Maybe "Execute" should be renamed "Respond" or "Reply" or "Send"?
> 
> I decided this was better than making those RespondNow() functions immediately
> do their action, since they could declare those "ResponseAction" variables
> locally and expect them not to do anything. This could be made to work by
> changing Run() to return void again, and get rid of ResponseAction. An option
I
> didn't immediately consider since I like the "return" aspect, the point was
> enforcing at the type level that only a single thing can happen.
> 
> Another option would be to define a
> RunAndExecute()/RunAndRespond()/RunAndReply()/RunAndSend() function on
> ExtensionFunction(), define Run() as protected, then the only option here
would
> be to call that.

Of all the suggested verbs, I like "Send" the best, but I'd still like to think
about this some more now that you bring it up...

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

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:264: void Done(ResponseValue result);
On 2014/04/30 23:38:08, Ken Rockot wrote:
> On 2014/04/30 15:25:46, kalman wrote:
> > 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?
> 
> 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?

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").

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
extensions/browser/extension_function_dispatcher.cc:277:
function->Run()->Execute();
On 2014/04/30 23:38:08, Ken Rockot wrote:
> On 2014/04/30 15:25:46, kalman wrote:
> > I also find this a bit weird. At least Run() can have WARN_UNUSED_RESULT,
and
> > there are a very small number of call sites, but still... weird.
> > 
> > It's also weird that "Run" and "Execute" are synonyms yet sort-of mean
> different
> > things here. Maybe "Execute" should be renamed "Respond" or "Reply" or
"Send"?
> > 
> > I decided this was better than making those RespondNow() functions
immediately
> > do their action, since they could declare those "ResponseAction" variables
> > locally and expect them not to do anything. This could be made to work by
> > changing Run() to return void again, and get rid of ResponseAction. An
option
> I
> > didn't immediately consider since I like the "return" aspect, the point was
> > enforcing at the type level that only a single thing can happen.
> > 
> > Another option would be to define a
> > RunAndExecute()/RunAndRespond()/RunAndReply()/RunAndSend() function on
> > ExtensionFunction(), define Run() as protected, then the only option here
> would
> > be to call that.
> 
> Of all the suggested verbs, I like "Send" the best, but I'd still like to
think
> about this some more now that you bring it up...

No rush :)

[Ken Rockot(use gerrit already), 2014-05-01 02:14:47]

On 2014/04/30 23:54:38, kalman wrote:
>
https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
> File extensions/browser/extension_function.h (right):
> 
>
https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
> extensions/browser/extension_function.h:264: void Done(ResponseValue result);
> On 2014/04/30 23:38:08, Ken Rockot wrote:
> > On 2014/04/30 15:25:46, kalman wrote:
> > > 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?
> > 
> > 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?
> 
> 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").
> 
>
https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
> File extensions/browser/extension_function_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/257333002/diff/20001/extensions/browser/exten...
> extensions/browser/extension_function_dispatcher.cc:277:
> function->Run()->Execute();
> On 2014/04/30 23:38:08, Ken Rockot wrote:
> > On 2014/04/30 15:25:46, kalman wrote:
> > > I also find this a bit weird. At least Run() can have WARN_UNUSED_RESULT,
> and
> > > there are a very small number of call sites, but still... weird.
> > > 
> > > It's also weird that "Run" and "Execute" are synonyms yet sort-of mean
> > different
> > > things here. Maybe "Execute" should be renamed "Respond" or "Reply" or
> "Send"?
> > > 
> > > I decided this was better than making those RespondNow() functions
> immediately
> > > do their action, since they could declare those "ResponseAction" variables
> > > locally and expect them not to do anything. This could be made to work by
> > > changing Run() to return void again, and get rid of ResponseAction. An
> option
> > I
> > > didn't immediately consider since I like the "return" aspect, the point
was
> > > enforcing at the type level that only a single thing can happen.
> > > 
> > > Another option would be to define a
> > > RunAndExecute()/RunAndRespond()/RunAndReply()/RunAndSend() function on
> > > ExtensionFunction(), define Run() as protected, then the only option here
> > would
> > > be to call that.
> > 
> > Of all the suggested verbs, I like "Send" the best, but I'd still like to
> think
> > about this some more now that you bring it up...
> 
> No rush :)

Ok, the meditative calm of high-pitched dental machinery had me thinking.

What if Run() just returns a value, and there's a special static const value
(e.g., kDeferred) defined in ExtensionFunction which means
"I am not a real value, but I promise someone will call Respond() eventually."

Then there's a single, synchronous Respond() method used by async
completions and ExtensionFunctionDispatcher alike. Example:

class RadSyncFunction : ExtensionFunction {
 public:
  ResponseValue Run() {
    return SingleArgument(42);
  }
};

class MoreRadAsyncFunction : ExtensionFunction {
 public:
  ResponseValue Run() {
    DoAsyncStuff();
    return kDeferred;
  }
 private:
  void AsyncStuffIsDone() {
    Respond(MultipleArguments(9, 6));
  }
};

// ExtensionFunctionDispatcher code...
ExtensionFunction::ResponseValue value = func->Run();
if (value != ExtensionFunction::kDeferred)
  func->Respond(value);

Or to avoid the copy and test, Respond could freely ignore
kDeferred values. I guess there might be some weirdness if
someone thought it was ever OK to call Respond(kDeferred),
but, eh.

WDYT?

[not at google - send to devlin, 2014-05-01 15:38:00]

> Ok, the meditative calm of high-pitched dental machinery had me thinking.
> 
> What if Run() just returns a value, and there's a special static const value
> (e.g., kDeferred) defined in ExtensionFunction which means
> "I am not a real value, but I promise someone will call Respond() eventually."
> 
> Then there's a single, synchronous Respond() method used by async
> completions and ExtensionFunctionDispatcher alike. Example:
> 
> class RadSyncFunction : ExtensionFunction {
>  public:
>   ResponseValue Run() {
>     return SingleArgument(42);
>   }
> };
> 
> class MoreRadAsyncFunction : ExtensionFunction {
>  public:
>   ResponseValue Run() {
>     DoAsyncStuff();
>     return kDeferred;
>   }
>  private:
>   void AsyncStuffIsDone() {
>     Respond(MultipleArguments(9, 6));
>   }
> };
> 
> // ExtensionFunctionDispatcher code...
> ExtensionFunction::ResponseValue value = func->Run();
> if (value != ExtensionFunction::kDeferred)
>   func->Respond(value);
> 
> Or to avoid the copy and test, Respond could freely ignore
> kDeferred values. I guess there might be some weirdness if
> someone thought it was ever OK to call Respond(kDeferred),
> but, eh.
> 
> WDYT?

I like that with the current system callers are forced to choose between two
options, and the type system enforces that.

Riffing on your idea a bit though, we could get rid of RespondNow() (and
ResponseAction) and have RespondLater() return a ResponseValue, which...
magically is detected as meaning "respond later". Either way you'll still need
to wrap the Run() function in a second layer of utility, like
"RunAndMaybeRespond" or whatever, since it'd be unreasonable to expect direct
callers of Run() to know what kDeferred means. There are more than just
ExtensionFunctionDispatcher.

So it comes down a bit to how you value terseness over type safety. I like type
safety, as you might expect. I also like the way that the ResponseActions
actually define what their actions are (i.e. overload Execute()).

btw I realised that renaming "Execute" to any synonym of "Send" would be
misleading since RespondLater() doesn't actually Send anything.

And I thought of an issue with what I have -- EXTENSION_FUNCTION_VALIDATE -- it
would be nice to get rid of EXTENSION_FUNCTION_VALIDATE_TYPESAFE and go back to
just that single function. It looks like you can make C++ do that and I'm
consulting the expert :)

[Ken Rockot(use gerrit already), 2014-05-01 17:22:55]

On 2014/05/01 15:38:00, kalman wrote:
> > Ok, the meditative calm of high-pitched dental machinery had me thinking.
> > 
> > What if Run() just returns a value, and there's a special static const value
> > (e.g., kDeferred) defined in ExtensionFunction which means
> > "I am not a real value, but I promise someone will call Respond()
eventually."
> > 
> > Then there's a single, synchronous Respond() method used by async
> > completions and ExtensionFunctionDispatcher alike. Example:
> > 
> > class RadSyncFunction : ExtensionFunction {
> >  public:
> >   ResponseValue Run() {
> >     return SingleArgument(42);
> >   }
> > };
> > 
> > class MoreRadAsyncFunction : ExtensionFunction {
> >  public:
> >   ResponseValue Run() {
> >     DoAsyncStuff();
> >     return kDeferred;
> >   }
> >  private:
> >   void AsyncStuffIsDone() {
> >     Respond(MultipleArguments(9, 6));
> >   }
> > };
> > 
> > // ExtensionFunctionDispatcher code...
> > ExtensionFunction::ResponseValue value = func->Run();
> > if (value != ExtensionFunction::kDeferred)
> >   func->Respond(value);
> > 
> > Or to avoid the copy and test, Respond could freely ignore
> > kDeferred values. I guess there might be some weirdness if
> > someone thought it was ever OK to call Respond(kDeferred),
> > but, eh.
> > 
> > WDYT?
> 
> I like that with the current system callers are forced to choose between two
> options, and the type system enforces that.
> 
> Riffing on your idea a bit though, we could get rid of RespondNow() (and
> ResponseAction) and have RespondLater() return a ResponseValue, which...
> magically is detected as meaning "respond later". Either way you'll still need
> to wrap the Run() function in a second layer of utility, like
> "RunAndMaybeRespond" or whatever, since it'd be unreasonable to expect direct
> callers of Run() to know what kDeferred means. There are more than just
> ExtensionFunctionDispatcher.
> 
> So it comes down a bit to how you value terseness over type safety. I like
type
> safety, as you might expect. I also like the way that the ResponseActions
> actually define what their actions are (i.e. overload Execute()).
> 
> btw I realised that renaming "Execute" to any synonym of "Send" would be
> misleading since RespondLater() doesn't actually Send anything.
> 
> And I thought of an issue with what I have -- EXTENSION_FUNCTION_VALIDATE --
it
> would be nice to get rid of EXTENSION_FUNCTION_VALIDATE_TYPESAFE and go back
to
> just that single function. It looks like you can make C++ do that and I'm
> consulting the expert :)

I don't understand the type safety concern. Run must return a ResponseValue, so
the
choice between returning something now or deferring is still explicit and
enforced
by the compiler.

Unless there is cause to add other types of ResponseActions it seems like an
unnecessary layer of indirection. You're either returning a functor which
sends a response or a functor which does nothing. And to generate that functor
you have to call one of two methods (RespondNow() or RespondLater()), and return
their return value.

"return RespondNow(value);" seems less clear than "return value;"

and "return RespondLater();" seems less clear than "return DEFERRED;"

(I'm probably missing some part of the type safety argument)

As for the callers of Run(), what do they do with the result now? If everyone
calling Run() is immediately going to call Execute() on the result, why instead
ask them to immediately pass the result into Respond()? Respond() can silently
ignore the special deferred value.

[not at google - send to devlin, 2014-05-01 18:10:11]

On 2014/05/01 17:22:55, Ken Rockot wrote:
> On 2014/05/01 15:38:00, kalman wrote:
> > > Ok, the meditative calm of high-pitched dental machinery had me thinking.
> > > 
> > > What if Run() just returns a value, and there's a special static const
value
> > > (e.g., kDeferred) defined in ExtensionFunction which means
> > > "I am not a real value, but I promise someone will call Respond()
> eventually."
> > > 
> > > Then there's a single, synchronous Respond() method used by async
> > > completions and ExtensionFunctionDispatcher alike. Example:
> > > 
> > > class RadSyncFunction : ExtensionFunction {
> > >  public:
> > >   ResponseValue Run() {
> > >     return SingleArgument(42);
> > >   }
> > > };
> > > 
> > > class MoreRadAsyncFunction : ExtensionFunction {
> > >  public:
> > >   ResponseValue Run() {
> > >     DoAsyncStuff();
> > >     return kDeferred;
> > >   }
> > >  private:
> > >   void AsyncStuffIsDone() {
> > >     Respond(MultipleArguments(9, 6));
> > >   }
> > > };
> > > 
> > > // ExtensionFunctionDispatcher code...
> > > ExtensionFunction::ResponseValue value = func->Run();
> > > if (value != ExtensionFunction::kDeferred)
> > >   func->Respond(value);
> > > 
> > > Or to avoid the copy and test, Respond could freely ignore
> > > kDeferred values. I guess there might be some weirdness if
> > > someone thought it was ever OK to call Respond(kDeferred),
> > > but, eh.
> > > 
> > > WDYT?
> > 
> > I like that with the current system callers are forced to choose between two
> > options, and the type system enforces that.
> > 
> > Riffing on your idea a bit though, we could get rid of RespondNow() (and
> > ResponseAction) and have RespondLater() return a ResponseValue, which...
> > magically is detected as meaning "respond later". Either way you'll still
need
> > to wrap the Run() function in a second layer of utility, like
> > "RunAndMaybeRespond" or whatever, since it'd be unreasonable to expect
direct
> > callers of Run() to know what kDeferred means. There are more than just
> > ExtensionFunctionDispatcher.
> > 
> > So it comes down a bit to how you value terseness over type safety. I like
> type
> > safety, as you might expect. I also like the way that the ResponseActions
> > actually define what their actions are (i.e. overload Execute()).
> > 
> > btw I realised that renaming "Execute" to any synonym of "Send" would be
> > misleading since RespondLater() doesn't actually Send anything.
> > 
> > And I thought of an issue with what I have -- EXTENSION_FUNCTION_VALIDATE --
> it
> > would be nice to get rid of EXTENSION_FUNCTION_VALIDATE_TYPESAFE and go back
> to
> > just that single function. It looks like you can make C++ do that and I'm
> > consulting the expert :)
> 
> I don't understand the type safety concern. Run must return a ResponseValue,
so
> the
> choice between returning something now or deferring is still explicit and
> enforced
> by the compiler.
> 
> Unless there is cause to add other types of ResponseActions it seems like an
> unnecessary layer of indirection. You're either returning a functor which
> sends a response or a functor which does nothing. And to generate that functor
> you have to call one of two methods (RespondNow() or RespondLater()), and
return
> their return value.
> 
> "return RespondNow(value);" seems less clear than "return value;"
> 
> and "return RespondLater();" seems less clear than "return DEFERRED;"
> 
> (I'm probably missing some part of the type safety argument)

I did experiment a bit with this way actually. It's type safe in the sense that
we don't have true/false meaning different things, and being meaningless
besides.

What I found odd was that returning all but one value from a function implies
that you reply now, with a single special value meaning to not reply now. It
means the caller needs to read more documentation, and it's subtle that changing
just the value of a type entirely changes the meaning and behaviour of the
function. I think I'm catering to my own pain in doing code reviews and having
to dig up what the return values from RunImpl mean again. Another manifestation
of the oddness is as you say -- a Respond() function could take that
RespondLater/Deferred value, which is nonsensical.

As my first year lecturer used to say -- avoid mixing data and control. He was a
very wise man. Enums are almost an exception to this, but they're not really
data.

I certainly concede it's cleaner not to need that extra wrapping. Maybe that is
a strong enough reason.

> 
> As for the callers of Run(), what do they do with the result now? If everyone
> calling Run() is immediately going to call Execute() on the result, why
instead
> ask them to immediately pass the result into Respond()? Respond() can silently
> ignore the special deferred value.

I think that function->Run()->Execute() is slightly better than
function->Respond(function->Run()) because we can enforce Execute gets called
with WARN_UNUSED_RESULT. Type safety!

[not at google - send to devlin, 2014-05-02 23:04:40]

as discussed, just going with what I already have, though I renamed "Done" to
"Respond" since it's more consistent.

[not at google - send to devlin, 2014-05-02 23:04:52]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-05-02 23:05:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/257333002/60001

[commit-bot: I haz the power, 2014-05-03 00:01:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-03 00:01:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[not at google - send to devlin, 2014-05-03 01:48:24]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-05-03 01:48:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/257333002/90001

[commit-bot: I haz the power, 2014-05-03 03:41:53]

Change committed as 268033