Updating message pump to store java ptr in class

Updating message pump to store java ptr in class

This enables us to have more than one MessagePumpForUI on Android,
and gets rid of an unneeded global variable.

BUG=

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

[Kristian Monsen, 2013-06-13 08:12:53]

I think this is a better design, and allows more than one messagepumpforui in
Android.

[michaelbai, 2013-06-13 15:56:25]

The question is why we need more than one MessagePumpForUI ?

[Kristian Monsen, 2013-06-13 17:48:40]

On 2013/06/13 15:56:25, michaelbai wrote:
> The question is why we need more than one MessagePumpForUI ?

I need it so other threads can use the combined java/native messageloop. I want
to land the CL's separately so if there is a problem it is easier to isolate.

More importantly, I think the correct way to ensure that there is only one of an
object is through an understandable API, not by having one of the member
functions DCHECK on the second object.

[michaelbai, 2013-06-13 18:25:05]

On 2013/06/13 17:48:40, Kristian Monsen wrote:
> On 2013/06/13 15:56:25, michaelbai wrote:
> > The question is why we need more than one MessagePumpForUI ?
> 
> I need it so other threads can use the combined java/native messageloop. I
want
> to land the CL's separately so if there is a problem it is easier to isolate.
> 
> More importantly, I think the correct way to ensure that there is only one of
an
> object is through an understandable API, not by having one of the member
> functions DCHECK on the second object.

So, you will have a thread which has message loops in both Java and native
thread?

If so, I think the problem is the class named MessagePumpForUI, it's for UI
thread, you may
have a base class like JavaAndNativeCombinedMessagePump and subclass
MessagePumpForUI
from it. (BTW, I still prefer to use static variable to enforce the singleton)

And you might have another type of MessageLoop, like COMBINED, so you can create
the
message loop you need, but COMBINED MessageLoop is not valid for other platform,
hmm
not sure it is a good design.

You might talk to brettw@ about your requirement.

[Kristian Monsen, 2013-06-13 18:45:28]

On 2013/06/13 18:25:05, michaelbai wrote:
> On 2013/06/13 17:48:40, Kristian Monsen wrote:
> > On 2013/06/13 15:56:25, michaelbai wrote:
> > > The question is why we need more than one MessagePumpForUI ?
> > 
> > I need it so other threads can use the combined java/native messageloop. I
> want
> > to land the CL's separately so if there is a problem it is easier to
isolate.
> > 
> > More importantly, I think the correct way to ensure that there is only one
of
> an
> > object is through an understandable API, not by having one of the member
> > functions DCHECK on the second object.
> 
> So, you will have a thread which has message loops in both Java and native
> thread?
> 
> If so, I think the problem is the class named MessagePumpForUI, it's for UI
> thread, you may
> have a base class like JavaAndNativeCombinedMessagePump and subclass
> MessagePumpForUI
> from it. (BTW, I still prefer to use static variable to enforce the singleton)
> 
> And you might have another type of MessageLoop, like COMBINED, so you can
create
> the
> message loop you need, but COMBINED MessageLoop is not valid for other
platform,
> hmm
> not sure it is a good design.
> 
> You might talk to brettw@ about your requirement.

Message loop for UI means it can support UI events, not that is should be used
only by the UI thread.

 // TYPE_UI
 //   This type of ML also supports native UI events (e.g., Windows messages).
 //   See also MessageLoopForUI.

It would makes sense to use this for Java in Android instead of inventing new
classes.

This is however unrelated to this review. I don't understand why we want to use
a global variable when there is no need.

[michaelbai, 2013-06-13 18:58:44]

