Add initial support for NV_path_rendering extension to gpu command buffer

Add initial support for NV_path_rendering extension to gpu command buffer

Add initial support for NV_path_rendering extension to gpu command
buffer. This is needed in order to support NV_path_rendering
implementation in Skia.

Implement this by creating a new GL ES pseudo-extension called
CHROMIUM_path_rendering. The CHROMIUM_path_rendering extension
will be exposed to clients only if the context is not a webgl context,
NV_path_rendering extension is present. Also, EXT_direct_state_access
extension must be present for Open GL.

Add matrix functions needed to support the feature:

void glMatrixLoadfEXT(GLenum matrixMode, const GLfloat* m)
void glMatrixLoadIdentityEXT(GLenum matrixMode)

NV_path_rendering needs these functions to implement transformations
for the path drawing.

Changes the build_gles2_cmd_buffer.py to be able to generate context
state variables that are arrays. The state variable is defined to be an
an array when its default value is an array.

Changes the build_gles2_cmd_buffer.py to round the float GL state
variables with round() when querying them with GetInteger.

The values GL_*_CHROMIUM tokens added match the values of corresponding
GL_*_NV tokens from the NV_path_rendering extensions. This is why the
values are passed after validation to the receiving GL function, without
any mapping.

BUG=344330
Committed: https://crrev.com/337d59638d630d0892b444ca2ab0a2989921d00e
Cr-Commit-Position: refs/heads/master@{#291878}

[Kimmo Kinnunen, 2014-02-18 16:18:20]

Piman, Darin, Vangelis,
I wonder who could be best person to maybe look at the issue, maybe even the
code, and give some thoughts?

[Vangelis Kokkevis, 2014-03-12 21:04:46]

On 2014/02/18 16:18:20, kkinnunen wrote:
> Piman, Darin, Vangelis,
> I wonder who could be best person to maybe look at the issue, maybe even the
> code, and give some thoughts?

Sorry for the much delayed response, this review completely fell of my radar. 

What would be really helpful before any code gets committed would be a design
doc that outlines what the proposed solution is, how it will integrate with the
rest of the Skia and Chrome GPU code, what would be the list of exposed entry
points, etc. 

As a side node, what would make the additional entry points more palatable would
be if we could generalize the extension entry points to the extent that a
vanilla GL ES implementation can be written for drivers that don't natively
support that functionality. Is that a possibility?

[Kimmo Kinnunen, 2014-03-13 13:48:18]

On 2014/03/12 21:04:46, Vangelis Kokkevis wrote:
> On 2014/02/18 16:18:20, kkinnunen wrote:
> > Piman, Darin, Vangelis,
> > I wonder who could be best person to maybe look at the issue, maybe even the
> > code, and give some thoughts?
> 
> Sorry for the much delayed response, this review completely fell of my radar. 
> 
> What would be really helpful before any code gets committed would be a design
> doc that outlines what the proposed solution is, how it will integrate with
the
> rest of the Skia and Chrome GPU code, what would be the list of exposed entry
> points, etc. 

Thanks.
Feel free to modify any errors:
https://docs.google.com/document/d/1bpmw-VPEdODdtkNLMPUVzliFOMvTNENwuEs5zNOOE...
 
> As a side node, what would make the additional entry points more palatable
would
> be if we could generalize the extension entry points to the extent that a
> vanilla GL ES implementation can be written for drivers that don't natively
> support that functionality. Is that a possibility?

Tried to answer the question in the doc. You can also formulate the feedback in
more direct fashion, if there's need.

[Vangelis Kokkevis, 2014-03-14 05:37:12]

On 2014/03/13 13:48:18, kkinnunen wrote:
> On 2014/03/12 21:04:46, Vangelis Kokkevis wrote:
> > On 2014/02/18 16:18:20, kkinnunen wrote:
> > > Piman, Darin, Vangelis,
> > > I wonder who could be best person to maybe look at the issue, maybe even
the
> > > code, and give some thoughts?
> > 
> > Sorry for the much delayed response, this review completely fell of my
radar. 
> > 
> > What would be really helpful before any code gets committed would be a
design
> > doc that outlines what the proposed solution is, how it will integrate with
> the
> > rest of the Skia and Chrome GPU code, what would be the list of exposed
entry
> > points, etc. 
> 
> Thanks.
> Feel free to modify any errors:
>
https://docs.google.com/document/d/1bpmw-VPEdODdtkNLMPUVzliFOMvTNENwuEs5zNOOE...
>  

Thank you, that's very helpful.  I left a couple of comments directly in the
doc.


> > As a side node, what would make the additional entry points more palatable
> would
> > be if we could generalize the extension entry points to the extent that a
> > vanilla GL ES implementation can be written for drivers that don't natively
> > support that functionality. Is that a possibility?
> 
> Tried to answer the question in the doc. You can also formulate the feedback
in
> more direct fashion, if there's need.

[vmiura, 2014-04-03 22:39:34]

On 2014/03/13 13:48:18, kkinnunen wrote:
> On 2014/03/12 21:04:46, Vangelis Kokkevis wrote:
> > On 2014/02/18 16:18:20, kkinnunen wrote:
> > > Piman, Darin, Vangelis,
> > > I wonder who could be best person to maybe look at the issue, maybe even
the
> > > code, and give some thoughts?
> > 
> > Sorry for the much delayed response, this review completely fell of my
radar. 
> > 
> > What would be really helpful before any code gets committed would be a
design
> > doc that outlines what the proposed solution is, how it will integrate with
> the
> > rest of the Skia and Chrome GPU code, what would be the list of exposed
entry
> > points, etc. 
> 
> Thanks.
> Feel free to modify any errors:
>
https://docs.google.com/document/d/1bpmw-VPEdODdtkNLMPUVzliFOMvTNENwuEs5zNOOE...
>  
> > As a side node, what would make the additional entry points more palatable
> would
> > be if we could generalize the extension entry points to the extent that a
> > vanilla GL ES implementation can be written for drivers that don't natively
> > support that functionality. Is that a possibility?
> 
> Tried to answer the question in the doc. You can also formulate the feedback
in
> more direct fashion, if there's need.

Kimmo, I'll be available to look at this project.  I hope we can start moving
things forward.

Is this CL still up to date for review?

[Kimmo Kinnunen, 2014-04-04 05:01:53]

On 2014/04/03 22:39:34, vmiura wrote:
> Kimmo, I'll be available to look at this project.  I hope we can start moving
> things forward.

Great!

> Is this CL still up to date for review?

Yep

[vmiura, 2014-04-11 21:54:48]

Looks good.  Some small comments.

Also could please rebase, and re-run  
pu/command_buffer/build_gles2_cmd_buffer.py as formatting has changed.

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:28: PROJECTION     
                                  0x1701
Should add GL_MATRIX_MODE?

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc:50:
EXPECT_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError());
Can we add a test of reading back the matrices?

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h
File ppapi/c/ppb_opengles2.h (right):

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h...
ppapi/c/ppb_opengles2.h:502: void (*LoadIdentity)(PP_Resource context);
Should the other APIs also be added here?

