Bug 98977 – Shader translator needs option to clamp uniform array accesses in vertex shaders

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

This is the WebKit version of https://code.google.com/p/angleproject/issues/detail?id=49

<rdar://problem/12402024>

[Dean Jackson]

I'm about to upload a patch. Unfortunately, I'm not permitted to submit this to ANGLE, so I'll submit here under a license that would allow it to be merged into the ANGLE trunk.

This is my first time in the ANGLE code, so I won't be surprised if I'm doing something incorrectly. Unfortunately there were some complications which I've described in the ChangeLog.

I need to write some LayoutTests, but for now here are some examples:

TEST 1 INPUT
------------
attribute vec4 in_position;
attribute float in_index;
uniform vec4 weights[16];
void main() {
  gl_Position = in_position * weights[int(in_index)];
}

TEST 1 OUTPUT
-------------
attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weights[16];
void main(){
(gl_Position = (in_position * weights[int(clamp(float(int(in_index)), 0.0, 15.0))]));
}

TEST 2 INPUT
------------
attribute vec4 in_position;
attribute float in_index;
uniform vec4 weight;
void main() {
  gl_Position = in_position * weight[int(in_index)];
}

TEST 2 OUTPUT
-------------
attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weight;
void main(){
(gl_Position = (in_position * weight[int(clamp(float(int(in_index)), 0.0, 3.0))]));
}

TEST 3 INPUT
------------
attribute vec4 in_position;
attribute float in_index;
uniform mat2 weights;
void main() {
  gl_Position = in_position * weights[int(in_index)][int(in_index * 5.0)];
}

TEST 3 OUTPUT
-------------
attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp mat2 weights;
void main(){
(gl_Position = (in_position * weights[int(clamp(float(int(in_index)), 0.0, 1.0))][int(clamp(float(int((in_index * 5.0))), 0.0, 1.0))]));
}

TEST 4 INPUT
------------
attribute vec4 in_position;
attribute float in_index;
uniform vec4 weights[10];
vec4 temp[10]; // not a uniform
void main() {
  temp[int(in_index)] = weights[9];
  gl_Position = in_position * temp[int(in_index * 5.0)];
}


TEST 4 OUTPUT
-------------
attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weights[10];
highp vec4 temp[10];
void main(){
(temp[int(clamp(float(int(in_index)), 0.0, 9.0))] = weights[9]);
(gl_Position = (in_position * temp[int(clamp(float(int((in_index * 5.0))), 0.0, 9.0))]));
}

[Dean Jackson]

Created attachment 168110 [details]
Patch

[Dean Jackson]

Style bot will hate this!

[WebKit Review Bot]

Attachment 168110 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.h:12:  Alphabetical sorting problem.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:39:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:42:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:50:  One space before end of line comments  [whitespace/comments] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:26:  #ifndef header guard has wrong style, please use: ArrayBoundsClamper_h  [build/header_guard] [5]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:242:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:244:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:247:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:248:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:250:  Declaration has space between type name and * in TIntermTyped *left  [whitespace/declaration] [3]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:254:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:258:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:259:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:265:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:266:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:267:  Use 0 instead of NULL.  [readability/null] [5]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:145:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:147:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:148:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:149:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:150:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:151:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 23 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 168110 [details]
Patch

Attachment 168110 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14259291

[Early Warning System Bot]

Comment on attachment 168110 [details]
Patch

Attachment 168110 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14259296

[Kenneth Russell]

Comment on attachment 168110 [details]
Patch

This looks pretty good to me as a first step. Alok, could you take a look too?

Any indication of sites that break while enforcing this new rule?

[Dean Jackson]

I haven't tested much. I'll try living on it.

Our GL team suggested that it is inefficient to have all the float<->int casts, so I'll submit a second patch that has a single global int variable used for the array index expression, and then does a simple ?: test. Unfortunately this might be a little more work in the ANGLE code.

I'll also make sure to update the other port build configurations.