On 2013/06/13 18:45:28, Kristian Monsen wrote:
> On 2013/06/13 18:25:05, michaelbai wrote:
> > On 2013/06/13 17:48:40, Kristian Monsen wrote:
> > > On 2013/06/13 15:56:25, michaelbai wrote:
> > > > The question is why we need more than one MessagePumpForUI ?
> > > 
> > > I need it so other threads can use the combined java/native messageloop. I
> > want
> > > to land the CL's separately so if there is a problem it is easier to
> isolate.
> > > 
> > > More importantly, I think the correct way to ensure that there is only one
> of
> > an
> > > object is through an understandable API, not by having one of the member
> > > functions DCHECK on the second object.
> > 
> > So, you will have a thread which has message loops in both Java and native
> > thread?
> > 
> > If so, I think the problem is the class named MessagePumpForUI, it's for UI
> > thread, you may
> > have a base class like JavaAndNativeCombinedMessagePump and subclass
> > MessagePumpForUI
> > from it. (BTW, I still prefer to use static variable to enforce the
singleton)
> > 
> > And you might have another type of MessageLoop, like COMBINED, so you can
> create
> > the
> > message loop you need, but COMBINED MessageLoop is not valid for other
> platform,
> > hmm
> > not sure it is a good design.
> > 
> > You might talk to brettw@ about your requirement.
> 
> Message loop for UI means it can support UI events, not that is should be used
> only by the UI thread.
> 
>  // TYPE_UI
>  //   This type of ML also supports native UI events (e.g., Windows messages).
>  //   See also MessageLoopForUI.
> 
> It would makes sense to use this for Java in Android instead of inventing new
> classes.
> 
> This is however unrelated to this review. I don't understand why we want to
use
> a global variable when there is no need.




It's hard to review this without the context, Is your Java thread is associated
with UI?

The reason to use the static variable is the MessagePumpForUI is designed for UI
thread, there could only have one UI thread in a process, the static variable
enforce it. we didn't think there is a thread other than UI could have both Java
and Native message loop.

[joth, 2013-06-13 18:59:38]

On 2013/06/13 18:25:05, michaelbai wrote:
> On 2013/06/13 17:48:40, Kristian Monsen wrote:
> > On 2013/06/13 15:56:25, michaelbai wrote:
> > > The question is why we need more than one MessagePumpForUI ?
> > 
> > I need it so other threads can use the combined java/native messageloop. I
> want
> > to land the CL's separately so if there is a problem it is easier to
isolate.
> > 
> > More importantly, I think the correct way to ensure that there is only one
of
> an
> > object is through an understandable API, not by having one of the member
> > functions DCHECK on the second object.
> 
> So, you will have a thread which has message loops in both Java and native
> thread?
> 
> If so, I think the problem is the class named MessagePumpForUI, it's for UI
> thread, you may
> have a base class like JavaAndNativeCombinedMessagePump and subclass
> MessagePumpForUI
> from it. (BTW, I still prefer to use static variable to enforce the singleton)
> 
> And you might have another type of MessageLoop, like COMBINED, so you can
create
> the
> message loop you need, but COMBINED MessageLoop is not valid for other
platform,
> hmm
> not sure it is a good design.
> 
> You might talk to brettw@ about your requirement.

