Set GpuPreferences correctly in GpuDataManagerImplPrivate::AppendGpuCommandLine.

Set GpuPreferences correctly in GpuDataManagerImplPrivate::AppendGpuCommandLine.

GpuDataManagerImplPrivate::AppendGpuCommandLine() may add some GPU
command line switches which have been replaced by GpuPreferences. So we
need set those preferences in GpuDataManagerImplPrivate::AppendGpuCommandLine()
to allow GPU process get the correct GpuPreferences via IPC message.

BUG=601647
Committed: https://crrev.com/4c901e8f2249a4efacb311d0e24d09f3c88c8e0c
Cr-Commit-Position: refs/heads/master@{#386669}

[Peng, 2016-04-11 15:32:16]

penghuang@chromium.org changed reviewers:
+ jbauman@chromium.org

[Peng, 2016-04-11 15:32:17]

hi jbauman, PTAL. Thanks.

[jbauman, 2016-04-12 01:30:49]

https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl.h:121: gpu::GpuPreferences*
gpu_preferences) const;
It would probably be best to make the GpuPreferences modification into a new
method.

[Peng, 2016-04-12 02:13:24]

https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_data_manager_impl.h (right):

https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl.h:121: gpu::GpuPreferences*
gpu_preferences) const;
On 2016/04/12 01:30:49, jbauman wrote:
> It would probably be best to make the GpuPreferences modification into a new
> method.

I use one method to modify command_line & gpu_preferences together, because if
the gpu_preferences is used, some switches should not be in the command line. So
it is very easy to do it in one method, and it is hard to do it in two methods.

Other solution is keeping this method unchanged, and modifying the
gpu_preferences in GpuProcessHost by checking the command_line returned by this
method, and removing some unnecessary switches from the it.

Or maybe we could rename this method to
PrepareGpuCommandLineAndGpuPreferences()?

Thought?

[jbauman, 2016-04-12 02:41:49]

On 2016/04/12 02:13:24, Peng wrote:
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> File content/browser/gpu/gpu_data_manager_impl.h (right):
> 
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> content/browser/gpu/gpu_data_manager_impl.h:121: gpu::GpuPreferences*
> gpu_preferences) const;
> On 2016/04/12 01:30:49, jbauman wrote:
> > It would probably be best to make the GpuPreferences modification into a new
> > method.
> 
> I use one method to modify command_line & gpu_preferences together, because if
> the gpu_preferences is used, some switches should not be in the command line.
So
> it is very easy to do it in one method, and it is hard to do it in two
methods.

Why do we want to modify the GpuPreferences in some cases and the command line
in others? Makes more sense to me to always go through GpuPreferences for these
options.
> 
> Other solution is keeping this method unchanged, and modifying the
> gpu_preferences in GpuProcessHost by checking the command_line returned by
this
> method, and removing some unnecessary switches from the it.
> 
> Or maybe we could rename this method to
> PrepareGpuCommandLineAndGpuPreferences()?
> 
> Thought?

[jbauman, 2016-04-12 02:43:58]

On 2016/04/12 02:41:49, jbauman wrote:
> On 2016/04/12 02:13:24, Peng wrote:
> >
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> > File content/browser/gpu/gpu_data_manager_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> > content/browser/gpu/gpu_data_manager_impl.h:121: gpu::GpuPreferences*
> > gpu_preferences) const;
> > On 2016/04/12 01:30:49, jbauman wrote:
> > > It would probably be best to make the GpuPreferences modification into a
new
> > > method.
> > 
> > I use one method to modify command_line & gpu_preferences together, because
if
> > the gpu_preferences is used, some switches should not be in the command
line.
> So
> > it is very easy to do it in one method, and it is hard to do it in two
> methods.
> 
> Why do we want to modify the GpuPreferences in some cases and the command line
> in others? Makes more sense to me to always go through GpuPreferences for
these
> options.

Or we could set both, if necessary.
> > 
> > Other solution is keeping this method unchanged, and modifying the
> > gpu_preferences in GpuProcessHost by checking the command_line returned by
> this
> > method, and removing some unnecessary switches from the it.
> > 
> > Or maybe we could rename this method to
> > PrepareGpuCommandLineAndGpuPreferences()?
> > 
> > Thought?

[chromium-reviews, 2016-04-12 02:54:40]

I think for single process mode, we need only modify the command line for
the chrome. And later, we will parse the command line into GPU preferences
for launching in process GPU service.

<jbauman@chromium.org>于2016年4月11日周一 22:43写道：

