Making a way to create thread with a Java Looper for Android

Making a way to create thread with a Java Looper for Android

We need to create a new message loop type for this as for
testing the Android UI message pump type is not the standard Java, but gets overridden to a different one that can handle nested message loops.

Using the new Java thread for the java bridge thread, so the thread used for AJI callbacks will have a prepared Looper.

BUG=b/8680913
TBR=jochen@chromium.org

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

[benm (inactive), 2013-07-10 16:50:05]

https://codereview.chromium.org/18584006/diff/5002/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/5002/base/android/java/src/org/...
base/android/java/src/org/chromium/base/JavaThread.java:23: return new
JavaThread(name);
What is the lifetime of the thread? Will it loop forever once it's created?

https://codereview.chromium.org/18584006/diff/5002/base/android/java/src/org/...
base/android/java/src/org/chromium/base/JavaThread.java:32: public boolean
handleMessage(Message msg) {
nit: @Override

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.cc
File base/android/java_thread.cc (right):

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.c...
base/android/java_thread.cc:38: delete(waitable_event_);
nit: no parens

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.c...
base/android/java_thread.cc:39: waitable_event_ = NULL;
maybe leave it non null and dcheck == null at the top?

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.c...
base/android/java_thread.cc:42: void JavaThread::Stop() {
Do we need a way for java to call this so we can clean up? (i.e. delete this so
we can release java_thread_)

https://codereview.chromium.org/18584006/diff/5002/content/browser/renderer_h...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/18584006/diff/5002/content/browser/renderer_h...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:9: #endif
I think conditional includes go at the bottom in a separate section.

[Kristian Monsen, 2013-07-10 17:50:19]

https://codereview.chromium.org/18584006/diff/5002/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/5002/base/android/java/src/org/...
base/android/java/src/org/chromium/base/JavaThread.java:23: return new
JavaThread(name);
On 2013/07/10 16:50:06, benm wrote:
> What is the lifetime of the thread? Will it loop forever once it's created?

Currently it lives forever since that is what is needed for the JavaBridge. I am
not sure if there are other potential users.

I remember joth had some comments around this.

https://codereview.chromium.org/18584006/diff/5002/base/android/java/src/org/...
base/android/java/src/org/chromium/base/JavaThread.java:32: public boolean
handleMessage(Message msg) {
On 2013/07/10 16:50:06, benm wrote:
> nit: @Override

Done.

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.cc
File base/android/java_thread.cc (right):

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.c...
base/android/java_thread.cc:38: delete(waitable_event_);
On 2013/07/10 16:50:06, benm wrote:
> nit: no parens

Done.

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.c...
base/android/java_thread.cc:39: waitable_event_ = NULL;
On 2013/07/10 16:50:06, benm wrote:
> maybe leave it non null and dcheck == null at the top?

I don't like to leave deleted variables around as not null. It would maybe be
better to have it as a scoped ptr and pass it through Java.

I guess that doesn't solve what you want to fix, which is that start is only
called once? If that is what you want I would prefer to use a bool for it.

https://codereview.chromium.org/18584006/diff/5002/base/android/java_thread.c...
base/android/java_thread.cc:42: void JavaThread::Stop() {
On 2013/07/10 16:50:06, benm wrote:
> Do we need a way for java to call this so we can clean up? (i.e. delete this
so
> we can release java_thread_)

I think it is more the other way round. Currently this is initated and owned
from native. Stop could be called to terminate the Java Thread cleanly.

https://codereview.chromium.org/18584006/diff/5002/content/browser/renderer_h...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/18584006/diff/5002/content/browser/renderer_h...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:9: #endif
On 2013/07/10 16:50:06, benm wrote:
> I think conditional includes go at the bottom in a separate section.

You are right, did not know this :-) Makes the logic a bit easier to see.

Done.

[joth, 2013-07-10 20:33:48]

nice!

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:13: class JavaThread
implements Handler.Callback {
comment? It's mainly an implementation detail of the corresponding native class.

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:29: new
Handler(mThread.getLooper(), this).sendEmptyMessage(nativeThread);
rather than have the outer class subclass Handler.Callback, why not just do
new Handler(mThread.getLooper()).post(new Runnable() {
  @Override
  public void run() {
    nativeInitializeThread(nativeThread);
  }
}

- you're allocating garbage either way, so may as well at least make it easy to
read :)

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread.cc
File base/android/java_thread.cc (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:19: JavaThread::JavaThread(const char* name) :
started_(false) {
unindent

: started_(false) on its own line (unless entire c'tor body fits on one line...)

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:21: DCHECK(env);
spurious

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:28: // Check the thread has not already been started
nit: end sentence in . on all the comments

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:32: DCHECK(env);
ditto

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:34: waitable_event_ = new base::WaitableEvent(false,
false);
cleaner as:

base::WaitableEvent event(false, false);
base::AutoReset(&waitable_event_, &event);

could give waitable_event_ a more useful name :)  initialize_completed_event_ ?

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:35: Java_JavaThread_start(env, java_thread_.obj(),
reinterpret_cast<jint>(this));
slight tempted to pass &event through as an additional param here, and have it
passed back as a param into InitializeThread(), and then you can avoid the
pointer hanging around as a member for the rest of the time

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:52: MessageLoopForUI::current()->Start();
message_loop_->Start()  ? (/shrug)

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread.h
File base/android/java_thread.h (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.h:20: class JavaThread {
comment. I guess this is where the primary API docs of sorts will go.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.h:29: // Called from java
.. on the inner thread when it has started. Execution blocks the parent thread's
call to Start()

https://codereview.chromium.org/18584006/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/18584006/diff/11001/content/browser/renderer_...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:19: #ifdef
ANDROID
#if defined(OS_ANDROID)

although this entire file is only built on android anyway, so I'd just skip
using ifdef here.
(you could rename the /java/ folder to android if you really want, and greatly
simplify the .gypi. Separate patch if you want to do that though).

https://codereview.chromium.org/18584006/diff/11001/content/browser/renderer_...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:29: #define
THREAD base::Thread
typedef is nicer (but I'd not use the ifdef at all, as per comment above. this
else cases are dead code)

[Kristian Monsen, 2013-07-16 21:54:41]

Addressed comments, PTAL.

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:13: class JavaThread
implements Handler.Callback {
On 2013/07/10 20:33:48, joth wrote:
> comment? It's mainly an implementation detail of the corresponding native
class.

See below.

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:29: new
Handler(mThread.getLooper(), this).sendEmptyMessage(nativeThread);
On 2013/07/10 20:33:48, joth wrote:
> rather than have the outer class subclass Handler.Callback, why not just do
> new Handler(mThread.getLooper()).post(new Runnable() {
>   @Override
>   public void run() {
>     nativeInitializeThread(nativeThread);
>   }
> }
> 
> - you're allocating garbage either way, so may as well at least make it easy
to
> read :)

Done. Removed the other parts that are no longer needed.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread.cc
File base/android/java_thread.cc (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:19: JavaThread::JavaThread(const char* name) :
started_(false) {
On 2013/07/10 20:33:48, joth wrote:
> unindent
> 
> : started_(false) on its own line (unless entire c'tor body fits on one
line...)

Done.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:21: DCHECK(env);
On 2013/07/10 20:33:48, joth wrote:
> spurious

Done.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:28: // Check the thread has not already been started
On 2013/07/10 20:33:48, joth wrote:
> nit: end sentence in . on all the comments

Done.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:32: DCHECK(env);
On 2013/07/10 20:33:48, joth wrote:
> ditto

Done.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:34: waitable_event_ = new base::WaitableEvent(false,
false);
On 2013/07/10 20:33:48, joth wrote:
> cleaner as:
> 
> base::WaitableEvent event(false, false);
> base::AutoReset(&waitable_event_, &event);
> 
> could give waitable_event_ a more useful name :)  initialize_completed_event_
?

I made it a stack variable, see what you think?

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:35: Java_JavaThread_start(env, java_thread_.obj(),
reinterpret_cast<jint>(this));
On 2013/07/10 20:33:48, joth wrote:
> slight tempted to pass &event through as an additional param here, and have it
> passed back as a param into InitializeThread(), and then you can avoid the
> pointer hanging around as a member for the rest of the time

Done.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.cc:52: MessageLoopForUI::current()->Start();
On 2013/07/10 20:33:48, joth wrote:
> message_loop_->Start()  ? (/shrug)

Sadly not possible, Start() is an #ifdef ANDROID in MeesageLoopForUI.

Actually, I want to remove that #ifdef and just call start in the CTOR. I don't
think it is needed. But that is another CL.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread.h
File base/android/java_thread.h (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.h:20: class JavaThread {
On 2013/07/10 20:33:48, joth wrote:
> comment. I guess this is where the primary API docs of sorts will go.

I made a start, See what you think.

https://codereview.chromium.org/18584006/diff/11001/base/android/java_thread....
base/android/java_thread.h:29: // Called from java
On 2013/07/10 20:33:48, joth wrote:
> .. on the inner thread when it has started. Execution blocks the parent
thread's
> call to Start()

Done.

https://codereview.chromium.org/18584006/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/18584006/diff/11001/content/browser/renderer_...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:19: #ifdef
ANDROID
On 2013/07/10 20:33:48, joth wrote:
> #if defined(OS_ANDROID)
> 
> although this entire file is only built on android anyway, so I'd just skip
> using ifdef here.
> (you could rename the /java/ folder to android if you really want, and greatly
> simplify the .gypi. Separate patch if you want to do that though).

Will remove the #ifdef after renaming the folder to android.

https://codereview.chromium.org/18584006/diff/11001/content/browser/renderer_...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:29: #define
THREAD base::Thread
On 2013/07/10 20:33:48, joth wrote:
> typedef is nicer (but I'd not use the ifdef at all, as per comment above. this
> else cases are dead code)

Done.

[benm (inactive), 2013-07-17 10:52:28]

lgtm

looks good, just some nits from me!

Thanks Kristian!

https://chromiumcodereview.appspot.com/18584006/diff/19001/base/android/java/...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://chromiumcodereview.appspot.com/18584006/diff/19001/base/android/java/...
base/android/java/src/org/chromium/base/JavaThread.java:13: class JavaThread {
Naming nit: should this be AndroidJavaThread? Or HandlerJavaThread?

Reason being - we're doing more than just creating a java.lang.Thread.

https://chromiumcodereview.appspot.com/18584006/diff/19001/base/android/java_...
File base/android/java_thread.cc (right):

https://chromiumcodereview.appspot.com/18584006/diff/19001/base/android/java_...
base/android/java_thread.cc:41: // Delete object only used in initialization.
nit, don't think this comment makes sense anymore?

https://chromiumcodereview.appspot.com/18584006/diff/19001/content/browser/re...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/18584006/diff/19001/content/browser/re...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:20: #include
"base/android/java_thread.h"
nit, maybe a comment here to say that we require a HandlerThread for backwards
compatibility?

[joth, 2013-07-17 16:29:18]

lgtm - just some nits to add to ben's

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:13: class JavaThread
implements Handler.Callback {
On 2013/07/16 21:54:41, Kristian Monsen wrote:
> On 2013/07/10 20:33:48, joth wrote:
> > comment? It's mainly an implementation detail of the corresponding native
> class.
> 
> See below.

Cool.
I was actually meaning add a class comment
/*
  This class is an internal detail of the native counterpart. It is instantiated
and owned by the native object.
*.

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread.cc
File base/android/java_thread.cc (right):

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.cc:51: MessageLoopForUI::current()->Start();
So if message_loop_ was of type MessageLoopForUI you could then do
message_loop_->Start()  ?

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread.h
File base/android/java_thread.h (right):

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.h:36: void InitializeThread(JNIEnv* env, jobject obj,
jint event);
guess this can be private

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.h:48: 
uber nit: no blank line here (nor in the .cc, nor in the open-namespace when
there's nothing in between them)

[Kristian Monsen, 2013-07-17 21:02:37]

Addressed comments. PTAL.

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/11001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:13: class JavaThread
implements Handler.Callback {
On 2013/07/17 16:29:18, joth wrote:
> On 2013/07/16 21:54:41, Kristian Monsen wrote:
> > On 2013/07/10 20:33:48, joth wrote:
> > > comment? It's mainly an implementation detail of the corresponding native
> > class.
> > 
> > See below.
> 
> Cool.
> I was actually meaning add a class comment
> /*
>   This class is an internal detail of the native counterpart. It is
instantiated
> and owned by the native object.
> *.

Done.

https://codereview.chromium.org/18584006/diff/19001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/19001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:13: class JavaThread {
On 2013/07/17 10:52:29, benm wrote:
> Naming nit: should this be AndroidJavaThread? Or HandlerJavaThread?
> 
> Reason being - we're doing more than just creating a java.lang.Thread.

I don't like Android being part of the name, it is already in an android folder.
I'm changing it to JavaHandlerThread, speak up if you prefer something else :-)

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread.cc
File base/android/java_thread.cc (right):

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.cc:41: // Delete object only used in initialization.
On 2013/07/17 10:52:29, benm wrote:
> nit, don't think this comment makes sense anymore?

Done.

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.cc:51: MessageLoopForUI::current()->Start();
On 2013/07/17 16:29:18, joth wrote:
> So if message_loop_ was of type MessageLoopForUI you could then do
> message_loop_->Start()  ?

Yeah ...
But we call the MessageLoop CTOR, and clank does it the same way:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Sorry if I am not super responsive to the suggestions, and do agree it is not
super great as is, but I think it is better than casting it and changing the
type to MesageLoopUI. I would mostly like to just remove that Start method.

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread.h
File base/android/java_thread.h (right):

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.h:36: void InitializeThread(JNIEnv* env, jobject obj,
jint event);
On 2013/07/17 16:29:18, joth wrote:
> guess this can be private

Done.

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.h:48: 
On 2013/07/17 16:29:18, joth wrote:
> uber nit: no blank line here (nor in the .cc, nor in the open-namespace when
> there's nothing in between them)

Done. You mean in general for closing two namespaces?

https://codereview.chromium.org/18584006/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/18584006/diff/19001/content/browser/renderer_...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:20: #include
"base/android/java_thread.h"
On 2013/07/17 10:52:29, benm wrote:
> nit, maybe a comment here to say that we require a HandlerThread for backwards
> compatibility?

Sure, will add a comment. I don't think it is just backwards compatibility, as
long as we send the callback to a third-party app.

[benm (inactive), 2013-07-17 21:07:57]

https://codereview.chromium.org/18584006/diff/19001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/JavaThread.java (right):

https://codereview.chromium.org/18584006/diff/19001/base/android/java/src/org...
base/android/java/src/org/chromium/base/JavaThread.java:13: class JavaThread {
Perfect!

[benm (inactive), 2013-07-17 21:09:45]

lgtm

https://codereview.chromium.org/18584006/diff/32001/base/android/java_handler...
File base/android/java_handler_thread.h (right):

https://codereview.chromium.org/18584006/diff/32001/base/android/java_handler...
base/android/java_handler_thread.h:21: // to the message loop and they will be
scheduled along Java tasks
nit: alongside? along with?

[Kristian Monsen, 2013-07-17 21:13:27]

https://codereview.chromium.org/18584006/diff/32001/base/android/java_handler...
File base/android/java_handler_thread.h (right):

https://codereview.chromium.org/18584006/diff/32001/base/android/java_handler...
base/android/java_handler_thread.h:21: // to the message loop and they will be
scheduled along Java tasks
On 2013/07/17 21:09:45, benm wrote:
> nit: alongside? along with?

Done.

[joth, 2013-07-17 21:16:19]

https://codereview.chromium.org/18584006/diff/32001/base/android/java_handler...
File base/android/java_handler_thread.cc (right):

https://codereview.chromium.org/18584006/diff/32001/base/android/java_handler...
base/android/java_handler_thread.cc:29: DCHECK(!started_);
nit: the comment is kind of spurious :)

nit: we'd probably be just as well off doing DCHECK(!message_loop_); and avoid
the need for the extra bool (that is never used in a release build).

https://codereview.chromium.org/18584006/diff/32001/content/browser/renderer_...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/18584006/diff/32001/content/browser/renderer_...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:22: #include
"base/threading/thread.h"
I don't see what purpose this is serving - I don't think it's possible that you
compiled these #else lines of code?

At most, just say:-
#if !defined(OS_ANDROID)
#error "JavaBridge currently only supports OS_ANDROID"
#endif

[Kristian Monsen, 2013-07-17 21:54:40]

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread.h
File base/android/java_thread.h (right):

https://codereview.chromium.org/18584006/diff/19001/base/android/java_thread....
base/android/java_thread.h:36: void InitializeThread(JNIEnv* env, jobject obj,
jint event);
On 2013/07/17 21:02:37, Kristian Monsen wrote:
> On 2013/07/17 16:29:18, joth wrote:
> > guess this can be private
> 
> Done.

Actually it cannot be private since it is called by the generated JNI file for
convenience.

https://codereview.chromium.org/18584006/diff/32001/content/browser/renderer_...
File content/browser/renderer_host/java/java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/18584006/diff/32001/content/browser/renderer_...
content/browser/renderer_host/java/java_bridge_dispatcher_host.cc:22: #include
"base/threading/thread.h"
On 2013/07/17 21:16:19, joth wrote:
> I don't see what purpose this is serving - I don't think it's possible that
you
> compiled these #else lines of code?
> 
> At most, just say:-
> #if !defined(OS_ANDROID)
> #error "JavaBridge currently only supports OS_ANDROID"
> #endif

True, updated.

[joth, 2013-07-22 17:46:31]

lgtm

[Kristian Monsen, 2013-07-23 03:32:58]

I had to add thread restrictions as Start (which means WaitableEvent as well),
was called from the wrong threads in tests. This seems pretty verboten, so
wondering if I should solve it in a different way.

The one thing I can think of is to have a loop inside message_loop() and not
return until it is not-null. I think that is the only place it is observable
from outside the class if the thread is running or not.

What do you think?

[joth, 2013-07-23 04:59:04]

This usage of ScopedAllowWait seems totally legit:
- it's exactly why all the other Thread classes are also white-listed as
permitted
- when the child thread is first create you know for sure it is lively and
will service the init message immediately without compound blocking. so
waiting is no worse than if we'd done that init job on the parent thread
anyway. (modulo thread creation cost)

We could work around this by smashing this functionality into
PlatformThread under ifdef OS_ANDROID, but I think that'll cause more
ambiguity than it solves




On 22 July 2013 20:32, <kristianm@chromium.org> wrote:

> I had to add thread restrictions as Start (which means WaitableEvent as
> well),
> was called from the wrong threads in tests. This seems pretty verboten, so
> wondering if I should solve it in a different way.
>
> The one thing I can think of is to have a loop inside message_loop() and
> not
> return until it is not-null. I think that is the only place it is
> observable
> from outside the class if the thread is running or not.
>
> What do you think?
>
>
https://codereview.chromium.**org/18584006/<https://codereview.chromium.org/1...
>

[Kristian Monsen, 2013-07-24 18:53:41]

Updated by adding a TYPE_JAVA message loop type to get around that tests create
a totally different TYPE_UI message loop type.

In other news it looks like the code review is totally broken?

[joth, 2013-07-24 21:59:30]

On 2013/07/24 18:53:41, Kristian Monsen wrote:
> Updated by adding a TYPE_JAVA message loop type to get around that tests
create
> a totally different TYPE_UI message loop type.
> 

sgtm

> In other news it looks like the code review is totally broken?

I think you were hit by this
https://code.google.com/p/rietveld/issues/detail?id=357

[Kristian Monsen, 2013-07-25 00:20:11]

Continued in https://codereview.chromium.org/20207002/ since I apparently broke
this code review.

[joth, 2013-07-25 04:19:50]

Ps16 lgtm

You'll need to upload again though to commit. (And need base owner approval)

[Kristian Monsen, 2013-07-25 07:19:11]

Need review from base owner.

[brettw, 2013-07-29 04:45:16]

Sorry I took so long on this, I had to find a moment to page everything in.

Previously we had a conversation about this where you were asking about changing
other threads' message loop types over. Do you anticipate doing this or will
this only be for new threads?

The question before was whether we were going to re-use TYPE_UI for the Java
thread thing. That sounded OK to me, but I was concerned since I didn't want to
see a bunch of ifdefs in setting threads up. What made you pick this approach?
It might be nice to document a little more of the background in the commit
message.

[Kristian Monsen, 2013-07-31 20:40:42]

On 2013/07/29 04:45:16, brettw wrote:
> Sorry I took so long on this, I had to find a moment to page everything in.
> 
> Previously we had a conversation about this where you were asking about
changing
> other threads' message loop types over. Do you anticipate doing this or will
> this only be for new threads?
> 

We are not anticipating changing changing more threads. It is possible we have
overlooked something, but I don't think so.

> The question before was whether we were going to re-use TYPE_UI for the Java
> thread thing. That sounded OK to me, but I was concerned since I didn't want
to
> see a bunch of ifdefs in setting threads up. What made you pick this approach?
> It might be nice to document a little more of the background in the commit
> message.

Sorry, I updated the commit message. It works to use the UI type when we run as
a browser/webview, but during testing there is an override in message_loop.cc
that uses a different message pump if you specify you want a message loop of
type UI. I did not know about this, and that would mean these threads would get
the wrong type of message loop during testing.

Note that the override for testing is needed as the UI message pump for Android
is not nestable, so cannot be used for testing. If you have a better solution
for this that would be great.

[brettw, 2013-08-06 21:43:47]

lgtm

https://codereview.chromium.org/18584006/diff/156001/base/message_loop/messag...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/18584006/diff/156001/base/message_loop/messag...
base/message_loop/message_loop.cc:187: #if defined(OS_ANDROID)
Can you add a comment here that the requirements are the same as for UI so we
can use that object, but we do this to avoid hitting the factory case in the
TYPE_UI handler above?

[commit-bot: I haz the power, 2013-08-07 20:39:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/18584006/165001

[commit-bot: I haz the power, 2013-08-07 20:53:18]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Kristian Monsen, 2013-08-07 21:20:09]

TBR for a compile fix in a unit test on Android due to API change.

[commit-bot: I haz the power, 2013-08-07 21:21:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/18584006/165001

[commit-bot: I haz the power, 2013-08-07 22:27:46]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-07 23:32:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/18584006/165001

[commit-bot: I haz the power, 2013-08-08 01:11:25]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-08 01:13:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/18584006/165001

[jochen (gone - plz use gerrit), 2013-08-08 01:15:34]

lgtm

[commit-bot: I haz the power, 2013-08-08 01:31:54]

Change committed as 216349