Call AppendGpuCommandLine for sync compositor

Insert in-process GPU switches on UI thread

For in-process gpu, the CommandLine of the current process is a shared
resource and cannot be safely manipulated on the IO thread.

This moves inserting the gpu command line switches to the UI thread, and
slightly start up. Although still need additional fixes to start up code
to ensure it's safe to manipulate it on UI thread.

This also fixes the bug of inserting gpu switches when GpuProcessHost is
not used, for example in android webview.

BUG=450396
Committed: https://crrev.com/6d695e6f5b99b8631e06788d2f9f9f9ed12c057e
Cr-Commit-Position: refs/heads/master@{#313177}

[boliu, 2015-01-22 01:29:54]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2015-01-22 01:29:55]

How does this look?

[boliu, 2015-01-23 18:10:21]

On 2015/01/22 01:29:55, boliu wrote:
> How does this look?

Ping! (will want to merge this)

[no sievers, 2015-01-23 20:21:50]

https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
base::CommandLine::ForCurrentProcess());
We shouldn't append the command line twice. 
Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try to add it?

We don't ever try to spin up the GPU channel with WebView, right?
You should also add something in browser_main_loop.cc so that it doesn't call 
BrowserGpuChannelHostFactory::Initialize(established_gpu_channel) if running
in-process, even after the TODO there gets addressed.

Can you then file another bug for how to sort this out when we use mixed
contexts?
It can probably be done by making context creation here atomic or so.

[boliu, 2015-01-23 20:30:29]

https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
base::CommandLine::ForCurrentProcess());
On 2015/01/23 20:21:50, sievers wrote:
> We shouldn't append the command line twice. 
> Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
> DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try to add it?

I did think about that.

One is the factory here is a singleton already, so this should only be called
once.

And even if some other code calls AppendGpuCommandLine, command line args is
actually a hash map, so technically "doesn't hurt", except run time cost.

> 
> We don't ever try to spin up the GPU channel with WebView, right?

Doesn't matter if we do or not. We used to just do it in webview (before this
broke) and it didn't hurt.

So I'm inclined to leave browser_main_loop alone in this CL. Make it easier to
merge etc.

WYDT?

> You should also add something in browser_main_loop.cc so that it doesn't call 
> BrowserGpuChannelHostFactory::Initialize(established_gpu_channel) if running
> in-process, even after the TODO there gets addressed.
> 
> Can you then file another bug for how to sort this out when we use mixed
> contexts?
> It can probably be done by making context creation here atomic or so.

I don't quite understand what you mean by "atomic context creation".

[boliu, 2015-01-23 20:36:24]

https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
base::CommandLine::ForCurrentProcess());
On 2015/01/23 20:30:29, boliu wrote:
> On 2015/01/23 20:21:50, sievers wrote:
> > We shouldn't append the command line twice. 
> > Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
> > DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try to add
it?

Actually DCHECK isn't that bad. At least will notify us if something else ends
up calling AppendGpuCommandLine on initialization.

Added that in PS2

> 
> I did think about that.
> 
> One is the factory here is a singleton already, so this should only be called
> once.
> 
> And even if some other code calls AppendGpuCommandLine, command line args is
> actually a hash map, so technically "doesn't hurt", except run time cost.
> 
> > 
> > We don't ever try to spin up the GPU channel with WebView, right?
> 
> Doesn't matter if we do or not. We used to just do it in webview (before this
> broke) and it didn't hurt.
> 
> So I'm inclined to leave browser_main_loop alone in this CL. Make it easier to
> merge etc.
> 
> WYDT?
> 
> > You should also add something in browser_main_loop.cc so that it doesn't
call 
> > BrowserGpuChannelHostFactory::Initialize(established_gpu_channel) if running
> > in-process, even after the TODO there gets addressed.
> > 
> > Can you then file another bug for how to sort this out when we use mixed
> > contexts?
> > It can probably be done by making context creation here atomic or so.
> 
> I don't quite understand what you mean by "atomic context creation".

[no sievers, 2015-01-23 20:58:57]

On 2015/01/23 20:30:29, boliu wrote:
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
> (right):
> 
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
> base::CommandLine::ForCurrentProcess());
> On 2015/01/23 20:21:50, sievers wrote:
> > We shouldn't append the command line twice. 
> > Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
> > DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try to add
it?
> 
> I did think about that.
> 
> One is the factory here is a singleton already, so this should only be called
> once.
> 
> And even if some other code calls AppendGpuCommandLine, command line args is
> actually a hash map, so technically "doesn't hurt", except run time cost.
> 