So it's unfortunate the "UI thread" is an ambiguous term, and in many contexts
there is an assumption (implicit or explicit) that there can only be one UI
thread per process (and often, it must be the process's main thread).

But I think Kristian has a good point here, specifically in the context of
base/message_loop.h :-

  // TYPE_UI
  //   This type of ML also supports native UI events (e.g., Windows messages).
  //   See also MessageLoopForUI.

On Android, a 'native UI event' can reasonably be defined a java Message sent to
the java-side looper, and on Android it *is* allowable to have multiple threads
be "UI" threads (in as much as, it's allowable for different view trees /
windows to live on different threads in the same process). So together, if we
want other threads to run combined native/java event loops, it is reasonable to
apply TYPE_UI to cover this rather than introduce a new concept in the cross
plat header that would be only meaningful to android anyway.

if we go this way, would be worth calling all this out in comments, either in
the TYPE_UI declaration, or at least in message_pump_android.h.

[michaelbai, 2013-06-13 20:06:25]

On 2013/06/13 18:59:38, joth wrote:
> On 2013/06/13 18:25:05, michaelbai wrote:
> > On 2013/06/13 17:48:40, Kristian Monsen wrote:
> > > On 2013/06/13 15:56:25, michaelbai wrote:
> > > > The question is why we need more than one MessagePumpForUI ?
> > > 
> > > I need it so other threads can use the combined java/native messageloop. I
> > want
> > > to land the CL's separately so if there is a problem it is easier to
> isolate.
> > > 
> > > More importantly, I think the correct way to ensure that there is only one
> of
> > an
> > > object is through an understandable API, not by having one of the member
> > > functions DCHECK on the second object.
> > 
> > So, you will have a thread which has message loops in both Java and native
> > thread?
> > 
> > If so, I think the problem is the class named MessagePumpForUI, it's for UI
> > thread, you may
> > have a base class like JavaAndNativeCombinedMessagePump and subclass
> > MessagePumpForUI
> > from it. (BTW, I still prefer to use static variable to enforce the
singleton)
> > 
> > And you might have another type of MessageLoop, like COMBINED, so you can
> create
> > the
> > message loop you need, but COMBINED MessageLoop is not valid for other
> platform,
> > hmm
> > not sure it is a good design.
> > 
> > You might talk to brettw@ about your requirement.
> 
> So it's unfortunate the "UI thread" is an ambiguous term, and in many contexts
> there is an assumption (implicit or explicit) that there can only be one UI
> thread per process (and often, it must be the process's main thread).
> 

Based on this assumption, I'd prefer to use something like Combined type of
message loop, instead of UI, as we couldn't find any existing thread other than
UI thread has UI type of MessageLoop; otherwise, it might introduce confusion.


> But I think Kristian has a good point here, specifically in the context of
> base/message_loop.h :-
> 
>   // TYPE_UI
>   //   This type of ML also supports native UI events (e.g., Windows
messages).
>   //   See also MessageLoopForUI.
> 
> On Android, a 'native UI event' can reasonably be defined a java Message sent
to
> the java-side looper, and on Android it *is* allowable to have multiple
threads
> be "UI" threads (in as much as, it's allowable for different view trees /
> windows to live on different threads in the same process). So together, if we
> want other threads to run combined native/java event loops, it is reasonable
to
> apply TYPE_UI to cover this rather than introduce a new concept in the cross
> plat header that would be only meaningful to android anyway.
> 
> if we go this way, would be worth calling all this out in comments, either in
> the TYPE_UI declaration, or at least in message_pump_android.h.

[michaelbai, 2013-06-13 20:21:33]

You might can find out there is a lot of places to use below statement to make
sure it is in UI thread,

base::MessageLoop::current()->type() == base::MessageLoop::TYPE_UI

This might be the reason we need anther type of MessageLoop.

[Kristian Monsen, 2013-06-13 21:14:39]

Added brettw from reviewer suggestions.

On 2013/06/13 20:21:33, michaelbai wrote:
> You might can find out there is a lot of places to use below statement to make
> sure it is in UI thread,
> 
> base::MessageLoop::current()->type() == base::MessageLoop::TYPE_UI
> 
> This might be the reason we need anther type of MessageLoop.

From my understanding that is just wrong code (but if it is widespread enough
that might be a moot point). And of course I could be just wrong here as I often
am :-) I agree this is an important point and will look more into it.

If you want to check you can do UI type events, check
base::MessageLoop::current()->type() == base::MessageLoop::TYPE_UI
If you want to check you are on the UI thread, check
BrowserThread::CurrentlyOn(BrowserThread::UI)

On the higher level, there is only one of each of the named BrowserThread's, on
a lower level there can be more of the threads with each type of message loops.
If not the API should be slightly redesigned I think.

I think if we want to make the MessageLoopForUI a singleton, we should do it for
all platforms, and make it clear in the design/API.

As for adding a new type of MessageLoop. That will be only for Android, and we
will use the same type for the UI message loop for code reuse, so we will
quickly get back to the same code with slightly different names.

[Kristian Monsen, 2013-06-13 21:40:40]

On 2013/06/13 21:14:39, Kristian Monsen wrote:
> Added brettw from reviewer suggestions.
> 
> On 2013/06/13 20:21:33, michaelbai wrote:
> > You might can find out there is a lot of places to use below statement to
make
> > sure it is in UI thread,
> > 
> > base::MessageLoop::current()->type() == base::MessageLoop::TYPE_UI
> > 
> > This might be the reason we need anther type of MessageLoop.
> 
> From my understanding that is just wrong code (but if it is widespread enough
> that might be a moot point). And of course I could be just wrong here as I
often
> am :-) I agree this is an important point and will look more into it.
I looked around, and this pattern is not much used. There is only 5 instances,
and I would be happy to look into and maybe update them if needed:
https://code.google.com/p/chromium/codesearch#search/&q=%22%20==%20base::Mess...