void glMatrixMode(GLenum mode) 
void glLoadMatrixf(const GLfloat* m)

https://codereview.chromium.org/169603002/diff/200001/ppapi/lib/gl/gles2/gles2.c
File ppapi/lib/gl/gles2/gles2.c (right):

https://codereview.chromium.org/169603002/diff/200001/ppapi/lib/gl/gles2/gles...
ppapi/lib/gl/gles2/gles2.c:1068: }
... also here?

void glMatrixMode(GLenum mode) 
void glLoadMatrixf(const GLfloat* m)

[piman, 2014-04-11 22:19:27]

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:3:
CHROMIUM_NV_path_rendering
nit: s/CHROMIUM_NV/CHROMIUM/

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:19: This extensions
is pretty much the same as NV_path_rendering
Can we document the extension independently of the existing other extension?

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:19: This extensions
is pretty much the same as NV_path_rendering
Also, I don't see any path rendering in this extension...

Should we have 2 extensions?
e.g.
GL_CHROMIUM_transform_matrix
GL_CHROMIUM_path_rendering

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/gl2chromium.h
File gpu/GLES2/gl2chromium.h (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/gl2chromium.h...
gpu/GLES2/gl2chromium.h:13: #define GL_MATRIX_MODE 0x0BA0
nit: GL_MATRIX_MODE_CHROMIUM (same below)

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/cmd_...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/cmd_...
gpu/command_buffer/cmd_buffer_functions.txt:227: GL_APICALL void        
GL_APIENTRY glLoadIdentity (void);
nit: These shouldn't be in the global GL namespace. We need to tag the entry
points as CHROMIUM.

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:782:
AddExtensionString("GL_CHROMIUM_NV_path_rendering");
nit: GL_CHROMIUM_path_rendering

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info_unittest.cc:995: TEST_F(FeatureInfoTest,
InitializeCHROMIUM_NV_path_rendering) {
nit:  InitializeCHROMIUM_path_rendering

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc:16: class
CHROMIUMNVPathRenderingTest : public testing::Test {
nit: s/CHROMIUMNV/CHROMIUM/

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h
File ppapi/c/ppb_opengles2.h (right):

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h...
ppapi/c/ppb_opengles2.h:502: void (*LoadIdentity)(PP_Resource context);
On 2014/04/11 21:54:48, vmiura wrote:
> Should the other APIs also be added here?
> 
> void glMatrixMode(GLenum mode) 
> void glLoadMatrixf(const GLfloat* m) 

Actually, we shouldn't change this interface, otherwise we can break existing
plugins. If we want to expose these as extensions, they need to be described as
such, like below.

https://codereview.chromium.org/169603002/diff/200001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/169603002/diff/200001/ui/gl/generate_bindings...
ui/gl/generate_bindings.py:837: 'arguments': 'GLenum mode' },
please tag on gl_versions 'gl2', 'gl3', 'gl4'
(same below)

[jbauman, 2014-04-11 22:25:24]

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2574: },
LoadIdentity (CHROMIUM) should be added here and should have "'extension': True"
to avoid messing with pepper.

[vmiura, 2014-04-11 22:49:49]

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h
File ppapi/c/ppb_opengles2.h (right):

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h...
ppapi/c/ppb_opengles2.h:502: void (*LoadIdentity)(PP_Resource context);
On 2014/04/11 22:19:27, piman wrote:
> On 2014/04/11 21:54:48, vmiura wrote:
> > Should the other APIs also be added here?
> > 
> > void glMatrixMode(GLenum mode) 
> > void glLoadMatrixf(const GLfloat* m) 
> 
> Actually, we shouldn't change this interface, otherwise we can break existing
> plugins. If we want to expose these as extensions, they need to be described
as
> such, like below.

Right.  Please follow @jbauman's comment to remove LoadIdentity from ppapi.

[Kimmo Kinnunen, 2014-04-14 12:56:29]

Thanks for the review.

I didn't add the existing plain-old-GL functions to the CHROMIUM extension
namespace as I thought it would create confusion as they're not extensions. Of
course they're extensions in GL ES, I suppose.

I can rename them, if that's what is best. 

I just want to confirm: the other option is to use EXT_direct_state_access.
Should this be used, or should I proceed with current one and CHROMIUM
namespace?

The advantage of EXT_d_s_a would maybe be less confusion, since the names would
be already in EXT namespace. Or at least different kind of confusion. Of course,
the problem with EXT_direct_state_access in this use-case is that the whole
extension probably should not be exposed, which in turn would mean CHROMIÚM
extension should maybe be used to avoid confusion... 

BTW: I dont know if you saw it, but the continuation of the patchset is in:
https://chromiumcodereview.appspot.com/169403005/

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:19: This extensions
is pretty much the same as NV_path_rendering
On 2014/04/11 22:19:27, piman wrote:
> Can we document the extension independently of the existing other extension?

Well, the path-rendering part is rather elaborate to document or
cut-and-paste-and-maintain.

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:19: This extensions
is pretty much the same as NV_path_rendering
On 2014/04/11 22:19:27, piman wrote:
> Also, I don't see any path rendering in this extension...

This is augmented in the next patch:
https://chromiumcodereview.appspot.com/169403005/


> Should we have 2 extensions?
> e.g.
> GL_CHROMIUM_transform_matrix
> GL_CHROMIUM_path_rendering

Well, personally I'd rather not split them. The transform_matrix extension would
be pointless in isolation, as only thing it can affect is the path rendering.

If split is a requirement, then I could of course cut-paste-and-maintain a
partial version of  direct_state_access extension?

http://www.opengl.org/registry/specs/EXT/direct_state_access.txt

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc:50:
EXPECT_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError());
On 2014/04/11 21:54:48, vmiura wrote:
> Can we add a test of reading back the matrices?

