Change the GPU process to use a CFRunLoop

Change the GPU process to use a CFRunLoop

This is necessary for CAContext sharing (determined empirically, as
that API is undocumented).

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

[ccameron, 2014-05-12 20:06:50]

ptal (I'll add the OWNERS if it looks reasonable to you).

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
content/gpu/gpu_main.cc:136: #if defined(OS_WIN)
IMO the MessageLoop type enums are of limited utility, since many platforms
don't have a clear definition of what a UI loop is (see the breakdown in
https://code.google.com/p/chromium/issues/detail?id=312462#c51).

I didn't try to shoehorn another type into the enum, but rather, followed the
model in
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[Robert Sesek, 2014-05-12 20:11:43]

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
content/gpu/gpu_main.cc:136: #if defined(OS_WIN)
On 2014/05/12 20:06:50, ccameron1 wrote:
> IMO the MessageLoop type enums are of limited utility, since many platforms
> don't have a clear definition of what a UI loop is (see the breakdown in
> https://code.google.com/p/chromium/issues/detail?id=312462#c51).
> 
> I didn't try to shoehorn another type into the enum, but rather, followed the
> model in
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

They do have limited utility, but we're starting to encounter other main-thread
non-TYPE_UI MessageLoops, which is what this is. I agree this is the right
approach.

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
content/gpu/gpu_main.cc:150: // This is necessary for CoreAnimation layers
hosted in the GPU process to be
Do you know what specifically requires this? I'm assuming something installs a
CFRunLoopSource that needs to get serviced, likely because the library doesn't
use its own thread for this.

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
content/gpu/gpu_main.cc:152: // http://crbug.com/312462
nit: No need for this to be on its own line.

[ccameron, 2014-05-12 20:30:44]

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
content/gpu/gpu_main.cc:150: // This is necessary for CoreAnimation layers
hosted in the GPU process to be
On 2014/05/12 20:11:43, rsesek wrote:
> Do you know what specifically requires this? I'm assuming something installs a
> CFRunLoopSource that needs to get serviced, likely because the library doesn't
> use its own thread for this.

Not sure. I've verified in Chrome and in stand-alone applications. I tried
running an example application with breakpoints in CFRunLoopSourceCreate and
CFMachPortCreateRunLoopSource, and didn't hit anything.

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
content/gpu/gpu_main.cc:152: // http://crbug.com/312462
On 2014/05/12 20:11:43, rsesek wrote:
> nit: No need for this to be on its own line.

Done.

[Robert Sesek, 2014-05-12 22:11:50]

LGTM. The question is non-blocking but is more for my own curiosity/documenting
why we need this type of MessagePump.

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
content/gpu/gpu_main.cc:150: // This is necessary for CoreAnimation layers
hosted in the GPU process to be
On 2014/05/12 20:30:44, ccameron1 wrote:
> On 2014/05/12 20:11:43, rsesek wrote:
> > Do you know what specifically requires this? I'm assuming something installs
a
> > CFRunLoopSource that needs to get serviced, likely because the library
doesn't
> > use its own thread for this.
> 
> Not sure. I've verified in Chrome and in stand-alone applications. I tried
> running an example application with breakpoints in CFRunLoopSourceCreate and
> CFMachPortCreateRunLoopSource, and didn't hit anything.

Interesting. What about CFRunLoopAddSource?

[ccameron, 2014-05-13 21:03:17]

On 2014/05/12 22:11:50, rsesek wrote:
> LGTM. The question is non-blocking but is more for my own
curiosity/documenting
> why we need this type of MessagePump.
> 
> https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc
> File content/gpu/gpu_main.cc (right):
> 
>
https://codereview.chromium.org/279163005/diff/1/content/gpu/gpu_main.cc#newc...
> content/gpu/gpu_main.cc:150: // This is necessary for CoreAnimation layers
> hosted in the GPU process to be
> On 2014/05/12 20:30:44, ccameron1 wrote:
> > On 2014/05/12 20:11:43, rsesek wrote:
> > > Do you know what specifically requires this? I'm assuming something
installs
> a
> > > CFRunLoopSource that needs to get serviced, likely because the library
> doesn't
> > > use its own thread for this.
> > 
> > Not sure. I've verified in Chrome and in stand-alone applications. I tried
> > running an example application with breakpoints in CFRunLoopSourceCreate and
> > CFMachPortCreateRunLoopSource, and didn't hit anything.
> 
> Interesting. What about CFRunLoopAddSource?

Not that guy either.

[ccameron, 2014-05-13 21:07:25]

Adding brettw for OWNER stamp.

[brettw, 2014-05-13 22:36:19]

base lgtm with change

https://codereview.chromium.org/279163005/diff/20001/base/message_loop/messag...
File base/message_loop/message_pump_mac.h (right):

https://codereview.chromium.org/279163005/diff/20001/base/message_loop/messag...
base/message_loop/message_pump_mac.h:224: BASE_EXPORT MessagePumpCFRunLoop();
Usually if you want to export a class you write "class BASE_EXPORT
MessagePumpCFRunLoop" which I think is what you want here.EXporting one function
on a class with virtual methods might cause the same thing to happen, but it
seems odd to me.

[ccameron, 2014-05-13 23:24:07]

Thanks!

https://codereview.chromium.org/279163005/diff/20001/base/message_loop/messag...
File base/message_loop/message_pump_mac.h (right):

https://codereview.chromium.org/279163005/diff/20001/base/message_loop/messag...
base/message_loop/message_pump_mac.h:224: BASE_EXPORT MessagePumpCFRunLoop();
On 2014/05/13 22:36:19, brettw wrote:
> Usually if you want to export a class you write "class BASE_EXPORT
> MessagePumpCFRunLoop" which I think is what you want here.EXporting one
function
> on a class with virtual methods might cause the same thing to happen, but it
> seems odd to me.

That seemed to be the precedent for the file (which I thought odd, too). I've
change it to class level across the board.

[ccameron, 2014-05-13 23:24:21]

The CQ bit was checked by ccameron@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/279163005/40001

[commit-bot: I haz the power, 2014-05-14 02:59:03]

Change committed as 270309