Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}

Committed: https://crrev.com/2084d99587adfa5eeb4025d5a12c8856fe8d2f3a
Cr-Commit-Position: refs/heads/master@{#380320}

[Peng, 2016-03-03 16:40:18]

Description was changed from

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
==========

to

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Peng, 2016-03-03 19:26:29]

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

[Peng, 2016-03-03 19:26:30]

Hi piman, PTAL. Thanks.

[piman, 2016-03-03 20:18:12]

https://codereview.chromium.org/1753383003/diff/80001/content/public/browser/...
File content/public/browser/gpu_utils.cc (right):

https://codereview.chromium.org/1753383003/diff/80001/content/public/browser/...
content/public/browser/gpu_utils.cc:94: g_gpu_preferences =
LAZY_INSTANCE_INITIALIZER;
Do we need this global, in particular, do we need it to be leaky? It doesn't
seem overly taxing to return a gpu::GpuPreferences from GetGpuPreferences()
(rather than a const&), I suppose it means we may re-parse the command-line
flags, but we're already doing that today - this is so seldom (restarting the
GPU process) that we shouldn't worry about it. OTOH, it makes testing possibly
harder (test side effects, if one tries to change the command line, it may not
apply any more).

[Peng, 2016-03-03 22:20:55]

https://codereview.chromium.org/1753383003/diff/80001/content/public/browser/...
File content/public/browser/gpu_utils.cc (right):

https://codereview.chromium.org/1753383003/diff/80001/content/public/browser/...
content/public/browser/gpu_utils.cc:94: g_gpu_preferences =
LAZY_INSTANCE_INITIALIZER;
On 2016/03/03 20:18:12, piman - OOO back 2016-03-04 wrote:
> Do we need this global, in particular, do we need it to be leaky? It doesn't
> seem overly taxing to return a gpu::GpuPreferences from GetGpuPreferences()
> (rather than a const&), I suppose it means we may re-parse the command-line
> flags, but we're already doing that today - this is so seldom (restarting the
> GPU process) that we shouldn't worry about it. OTOH, it makes testing possibly
> harder (test side effects, if one tries to change the command line, it may not
> apply any more).

Done.

[piman, 2016-03-04 20:39:16]

lgtm

[Peng, 2016-03-04 20:42:26]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-03-04 20:42:30]

The CQ bit was unchecked by penghuang@chromium.org

[Peng, 2016-03-04 20:45:57]

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

[Peng, 2016-03-04 20:45:57]

Hi boliu, PTAL. Thanks.

[boliu, 2016-03-05 00:34:37]

lgtm

[Peng, 2016-03-05 00:42:18]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2016-03-05 00:42:31]

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

[boliu, 2016-03-05 00:43:19]

On 2016/03/05 00:42:31, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1753383003/100001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1753383003/100001

You'll need security review for content/common/gpu/gpu_host_messages.h I think

[commit-bot: I haz the power, 2016-03-05 00:52:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-05 00:52:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peng, 2016-03-07 15:33:50]

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

[Peng, 2016-03-07 15:33:51]

+palmer@ for content/common/gpu/gpu_host_messages.h

Hi palmer, PTAL. Thanks.

[palmer, 2016-03-07 19:39:56]

https://codereview.chromium.org/1753383003/diff/120001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/1753383003/diff/120001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:342: gpu_preferences_ = gpu_preferences;
Is it necessary to validate the sanity of any of the fields in the struct?
Mainly the memory sizes, I am thinking.

Also, what ensure that this IPC came only from the browser process, and not from
(say) a renderer?

https://codereview.chromium.org/1753383003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gpu_preferences.h (right):

https://codereview.chromium.org/1753383003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:69: unsigned int
force_gpu_mem_available = 0;
Use explicitly-sied integer types:
https://www.chromium.org/Home/chromium-security/education/security-tips-for-i....

Probably uint32_t in this case.

[piman, 2016-03-07 19:53:32]

https://codereview.chromium.org/1753383003/diff/120001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/1753383003/diff/120001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:342: gpu_preferences_ = gpu_preferences;
On 2016/03/07 19:39:56, palmer wrote:
> Is it necessary to validate the sanity of any of the fields in the struct?
> Mainly the memory sizes, I am thinking.

I don't think this is any more necessary than when it was a command-line option.

> Also, what ensure that this IPC came only from the browser process, and not
from
> (say) a renderer?

It comes on the master channel from the browser process, not from a channel
vended to arbitrary clients (GpuChannel).

[Peng, 2016-03-07 19:56:20]

https://codereview.chromium.org/1753383003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gpu_preferences.h (right):

https://codereview.chromium.org/1753383003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:69: unsigned int
force_gpu_mem_available = 0;
On 2016/03/07 19:39:56, palmer wrote:
> Use explicitly-sied integer types:
>
https://www.chromium.org/Home/chromium-security/education/security-tips-for-i....
> 
> Probably uint32_t in this case.

Done.

[palmer, 2016-03-07 20:01:22]

LGTM

[Peng, 2016-03-07 20:13:12]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-03-07 20:13:12]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1753383003/#ps140001
(title: "Update")

[commit-bot: I haz the power, 2016-03-07 20:13:46]

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

[commit-bot: I haz the power, 2016-03-07 21:49:52]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-07 21:52:25]

Description was changed from

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}
==========

[commit-bot: I haz the power, 2016-03-07 21:52:26]

Patchset 8 (id:??) landed as
https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}

[shatch, 2016-03-08 22:48:57]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1778693002/ by simonhatch@chromium.org.

The reason for reverting is: Suspecting this is breaking a lot of perf bots.

BUG=593045.

[Peng, 2016-03-09 16:24:56]

Description was changed from

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}
==========

to

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}
==========

[Peng, 2016-03-10 02:16:13]

On 2016/03/08 22:48:57, shatch wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/1778693002/ by mailto:simonhatch@chromium.org.
> 
> The reason for reverting is: Suspecting this is breaking a lot of perf bots.
> 
> BUG=593045.

Updated this CL to fix the issue. Tried with
https://codereview.chromium.org/1778493006/ &
https://codereview.chromium.org/1779913002/

[Peng, 2016-03-10 02:16:22]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-03-10 02:16:22]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
boliu@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1753383003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-10 02:16:44]

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

[commit-bot: I haz the power, 2016-03-10 03:33:05]

Description was changed from

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}
==========

to

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}
==========

[commit-bot: I haz the power, 2016-03-10 03:33:07]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-10 03:35:04]

Description was changed from

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}
==========

to

==========
Get rid of gpu related switches by passing gpu::GpuPreferences via IPC.

BUG=590825
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/248b114c9d3034964c7259222712dfcccfad8f48
Cr-Commit-Position: refs/heads/master@{#379651}

Committed: https://crrev.com/2084d99587adfa5eeb4025d5a12c8856fe8d2f3a
Cr-Commit-Position: refs/heads/master@{#380320}
==========

[commit-bot: I haz the power, 2016-03-10 03:35:05]

Patchset 10 (id:??) landed as
https://crrev.com/2084d99587adfa5eeb4025d5a12c8856fe8d2f3a
Cr-Commit-Position: refs/heads/master@{#380320}