In theory it can be added. That entry point would never be used by the client
code. Do we want such dead code?

[piman, 2014-04-14 18:51:07]

On Mon, Apr 14, 2014 at 5:56 AM, <kkinnunen@nvidia.com> wrote:

> Thanks for the review.
>
> I didn't add the existing plain-old-GL functions to the CHROMIUM extension
> namespace as I thought it would create confusion as they're not
> extensions. Of
> course they're extensions in GL ES, I suppose.
>

Yes, they are... What we expose on the client side is GLES2, any additional
functionality is exposed through extensions.


> I can rename them, if that's what is best.
>

Please.


>
> I just want to confirm: the other option is to use EXT_direct_state_access.
> Should this be used, or should I proceed with current one and CHROMIUM
> namespace?


I don't have a strong opinion either way, but as you mention, we'd only
expose a subset, so it'd have to be a different extension anyway.

>


> The advantage of EXT_d_s_a would maybe be less confusion, since the names
> would
> be already in EXT namespace. Or at least different kind of confusion. Of
> course,
> the problem with EXT_direct_state_access in this use-case is that the whole
> extension probably should not be exposed, which in turn would mean CHROMIÚM
> extension should maybe be used to avoid confusion...
>
> BTW: I dont know if you saw it, but the continuation of the patchset is in:
> https://chromiumcodereview.appspot.com/169403005/
>
>
>
>
> https://codereview.chromium.org/169603002/diff/200001/gpu/
> GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt
> (right):
>
> https://codereview.chromium.org/169603002/diff/200001/gpu/
> GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt#newcode19
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:19: This
> extensions is pretty much the same as NV_path_rendering
> On 2014/04/11 22:19:27, piman wrote:
>
>> Can we document the extension independently of the existing other
>>
> extension?
>
> Well, the path-rendering part is rather elaborate to document or
> cut-and-paste-and-maintain.