I'm not worried about runtime cost. I'm worried about something weird happening
eventually, because we add the command line twice.
Simply never doing that avoids any potential for problems.

Also, I'm not sure about the existing code in GpuProcessHost. How can mutating
the CommandLine::ForCurrentProcess() from the IO thread be safe?

What thread is the factory being created on?

> > 
> > We don't ever try to spin up the GPU channel with WebView, right?
> 
> Doesn't matter if we do or not. We used to just do it in webview (before this
> broke) and it didn't hurt.
> 
> So I'm inclined to leave browser_main_loop alone in this CL. Make it easier to
> merge etc.
> 
> WYDT?
> 
> > You should also add something in browser_main_loop.cc so that it doesn't
call 
> > BrowserGpuChannelHostFactory::Initialize(established_gpu_channel) if running
> > in-process, even after the TODO there gets addressed.
> > 
> > Can you then file another bug for how to sort this out when we use mixed
> > contexts?
> > It can probably be done by making context creation here atomic or so.
> 
> I don't quite understand what you mean by "atomic context creation".

If you make it atomic then you can have a flag that tracks whether a context was
created.
When another one (in-process) is created you only need to append the command
line if the flag is not set yet.

[boliu, 2015-01-23 21:12:47]

On 2015/01/23 20:58:57, sievers wrote:
> On 2015/01/23 20:30:29, boliu wrote:
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> > File
content/browser/android/in_process/synchronous_compositor_factory_impl.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> >
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
> > base::CommandLine::ForCurrentProcess());
> > On 2015/01/23 20:21:50, sievers wrote:
> > > We shouldn't append the command line twice. 
> > > Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
> > > DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try to add
> it?
> > 
> > I did think about that.
> > 
> > One is the factory here is a singleton already, so this should only be
called
> > once.
> > 
> > And even if some other code calls AppendGpuCommandLine, command line args is
> > actually a hash map, so technically "doesn't hurt", except run time cost.
> > 
> 
> I'm not worried about runtime cost. I'm worried about something weird
happening
> eventually, because we add the command line twice.
> Simply never doing that avoids any potential for problems.
> 
> Also, I'm not sure about the existing code in GpuProcessHost. How can mutating
> the CommandLine::ForCurrentProcess() from the IO thread be safe?

I don't think it's thread safe there either for in-process. It's gonna race with
other things that are reading command line args during start up. For
cross-process a new instance of CommandLine is created, so it's fine.

> 
> What thread is the factory being created on?

UI thread. But it happens after content initialization, so probably not safe to
modify that from any thread at that point.

Hmm...maybe the right fix is FeatureInfo need to take a CommandLine object as an
argument instead of grabbing the current process?

> 
> > > 
> > > We don't ever try to spin up the GPU channel with WebView, right?
> > 
> > Doesn't matter if we do or not. We used to just do it in webview (before
this
> > broke) and it didn't hurt.
> > 
> > So I'm inclined to leave browser_main_loop alone in this CL. Make it easier
to
> > merge etc.
> > 
> > WYDT?
> > 
> > > You should also add something in browser_main_loop.cc so that it doesn't
> call 
> > > BrowserGpuChannelHostFactory::Initialize(established_gpu_channel) if
running
> > > in-process, even after the TODO there gets addressed.
> > > 
> > > Can you then file another bug for how to sort this out when we use mixed
> > > contexts?
> > > It can probably be done by making context creation here atomic or so.
> > 
> > I don't quite understand what you mean by "atomic context creation".
> 
> If you make it atomic then you can have a flag that tracks whether a context
was
> created.
> When another one (in-process) is created you only need to append the command
> line if the flag is not set yet.

Oh, I guess the promise is all context for webview will be created by the
factory.

[no sievers, 2015-01-23 21:30:28]

