Add API definition and error values for running message loops.

Add API definition and error values for running message loops.

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

[dmichael (off chromium), 2011-12-29 19:41:21]

I think this is a really good first cut.

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:91: PP_Resource GetCurrent();
Does it have to be on the stack? I.e., if I've done 'AttachToCurrentThread' on
this thread, but I'm calling from the outer thread-function (not a pepper event
on this thread), what does this return?

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:147: int32_t Run([in] PP_Resource
message_loop);
I think we probably want to allow a timeout and/or max-event parameter, for
applications that want to do something periodically on the outer event loop for
a thread.

[brettw, 2011-12-29 20:07:39]

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:91: PP_Resource GetCurrent();
"Once you associate a message loop for a thread, you don't have to keep a
reference to it." So this would always return the attached message loop.

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:147: int32_t Run([in] PP_Resource
message_loop);
If you want to run the message loop for some time, you should PostWorkEx with a
timeout using a callback that exits. This seems sufficient for me. This API is
mostly just wrapping the Chrome MesasgeLoop, and we've never needed the feature
you're requesting.

[viettrungluu, 2011-12-30 16:52:19]

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:220: int32_t PostWorkEx([in] PP_Resource
message_loop,
Ugh. For the C API, I think this should be folded into PostWork().

[viettrungluu, 2011-12-30 16:58:35]

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:245: };
I think there should be a distinction between leaving ("quitting") the message
loop and destroying (or preparing to destroy) the message loop. To be consistent
with what happens on the main thread, we should probably call all callbacks with
PP_ERROR_ABORTED, regardless of whether the message loop is actually running.
(If it's not running, this would require an additional call from the plugin on
the message loop's thread.)

[yzshen1, 2011-12-30 21:08:24]

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:156: * If there are nested message loops,
this will only quit the outermost one.
Do you mean that I can create nested message loops by calling Run() with the
same |message_loop| resource for multiple times? (This sentence seems to suggest
so.)

And I can also create nested message loops using different message loop
resources, right? (Otherwise, we don't need the |message_loop| parameter.)

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:215: * only the outermost invocation of
the message loop's Run() function on the
I think 'outermost' on this line has different meaning from the ones on line
128, 156 and 233, isn't it? (Maybe I am confused. :))

[brettw, 2011-12-31 23:01:46]

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:156: * If there are nested message loops,
this will only quit the outermost one.
On 2011/12/30 21:08:24, yzshen1 wrote:
> Do you mean that I can create nested message loops by calling Run() with the
> same |message_loop| resource for multiple times? (This sentence seems to
suggest
> so.)

Yes.


> And I can also create nested message loops using different message loop
> resources, right? (Otherwise, we don't need the |message_loop| parameter.)

Technically not. Run says "The message loop identified by the argument must have
been previously successfully attached to the current thread." So the PP_Resource
parameters for Run and QuitNow are redundant. But I thought it was more clear to
have these since they're working "on the message loop object" for consistency.

[dmichael (off chromium), 2012-01-04 04:30:26]

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/1/ppapi/api/dev/ppb_message_loop_...
ppapi/api/dev/ppb_message_loop_dev.idl:147: int32_t Run([in] PP_Resource
message_loop);
On 2011/12/29 20:07:39, brettw wrote:
> If you want to run the message loop for some time, you should PostWorkEx with
a
> timeout using a callback that exits. This seems sufficient for me. This API is
> mostly just wrapping the Chrome MesasgeLoop, and we've never needed the
feature
> you're requesting.
In Chromium, we own the message loop and can add to it whatever we please (e.g.,
histogramming). PPAPI apps don't have the option of changing the behavior of the
loop itself. Posting a timed event that quits helps, but it's not as obvious,
and there's still not a way to only run N events before quitting.

But I can't think of any really important use case for a PPAPI app. Normally,
you'd want this capability so you could handle multiple event sources on one
thread, but in our case, I think we own all the event sources anyway. So I'm
sure we can live without. I just have a feeling some apps are going to want (not
need) this.

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:67: * long as the thread is running. The
current message loop can be retrieved
"as long as the thread is running" means as long as there's a message loop
"Run()" call on the stack? Or as long as the pthread has not exited?

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:71: * PPAPI calls will fail.
It makes me a little sad that we won't allow calls that don't require callbacks
(e.g. PostMessage, and all of PPB_Var). I can imagine cases where the
application will have to create a message loop but will never actually run it.
We could consider only disallowing calls that require a callback.

At a minimum, I think CallOnMainThread has to be an exception, since we support
it already off the main thread.

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:154: * est to PP_TRUE.
nit: est->set

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:239: * executed in order, as with
PostWork(). In error cases, the callback may
nit: don't want "as with PostWork()" anymore.

[brettw, 2012-01-05 18:34:38]

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:67: * long as the thread is running. The
current message loop can be retrieved
On 2012/01/04 04:30:26, dmichael wrote:
> "as long as the thread is running" means as long as there's a message loop
> "Run()" call on the stack? Or as long as the pthread has not exited?

The latter.

http://codereview.chromium.org/8965082/diff/11002/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:71: * PPAPI calls will fail.
There are two related concepts. The first is registering your thread with us so
we can associate tracking info with it, etc. The second is running a message
loop so we can give you callbacks.

You could envision that these could be separate, and I considered this. However,
I think it's potentially confusing and adds another wrinkle to completion
callback error checking: a type of thread that you can do blocking but not async
calls on. I'd rather avoid this complexity.

Probably we can allow the basic var/refcounting calls to work on all threads.
We'll have to see how the implementation goes.

[brettw, 2012-01-06 21:36:58]

This patch tries to simplify things for our initial effort. It removes support
for nested message loops, and removes QuitNow (the main use for this is in
nested contexts).

[brettw, 2012-01-06 23:35:02]

Please do a real review of this, I'd like to get it checked in as a first pass.

[brettw, 2012-01-18 05:09:54]

ping

[brettw, 2012-01-18 05:10:12]

dmichael: can you be primary on this?

[dmichael (off chromium), 2012-01-18 15:55:44]

I'll do it today. I've been putting it off, since you're supposedly not
working right now :-p

On Tue, Jan 17, 2012 at 10:10 PM, <brettw@chromium.org> wrote:

> dmichael: can you be primary on this?
>
>
http://codereview.chromium.**org/8965082/<http://codereview.chromium.org/8965...
>

[dmichael (off chromium), 2012-01-18 21:16:53]

Mostly nits. I like the idea of keeping it simple (i.e., no nesting) for the
first version, at least.

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:1: /* Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: 2012

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:16: * allows you to post work to threads
that will be issued on the message loop.
suggest: might read better rephrased:
"allows you to post work to the message loop for a thread."

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:42: *          return;
Would it be better to add a smidge more detail to the example?
if (status != PP_OK) {
  Cleanup();  // e.g., free(user_data)
  return;
}

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:45: * For a C++ example, see
ppapi/utility/threading/simple_thread.h
I don't see this in the CL; did you intend to add it?

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:66: * Once you associate a message loop
for a thread, you don't have to keep a
for->with?

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:72: * PPAPI calls will fail.
should we have a whitelist, or say "(unless the operation documentation
specifically says otherwise)"?
CallOnMainThread must be allowed, or we break most existing apps.

Others that might make sense:
- All of PPB_Var
- All of PPB_Core
- PPB_MessageLoop_Dev.GetForMainThread, PostWork, PostQuit

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:98: *  - You quit the message loop via
QuitNow or PostQuit with should_destroy
QuitNow is gone

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:143: * thread. If there is no attached
message loop, the return value will be 0.
Maybe we should define something like:
const PP_Resource PP_INVALID_RESOURCE = 0
in pp_resource.h :-/. We use kInvalidResource in various places; it would be
nice to explicitly document that 0 is always an invalid resource, to make
comments like this & code clearer. If you think that's a good idea, I can add it
to my queue...

(not for this CL, obviously)

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:187: *     success, the message loop will
only exit when you call PostQuit().
Should we have a code to distinguish "quit without destroy, probably should
Run() again" from "quit with destroy; it's time to exit the thread". Or are we
just assuming the thread will keep a "I'm really quitting" flag somewhere?

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:219: * currently on the stack executes.
delete prevent_nested comment

http://codereview.chromium.org/8965082/diff/22001/ppapi/cpp/dev/message_loop_...
File ppapi/cpp/dev/message_loop_dev.cc (right):

http://codereview.chromium.org/8965082/diff/22001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.cc:38: : Resource(pp_message_loop) {
it's a tradeoff, but you could check that pp_message_loop is a message loop, and
then either PP_NOTREACHED or set the resource to Null if it's not a message
loop. Would be nice to only do that in debug builds, though.

http://codereview.chromium.org/8965082/diff/22001/ppapi/cpp/dev/message_loop_...
File ppapi/cpp/dev/message_loop_dev.h (right):

http://codereview.chromium.org/8965082/diff/22001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.h:26: MessageLoop_Dev(PP_Resource
pp_message_loop);
Is it worth noting that the MessageLoop_Dev takes a reference to the
PP_Resource, which it releases on destruction?

http://codereview.chromium.org/8965082/diff/22001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.h:35: int32_t PostQuit(bool should_destroy);
you should adapt the comments from the C file, once you're happy with those.

[brettw, 2012-01-19 03:04:44]

PTAL

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
File ppapi/api/dev/ppb_message_loop_dev.idl (right):

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:45: * For a C++ example, see
ppapi/utility/threading/simple_thread.h
On 2012/01/18 21:16:53, dmichael wrote:
> I don't see this in the CL; did you intend to add it?

That's in my impl patch: http://codereview.chromium.org/9097006/

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:143: * thread. If there is no attached
message loop, the return value will be 0.
On 2012/01/18 21:16:53, dmichael wrote:
> Maybe we should define something like:
> const PP_Resource PP_INVALID_RESOURCE = 0
> in pp_resource.h :-/. We use kInvalidResource in various places; it would be
> nice to explicitly document that 0 is always an invalid resource, to make
> comments like this & code clearer. If you think that's a good idea, I can add
it
> to my queue...

IMO 0 is fine and makes perfect sense to me, and I opposed adding
kInvalidResource. I think this is only used in NaCl?

http://codereview.chromium.org/8965082/diff/22001/ppapi/api/dev/ppb_message_l...
ppapi/api/dev/ppb_message_loop_dev.idl:187: *     success, the message loop will
only exit when you call PostQuit().
The thread needs to keep a flag anyway.

http://codereview.chromium.org/8965082/diff/22001/ppapi/cpp/dev/message_loop_...
File ppapi/cpp/dev/message_loop_dev.cc (right):

http://codereview.chromium.org/8965082/diff/22001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.cc:38: : Resource(pp_message_loop) {
We have this problem for all resources. If we're going to do it, we should do it
for all of them at the same time.

[dmichael (off chromium), 2012-01-19 16:14:57]

lgtm

http://codereview.chromium.org/8965082/diff/30001/ppapi/cpp/dev/message_loop_...
File ppapi/cpp/dev/message_loop_dev.h (right):

http://codereview.chromium.org/8965082/diff/30001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.h:138: MessageLoop_Dev(Instance* instance);
explicit

http://codereview.chromium.org/8965082/diff/30001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.h:143: MessageLoop_Dev(PP_Resource
pp_message_loop);
explicit?

http://codereview.chromium.org/8965082/diff/30001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.h:199: /// @arg message_loop The message loop
resource.
doesn't apply for the C++ interface

http://codereview.chromium.org/8965082/diff/30001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.h:201: /// @arg callback A pointer to the
completion callback to execute from the
Not a pointer.

Also, I think we're using @param[in] elsewhere. I don't know if @arg works, but
we should probably be consistent.

http://codereview.chromium.org/8965082/diff/30001/ppapi/cpp/dev/message_loop_...
ppapi/cpp/dev/message_loop_dev.h:230: ///     queue. As described above, this
does not mean that the work has been or
nit: >80 characters