NV_path_rendering is an extension on top of GL, this extension is
necessarily different, so please make it self-contained.

>
>
> https://codereview.chromium.org/169603002/diff/200001/gpu/
> GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt#newcode19
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:19: This
> extensions is pretty much the same as NV_path_rendering
> On 2014/04/11 22:19:27, piman wrote:
>
>> Also, I don't see any path rendering in this extension...
>>
>
> This is augmented in the next patch:
> https://chromiumcodereview.appspot.com/169403005/


I see...

Should we have 2 extensions?
>> e.g.
>> GL_CHROMIUM_transform_matrix
>> GL_CHROMIUM_path_rendering
>>
>
> Well, personally I'd rather not split them. The transform_matrix
> extension would be pointless in isolation, as only thing it can affect
> is the path rendering.
>

Ok.


>
> If split is a requirement, then I could of course cut-paste-and-maintain
> a partial version of  direct_state_access extension?
>
> http://www.opengl.org/registry/specs/EXT/direct_state_access.txt
>
>
> https://codereview.chromium.org/169603002/diff/200001/gpu/
> command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc
> File gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc
> (right):
>
> https://codereview.chromium.org/169603002/diff/200001/gpu/
> command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc#newcode50
> gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc:50:
> EXPECT_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError());
> On 2014/04/11 21:54:48, vmiura wrote:
>
>> Can we add a test of reading back the matrices?
>>
>
> In theory it can be added. That entry point would never be used by the
> client code.


Maybe for debugging?


> Do we want such dead code?


> https://codereview.chromium.org/169603002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmiura, 2014-04-14 19:08:29]

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc:50:
EXPECT_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError());
On 2014/04/14 12:56:30, kkinnunen wrote:
> On 2014/04/11 21:54:48, vmiura wrote:
> > Can we add a test of reading back the matrices?
> 
> In theory it can be added. That entry point would never be used by the client
> code. Do we want such dead code?

I guess we have GL_MATRIX_MODE, but not
GL_MODELVIEW_MATRIX/GL_PROJECTION_MATRIX.

I would suggest adding them so this can be tested in isolation of running a
complete NV path test.

[vmiura, 2014-04-14 19:25:11]

On 2014/04/14 12:56:29, kkinnunen wrote:
> I just want to confirm: the other option is to use EXT_direct_state_access.
> Should this be used, or should I proceed with current one and CHROMIUM
> namespace?
> 
> The advantage of EXT_d_s_a would maybe be less confusion, since the names
would
> be already in EXT namespace. Or at least different kind of confusion. Of
course,
> the problem with EXT_direct_state_access in this use-case is that the whole
> extension probably should not be exposed, which in turn would mean CHROMIÚM
> extension should maybe be used to avoid confusion... 

GL_EXT_direct_state_access APIs seem slightly better.  If planning to use them
later it may be better to implement that upfront?

Btw, we'll need to add support for restoring the new states in
ContextState::RestoreState().

[Kimmo Kinnunen, 2014-04-15 16:16:57]

Patch rebased and updated according to the review comments.
Changed away from glmatrixmode to direct_state_access -type functions in
CHROMIUM namespace.

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:3:
CHROMIUM_NV_path_rendering
On 2014/04/11 22:19:27, piman wrote:
> nit: s/CHROMIUM_NV/CHROMIUM/

Done.

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:19: This extensions
is pretty much the same as NV_path_rendering
On 2014/04/11 22:19:27, piman wrote:
> Can we document the extension independently of the existing other extension?

Done.

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_NV_path_rendering.txt:28: PROJECTION     
                                  0x1701
On 2014/04/11 21:54:48, vmiura wrote:
> Should add GL_MATRIX_MODE?

Removed.

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/gl2chromium.h
File gpu/GLES2/gl2chromium.h (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/GLES2/gl2chromium.h...
gpu/GLES2/gl2chromium.h:13: #define GL_MATRIX_MODE 0x0BA0
On 2014/04/11 22:19:27, piman wrote:
> nit: GL_MATRIX_MODE_CHROMIUM (same below)

Removed GL_MATRIX_MODE_CHROMIUM.

Done (for below).

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2574: },
On 2014/04/11 22:25:25, jbauman wrote:
> LoadIdentity (CHROMIUM) should be added here and should have "'extension':
True"
> to avoid messing with pepper.

Done.

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/cmd_...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/cmd_...
gpu/command_buffer/cmd_buffer_functions.txt:227: GL_APICALL void        
GL_APIENTRY glLoadIdentity (void);
On 2014/04/11 22:19:27, piman wrote:
> nit: These shouldn't be in the global GL namespace. We need to tag the entry
> points as CHROMIUM.

Done.

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.cc:782:
AddExtensionString("GL_CHROMIUM_NV_path_rendering");
On 2014/04/11 22:19:27, piman wrote:
> nit: GL_CHROMIUM_path_rendering

Done.

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info_unittest.cc:995: TEST_F(FeatureInfoTest,
InitializeCHROMIUM_NV_path_rendering) {
On 2014/04/11 22:19:27, piman wrote:
> nit:  InitializeCHROMIUM_path_rendering

Done.

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc:16: class
CHROMIUMNVPathRenderingTest : public testing::Test {
On 2014/04/11 22:19:27, piman wrote:
> nit: s/CHROMIUMNV/CHROMIUM/

Done.

https://codereview.chromium.org/169603002/diff/200001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_nv_path_rendering_unittest.cc:50:
EXPECT_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError());
On 2014/04/11 21:54:48, vmiura wrote:
> Can we add a test of reading back the matrices?

Done.

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h
File ppapi/c/ppb_opengles2.h (right):

https://codereview.chromium.org/169603002/diff/200001/ppapi/c/ppb_opengles2.h...
ppapi/c/ppb_opengles2.h:502: void (*LoadIdentity)(PP_Resource context);
On 2014/04/11 22:49:50, vmiura wrote:
> On 2014/04/11 22:19:27, piman wrote:
> > On 2014/04/11 21:54:48, vmiura wrote:
> > > Should the other APIs also be added here?
> > > 
> > > void glMatrixMode(GLenum mode) 
> > > void glLoadMatrixf(const GLfloat* m) 
> > 
> > Actually, we shouldn't change this interface, otherwise we can break
existing
> > plugins. If we want to expose these as extensions, they need to be described
> as
> > such, like below.
> 
> Right.  Please follow @jbauman's comment to remove LoadIdentity from ppapi.

Done.

https://codereview.chromium.org/169603002/diff/200001/ppapi/lib/gl/gles2/gles2.c
File ppapi/lib/gl/gles2/gles2.c (right):

https://codereview.chromium.org/169603002/diff/200001/ppapi/lib/gl/gles2/gles...
ppapi/lib/gl/gles2/gles2.c:1068: }
On 2014/04/11 21:54:48, vmiura wrote:
> ... also here?
> 
> void glMatrixMode(GLenum mode) 
> void glLoadMatrixf(const GLfloat* m) 

Done (removed).

https://codereview.chromium.org/169603002/diff/200001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/169603002/diff/200001/ui/gl/generate_bindings...
ui/gl/generate_bindings.py:837: 'arguments': 'GLenum mode' },
On 2014/04/11 22:19:27, piman wrote:
> please tag on gl_versions 'gl2', 'gl3', 'gl4'
> (same below)

Done.
Tagged as 'gl4'.

[Kimmo Kinnunen, 2014-04-15 17:52:27]

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:490: 'default': ['0.0f'] * 16,
This is a mistake, the these defaults should be identity matrices..

[vmiura, 2014-04-15 18:02:43]

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7129: len(item['default'])))
nit: %d for len(..)

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7173: file.Write("     
*num_written = %s;\n" % item_len)
nit: %d for item_len

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7177: (item['name'], item['type'],
item_len))
nit: %d for item_len

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7226: file.Write("  %s[%s] = %s;\n"
% (item['name'], ndx, value))
nit: %d for ndx

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7286: len(item['default'])))
nit: %d

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10410: || matrixMode ==
GL_MODELVIEW_CHROMIUM);
Instead of DCHECK, set GL error & return.

LOCAL_SET_GL_ERROR(
        GL_INVALID_ENUM,
        "DoMatrixLoadfCHROMIUM", "invalid enum");
    return;

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10422: || matrixMode ==
GL_MODELVIEW_CHROMIUM);
Same, LOCAL_SET_GL_ERROR instead of DCHECK.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_path_rendering_unittest.cc:34:
ASSERT_EQ(static_cast<T>(expected[i]), actual[i]);
nit: EXPECT_EQ instead of ASSERT_EQ?

[piman, 2014-04-15 22:30:22]

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10410: || matrixMode ==
GL_MODELVIEW_CHROMIUM);
On 2014/04/15 18:02:43, vmiura wrote:
> Instead of DCHECK, set GL error & return.
> 
> LOCAL_SET_GL_ERROR(
>         GL_INVALID_ENUM,
>         "DoMatrixLoadfCHROMIUM", "invalid enum");
>     return;

The check should already be done by the autogen'd code (see e.g.
GLES2DecoderImpl::HandleMatrixLoadfCHROMIUM in gles2_cmd_decoder_autogen.h)