On 2015/01/23 21:12:47, boliu wrote:
> On 2015/01/23 20:58:57, sievers wrote:
> > On 2015/01/23 20:30:29, boliu wrote:
> > >
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> > > File
> content/browser/android/in_process/synchronous_compositor_factory_impl.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> > >
> content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
> > > base::CommandLine::ForCurrentProcess());
> > > On 2015/01/23 20:21:50, sievers wrote:
> > > > We shouldn't append the command line twice. 
> > > > Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
> > > > DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try to
add
> > it?
> > > 
> > > I did think about that.
> > > 
> > > One is the factory here is a singleton already, so this should only be
> called
> > > once.
> > > 
> > > And even if some other code calls AppendGpuCommandLine, command line args
is
> > > actually a hash map, so technically "doesn't hurt", except run time cost.
> > > 
> > 
> > I'm not worried about runtime cost. I'm worried about something weird
> happening
> > eventually, because we add the command line twice.
> > Simply never doing that avoids any potential for problems.
> > 
> > Also, I'm not sure about the existing code in GpuProcessHost. How can
mutating
> > the CommandLine::ForCurrentProcess() from the IO thread be safe?
> 
> I don't think it's thread safe there either for in-process. It's gonna race
with
> other things that are reading command line args during start up. For
> cross-process a new instance of CommandLine is created, so it's fine.
> 
> > 
> > What thread is the factory being created on?
> 
> UI thread. But it happens after content initialization, so probably not safe
to
> modify that from any thread at that point.
> 
> Hmm...maybe the right fix is FeatureInfo need to take a CommandLine object as
an
> argument instead of grabbing the current process?

It already supports that for unit tests. But you'd have to plumb it all the way
through.

Can we just remove the buggy call site for in-process in GpuProcessHost and do
it in a safe place.
Maybe just in GpuDataManagerImpl::Initialize() if in-process?
Because that actually has to happen first for the blacklist to be parsed.

> 
> > 
> > > > 
> > > > We don't ever try to spin up the GPU channel with WebView, right?
> > > 
> > > Doesn't matter if we do or not. We used to just do it in webview (before
> this
> > > broke) and it didn't hurt.
> > > 
> > > So I'm inclined to leave browser_main_loop alone in this CL. Make it
easier
> to
> > > merge etc.
> > > 
> > > WYDT?
> > > 
> > > > You should also add something in browser_main_loop.cc so that it doesn't
> > call 
> > > > BrowserGpuChannelHostFactory::Initialize(established_gpu_channel) if
> running
> > > > in-process, even after the TODO there gets addressed.
> > > > 
> > > > Can you then file another bug for how to sort this out when we use mixed
> > > > contexts?
> > > > It can probably be done by making context creation here atomic or so.
> > > 
> > > I don't quite understand what you mean by "atomic context creation".
> > 
> > If you make it atomic then you can have a flag that tracks whether a context
> was
> > created.
> > When another one (in-process) is created you only need to append the command
> > line if the flag is not set yet.
> 
> Oh, I guess the promise is all context for webview will be created by the
> factory.

[boliu, 2015-01-23 22:32:13]

On 2015/01/23 21:30:28, sievers wrote:
> On 2015/01/23 21:12:47, boliu wrote:
> > On 2015/01/23 20:58:57, sievers wrote:
> > > On 2015/01/23 20:30:29, boliu wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> > > > File
> > content/browser/android/in_process/synchronous_compositor_factory_impl.cc
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> > > >
> >
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
> > > > base::CommandLine::ForCurrentProcess());
> > > > On 2015/01/23 20:21:50, sievers wrote:
> > > > > We shouldn't append the command line twice. 
> > > > > Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
> > > > > DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try to
> add
> > > it?
> > > > 
> > > > I did think about that.
> > > > 
> > > > One is the factory here is a singleton already, so this should only be
> > called
> > > > once.
> > > > 
> > > > And even if some other code calls AppendGpuCommandLine, command line
args
> is
> > > > actually a hash map, so technically "doesn't hurt", except run time
cost.
> > > > 
> > > 
> > > I'm not worried about runtime cost. I'm worried about something weird
> > happening
> > > eventually, because we add the command line twice.
> > > Simply never doing that avoids any potential for problems.
> > > 
> > > Also, I'm not sure about the existing code in GpuProcessHost. How can
> mutating
> > > the CommandLine::ForCurrentProcess() from the IO thread be safe?
> > 
> > I don't think it's thread safe there either for in-process. It's gonna race
> with
> > other things that are reading command line args during start up. For
> > cross-process a new instance of CommandLine is created, so it's fine.
> > 
> > > 
> > > What thread is the factory being created on?
> > 
> > UI thread. But it happens after content initialization, so probably not safe
> to
> > modify that from any thread at that point.
> > 
> > Hmm...maybe the right fix is FeatureInfo need to take a CommandLine object
as
> an
> > argument instead of grabbing the current process?
> 
> It already supports that for unit tests. But you'd have to plumb it all the
way
> through.
> 
> Can we just remove the buggy call site for in-process in GpuProcessHost and do
> it in a safe place.
> Maybe just in GpuDataManagerImpl::Initialize() if in-process?
> Because that actually has to happen first for the blacklist to be parsed.

