Adds the ability for MessageLoop to take a MessagePump

Adds the ability for MessageLoop to take a MessagePump

Using default args is against style guide, but there are a ton of
places (mostly tests) that define a MessageLoop as a member. I'll see
about a cleanup pass that removes the default args.

BUG=none
TEST=NONE
R=darin@chromium.org

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

[darin (slow to review), 2013-11-07 00:17:54]

https://codereview.chromium.org/61643006/diff/1/base/message_loop/message_loop.h
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/61643006/diff/1/base/message_loop/message_loo...
base/message_loop/message_loop.h:149: MessageLoop(Type type = TYPE_DEFAULT,
MessagePump* message_pump = NULL);
Can we use scoped_ptr<MessagePump> here? Is it hard to express a null default
value in that case?

Another option would be to just override the constructor entirely.  I suspect
the Type field is not meaningful when initializing a MessageLoop with a custom
MessagePump. Internally, we should probably have a MessageLoop type value of
TYPE_CUSTOM.

https://codereview.chromium.org/61643006/diff/1/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/61643006/diff/1/base/threading/thread.h#newco...
base/threading/thread.h:50: MessagePump* message_pump;
Perhaps it would be nice to use scoped_ptr here as well?

[sky, 2013-11-07 01:06:38]

Updated to add TYPE_CUSTOM. I can't use a scoped_ptr in Thread::Options because
of how we use the struct. I likely could make it work by having StartWithOptions
take a Options*. Let me know if you feel strongly.

Sadrul, I'm adding you to see if this addresses your needs. I'm not sure if you
need an earlier patch that lets you specify the type and MessagePump.

I plan on doing more MessageLoop changes later on, but this is the start.

https://codereview.chromium.org/61643006/diff/70001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/61643006/diff/70001/base/threading/thread.h#n...
base/threading/thread.h:50: MessagePump* message_pump;
This can't be a scoped_ptr because of how we pass things around. Nor would it
make sense if I start copying the struct.

[darin (slow to review), 2013-11-07 02:27:21]

How about storing a Callback<MessagePump*()> instead, so we can defer
construction of the pump to the thread on which it will live? That may be
required by some pumps, or it may help keep pumps simpler.
On Nov 6, 2013 5:06 PM, <sky@chromium.org> wrote:

> Updated to add TYPE_CUSTOM. I can't use a scoped_ptr in Thread::Options
> because
> of how we use the struct. I likely could make it work by having
> StartWithOptions
> take a Options*. Let me know if you feel strongly.
>
> Sadrul, I'm adding you to see if this addresses your needs. I'm not sure
> if you
> need an earlier patch that lets you specify the type and MessagePump.
>
> I plan on doing more MessageLoop changes later on, but this is the start.
>
>
> https://codereview.chromium.org/61643006/diff/70001/base/
> threading/thread.h
> File base/threading/thread.h (right):
>
> https://codereview.chromium.org/61643006/diff/70001/base/
> threading/thread.h#newcode50
> base/threading/thread.h:50: MessagePump* message_pump;
> This can't be a scoped_ptr because of how we pass things around. Nor
> would it make sense if I start copying the struct.
>
> https://codereview.chromium.org/61643006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-11-07 16:35:29]

Updated to take callback for creating MessagePump.

[darin (slow to review), 2013-11-07 21:03:37]

LGTM

https://codereview.chromium.org/61643006/diff/110006/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/61643006/diff/110006/base/threading/thread.h#...
base/threading/thread.h:51: MessagePumpFactory message_pump_factory;
You might mention that message_pump_factory overrides message_loop_type.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/61643006/250001

[commit-bot: I haz the power, 2013-11-07 21:34:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-07 21:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/61643006/510001

[commit-bot: I haz the power, 2013-11-07 22:16:08]

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=win_rel&nu...

[commit-bot: I haz the power, 2013-11-07 22:30:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/61643006/690003

[commit-bot: I haz the power, 2013-11-07 22:54:44]

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=linux_clan...

[commit-bot: I haz the power, 2013-11-07 23:57:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/61643006/690003

[commit-bot: I haz the power, 2013-11-08 06:16:57]

Change committed as 233800