> 
> If you want to check you can do UI type events, check
> base::MessageLoop::current()->type() == base::MessageLoop::TYPE_UI
> If you want to check you are on the UI thread, check
> BrowserThread::CurrentlyOn(BrowserThread::UI)
> 
> On the higher level, there is only one of each of the named BrowserThread's,
on
> a lower level there can be more of the threads with each type of message
loops.
> If not the API should be slightly redesigned I think.
> 
> I think if we want to make the MessageLoopForUI a singleton, we should do it
for
> all platforms, and make it clear in the design/API.
> 
> As for adding a new type of MessageLoop. That will be only for Android, and we
> will use the same type for the UI message loop for code reuse, so we will
> quickly get back to the same code with slightly different names.

[Kristian Monsen, 2013-06-13 22:26:40]

On 2013/06/13 21:40:40, Kristian Monsen wrote:
> On 2013/06/13 21:14:39, Kristian Monsen wrote:
> > Added brettw from reviewer suggestions.
> > 
> > On 2013/06/13 20:21:33, michaelbai wrote:
> > > You might can find out there is a lot of places to use below statement to
> make
> > > sure it is in UI thread,
> > > 
> > > base::MessageLoop::current()->type() == base::MessageLoop::TYPE_UI
> > > 
> > > This might be the reason we need anther type of MessageLoop.
> > 
> > From my understanding that is just wrong code (but if it is widespread
enough
> > that might be a moot point). And of course I could be just wrong here as I
> often
> > am :-) I agree this is an important point and will look more into it.
> I looked around, and this pattern is not much used. There is only 5 instances,
> and I would be happy to look into and maybe update them if needed:
>
https://code.google.com/p/chromium/codesearch#search/&q=%2522%2520==%2520base...
> 

Also it seems like Windows often uses the UI messageloop concept outside the
main thread. See the see these two:
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...
https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_ma...

> > 
> > If you want to check you can do UI type events, check
> > base::MessageLoop::current()->type() == base::MessageLoop::TYPE_UI
> > If you want to check you are on the UI thread, check
> > BrowserThread::CurrentlyOn(BrowserThread::UI)
> > 
> > On the higher level, there is only one of each of the named BrowserThread's,
> on
> > a lower level there can be more of the threads with each type of message
> loops.
> > If not the API should be slightly redesigned I think.
> > 
> > I think if we want to make the MessageLoopForUI a singleton, we should do it
> for
> > all platforms, and make it clear in the design/API.
> > 
> > As for adding a new type of MessageLoop. That will be only for Android, and
we
> > will use the same type for the UI message loop for code reuse, so we will
> > quickly get back to the same code with slightly different names.

[Kristian Monsen, 2013-06-18 17:29:48]

ping?

[brettw, 2013-06-23 22:32:01]

base owners lgtm (although I'm not a good person to make Android design
decisions).

[Kristian Monsen, 2013-06-26 23:21:51]

On 2013/06/23 22:32:01, brettw wrote:
> base owners lgtm (although I'm not a good person to make Android design
> decisions).

Thank you Brett! Waiting for Michael and Joth to be back next week before
resuming discussions.

[joth, 2013-07-01 21:53:01]

lgtm

https://codereview.chromium.org/16926003/diff/1/base/message_pump_android.h
File base/message_pump_android.h (right):

https://codereview.chromium.org/16926003/diff/1/base/message_pump_android.h#n...
base/message_pump_android.h:40: private:
remove duplicate label

[commit-bot: I haz the power, 2013-07-02 16:06:05]

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

[Kristian Monsen, 2013-07-02 16:06:35]

On 2013/07/01 21:53:01, joth wrote:
> lgtm
> 
> https://codereview.chromium.org/16926003/diff/1/base/message_pump_android.h
> File base/message_pump_android.h (right):
> 
>
https://codereview.chromium.org/16926003/diff/1/base/message_pump_android.h#n...
> base/message_pump_android.h:40: private:
> remove duplicate label

Committing as I have other CL's depending on this. Michael: If you have any
further comments I will address them in a follow up CL.

[michaelbai, 2013-07-02 16:08:42]

I don't have issue about this, thanks.

[commit-bot: I haz the power, 2013-07-02 16:13:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-07-02 16:59:18]

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

[commit-bot: I haz the power, 2013-07-02 18:54:07]

Change committed as 209739