Use GpuPreferences to avoid directly accessing switches in gpu related code

Use GpuPreferences to avoid directly accessing switches in gpu related code

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7404df98850f7d6e4bfc0f88faf6629c3ded36e8
Cr-Commit-Position: refs/heads/master@{#378305}

[Peng, 2016-02-19 16:16:35]

Description was changed from

==========
GpuPreferences

BUG=586369
==========

to

==========
GpuPreferences

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Fady Samuel, 2016-02-22 17:14:15]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-02-22 17:14:15]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-02-22 17:14:16]

The primary goal here is to decouple from content_switches but piman@ suggested
to me that perhaps gpu code should not depend on switches at all. I think we
probably want to pass preferences objects on construction. Please discuss with
piman@

https://codereview.chromium.org/1716813002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_preferences.cc (right):

https://codereview.chromium.org/1716813002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_preferences.cc:34:
GpuPreferences::GpuPreferences() {
Drive-by: I don't think we want a global object here. I think we want to plumb
this through to the relevant areas. Please discuss with piman@

[Fady Samuel, 2016-02-22 17:14:16]

The primary goal here is to decouple from content_switches but piman@ suggested
to me that perhaps gpu code should not depend on switches at all. I think we
probably want to pass preferences objects on construction. Please discuss with
piman@

https://codereview.chromium.org/1716813002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_preferences.cc (right):

https://codereview.chromium.org/1716813002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_preferences.cc:34:
GpuPreferences::GpuPreferences() {
Drive-by: I don't think we want a global object here. I think we want to plumb
this through to the relevant areas. Please discuss with piman@

[Peng, 2016-02-23 21:16:20]

Description was changed from

==========
GpuPreferences

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
GpuPreferences

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Peng, 2016-02-23 21:16:37]

Description was changed from

==========
GpuPreferences

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use GpuPreferences to avoid directly accessing switches in gpu related code

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Peng, 2016-02-24 20:48:01]

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

[Peng, 2016-02-24 20:48:02]

Hi piman, PTAL. Thanks.

[Peng, 2016-02-24 20:48:32]

https://codereview.chromium.org/1716813002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_preferences.cc (right):