I think the battle is already lost by the time
BrowserMainLoop::BrowserThreadsStarted is called, which calls
GpuDataManagerImpl::Initialize and BrowserGpuChannelHostFactory::Initialize..

[no sievers, 2015-01-23 23:00:50]

On 2015/01/23 22:32:13, boliu wrote:
> On 2015/01/23 21:30:28, sievers wrote:
> > On 2015/01/23 21:12:47, boliu wrote:
> > > On 2015/01/23 20:58:57, sievers wrote:
> > > > On 2015/01/23 20:30:29, boliu wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> > > > > File
> > > content/browser/android/in_process/synchronous_compositor_factory_impl.cc
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/865063002/diff/1/content/browser/android/in_p...
> > > > >
> > >
> content/browser/android/in_process/synchronous_compositor_factory_impl.cc:172:
> > > > > base::CommandLine::ForCurrentProcess());
> > > > > On 2015/01/23 20:21:50, sievers wrote:
> > > > > > We shouldn't append the command line twice. 
> > > > > > Can we have GpuDataManagerImplPrivate::AppendGpuCommandLine()
> > > > > > DCHECK(!cmdline->HasSwitch(kGpuDriverBugWorkarounds)) before we try
to
> > add
> > > > it?
> > > > > 
> > > > > I did think about that.
> > > > > 
> > > > > One is the factory here is a singleton already, so this should only be
> > > called
> > > > > once.
> > > > > 
> > > > > And even if some other code calls AppendGpuCommandLine, command line
> args
> > is
> > > > > actually a hash map, so technically "doesn't hurt", except run time
> cost.
> > > > > 
> > > > 
> > > > I'm not worried about runtime cost. I'm worried about something weird
> > > happening
> > > > eventually, because we add the command line twice.
> > > > Simply never doing that avoids any potential for problems.
> > > > 
> > > > Also, I'm not sure about the existing code in GpuProcessHost. How can
> > mutating
> > > > the CommandLine::ForCurrentProcess() from the IO thread be safe?
> > > 
> > > I don't think it's thread safe there either for in-process. It's gonna
race
> > with
> > > other things that are reading command line args during start up. For
> > > cross-process a new instance of CommandLine is created, so it's fine.
> > > 
> > > > 
> > > > What thread is the factory being created on?
> > > 
> > > UI thread. But it happens after content initialization, so probably not
safe
> > to
> > > modify that from any thread at that point.
> > > 
> > > Hmm...maybe the right fix is FeatureInfo need to take a CommandLine object
> as
> > an
> > > argument instead of grabbing the current process?
> > 
> > It already supports that for unit tests. But you'd have to plumb it all the
> way
> > through.
> > 
> > Can we just remove the buggy call site for in-process in GpuProcessHost and
do
> > it in a safe place.
> > Maybe just in GpuDataManagerImpl::Initialize() if in-process?
> > Because that actually has to happen first for the blacklist to be parsed.
> 
> I think the battle is already lost by the time
> BrowserMainLoop::BrowserThreadsStarted is called, which calls
> GpuDataManagerImpl::Initialize and BrowserGpuChannelHostFactory::Initialize..

Then your current patch wouldn't even work because the bug list wouldn't be
parsed yet and no workaround would get appended to the cmdline.

[boliu, 2015-01-23 23:08:39]

On 2015/01/23 23:00:50, sievers wrote:
> > I think the battle is already lost by the time
> > BrowserMainLoop::BrowserThreadsStarted is called, which calls
> > GpuDataManagerImpl::Initialize and
BrowserGpuChannelHostFactory::Initialize..
> 
> Then your current patch wouldn't even work because the bug list wouldn't be
> parsed yet and no workaround would get appended to the cmdline.

Err, guess I wasn't clear.

I meant moving AppendGpuCommandLine for single-process to
GpuDataManagerImpl::Initialize wouldn't be thread safe. BrowserThreadsStarted
calls GpuDataManagerImpl::Initialize, but by the time BrowserThreadsStarted
runs, other threads have already started and can start using the CommandLine
objects.

For the current CL, the factory is created after BrowserThreadsStarted, so it
"works" but not in a thread safe way either.

The only way I see it could work for single-process is to use a separate
CommandLine object. Umm, maybe GpuDataManagerImpl can shove it into FeatureInfo
if it's single-process?

[no sievers, 2015-01-23 23:25:33]

