Allow WebGL on Android if the GPU has it's own process

Allow WebGL on Android if the GPU has it's own process

BUG=512992
Committed: https://crrev.com/29443deeb43de94ed4c8cd38329f1527e4641d03
Cr-Commit-Position: refs/heads/master@{#340198}

[bajones, 2015-07-22 23:10:56]

bajones@chromium.org changed reviewers:
+ kbr@chromium.org, mkwst@chromium.org, zmo@chromium.org

[bajones, 2015-07-22 23:10:56]

kbr@, zmo@: Please take a look!
mkwst@: Please review the one-line change gpu_messages

[Zhenyao Mo, 2015-07-22 23:18:16]

On 2015/07/22 23:10:56, bajones wrote:
> kbr@, zmo@: Please take a look!
> mkwst@: Please review the one-line change gpu_messages

Looks good to me, but can you add a unittest in gpu_control_list_unittest.cc for
this new field?

[Ken Russell (switch to Gerrit), 2015-07-22 23:42:23]

+aelias and sievers as FYI

[aelias_OOO_until_Jul13, 2015-07-23 00:03:28]

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

[aelias_OOO_until_Jul13, 2015-07-23 00:03:28]

https://codereview.chromium.org/1247093006/diff/20001/gpu/config/software_ren...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/1247093006/diff/20001/gpu/config/software_ren...
gpu/config/software_rendering_list_json.cc:740: "exceptions": [
"exceptions"-based rules are confusing, and I don't think there's much point in
leaving the "gl_reset_notification_strategy" and "EXT_robustness" entries in
here, since they'd cause WebGL to be enabled on a subset of 512MB devices and
we've decided we don't trust them enough anyway.  How about making a clean sweep
of this rule and just writing:

"description": "WebGL is disabled when the GPU driver is not isolated in a
separate process",
"os": {
  "type": "android"
},
"in_process_gpu": true,
"features": [
  "webgl"
]

[Zhenyao Mo, 2015-07-23 01:34:33]

On 2015/07/23 00:03:28, aelias wrote:
>
https://codereview.chromium.org/1247093006/diff/20001/gpu/config/software_ren...
> File gpu/config/software_rendering_list_json.cc (right):
> 
>
https://codereview.chromium.org/1247093006/diff/20001/gpu/config/software_ren...
> gpu/config/software_rendering_list_json.cc:740: "exceptions": [
> "exceptions"-based rules are confusing, and I don't think there's much point
in
> leaving the "gl_reset_notification_strategy" and "EXT_robustness" entries in
> here, since they'd cause WebGL to be enabled on a subset of 512MB devices and
> we've decided we don't trust them enough anyway.  How about making a clean
sweep
> of this rule and just writing:
> 
> "description": "WebGL is disabled when the GPU driver is not isolated in a
> separate process",
> "os": {
>   "type": "android"
> },
> "in_process_gpu": true,
> "features": [
>   "webgl"
> ]

LGTM after aelias's comment is addressed.

[Mike West, 2015-07-23 08:05:25]

LGTM % bikeshed

https://codereview.chromium.org/1247093006/diff/20001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/1247093006/diff/20001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:210: bool in_process_gpu;
I initially read "In process GPU" as meaning "I'm in the process named 'GPU'!"
Would you mind renaming this to something like 'running_in_browser_process'? Or
"inprocess_gpu", I suppose, if "in-process" is well-understood by GPU folks.

[bajones, 2015-07-23 16:54:02]

On 2015/07/23 08:05:25, Mike West wrote:
>
https://codereview.chromium.org/1247093006/diff/20001/gpu/config/gpu_info.h#n...
> gpu/config/gpu_info.h:210: bool in_process_gpu;
> I initially read "In process GPU" as meaning "I'm in the process named 'GPU'!"
> Would you mind renaming this to something like 'running_in_browser_process'?
Or
> "inprocess_gpu", I suppose, if "in-process" is well-understood by GPU folks.

I originally was going to use "own_gpu_process", but the command line for
gpu-in-the-browser-process mode is "--in-process-gpu", and it's referred to
several times internally as such. I figured I was better off not inventing yet
another term for that mode. That said, I have no attachment to the phrasing and
am happy to change it if you feel strongly about it.

[bajones, 2015-07-23 17:27:05]

On 2015/07/23 00:03:28, aelias wrote:
> How about making a clean sweep
> of this rule and just writing [...]

Good call. Done!

[aelias_OOO_until_Jul13, 2015-07-23 18:35:48]

lgtm modulo comment

https://codereview.chromium.org/1247093006/diff/60001/gpu/config/software_ren...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/1247093006/diff/60001/gpu/config/software_ren...
gpu/config/software_rendering_list_json.cc:736: "description": "WebGL is
disabled on Android unless the GPU runs in a separate process or GPU reset
notifications are supported",
This description is out of date now.

[bajones, 2015-07-23 19:55:39]

On 2015/07/23 18:35:48, aelias wrote:
> This description is out of date now.

Thanks for catching that. Fixed.

[Ken Russell (switch to Gerrit), 2015-07-23 20:09:29]

lgtm

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

https://codereview.chromium.org/1247093006/diff/80001/content/gpu/gpu_main.cc...
content/gpu/gpu_main.cc:227: gpu_info.in_process_gpu = false;
Out of curiosity do you know where this would be set to true if the default for
GPUInfo::in_process_gpu were false?

https://codereview.chromium.org/1247093006/diff/80001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/1247093006/diff/80001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:209: // True if the GPU is running in the browser process
instead of it's own.
nit: it's -> its

[bajones, 2015-07-23 20:17:42]

On 2015/07/23 20:09:29, Ken Russell wrote:
> Out of curiosity do you know where this would be set to true if the default
for
> GPUInfo::in_process_gpu were false?

That's not as clear to me, but I think it's in the GpuChildThread constructor
called by InProcessGpuThread::Init().
(See:
https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_ch...)

> nit: it's -> its

Done.

[bajones, 2015-07-23 21:05:54]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-07-23 21:05:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
zmo@chromium.org, aelias@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1247093006/#ps100001
(title: "Grammar fix")

[commit-bot: I haz the power, 2015-07-23 21:07:22]

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

[commit-bot: I haz the power, 2015-07-23 22:57:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-23 22:58:42]

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

[alancutter (OOO until 2018), 2015-07-24 09:40:31]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1252293002/ by alancutter@chromium.org.

The reason for reverting is: Change suspected to have caused the Android
builders to start failing the following tests:

WebglConformance.conformance_attribs_gl_enable_vertex_attrib
WebglConformance.conformance_attribs_gl_vertex_attrib
WebglConformance.conformance_attribs_gl_vertexattribpointer
WebglConformance.conformance_attribs_gl_vertexattribpointer_offsets
WebglConformance.conformance_attribs_gl_vertex_attrib_zero_issues
WebglConformance.conformance_buffers_buffer_bind_test
WebglConformance.conformance_buffers_buffer_data_array_buffer
WebglConformance.conformance_buffers_index_validation_copies_indices
WebglConformance.conformance_buffers_index_validation_crash_with_buffer_sub_data
WebglConformance.conformance_buffers_index_validation_verifies_too_many_indices
WebglConformance.conformance_buffers_index_validation_with_resized_buffer

See build logs:
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus....