Mandoline: Add WebGL support

This change adds WebGL support in mandoline. It also adds GL
chromium_framebuffer_multisample extension which is need by
WebGL.

TODO:
  * Pass attributes to GPU when create WebGL context.
  * Get GPU info and fill it into glinfo.

TEST= mandoline --enable-webgl http://learningwebgl.com/lessons/lesson03/index.html
BUG=525159
NOPRESUBMIT=true

Committed: https://crrev.com/ab7a90daddbe7d44558d8703b12204570ae98431
Cr-Commit-Position: refs/heads/master@{#349206}

[Peng, 2015-09-14 19:19:59]

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

[Peng, 2015-09-14 19:20:00]

Hi Scott & Antoine, PTAL. Thanks.

[Fady Samuel, 2015-09-14 19:21:43]

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

[Fady Samuel, 2015-09-14 19:21:43]

Why is this a switch? Is there any reason not to always keep WebGL on?

https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
File components/html_viewer/blink_platform_impl.cc (right):

https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
components/html_viewer/blink_platform_impl.cc:182: if
(command_line->HasSwitch(kEnableWebGLSwitch)) {
Why make this a switch?

[Peng, 2015-09-14 19:28:46]

https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
File components/html_viewer/blink_platform_impl.cc (right):

https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
components/html_viewer/blink_platform_impl.cc:182: if
(command_line->HasSwitch(kEnableWebGLSwitch)) {
On 2015/09/14 19:21:43, Fady Samuel wrote:
> Why make this a switch?

I tested it with some simple webgl pages, it works fine. But it could have
issues with some complex webgl pages. So having a switch is safer. WDYT?

[Fady Samuel, 2015-09-14 19:30:27]

On 2015/09/14 19:28:46, Peng wrote:
>
https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
> File components/html_viewer/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
> components/html_viewer/blink_platform_impl.cc:182: if
> (command_line->HasSwitch(kEnableWebGLSwitch)) {
> On 2015/09/14 19:21:43, Fady Samuel wrote:
> > Why make this a switch?
> 
> I tested it with some simple webgl pages, it works fine. But it could have
> issues with some complex webgl pages. So having a switch is safer. WDYT?

I'll defer to sky@ for a final decision but my thought was that since Mandoline
is a new project, we won't be breaking any users with a partially implemented
feature and so it's better to catch issues earlier than later.

[Peng, 2015-09-14 20:06:07]

On 2015/09/14 19:30:27, Fady Samuel wrote:
> On 2015/09/14 19:28:46, Peng wrote:
> >
>
https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
> > File components/html_viewer/blink_platform_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1338433002/diff/100001/components/html_viewer...
> > components/html_viewer/blink_platform_impl.cc:182: if
> > (command_line->HasSwitch(kEnableWebGLSwitch)) {
> > On 2015/09/14 19:21:43, Fady Samuel wrote:
> > > Why make this a switch?
> > 
> > I tested it with some simple webgl pages, it works fine. But it could have
> > issues with some complex webgl pages. So having a switch is safer. WDYT?
> 
> I'll defer to sky@ for a final decision but my thought was that since
Mandoline
> is a new project, we won't be breaking any users with a partially implemented
> feature and so it's better to catch issues earlier than later.

You are right. I updated CL to enable WebGL by default.

[piman, 2015-09-14 21:17:12]

https://codereview.chromium.org/1338433002/diff/120001/components/html_viewer...
File components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1338433002/diff/120001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:62: void
WebGraphicsContext3DCommandBufferImpl::ContextLost() {
You'll need to wire that to context_lost_callback_. See e.g.
WebGraphicsContext3DCommandBufferImpl::OnContextLost.

https://codereview.chromium.org/1338433002/diff/120001/gpu/command_buffer/bui...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1338433002/diff/120001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:4855:
func.GetInfo("extension_flag") == "chromium_framebuffer_multisample":
Can you just add the proper 'extension': line to the relevant calls instead of
special-casing?

[Peng, 2015-09-15 15:07:19]

Patchset #8 (id:140001) has been deleted

[Peng, 2015-09-15 15:07:30]

https://codereview.chromium.org/1338433002/diff/120001/components/html_viewer...
File components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1338433002/diff/120001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:62: void
WebGraphicsContext3DCommandBufferImpl::ContextLost() {
On 2015/09/14 21:17:12, piman (slow to review) wrote:
> You'll need to wire that to context_lost_callback_. See e.g.
> WebGraphicsContext3DCommandBufferImpl::OnContextLost.

Done.

https://codereview.chromium.org/1338433002/diff/120001/gpu/command_buffer/bui...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1338433002/diff/120001/gpu/command_buffer/bui...
gpu/command_buffer/build_gles2_cmd_buffer.py:4855:
func.GetInfo("extension_flag") == "chromium_framebuffer_multisample":
On 2015/09/14 21:17:12, piman (slow to review) wrote:
> Can you just add the proper 'extension': line to the relevant calls instead of
> special-casing?

Done.

[piman, 2015-09-15 20:00:39]

lgtm

[sky, 2015-09-15 22:10:59]

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
File components/html_viewer/blink_platform_impl.h (right):

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/blink_platform_impl.h:80: blink::WebGraphicsContext3D*
shareContext);
share_context (make sure all your names match chromium style, not blink style).

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
File components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:56: if
(!app)
Under what circumstances is app null?

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:60: 0,
app, active_url, attributes, shareContext, glInfo);
I don't see surface_id_ used. Is it needed?

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
File components/html_viewer/web_graphics_context_3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.h:53: int
surface_id_;
const

https://codereview.chromium.org/1338433002/diff/160001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1338433002/diff/160001/components/mus/gles2/c...
components/mus/gles2/command_buffer_driver.cc:118: if
(!decoder_->Initialize(surface_, context_, true /* offscreen */,
Should the comment read onscreen now?

[Fady Samuel, 2015-09-16 00:15:39]

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
File components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:25:
request->url = mojo::String::From("mojo:view_manager");
mojo:mus now.

[Peng, 2015-09-16 13:54:59]

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
File components/html_viewer/blink_platform_impl.h (right):

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/blink_platform_impl.h:80: blink::WebGraphicsContext3D*
shareContext);
On 2015/09/15 22:10:58, sky wrote:
> share_context (make sure all your names match chromium style, not blink
style).

Done.

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
File components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:25:
request->url = mojo::String::From("mojo:view_manager");
On 2015/09/16 00:15:39, Fady Samuel wrote:
> mojo:mus now.

Done.

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:56: if
(!app)
On 2015/09/15 22:10:58, sky wrote:
> Under what circumstances is app null?

Done.

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.cc:60: 0,
app, active_url, attributes, shareContext, glInfo);
On 2015/09/15 22:10:58, sky wrote:
> I don't see surface_id_ used. Is it needed?

It is not useful right now. Removed.

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
File components/html_viewer/web_graphics_context_3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/1338433002/diff/160001/components/html_viewer...
components/html_viewer/web_graphics_context_3d_command_buffer_impl.h:53: int
surface_id_;
On 2015/09/15 22:10:58, sky wrote:
> const

Removed it for now.

https://codereview.chromium.org/1338433002/diff/160001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1338433002/diff/160001/components/mus/gles2/c...
components/mus/gles2/command_buffer_driver.cc:118: if
(!decoder_->Initialize(surface_, context_, true /* offscreen */,
On 2015/09/15 22:10:59, sky wrote:
> Should the comment read onscreen now?

Actually not. The parameter's name is offscreen.

I changed the code to make it more readable.

Done

[sky, 2015-09-16 19:55:36]

LGTM

[Peng, 2015-09-16 19:56:16]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2015-09-16 19:56:16]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1338433002/#ps180001
(title: "Address review issues")

[commit-bot: I haz the power, 2015-09-16 19:56:50]

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

[Peng, 2015-09-16 20:08:37]

The CQ bit was unchecked by penghuang@chromium.org

[Peng, 2015-09-16 20:08:39]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2015-09-16 20:08:42]

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

[commit-bot: I haz the power, 2015-09-16 20:08:43]

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, 2015-09-16 20:08:46]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2015-09-16 20:09:33]

The CQ bit was unchecked by penghuang@chromium.org

[Peng, 2015-09-16 20:09:38]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2015-09-16 20:10:01]

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

[commit-bot: I haz the power, 2015-09-16 20:11:41]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-09-16 20:12:13]

Patchset 9 (id:??) landed as
https://crrev.com/ab7a90daddbe7d44558d8703b12204570ae98431
Cr-Commit-Position: refs/heads/master@{#349206}

[Dirk Pranke, 2015-09-16 20:54:04]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/1343303003/ by dpranke@chromium.org.

The reason for reverting is: Unfortunately, this broke `gn check` with an
improper dependency. 

http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32063/s...

I have a patch in-flight that will re-enable gn check on the trybots so that
this breakages don't get through, but in the meantime I'm reverting this.

Sorry!.

[Peng, 2015-09-16 21:10:35]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2015-09-16 21:10:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1338433002/#ps200001
(title: "Fix gn check issue")

[commit-bot: I haz the power, 2015-09-16 21:11:33]

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

[commit-bot: I haz the power, 2015-09-16 21:51:37]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-09-16 21:52:12]

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

[commit-bot: I haz the power, 2015-09-23 12:59:58]

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