https://codereview.chromium.org/1716813002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_preferences.cc:34:
GpuPreferences::GpuPreferences() {
On 2016/02/22 17:14:16, Fady Samuel wrote:
> Drive-by: I don't think we want a global object here. I think we want to plumb
> this through to the relevant areas. Please discuss with piman@

Done.

[Fady Samuel, 2016-02-24 20:51:55]

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

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:17: namespace base { class
CommandLine; };
Is this forward declaration necessary?

[piman, 2016-02-24 22:19:08]

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

https://codereview.chromium.org/1716813002/diff/160001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:232:
CHECK(base::CommandLine::InitializedForCurrentProcess());
nit: DCHECK

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

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:5: #ifndef GPU_PREFERENCES_H_
nit: GPU_COMMAND_BUFFER_SERVICE_GPU_PREFERENCES_H_

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:10: #include <set>
nit: This is not used.

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.h:143: class Service {
There is a derived class on Android, for WebView (in
android_webvier/browser/deferred_gpu_command_service.h), that needs to inject
the GpuPreferences taken from the command line. Maybe pass the GpuPreferences
via the constructor?

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_program_cache.h (right):

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_program_cache.h:31: const bool
disable_gpu_shader_disk_cache);
nit: no const in the parameters, unless they're references/pointers.

[Peng, 2016-02-25 16:18:36]

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

https://codereview.chromium.org/1716813002/diff/160001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:232:
CHECK(base::CommandLine::InitializedForCurrentProcess());
On 2016/02/24 22:19:08, piman wrote:
> nit: DCHECK

Done.

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

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:5: #ifndef GPU_PREFERENCES_H_
On 2016/02/24 22:19:08, piman wrote:
> nit: GPU_COMMAND_BUFFER_SERVICE_GPU_PREFERENCES_H_

Done.

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:10: #include <set>
On 2016/02/24 22:19:08, piman wrote:
> nit: This is not used.

Done.

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:17: namespace base { class
CommandLine; };
On 2016/02/24 20:51:54, Fady Samuel wrote:
> Is this forward declaration necessary?

Done.

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.h:143: class Service {
On 2016/02/24 22:19:08, piman wrote:
> There is a derived class on Android, for WebView (in
> android_webvier/browser/deferred_gpu_command_service.h), that needs to inject
> the GpuPreferences taken from the command line. Maybe pass the GpuPreferences
> via the constructor?

Done.

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_program_cache.h (right):

https://codereview.chromium.org/1716813002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_program_cache.h:31: const bool
disable_gpu_shader_disk_cache);
On 2016/02/24 22:19:08, piman wrote:
> nit: no const in the parameters, unless they're references/pointers.

Done.

[Fady Samuel, 2016-02-25 16:26:34]

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:21: #include
"content/common/gpu/gpu_messages.h"
You can probably delete this. I missed it in my patch, I think.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:23: #include
"content/public/common/content_switches.h"
Delete this.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:33: #include "ipc/message_router.h"
nit: you can probably delete these ipc includes. I missed htem in my patch.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:30: #include
"content/public/common/content_switches.h"
Delete this.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:46: #include "ui/gl/gl_switches.h"
Delete this?

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:287: 
nit: remove this extra line.

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:20: #include
"base/command_line.h"
No longer necessary?

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:47: #include
"gpu/command_buffer/service/gpu_switches.h"
No longer necessary?

[Peng, 2016-02-25 17:13:22]

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:21: #include
"content/common/gpu/gpu_messages.h"
On 2016/02/25 16:26:33, Fady Samuel wrote:
> You can probably delete this. I missed it in my patch, I think.

Done.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:23: #include
"content/public/common/content_switches.h"
On 2016/02/25 16:26:33, Fady Samuel wrote:
> Delete this.

Done.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_channel_manager.cc:33: #include "ipc/message_router.h"
On 2016/02/25 16:26:33, Fady Samuel wrote:
> nit: you can probably delete these ipc includes. I missed htem in my patch.

Done.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:30: #include
"content/public/common/content_switches.h"
On 2016/02/25 16:26:33, Fady Samuel wrote:
> Delete this.

Done.

https://codereview.chromium.org/1716813002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:46: #include "ui/gl/gl_switches.h"
On 2016/02/25 16:26:33, Fady Samuel wrote:
> Delete this?

Done.

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:287: 
On 2016/02/25 16:26:33, Fady Samuel wrote:
> nit: remove this extra line.

Done.

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:20: #include
"base/command_line.h"
On 2016/02/25 16:26:33, Fady Samuel wrote:
> No longer necessary?

Done.

https://codereview.chromium.org/1716813002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:47: #include
"gpu/command_buffer/service/gpu_switches.h"
On 2016/02/25 16:26:33, Fady Samuel wrote:
> No longer necessary?

Done.

[piman, 2016-02-25 21:56:05]

https://codereview.chromium.org/1716813002/diff/200001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/1716813002/diff/200001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.cc:81: :
gpu::InProcessCommandBuffer::Service(gpu_preferences_),
Oh, this is confusing/dangerous (and probably undefined behavior). We give
InProcessCommandBuffer::Service a set of uninitialized preferences, and then
change them. If InProcessCommandBuffer::Service uses the preferences in the
constructor, things will blow up.

How about instead: have InProcessCommandBuffer::Service own the GpuPreferences,
and we just create a GetGpuPreferences function that returns the GpuPreferences
initialized from the command line.

DeferredGpuCommandService::DeferredGpuCommandService()
    : gpu::InProcessCommandBuffer::Service(GetGpuPreferences()),
      sync_point_manager_(new gpu::SyncPointManager(true)) {
  [...]
}

[Peng, 2016-02-25 22:29:09]

https://codereview.chromium.org/1716813002/diff/200001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/1716813002/diff/200001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.cc:81: :
gpu::InProcessCommandBuffer::Service(gpu_preferences_),
On 2016/02/25 21:56:05, piman wrote:
> Oh, this is confusing/dangerous (and probably undefined behavior). We give
> InProcessCommandBuffer::Service a set of uninitialized preferences, and then
> change them. If InProcessCommandBuffer::Service uses the preferences in the
> constructor, things will blow up.
> 
> How about instead: have InProcessCommandBuffer::Service own the
GpuPreferences,
> and we just create a GetGpuPreferences function that returns the
GpuPreferences
> initialized from the command line.
> 
> DeferredGpuCommandService::DeferredGpuCommandService()
>     : gpu::InProcessCommandBuffer::Service(GetGpuPreferences()),
>       sync_point_manager_(new gpu::SyncPointManager(true)) {
>   [...]
> }

Done.

[piman, 2016-02-25 22:58:29]

lgtm

https://codereview.chromium.org/1716813002/diff/240001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/1716813002/diff/240001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.h:77: gpu::GpuPreferences
GetGpuPreferences();
static

[Peng, 2016-02-26 15:41:12]

https://codereview.chromium.org/1716813002/diff/240001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/1716813002/diff/240001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.h:77: gpu::GpuPreferences
GetGpuPreferences();
On 2016/02/25 22:58:29, piman wrote:
> static

Done. I moved this function into the anonymous namespace instead.

[Peng, 2016-02-26 15:44:32]

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

[Peng, 2016-02-26 15:44:33]

+boliu@ for android_webview/*

Hi boliu, PTAL. Thanks.

[boliu, 2016-02-26 16:57:05]

https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.cc:37: gpu::GpuPreferences
GetGpuPreferencesFromCommandLine() {
This chunk of code is duplicated in gpu_child_thread. Is it possible to share
the implementation? Don't if there is a good place under //gpu since there are
switches coming from all over the place..

[Peng, 2016-02-26 17:04:48]

https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.cc:37: gpu::GpuPreferences
GetGpuPreferencesFromCommandLine() {
On 2016/02/26 16:57:05, boliu wrote:
> This chunk of code is duplicated in gpu_child_thread. Is it possible to share
> the implementation? Don't if there is a good place under //gpu since there are
> switches coming from all over the place..

The goal is removing directly accessing switches in gpu related code. And the
code uses gpu service is responsible for creating a GpuPreferences and passes it
into gpu objects' constructors. So it is not good to put this code in //gpu. In
future, probably we will remove all gpu related command line options, and pass
the GpuPreferences via IPC. So I think it is OK to have this duplicated code
here with a TODO for now. And we will remove it later. What do you think?

[boliu, 2016-02-26 17:39:24]

On 2016/02/26 17:04:48, Peng wrote:
>
https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
> File android_webview/browser/deferred_gpu_command_service.cc (right):
> 
>
https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
> android_webview/browser/deferred_gpu_command_service.cc:37:
gpu::GpuPreferences
> GetGpuPreferencesFromCommandLine() {
> On 2016/02/26 16:57:05, boliu wrote:
> > This chunk of code is duplicated in gpu_child_thread. Is it possible to
share
> > the implementation? Don't if there is a good place under //gpu since there
are
> > switches coming from all over the place..
> 
> The goal is removing directly accessing switches in gpu related code. And the
> code uses gpu service is responsible for creating a GpuPreferences and passes
it
> into gpu objects' constructors. So it is not good to put this code in //gpu.
In
> future, probably we will remove all gpu related command line options, and pass
> the GpuPreferences via IPC. So I think it is OK to have this duplicated code
> here with a TODO for now. And we will remove it later. What do you think?

Oh didn't realize goal was to get rid of command line switches. Then I've got
some other concerns.

* Android webview uses both in_process_command_buffer and gpu_child_thread, so
their GpuPreferences should match, so probably gpu_child_thread need to get its
GpuPreferences from webview as well. We do this for SyncPointManager already,
see SynchronousCompositor::SetGpuService. Otherwise, someone might modify the
code in gpu_child_thread and forget webview one, which leads to webview running
the two gpu threads with slightly different preferences.

* The way command lines (that are hardcoded) work is that content sets some
switches, and then webview can add more. So in general, preferences for webview
should match android, except where webview explicitly chooses to differ. Having
an independent implementation in webview removes that, so the risk again is,
someone modifies preference for android, and forgets that webview exists.

You see, the "forgot webview exists" concern is common enough thing in
chromium..

So in this CL before the switches are actually removed, I'd like to see the
first concern addressed.

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

https://codereview.chromium.org/1716813002/diff/260001/gpu/command_buffer/ser...
gpu/command_buffer/service/gpu_preferences.h:69: size_t force_gpu_mem_available
= 0;
Just a note, not all switches are "hardcoded preferences". Some of these are
debug switches that should be kept for development, like this one.

Turning them into preference is still a good idea, so this CL is fine.

[Peng, 2016-02-26 20:33:09]

https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/1716813002/diff/260001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.cc:37: gpu::GpuPreferences
GetGpuPreferencesFromCommandLine() {
On 2016/02/26 17:04:48, Peng wrote:
> On 2016/02/26 16:57:05, boliu wrote:
> > This chunk of code is duplicated in gpu_child_thread. Is it possible to
share
> > the implementation? Don't if there is a good place under //gpu since there
are
> > switches coming from all over the place..
> 
> The goal is removing directly accessing switches in gpu related code. And the
> code uses gpu service is responsible for creating a GpuPreferences and passes
it
> into gpu objects' constructors. So it is not good to put this code in //gpu.
In
> future, probably we will remove all gpu related command line options, and pass
> the GpuPreferences via IPC. So I think it is OK to have this duplicated code
> here with a TODO for now. And we will remove it later. What do you think?

Done.

[boliu, 2016-02-26 22:17:32]

https://codereview.chromium.org/1716813002/diff/280001/content/browser/androi...
File content/browser/android/synchronous_compositor_base.cc (right):

https://codereview.chromium.org/1716813002/diff/280001/content/browser/androi...
content/browser/android/synchronous_compositor_base.cc:24: const
gpu::GpuPreferences* g_gpu_preferences = nullptr;
LazyInstance<gpu::GpuPreferences>, also const won't compile

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:225: *gpu_preferences :
GetGpuPreferencesFromCommandLine()),
Don't do this. Either make the caller construct a GpuPreferences, or don't have
the parameter at all.

Again, this suggests once again there should not be two copies of
GetGpuPreferencesFromCommandLine. Maybe it should be a method in the public
content API?

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:250: *gpu_preferences :
GetGpuPreferencesFromCommandLine()),
ditto

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
these should be const&

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:404: const gpu::GpuPreferences* kGpuPreferences =
nullptr;
style: kGpuPreferences is only for static/global constants I think

[Peng, 2016-02-29 16:09:23]

https://codereview.chromium.org/1716813002/diff/280001/content/browser/androi...
File content/browser/android/synchronous_compositor_base.cc (right):

https://codereview.chromium.org/1716813002/diff/280001/content/browser/androi...
content/browser/android/synchronous_compositor_base.cc:24: const
gpu::GpuPreferences* g_gpu_preferences = nullptr;
On 2016/02/26 22:17:32, boliu wrote:
> LazyInstance<gpu::GpuPreferences>, also const won't compile

I don't see why need LazyInstance. But anyway, I removed g_gpu_preferences &
g_sync_point_manager by using a pointer of the g_gpu_service.

Done

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:225: *gpu_preferences :
GetGpuPreferencesFromCommandLine()),
On 2016/02/26 22:17:32, boliu wrote:
> Don't do this. Either make the caller construct a GpuPreferences, or don't
have
> the parameter at all.

Removed the gpu_preferences for this constructor. But for the other
constructor, may I ask why we cannot use a pointer there? The GpuChildThread are
created in several places. To modify there places to supply a GpuPreferences is
inconvenient. That is why piman@ suggested we put the GpuPreference and parse
the command line in this class.



> 
> Again, this suggests once again there should not be two copies of
> GetGpuPreferencesFromCommandLine. Maybe it should be a method in the public
> content API?

I would like to put GetGpuPreferencesFromCommandLine() in content/public, but it
has a little problem. Seems content/public/common are used by browser, gpu and
renderer processes, but this function uses gpu::GpuPreferences and it is in the
gpu/command_buffer/service . I believe gpu service code should not be used by
renderer and browser processes indirectly, right? Any suggestion?

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
On 2016/02/26 22:17:32, boliu wrote:
> these should be const&

pointer is necessary, so the caller can use a nullptr and the GpuChildThread
will construct a GpuPreferences by itself.

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:404: const gpu::GpuPreferences* kGpuPreferences =
nullptr;
On 2016/02/26 22:17:32, boliu wrote:
> style: kGpuPreferences is only for static/global constants I think

Done.

[boliu, 2016-02-29 16:26:41]

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:225: *gpu_preferences :
GetGpuPreferencesFromCommandLine()),
On 2016/02/29 16:09:22, Peng wrote:
> On 2016/02/26 22:17:32, boliu wrote:
> > Don't do this. Either make the caller construct a GpuPreferences, or don't
> have
> > the parameter at all.
> 
> Removed the gpu_preferences for this constructor. But for the other
> constructor, may I ask why we cannot use a pointer there?

const* is generally against the style guide. Either it should be const&, or just
non-const* for an output parameter.

> The GpuChildThread are
> created in several places. To modify there places to supply a GpuPreferences
is
> inconvenient. That is why piman@ suggested we put the GpuPreference and parse
> the command line in this class.
> 
> 
> 
> > 
> > Again, this suggests once again there should not be two copies of
> > GetGpuPreferencesFromCommandLine. Maybe it should be a method in the public
> > content API?
> 
> I would like to put GetGpuPreferencesFromCommandLine() in content/public, but
it
> has a little problem. Seems content/public/common are used by browser, gpu and
> renderer processes, but this function uses gpu::GpuPreferences and it is in
the
> gpu/command_buffer/service . I believe gpu service code should not be used by
> renderer and browser processes indirectly, right? Any suggestion?

Webview only needs it from browser side, so the public API only needs to be in
content/public/browser. But inside content, it's needed from child as well. I
don't know if there is any precedence for something like that. Ask piman or
another content owner maybe?

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
On 2016/02/29 16:09:22, Peng wrote:
> On 2016/02/26 22:17:32, boliu wrote:
> > these should be const&
> 
> pointer is necessary, so the caller can use a nullptr and the GpuChildThread
> will construct a GpuPreferences by itself.

Then maybe you want another constructor instead.

[Peng, 2016-02-29 19:29:50]

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:225: *gpu_preferences :
GetGpuPreferencesFromCommandLine()),
On 2016/02/29 16:26:40, boliu wrote:
> On 2016/02/29 16:09:22, Peng wrote:
> > On 2016/02/26 22:17:32, boliu wrote:
> > > Don't do this. Either make the caller construct a GpuPreferences, or don't
> > have
> > > the parameter at all.
> > 
> > Removed the gpu_preferences for this constructor. But for the other
> > constructor, may I ask why we cannot use a pointer there?
> 
> const* is generally against the style guide. Either it should be const&, or
just
> non-const* for an output parameter.

https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Base on google code style. non-const references is disallowed, but const pointer
is OK.

> 
> > The GpuChildThread are
> > created in several places. To modify there places to supply a GpuPreferences
> is
> > inconvenient. That is why piman@ suggested we put the GpuPreference and
parse
> > the command line in this class.
> > 
> > 
> > 
> > > 
> > > Again, this suggests once again there should not be two copies of
> > > GetGpuPreferencesFromCommandLine. Maybe it should be a method in the
public
> > > content API?
> > 
> > I would like to put GetGpuPreferencesFromCommandLine() in content/public,
but
> it
> > has a little problem. Seems content/public/common are used by browser, gpu
and
> > renderer processes, but this function uses gpu::GpuPreferences and it is in
> the
> > gpu/command_buffer/service . I believe gpu service code should not be used
by
> > renderer and browser processes indirectly, right? Any suggestion?
> 
> Webview only needs it from browser side, so the public API only needs to be in
> content/public/browser. But inside content, it's needed from child as well. I
> don't know if there is any precedence for something like that. Ask piman or
> another content owner maybe?

piman@ LGTMed the duplicate code in DeferredGpuCommandService & GpuChildThread
(I think he doesn't object it as a short term solution, but he is OOO, I cannot
confirm it right now). In followup CLs, we will get rid of gpu related switches
by sending gpu::GpuPreferences from browser to gpu service. It will remove those
duplicate code.
I filed a new issue for it(http://crbug.com/590825), and added it in the TODO
inline comment.

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
On 2016/02/29 16:26:40, boliu wrote:
> On 2016/02/29 16:09:22, Peng wrote:
> > On 2016/02/26 22:17:32, boliu wrote:
> > > these should be const&
> > 
> > pointer is necessary, so the caller can use a nullptr and the GpuChildThread
> > will construct a GpuPreferences by itself.
> 
> Then maybe you want another constructor instead.

Do we really need another constructor with duplicate code in it, since const
pointer is allowed by our code style?

[boliu, 2016-02-29 19:48:22]

a_w and synchronous_compositor* lgtm

We are debating about code I don't own anymore. You probably want a content or
gpu owner to go over the additional changes again since piman approved though.

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:225: *gpu_preferences :
GetGpuPreferencesFromCommandLine()),
On 2016/02/29 19:29:50, Peng wrote:
> On 2016/02/29 16:26:40, boliu wrote:
> > On 2016/02/29 16:09:22, Peng wrote:
> > > On 2016/02/26 22:17:32, boliu wrote:
> > > > Don't do this. Either make the caller construct a GpuPreferences, or
don't
> > > have
> > > > the parameter at all.
> > > 
> > > Removed the gpu_preferences for this constructor. But for the other
> > > constructor, may I ask why we cannot use a pointer there?
> > 
> > const* is generally against the style guide. Either it should be const&, or
> just
> > non-const* for an output parameter.
> 
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> 
> Base on google code style. non-const references is disallowed, but const
pointer
> is OK.
> 
> > 
> > > The GpuChildThread are
> > > created in several places. To modify there places to supply a
GpuPreferences
> > is
> > > inconvenient. That is why piman@ suggested we put the GpuPreference and
> parse
> > > the command line in this class.
> > > 
> > > 
> > > 
> > > > 
> > > > Again, this suggests once again there should not be two copies of
> > > > GetGpuPreferencesFromCommandLine. Maybe it should be a method in the
> public
> > > > content API?
> > > 
> > > I would like to put GetGpuPreferencesFromCommandLine() in content/public,
> but
> > it
> > > has a little problem. Seems content/public/common are used by browser, gpu
> and
> > > renderer processes, but this function uses gpu::GpuPreferences and it is
in
> > the
> > > gpu/command_buffer/service . I believe gpu service code should not be used
> by
> > > renderer and browser processes indirectly, right? Any suggestion?
> > 
> > Webview only needs it from browser side, so the public API only needs to be
in
> > content/public/browser. But inside content, it's needed from child as well.
I
> > don't know if there is any precedence for something like that. Ask piman or
> > another content owner maybe?
> 
> piman@ LGTMed the duplicate code in DeferredGpuCommandService & GpuChildThread
> (I think he doesn't object it as a short term solution, but he is OOO, I
cannot
> confirm it right now). In followup CLs, we will get rid of gpu related
switches
> by sending gpu::GpuPreferences from browser to gpu service. It will remove
those
> duplicate code.

My earlier point was that some of these switches should not be removed. How are
you going to deal with the duplicated code then?

> I filed a new issue for it(http://crbug.com/590825), and added it in the TODO
> inline comment.
>

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
On 2016/02/29 19:29:50, Peng wrote:
> On 2016/02/29 16:26:40, boliu wrote:
> > On 2016/02/29 16:09:22, Peng wrote:
> > > On 2016/02/26 22:17:32, boliu wrote:
> > > > these should be const&
> > > 
> > > pointer is necessary, so the caller can use a nullptr and the
GpuChildThread
> > > will construct a GpuPreferences by itself.
> > 
> > Then maybe you want another constructor instead.
> 
> Do we really need another constructor with duplicate code in it, since const
> pointer is allowed by our code style?

Just a general preference to not have parameters that can be missing if at all
possible. It's easy to have a mistake become a silent failure.

cs.chromium.org claims there is only a single callsite to each constructor, both
from content/gpu, so you don't have to duplicate more code by making
GpuPreference a required argument in these constructors.

https://codereview.chromium.org/1716813002/diff/320001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/1716813002/diff/320001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.h:19: #include
"gpu/command_buffer/service/gpu_preferences.h"
move to .cc file

[Peng, 2016-02-29 20:30:50]

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

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:225: *gpu_preferences :
GetGpuPreferencesFromCommandLine()),
On 2016/02/29 19:48:22, boliu wrote:
> On 2016/02/29 19:29:50, Peng wrote:
> > On 2016/02/29 16:26:40, boliu wrote:
> > > On 2016/02/29 16:09:22, Peng wrote:
> > > > On 2016/02/26 22:17:32, boliu wrote:
> > > > > Don't do this. Either make the caller construct a GpuPreferences, or
> don't
> > > > have
> > > > > the parameter at all.
> > > > 
> > > > Removed the gpu_preferences for this constructor. But for the other
> > > > constructor, may I ask why we cannot use a pointer there?
> > > 
> > > const* is generally against the style guide. Either it should be const&,
or
> > just
> > > non-const* for an output parameter.
> > 
> > https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> > 
> > Base on google code style. non-const references is disallowed, but const
> pointer
> > is OK.
> > 
> > > 
> > > > The GpuChildThread are
> > > > created in several places. To modify there places to supply a
> GpuPreferences
> > > is
> > > > inconvenient. That is why piman@ suggested we put the GpuPreference and
> > parse
> > > > the command line in this class.
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > Again, this suggests once again there should not be two copies of
> > > > > GetGpuPreferencesFromCommandLine. Maybe it should be a method in the
> > public
> > > > > content API?
> > > > 
> > > > I would like to put GetGpuPreferencesFromCommandLine() in
content/public,
> > but
> > > it
> > > > has a little problem. Seems content/public/common are used by browser,
gpu
> > and
> > > > renderer processes, but this function uses gpu::GpuPreferences and it is
> in
> > > the
> > > > gpu/command_buffer/service . I believe gpu service code should not be
used
> > by
> > > > renderer and browser processes indirectly, right? Any suggestion?
> > > 
> > > Webview only needs it from browser side, so the public API only needs to
be
> in
> > > content/public/browser. But inside content, it's needed from child as
well.
> I
> > > don't know if there is any precedence for something like that. Ask piman
or
> > > another content owner maybe?
> > 
> > piman@ LGTMed the duplicate code in DeferredGpuCommandService &
GpuChildThread
> > (I think he doesn't object it as a short term solution, but he is OOO, I
> cannot
> > confirm it right now). In followup CLs, we will get rid of gpu related
> switches
> > by sending gpu::GpuPreferences from browser to gpu service. It will remove
> those
> > duplicate code.
> 
> My earlier point was that some of these switches should not be removed. How
are
> you going to deal with the duplicated code then?
> 
> > I filed a new issue for it(http://crbug.com/590825), and added it in the
TODO
> > inline comment.
> > 
> 

If some switches cannot be removed, we will only removed them from gpu process's
command line, and keep them for browser process's command line, so we will only
parse command line in browser code (process), and pass the preferences to gpu
service code (process) by IPC or a parameter.

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
On 2016/02/29 19:48:22, boliu wrote:
> On 2016/02/29 19:29:50, Peng wrote:
> > On 2016/02/29 16:26:40, boliu wrote:
> > > On 2016/02/29 16:09:22, Peng wrote:
> > > > On 2016/02/26 22:17:32, boliu wrote:
> > > > > these should be const&
> > > > 
> > > > pointer is necessary, so the caller can use a nullptr and the
> GpuChildThread
> > > > will construct a GpuPreferences by itself.
> > > 
> > > Then maybe you want another constructor instead.
> > 
> > Do we really need another constructor with duplicate code in it, since const
> > pointer is allowed by our code style?
> 
> Just a general preference to not have parameters that can be missing if at all
> possible. It's easy to have a mistake become a silent failure.
> 
> http://cs.chromium.org claims there is only a single callsite to each
constructor, both
> from content/gpu, so you don't have to duplicate more code by making
> GpuPreference a required argument in these constructors.

I do agree it is better to avoid using const pointer in general. But in this
case, if use const reference,
1. we have to put GetGpuPreferences() in somewhere both gpu_main.cc and
in_process_gpu_thread.cc can access.
2. The InProcessGpuThread also need keep a duplicate GpuPreference instance.
3. With follow up CLs, the parameter gpu_preferences will be changed to const
reference.
So I think it is not necessary to do it in this short term CL.

https://codereview.chromium.org/1716813002/diff/320001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/1716813002/diff/320001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.h:19: #include
"gpu/command_buffer/service/gpu_preferences.h"
On 2016/02/29 19:48:22, boliu wrote:
> move to .cc file

Done.

[boliu, 2016-02-29 20:40:07]

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
On 2016/02/29 20:30:50, Peng wrote:
> On 2016/02/29 19:48:22, boliu wrote:
> > On 2016/02/29 19:29:50, Peng wrote:
> > > On 2016/02/29 16:26:40, boliu wrote:
> > > > On 2016/02/29 16:09:22, Peng wrote:
> > > > > On 2016/02/26 22:17:32, boliu wrote:
> > > > > > these should be const&
> > > > > 
> > > > > pointer is necessary, so the caller can use a nullptr and the
> > GpuChildThread
> > > > > will construct a GpuPreferences by itself.
> > > > 
> > > > Then maybe you want another constructor instead.
> > > 
> > > Do we really need another constructor with duplicate code in it, since
const
> > > pointer is allowed by our code style?
> > 
> > Just a general preference to not have parameters that can be missing if at
all
> > possible. It's easy to have a mistake become a silent failure.
> > 
> > http://cs.chromium.org claims there is only a single callsite to each
> constructor, both
> > from content/gpu, so you don't have to duplicate more code by making
> > GpuPreference a required argument in these constructors.
> 
> I do agree it is better to avoid using const pointer in general. But in this
> case, if use const reference,
> 1. we have to put GetGpuPreferences() in somewhere both gpu_main.cc and
> in_process_gpu_thread.cc can access.

content/gpu then? You can just expose that as a static method on GpuChildThread.

> 2. The InProcessGpuThread also need keep a duplicate GpuPreference instance.

That's better than const* imo.

> 3. With follow up CLs, the parameter gpu_preferences will be changed to const
> reference.
> So I think it is not necessary to do it in this short term CL.

[Peng, 2016-02-29 21:18:30]

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1716813002/diff/280001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.h:54: GpuChildThread(const gpu::GpuPreferences*
gpu_preferences,
On 2016/02/29 20:40:07, boliu wrote:
> On 2016/02/29 20:30:50, Peng wrote:
> > On 2016/02/29 19:48:22, boliu wrote:
> > > On 2016/02/29 19:29:50, Peng wrote:
> > > > On 2016/02/29 16:26:40, boliu wrote:
> > > > > On 2016/02/29 16:09:22, Peng wrote:
> > > > > > On 2016/02/26 22:17:32, boliu wrote:
> > > > > > > these should be const&
> > > > > > 
> > > > > > pointer is necessary, so the caller can use a nullptr and the
> > > GpuChildThread
> > > > > > will construct a GpuPreferences by itself.
> > > > > 
> > > > > Then maybe you want another constructor instead.
> > > > 
> > > > Do we really need another constructor with duplicate code in it, since
> const
> > > > pointer is allowed by our code style?
> > > 
> > > Just a general preference to not have parameters that can be missing if at
> all
> > > possible. It's easy to have a mistake become a silent failure.
> > > 
> > > http://cs.chromium.org claims there is only a single callsite to each
> > constructor, both
> > > from content/gpu, so you don't have to duplicate more code by making
> > > GpuPreference a required argument in these constructors.
> > 
> > I do agree it is better to avoid using const pointer in general. But in this
> > case, if use const reference,
> > 1. we have to put GetGpuPreferences() in somewhere both gpu_main.cc and
> > in_process_gpu_thread.cc can access.
> 
> content/gpu then? You can just expose that as a static method on
GpuChildThread.
> 
> > 2. The InProcessGpuThread also need keep a duplicate GpuPreference instance.
> 
> That's better than const* imo.
> 
> > 3. With follow up CLs, the parameter gpu_preferences will be changed to
const
> > reference.
> > So I think it is not necessary to do it in this short term CL.
> 

Done.

[Peng, 2016-02-29 21:18:51]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-02-29 21:18:51]

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

[commit-bot: I haz the power, 2016-02-29 21:19:22]

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

[commit-bot: I haz the power, 2016-02-29 21:43:05]

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

[commit-bot: I haz the power, 2016-02-29 21:43:07]

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...)

[Fady Samuel, 2016-02-29 21:54:23]

mus lgtm

[Peng, 2016-02-29 21:56:06]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2016-02-29 21:57:00]

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

[commit-bot: I haz the power, 2016-02-29 23:07:38]

Description was changed from

==========
Use GpuPreferences to avoid directly accessing switches in gpu related code

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use GpuPreferences to avoid directly accessing switches in gpu related code

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-02-29 23:07:39]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-02-29 23:09:08]

Description was changed from

==========
Use GpuPreferences to avoid directly accessing switches in gpu related code

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use GpuPreferences to avoid directly accessing switches in gpu related code

BUG=586369
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/7404df98850f7d6e4bfc0f88faf6629c3ded36e8
Cr-Commit-Position: refs/heads/master@{#378305}
==========

[commit-bot: I haz the power, 2016-02-29 23:09:10]

Patchset 20 (id:??) landed as
https://crrev.com/7404df98850f7d6e4bfc0f88faf6629c3ded36e8
Cr-Commit-Position: refs/heads/master@{#378305}