However we need to raise GL_INVALID_OPERATION if the extension isn't present
(otherwise we will call a NULL pointer).
(same below)

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10414: if (memcmp(target_matrix,
matrix, sizeof(GLfloat) * 16)) {
Is it worth doing this comparison? We're walking 64 bytes to possibly avoid a
function call?
(same below).

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10423: static GLfloat
identity_matrix[16] = {
nit: const, and use kIdentityMatrix naming convention.

[Kimmo Kinnunen, 2014-04-23 12:26:54]

Thanks for the review. Here's the new version.

Additional changes:
* added proper glGetIntegerv rounding for the matrix values.
* correct default values for the matrix, tests for this

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:490: 'default': ['0.0f'] * 16,
On 2014/04/15 17:52:27, kkinnunen wrote:
> This is a mistake, the these defaults should be identity matrices..

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7129: len(item['default'])))
On 2014/04/15 18:02:43, vmiura wrote:
> nit: %d for len(..)

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7173: file.Write("     
*num_written = %s;\n" % item_len)
On 2014/04/15 18:02:43, vmiura wrote:
> nit: %d for item_len

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7177: (item['name'], item['type'],
item_len))
On 2014/04/15 18:02:43, vmiura wrote:
> nit: %d for item_len

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7226: file.Write("  %s[%s] = %s;\n"
% (item['name'], ndx, value))
On 2014/04/15 18:02:43, vmiura wrote:
> nit: %d for ndx

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:7286: len(item['default'])))
On 2014/04/15 18:02:43, vmiura wrote:
> nit: %d

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10410: || matrixMode ==
GL_MODELVIEW_CHROMIUM);
On 2014/04/15 22:30:22, piman wrote:
> On 2014/04/15 18:02:43, vmiura wrote:
> > Instead of DCHECK, set GL error & return.
> > 
> > LOCAL_SET_GL_ERROR(
> >         GL_INVALID_ENUM,
> >         "DoMatrixLoadfCHROMIUM", "invalid enum");
> >     return;
> 
> The check should already be done by the autogen'd code (see e.g.
> GLES2DecoderImpl::HandleMatrixLoadfCHROMIUM in gles2_cmd_decoder_autogen.h)
> 
> However we need to raise GL_INVALID_OPERATION if the extension isn't present
> (otherwise we will call a NULL pointer).
> (same below)

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10410: || matrixMode ==
GL_MODELVIEW_CHROMIUM);
On 2014/04/15 18:02:43, vmiura wrote:
> Instead of DCHECK, set GL error & return.
DCHECK is for consistency with other code (which dchecks) and for the reader.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10414: if (memcmp(target_matrix,
matrix, sizeof(GLfloat) * 16)) {
On 2014/04/15 22:30:22, piman wrote:
> Is it worth doing this comparison? We're walking 64 bytes to possibly avoid a
> function call?
> (same below).

It avoids function call and a memcpy. The memcpy walks the same 64 bytes, so
they should be in cache and thus be a bit faster the second time? Due to
wrappers, GL function call is ~3 function calls, esp. on Android. Not that
either of these answer the question.

I just followed the existing code, which always checks for change before a call.
Your call, I can also remove the check?

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10422: || matrixMode ==
GL_MODELVIEW_CHROMIUM);
On 2014/04/15 18:02:43, vmiura wrote:
> Same, LOCAL_SET_GL_ERROR instead of DCHECK.
DCHECK is just a doc for the reader..

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10423: static GLfloat
identity_matrix[16] = {
On 2014/04/15 22:30:22, piman wrote:
> nit: const, and use kIdentityMatrix naming convention.

Done.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_chromium_path_rendering_unittest.cc (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_chromium_path_rendering_unittest.cc:34:
ASSERT_EQ(static_cast<T>(expected[i]), actual[i]);
On 2014/04/15 18:02:43, vmiura wrote:
> nit: EXPECT_EQ instead of ASSERT_EQ?

Done.

[vmiura, 2014-04-23 18:00:05]

LGTM, thanks!  Need owners LGTM.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10410: || matrixMode ==
GL_MODELVIEW_CHROMIUM);
On 2014/04/23 12:26:55, kkinnunen wrote:
> On 2014/04/15 18:02:43, vmiura wrote:
> > Instead of DCHECK, set GL error & return.
> DCHECK is for consistency with other code (which dchecks) and for the reader.

Sorry, my error.

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10414: if (memcmp(target_matrix,
matrix, sizeof(GLfloat) * 16)) {
On 2014/04/23 12:26:55, kkinnunen wrote:
> On 2014/04/15 22:30:22, piman wrote:
> > Is it worth doing this comparison? We're walking 64 bytes to possibly avoid
a
> > function call?
> > (same below).
> 
> It avoids function call and a memcpy. The memcpy walks the same 64 bytes, so
> they should be in cache and thus be a bit faster the second time? Due to
> wrappers, GL function call is ~3 function calls, esp. on Android. Not that
> either of these answer the question.
> 
> I just followed the existing code, which always checks for change before a
call.
> Your call, I can also remove the check?

GL calls are heavy (multiple calls, pthread_get_local, etc.), but how often do
we have repeated matrices?  I don't mind keeping the check and tune when we have
some data.

[piman, 2014-04-24 19:30:06]

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169603002/diff/260001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10414: if (memcmp(target_matrix,
matrix, sizeof(GLfloat) * 16)) {
On 2014/04/23 12:26:55, kkinnunen wrote:
> On 2014/04/15 22:30:22, piman wrote:
> > Is it worth doing this comparison? We're walking 64 bytes to possibly avoid
a
> > function call?
> > (same below).
> 
> It avoids function call and a memcpy. The memcpy walks the same 64 bytes, so
> they should be in cache and thus be a bit faster the second time? Due to
> wrappers, GL function call is ~3 function calls, esp. on Android. Not that
> either of these answer the question.

Right, you're really comparing:

(no test): 1 memcpy, 1 gl call

vs 

(test, hits): 1 memcmp (you only saved the gl call, memcmp is comparable in cost
to memcpy)
(test, misses): 1 memcmp + 1 memcpy + 1 gl call (additional cost of the memcmp)

So we're trading the cost of the extra memcmp on a miss for the cost of the gl
call that you save on a hit.


> 
> I just followed the existing code, which always checks for change before a
call.
> Your call, I can also remove the check?

I suspect you'll miss way more often than you'll hit, that's why it doesn't seem
worth it (unless you think otherwise?)
If we have redundancy, it'd be better to catch it on the client side so that we
avoid the commands, taking space in the command buffer, etc. on top of avoiding
the GL call.

[Kimmo Kinnunen, 2014-04-25 05:59:15]

On 2014/04/24 19:30:06, piman wrote:
> On 2014/04/23 12:26:55, kkinnunen wrote:
> > I just followed the existing code, which always checks for change before a
> call.
> > Your call, I can also remove the check?
> 
> I suspect you'll miss way more often than you'll hit, that's why it doesn't
seem
> worth it (unless you think otherwise?)
> If we have redundancy, it'd be better to catch it on the client side so that
we
> avoid the commands, taking space in the command buffer, etc. on top of
avoiding
> the GL call.
Done.

[piman, 2014-04-25 06:04:30]

lgtm

[Kimmo Kinnunen, 2014-04-25 09:02:17]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-04-25 09:03:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkinnunen@nvidia.com/169603002/320001

[commit-bot: I haz the power, 2014-04-25 12:38:08]

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

[commit-bot: I haz the power, 2014-04-25 12:38:08]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[vmiura, 2014-05-28 19:53:23]

On 2014/04/25 12:38:08, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on linux_chromium_rel

Kimmo, are you holding back on committing this?

[Kimmo Kinnunen, 2014-05-30 12:44:23]

On 2014/05/28 19:53:23, vmiura wrote:
> On 2014/04/25 12:38:08, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   tryserver.chromium on linux_chromium_rel
> 
> Kimmo, are you holding back on committing this?

I'm a bit holding it back until the API discussion clarifies in the other patch,
though I could of course rebase this if you want it in asap.

[vmiura, 2014-05-30 16:56:44]

On 2014/05/30 12:44:23, kkinnunen wrote:
> On 2014/05/28 19:53:23, vmiura wrote:
> > On 2014/04/25 12:38:08, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   tryserver.chromium on linux_chromium_rel
> > 
> > Kimmo, are you holding back on committing this?
> 
> I'm a bit holding it back until the API discussion clarifies in the other
patch,
> though I could of course rebase this if you want it in asap.

I don't have a need for it, but just thought I would ping this issue since it
was CQ'd before and then left open.

[Kimmo Kinnunen, 2014-08-14 12:13:09]

vmiura, would it be possible for you to go through the patch again? 

I think the patchset should be mergeable now, once every patch is updated.

Skia has been updated to counter many of the issues that earlier made me hold
off merging this patch.

Depends on:
https://chromiumcodereview.appspot.com/473853002/

[Kimmo Kinnunen, 2014-08-22 12:56:11]

On 2014/08/14 12:13:09, Kimmo Kinnunen wrote:
> vmiura, would it be possible for you to go through the patch again? 

Just a friendly ping..

[vmiura, 2014-08-25 19:42:22]

Just one thing.

https://codereview.chromium.org/169603002/diff/460001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169603002/diff/460001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10669:
glMatrixLoadfEXT(matrix_mode, matrix);
We're using GL_PATH_MODELVIEW_CHROMIUM/GL_PATH_PROJECTION_CHROMIUM directly with
glMatrixLoadfEXT.

Could you comment here, and in gl2chromium.h/gl_bindings.h that:
  GL_PATH_MODELVIEW_CHROMIUM == GL_MODELVIEW
  GL_PATH_PROJECTION_CHROMIUM == GL_PROJECTION

[Kimmo Kinnunen, 2014-08-26 06:48:34]

Thanks for looking at this.

https://codereview.chromium.org/169603002/diff/460001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/169603002/diff/460001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:10669:
glMatrixLoadfEXT(matrix_mode, matrix);
On 2014/08/25 19:42:22, vmiura wrote:
> We're using GL_PATH_MODELVIEW_CHROMIUM/GL_PATH_PROJECTION_CHROMIUM directly
with
> glMatrixLoadfEXT.
> Could you comment here, and in gl2chromium.h/gl_bindings.h that:
>   GL_PATH_MODELVIEW_CHROMIUM == GL_MODELVIEW
>   GL_PATH_PROJECTION_CHROMIUM == GL_PROJECTION

Done.

I added comments here and in gl_bindings.h and in the commit message. I left
gl2chromium.h out because it didn't seem to fit there, hope it's ok.

The thing to note is that conceptually the trick you noticed is done in the
extension spec, not in the patch. In the patch, GL_PATH_MODELVIEW_CHROMIUM
actually corresponds to GL_PATH_MODELVIEW_NV, similarly to
glSomePathFuncCHROMIUM corresponds to glSomePathFuncNV. 

The gl headers in chromium do not yet have the GL_PATH_MODELVIEW_NV, so it can
not, unfortunately, be documented by compile-time assertions.

[Kimmo Kinnunen, 2014-08-26 06:50:06]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-08-26 06:51:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkinnunen@nvidia.com/169603002/480001

[commit-bot: I haz the power, 2014-08-26 09:33:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-26 10:20:21]

Committed patchset #18 (480001) as 1a826716b378db9ef4eed563d01156e89ccc5b33

[commit-bot: I haz the power, 2014-09-10 02:41:52]

Patchset 18 (id:??) landed as
https://crrev.com/337d59638d630d0892b444ca2ab0a2989921d00e
Cr-Commit-Position: refs/heads/master@{#291878}