On 2015/01/23 23:08:39, boliu wrote:
> On 2015/01/23 23:00:50, sievers wrote:
> > > I think the battle is already lost by the time
> > > BrowserMainLoop::BrowserThreadsStarted is called, which calls
> > > GpuDataManagerImpl::Initialize and
> BrowserGpuChannelHostFactory::Initialize..
> > 
> > Then your current patch wouldn't even work because the bug list wouldn't be
> > parsed yet and no workaround would get appended to the cmdline.
> 
> Err, guess I wasn't clear.
> 
> I meant moving AppendGpuCommandLine for single-process to
> GpuDataManagerImpl::Initialize wouldn't be thread safe. BrowserThreadsStarted
> calls GpuDataManagerImpl::Initialize, but by the time BrowserThreadsStarted
> runs, other threads have already started and can start using the CommandLine
> objects.
> 

We can move Initialize() so that it happens earlier (in this patch or
separately).
Either way, appending the CommandLine right after we parsed the bug list seems
like the better thing to do for in-process.

> For the current CL, the factory is created after BrowserThreadsStarted, so it
> "works" but not in a thread safe way either.
> 
> The only way I see it could work for single-process is to use a separate
> CommandLine object. Umm, maybe GpuDataManagerImpl can shove it into
FeatureInfo
> if it's single-process?

[boliu, 2015-01-24 00:35:25]

Ok, here's what I came up with. I do want to separate out browser_main_loop bits
in a follow up, because modifying of potentially affecting start up performance
on other platforms.

Uploading the whole thing for review, and running through trybots.

[no sievers, 2015-01-26 19:51:45]

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:1029: // On Android,
GLSurface::InitializeOneOff() must be called before initalizing
Can you move this block into the ifdef-android on the right (1019-1025)?
And then set |initialize_gpu_data_manager| to false (for Android), so it won't
get called again below?
And then you also don't need UsingInProcessGpu().

And then put a comment for the Android GpuDataManager call site saying why for
WebView this needs to be initialized so early.

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:1157: if (initialize_gpu_data_manager)
And keep this comment at the call site at 1114.

[boliu, 2015-01-26 22:15:08]

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:1029: // On Android,
GLSurface::InitializeOneOff() must be called before initalizing
On 2015/01/26 19:51:45, sievers wrote:
> Can you move this block into the ifdef-android on the right (1019-1025)?
> And then set |initialize_gpu_data_manager| to false (for Android), so it won't
> get called again below?
> And then you also don't need UsingInProcessGpu().

I specifically don't want to do this because I imagine that will slow down
chrome start up. IE it's moving InitializeOneOff to *before* threads are
started, so there'd be less parallelism.

> 
> And then put a comment for the Android GpuDataManager call site saying why for
> WebView this needs to be initialized so early.

[boliu, 2015-01-26 23:08:34]

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/865063002/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:1029: // On Android,
GLSurface::InitializeOneOff() must be called before initalizing
On 2015/01/26 22:15:08, boliu wrote:
> On 2015/01/26 19:51:45, sievers wrote:
> > Can you move this block into the ifdef-android on the right (1019-1025)?
> > And then set |initialize_gpu_data_manager| to false (for Android), so it
won't
> > get called again below?
> > And then you also don't need UsingInProcessGpu().
> 
> I specifically don't want to do this because I imagine that will slow down
> chrome start up. IE it's moving InitializeOneOff to *before* threads are
> started, so there'd be less parallelism.
> 
> > 
> > And then put a comment for the Android GpuDataManager call site saying why
for
> > WebView this needs to be initialized so early.
> 

Bahh, I confused myself. I forgot a bunch of defined(OS) checks.

I'm splitting out browser_main_loop into a separate CL:
https://codereview.chromium.org/879703003/

I'll make the changes there and send it to you. In the mean time, you can stamp
the gpu bits? :p

[no sievers, 2015-01-26 23:20:11]

lgtm. Can you update the cl description to also say that this patch fixes the
cmdline being mutated on the IO thread (for in-process gpu)?

[boliu, 2015-01-26 23:29:17]

On 2015/01/26 23:20:11, sievers wrote:
> lgtm. Can you update the cl description to also say that this patch fixes the
> cmdline being mutated on the IO thread (for in-process gpu)?

CL description updated.

[boliu, 2015-01-26 23:29:28]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-01-26 23:30:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/865063002/60001

[commit-bot: I haz the power, 2015-01-27 00:29:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-27 00:31:12]

Patchset 4 (id:??) landed as
https://crrev.com/6d695e6f5b99b8631e06788d2f9f9f9ed12c057e
Cr-Commit-Position: refs/heads/master@{#313177}