[Kenneth Russell]

(In reply to comment #8)
> I haven't tested much. I'll try living on it.
> 
> Our GL team suggested that it is inefficient to have all the float<->int casts, so I'll submit a second patch that has a single global int variable used for the array index expression, and then does a simple ?: test. Unfortunately this might be a little more work in the ANGLE code.

That sounds problematic. How about just doing the clamp on an integer value? Is that supported?

[Dean Jackson]

(In reply to comment #9)
> (In reply to comment #8)
> > I haven't tested much. I'll try living on it.
> > 
> > Our GL team suggested that it is inefficient to have all the float<->int casts, so I'll submit a second patch that has a single global int variable used for the array index expression, and then does a simple ?: test. Unfortunately this might be a little more work in the ANGLE code.
> 
> That sounds problematic. How about just doing the clamp on an integer value? Is that supported?

Unfortunately clamp does not accept floats. I'm going to inject a _webgl_int_clamp function though. It seems the cleanest way to rewrite the shader, without doing major analysis and surgery on the AST.

[Dean Jackson]

Created attachment 168481 [details]
Patch

[Dean Jackson]

Updated patch. It will obviously fail the style check but hopefully build on Qt. I still need to add a couple of real WebGL tests.

Test 1 - basic array index
Test 2 - indexing on a vec rather than array
Test 3 - indexing on a mat rather than array
Test 4 - array indexing in an lvalue
Test 5 - multiple indexing on the same line
Test 6 - non-expression indexing, should be unchanged (no generated/injected function)

TEST 1 INPUT
-------------

attribute vec4 in_position;
attribute float in_index;
uniform vec4 weights[16];
void main() {
 gl_Position = in_position * weights[int(in_index)];
}

TEST 1 OUTPUT
--------------

// BEGIN: Generated code for array bounds clamping

int webgl_int_clamp(int value, int minValue, int maxValue) { return ((value < minValue) ? minValue : ((value > maxValue) ? maxValue : value)); }

// END: Generated code for array bounds clamping

attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weights[16];
void main(){
(gl_Position = (in_position * weights[webgl_int_clamp(int(in_index), 0, 15)]));
}


TEST 2 INPUT
-------------

attribute vec4 in_position;
attribute float in_index;
uniform vec4 weight;
void main() {
 gl_Position = in_position * weight[int(in_index)];
}

TEST 2 OUTPUT
--------------

// BEGIN: Generated code for array bounds clamping

int webgl_int_clamp(int value, int minValue, int maxValue) { return ((value < minValue) ? minValue : ((value > maxValue) ? maxValue : value)); }

// END: Generated code for array bounds clamping

attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weight;
void main(){
(gl_Position = (in_position * weight[webgl_int_clamp(int(in_index), 0, 3)]));
}


TEST 3 INPUT
-------------

attribute vec4 in_position;
attribute float in_index;
uniform mat2 weights;
void main() {
 gl_Position = in_position * weights[int(in_index)][int(in_index * 5.0)];
}

TEST 3 OUTPUT
--------------

// BEGIN: Generated code for array bounds clamping

int webgl_int_clamp(int value, int minValue, int maxValue) { return ((value < minValue) ? minValue : ((value > maxValue) ? maxValue : value)); }

// END: Generated code for array bounds clamping

attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp mat2 weights;
void main(){
(gl_Position = (in_position * weights[webgl_int_clamp(int(in_index), 0, 1)][webgl_int_clamp(int((in_index * 5.0)), 0, 1)]));
}

TEST 4 INPUT
-------------

attribute vec4 in_position;
attribute float in_index;
uniform vec4 weights[10];
vec4 temp[10];
void main() {
 temp[int(in_index)] = weights[9];
 gl_Position = in_position * temp[int(in_index * 5.0)];
}

TEST 4 OUTPUT
--------------

// BEGIN: Generated code for array bounds clamping

int webgl_int_clamp(int value, int minValue, int maxValue) { return ((value < minValue) ? minValue : ((value > maxValue) ? maxValue : value)); }

// END: Generated code for array bounds clamping

attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weights[10];
highp vec4 temp[10];
void main(){
(temp[webgl_int_clamp(int(in_index), 0, 9)] = weights[9]);
(gl_Position = (in_position * temp[webgl_int_clamp(int((in_index * 5.0)), 0, 9)]));
}

TEST 5 INPUT
-------------

attribute vec4 in_position;
attribute float in_index;
uniform vec4 weights[16];
void main() {
 int index = int(in_index);
 gl_Position = in_position * weights[index] + weights[index * 2] + weights[index * 3 + 1];
}

TEST 5 OUTPUT
--------------

// BEGIN: Generated code for array bounds clamping

int webgl_int_clamp(int value, int minValue, int maxValue) { return ((value < minValue) ? minValue : ((value > maxValue) ? maxValue : value)); }

// END: Generated code for array bounds clamping

attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weights[16];
void main(){
highp int index = int(in_index);
(gl_Position = (((in_position * weights[webgl_int_clamp(index, 0, 15)]) + weights[webgl_int_clamp((index * 2), 0, 15)]) + weights[webgl_int_clamp(((index * 3) + 1), 0, 15)]));
}

TEST 6 INPUT
-------------

attribute vec4 in_position;
attribute float in_index;
uniform vec4 weights[16];
void main() {
 gl_Position = in_position * weights[8];
}

TEST 6 OUTPUT
--------------

attribute highp vec4 in_position;
attribute highp float in_index;
uniform highp vec4 weights[16];
void main(){
(gl_Position = (in_position * weights[8]));
}

[WebKit Review Bot]

Attachment 168481 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.h:12:  Alphabetical sorting problem.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:42:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:45:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:56:  One space before end of line comments  [whitespace/comments] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:26:  #ifndef header guard has wrong style, please use: ArrayBoundsClamper_h  [build/header_guard] [5]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:242:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:244:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:247:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:248:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:250:  Declaration has space between type name and * in TIntermTyped *left  [whitespace/declaration] [3]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:254:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:258:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:259:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:265:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:266:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:267:  Use 0 instead of NULL.  [readability/null] [5]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:145:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:147:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:148:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:149:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:150:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:151:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 23 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 168760 [details]
Patch

[Dean Jackson]

Really ready for review now. Style bot will still obviously complain about the ANGLE coding style.

Includes code to enable the flag in WebKit, and a test case.

[Alok Priyadarshi]

Comment on attachment 168760 [details]
Patch

I do not think there is any need for one full traversal of the AST just to mark nodes with a bool. The work that is being done during that traversal can be easily done while writing the GLSL shader.

[Dean Jackson]

(In reply to comment #16)
> (From update of attachment 168760 [details])
> I do not think there is any need for one full traversal of the AST just to mark nodes with a bool. The work that is being done during that traversal can be easily done while writing the GLSL shader.

Except I need to traverse the tree in order to know if I should output the webgl_int_clamp function before the main shader content.

Alternatives would be:
- only do the traversal within the TranslateGLSL and TranslateESSL translate methods
- always inject this function when the compile option is set (and let the compiler do extra work to remove it)

[Dean Jackson]

Comment on attachment 168760 [details]
Patch

Setting back to r? waiting for feedback from Alok.

[Alok Priyadarshi]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 168760 [details] [details])
> > I do not think there is any need for one full traversal of the AST just to mark nodes with a bool. The work that is being done during that traversal can be easily done while writing the GLSL shader.
> 
> Except I need to traverse the tree in order to know if I should output the webgl_int_clamp function before the main shader content.
> 
> Alternatives would be:
> - only do the traversal within the TranslateGLSL and TranslateESSL translate methods
> - always inject this function when the compile option is set (and let the compiler do extra work to remove it)

Ah I see. I would always emit webgl_int_clamp function (or maybe just the declaration).

[Daniel Bates]

Comment on attachment 168760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168760&action=review

I noticed some minor nits. I have not reviewed this patch for correctness. The license block text in ArrayBoundsClamper.{cpp, h} is out-of-date as it references Apple Computer, Inc. It should reference Apple Inc. For comparison, see the license block text in <http://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE>.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:721
> +				31ED471D1624EFD100245078 /* ArrayBoundsClamper.h in Headers */,

I haven't opened this Xcode project file, will this header be listed in lexicographic order in the Project Navigator pane?

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:839
> +				31ED471C1624EFD100245078 /* ArrayBoundsClamper.cpp in Sources */,

Ditto.

> Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:39
> +    virtual bool visitBinary(Visit visit, TIntermBinary* node)

The parameter visit is unused and its name matches the name of its data type (so it doesn't provide additional clarification on the purpose of this parameter). I suggest that we omit the parameter name.

On another note, we may want to consider building ANGLE with -Wunused or -Wunused-parameter to catch such usage of unused parameters among other things. This can be done in a separate patch.

> Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:39
> +    bool GetArrayBoundsClampNeeded() { return mNeedsClamping; }

Can this function be made const?

> Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:250
> +                    TIntermTyped *left = node->getLeft();

Nit: The '*' should be on the other side.

> Source/ThirdParty/ANGLE/src/compiler/intermediate.h:405
> +

Can this function be made const?

[Dean Jackson]

(In reply to comment #19)

> Ah I see. I would always emit webgl_int_clamp function (or maybe just the declaration).

OK. I'll always emit the declaration and only add the function if it was needed. New patch coming soon.

[Dean Jackson]

(In reply to comment #20)
> (From update of attachment 168760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168760&action=review
> 
> I noticed some minor nits. I have not reviewed this patch for correctness. The license block text in ArrayBoundsClamper.{cpp, h} is out-of-date as it references Apple Computer, Inc. It should reference Apple Inc. For comparison, see the license block text in <http://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE>.
> 
> > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:721
> > +				31ED471D1624EFD100245078 /* ArrayBoundsClamper.h in Headers */,
> 
> I haven't opened this Xcode project file, will this header be listed in lexicographic order in the Project Navigator pane?

Yes. I hate when that's not done.

> > Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:39
> > +    virtual bool visitBinary(Visit visit, TIntermBinary* node)
> 
> The parameter visit is unused and its name matches the name of its data type (so it doesn't provide additional clarification on the purpose of this parameter). I suggest that we omit the parameter name.
> 
> On another note, we may want to consider building ANGLE with -Wunused or -Wunused-parameter to catch such usage of unused parameters among other things. This can be done in a separate patch.

Note that this intentionally does not conform to WebKit style. Rather to the style of the ANGLE project.

> 
> > Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:39
> > +    bool GetArrayBoundsClampNeeded() { return mNeedsClamping; }
> 
> Can this function be made const?

Yes.

> 
> > Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:250
> > +                    TIntermTyped *left = node->getLeft();
> 
> Nit: The '*' should be on the other side.

DItto on ANGLE style.

> 
> > Source/ThirdParty/ANGLE/src/compiler/intermediate.h:405
> > +
> 
> Can this function be made const?

yes

[Daniel Bates]

Comment on attachment 168760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168760&action=review

>>> Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:250
>>> +                    TIntermTyped *left = node->getLeft();
>> 
>> Nit: The '*' should be on the other side.
> 
> DItto on ANGLE style.

Notice that this will be the only line in this file that has the '*' on the right side. Briefly searching through the ANGLE code the placement of the '*' is inconsistent across files. I'll trust your judgement.

[Dean Jackson]

Created attachment 169029 [details]
Patch

[Dean Jackson]

OK. Here's a version that only adds clamping at the output/translation stage. I think it's a little more invasive, since we have to pass compile options down into the translators, and keep around an object to determine if we need to add definitions, but at least it avoids another tree traversal.

[Dean Jackson]

(In reply to comment #23)
> (From update of attachment 168760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168760&action=review
> 
> >>> Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:250
> >>> +                    TIntermTyped *left = node->getLeft();
> >> 
> >> Nit: The '*' should be on the other side.
> > 
> > DItto on ANGLE style.
> 
> Notice that this will be the only line in this file that has the '*' on the right side. Briefly searching through the ANGLE code the placement of the '*' is inconsistent across files. I'll trust your judgement.

Yeah, I was wrong. Fixed in new patch.

[WebKit Review Bot]

Comment on attachment 169029 [details]
Patch

Attachment 169029 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14392295

[Dean Jackson]

Of course changing the Compiler API breaks Chromium. I'm not sure what to do here. The other translation approaches all do a tree traversal.

[Dean Jackson]

Oops. No, the problem is that Chromium is not using the ANGLE from WebKit, and thus doesn't see the new flag. I'll wrap it in #if PLATFORM but I think the review can go ahead.

[Peter Beverloo (cr-android ews)]

Comment on attachment 169029 [details]
Patch

Attachment 169029 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14390304

[WebKit Review Bot]

Attachment 169029 [details] did not pass style-queue:

Source/ThirdParty/ANGLE/src/compiler/OutputGLSL.h:16:  The parameter name "arrayBoundsClamper" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:243:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:247:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:248:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:249:  Use 0 instead of NULL.  [readability/null] [5]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.h:12:  Alphabetical sorting problem.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.h:20:  The parameter name "arrayBoundsClamper" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:56:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:59:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:60:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:66:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:70:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:71:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:84:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorESSL.cpp:15:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorESSL.cpp:27:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorESSL.cpp:38:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorGLSL.cpp:28:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorGLSL.cpp:40:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorGLSL.cpp:51:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputESSL.h:15:  The parameter name "arrayBoundsClamper" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:26:  #ifndef header guard has wrong style, please use: ArrayBoundsClamper_h  [build/header_guard] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:48:  The parameter name "visit" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:145:  Weird number of spaces at line-start.Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:147:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:148:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:149:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:150:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:151:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 33 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 169062 [details]
Patch

[Dean Jackson]

Will hopefully build on Chromium now.

[WebKit Review Bot]

Attachment 169062 [details] did not pass style-queue:

Source/ThirdParty/ANGLE/src/compiler/OutputGLSL.h:16:  The parameter name "arrayBoundsClamper" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:243:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:247:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:248:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:249:  Use 0 instead of NULL.  [readability/null] [5]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.h:12:  Alphabetical sorting problem.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.h:20:  The parameter name "arrayBoundsClamper" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:56:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:59:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:60:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:66:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:70:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:71:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:84:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorESSL.cpp:15:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorESSL.cpp:27:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorESSL.cpp:38:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorGLSL.cpp:28:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorGLSL.cpp:40:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/TranslatorGLSL.cpp:51:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputESSL.h:15:  The parameter name "arrayBoundsClamper" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:26:  #ifndef header guard has wrong style, please use: ArrayBoundsClamper_h  [build/header_guard] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:48:  The parameter name "visit" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:145:  Weird number of spaces at line-start.Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:147:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:148:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:149:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:150:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:151:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 33 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 169062 [details]
Patch

Attachment 169062 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14392359

New failing tests:
fast/canvas/webgl/array-bounds-clamping.html
platform/chromium/virtual/gpu/fast/canvas/webgl/array-bounds-clamping.html

[WebKit Review Bot]

Comment on attachment 169062 [details]
Patch

Attachment 169062 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14395356

New failing tests:
fast/canvas/webgl/array-bounds-clamping.html
platform/chromium/virtual/gpu/fast/canvas/webgl/array-bounds-clamping.html

[Dean Jackson]

(In reply to comment #36)
> (From update of attachment 169062 [details])
> Attachment 169062 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14395356
> 
> New failing tests:
> fast/canvas/webgl/array-bounds-clamping.html
> platform/chromium/virtual/gpu/fast/canvas/webgl/array-bounds-clamping.html

I will add this to the Chromium test expectations. Obviously it will fail until Chromium upstreams this patch into ANGLE.

[Alok Priyadarshi]

Comment on attachment 169062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169062&action=review

TranslatorHLSL have a similar problem. You should look at it for inspiration on how they separate writing out a header and body. I would do it in a different way:

class TOutputGLSL {
  TOutputGLSL(Spec, bool clampArrayIndex);

  void output(TInfoSinkBase& sink) {
    TOutputXXX traverser(clamp);
    root->traverse(&traverser);
    sink << traverser.header();
    sink << traverser.body();
  }
};

If you want I can take submit a patch to ANGLE.

> Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:69
> +TOutputGLSLBase::TOutputGLSLBase(TInfoSinkBase& objSink, ArrayBoundsClamper& arrayBoundsClamper)

Adding a dependency on ArrayBoundsClamper does not seem right.

> Source/ThirdParty/ANGLE/src/compiler/TranslatorGLSL.cpp:49
> +    // Write array bounds clamping definition if necessary.

duplicated code from TranslatorESSL.

[Alok Priyadarshi]

I noticed that you are using the pattern used in BuiltInFunctionEmulator, which also unnecessarily makes an additional pass. I think it is better to be consistent with an existing pattern. After you land your patch I will do some refactoring to consolidate these traversals.

[Dean Jackson]

Created attachment 169476 [details]
Patch

Reverting to tree traversal approach after discussion with Alok

[WebKit Review Bot]

Attachment 169476 [details] did not pass style-queue:

Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:39:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:41:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:42:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:43:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:44:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:47:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:59:  One space before end of line comments  [whitespace/comments] [5]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:26:  #ifndef header guard has wrong style, please use: ArrayBoundsClamper_h  [build/header_guard] [5]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:242:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:244:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:247:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:248:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:250:  Declaration has space between type name and * in TIntermTyped *left  [whitespace/declaration] [3]
Source/ThirdParty/ANGLE/src/Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
compiler/OutputGLSLBase.cpp:254:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:258:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:259:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:265:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:266:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/OutputGLSLBase.cpp:267:  Use 0 instead of NULL.  [readability/null] [5]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:145:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:147:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:148:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:149:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:150:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/include/GLSLANG/ShaderLang.h:151:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 38 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alok Priyadarshi]

Comment on attachment 169476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169476&action=review

Minor nits. Looks fine otherwise.

> Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:36
> +    ArrayBoundsClamperMarker(ArrayBoundsClamper& clamper)

I think it will be better to not add a dependency on ArrayBoundsClamper.

> Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:56
> +    ArrayBoundsClamper& mClamper;

Instead of a dependency on ArrayBoundsClamper, you could have a bool that indicates if clamping is required. This bool can be queried by the client after traversal.

> Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.h:38
> +    bool GetArrayBoundsClampDefinitionNeeded() const { return mArrayBoundsClampDefinitionNeeded; }

These do not need to be public if you remove the dependency from Marker.

[WebKit Review Bot]

Comment on attachment 169476 [details]
Patch

Attachment 169476 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14455246

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/array-bounds-clamping.html

[Kenneth Russell]

Comment on attachment 169476 [details]
Patch

Thanks Alok for the thorough review. Those nits aside, looks good to me. (The cr-ews failure looks unrelated -- and unfortunate.) r=me

[Dean Jackson]

Comment on attachment 169476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169476&action=review

>> Source/ThirdParty/ANGLE/src/compiler/ArrayBoundsClamper.cpp:36
>> +    ArrayBoundsClamperMarker(ArrayBoundsClamper& clamper)
> 
> I think it will be better to not add a dependency on ArrayBoundsClamper.

Done!

[Dean Jackson]

Committed r131933: <http://trac.webkit.org/changeset/131933>