> On 2016/04/12 02:41:49, jbauman wrote:
> > On 2016/04/12 02:13:24, Peng wrote:
> > >
> >
>
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> > > File content/browser/gpu/gpu_data_manager_impl.h (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> > > content/browser/gpu/gpu_data_manager_impl.h:121: gpu::GpuPreferences*
> > > gpu_preferences) const;
> > > On 2016/04/12 01:30:49, jbauman wrote:
> > > > It would probably be best to make the GpuPreferences modification
> into a
> new
> > > > method.
> > >
> > > I use one method to modify command_line & gpu_preferences together,
> because
> if
> > > the gpu_preferences is used, some switches should not be in the command
> line.
> > So
> > > it is very easy to do it in one method, and it is hard to do it in two
> > methods.
> >
> > Why do we want to modify the GpuPreferences in some cases and the
> command line
> > in others? Makes more sense to me to always go through GpuPreferences for
> these
> > options.
>
> Or we could set both, if necessary.
>
> > >
> > > Other solution is keeping this method unchanged, and modifying the
> > > gpu_preferences in GpuProcessHost by checking the command_line
> returned by
> > this
> > > method, and removing some unnecessary switches from the it.
> > >
> > > Or maybe we could rename this method to
> > > PrepareGpuCommandLineAndGpuPreferences()?
> > >
> > > Thought?
>
>
>
> https://codereview.chromium.org/1881463002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2016-04-12 02:57:11]

I guess this works, then. lgtm

On 2016/04/12 02:54:40, chromium-reviews wrote:
> I think for single process mode, we need only modify the command line for
> the chrome. And later, we will parse the command line into GPU preferences
> for launching in process GPU service.
> 
> <mailto:jbauman@chromium.xn--org>2016411-vm8qo1do32bgo8a74yjbe 22:43写道：
> 
> > On 2016/04/12 02:41:49, jbauman wrote:
> > > On 2016/04/12 02:13:24, Peng wrote:
> > > >
> > >
> >
> >
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> > > > File content/browser/gpu/gpu_data_manager_impl.h (right):
> > > >
> > > >
> > >
> >
> >
>
https://codereview.chromium.org/1881463002/diff/20001/content/browser/gpu/gpu...
> > > > content/browser/gpu/gpu_data_manager_impl.h:121: gpu::GpuPreferences*
> > > > gpu_preferences) const;
> > > > On 2016/04/12 01:30:49, jbauman wrote:
> > > > > It would probably be best to make the GpuPreferences modification
> > into a
> > new
> > > > > method.
> > > >
> > > > I use one method to modify command_line & gpu_preferences together,
> > because
> > if
> > > > the gpu_preferences is used, some switches should not be in the command
> > line.
> > > So
> > > > it is very easy to do it in one method, and it is hard to do it in two
> > > methods.
> > >
> > > Why do we want to modify the GpuPreferences in some cases and the
> > command line
> > > in others? Makes more sense to me to always go through GpuPreferences for
> > these
> > > options.
> >
> > Or we could set both, if necessary.
> >
> > > >
> > > > Other solution is keeping this method unchanged, and modifying the
> > > > gpu_preferences in GpuProcessHost by checking the command_line
> > returned by
> > > this
> > > > method, and removing some unnecessary switches from the it.
> > > >
> > > > Or maybe we could rename this method to
> > > > PrepareGpuCommandLineAndGpuPreferences()?
> > > >
> > > > Thought?
> >
> >
> >
> > https://codereview.chromium.org/1881463002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Peng, 2016-04-12 12:06:30]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2016-04-12 12:06:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881463002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881463002/20001

[commit-bot: I haz the power, 2016-04-12 13:32:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-12 13:33:53]

Description was changed from

==========
Set GpuPreferences correctly in GpuDataManagerImplPrivate::AppendGpuCommandLine.

GpuDataManagerImplPrivate::AppendGpuCommandLine() may add some GPU
command line switches which have been replaced by GpuPreferences. So we
need set those preferences in GpuDataManagerImplPrivate::AppendGpuCommandLine()
to allow GPU process get the correct GpuPreferences via IPC message.

BUG=601647
==========

to

==========
Set GpuPreferences correctly in GpuDataManagerImplPrivate::AppendGpuCommandLine.

GpuDataManagerImplPrivate::AppendGpuCommandLine() may add some GPU
command line switches which have been replaced by GpuPreferences. So we
need set those preferences in GpuDataManagerImplPrivate::AppendGpuCommandLine()
to allow GPU process get the correct GpuPreferences via IPC message.

BUG=601647

Committed: https://crrev.com/4c901e8f2249a4efacb311d0e24d09f3c88c8e0c
Cr-Commit-Position: refs/heads/master@{#386669}
==========

[commit-bot: I haz the power, 2016-04-12 13:33:54]

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