Moves responsibility for shader resource string generation to ANGLE.

Moves responsibility for shader resource string generation to ANGLE.

BUG=374942
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271948

[Shannon Woods, 2014-05-19 21:41:33]

This review is pending the following ANGLE CL:
https://chromium-review.googlesource.com/#/c/200171

The ANGLE CL adds a function to ANGLE to retrieve a string describing its
ShBuiltInResources struct, so Chrome's source doesn't need to be separately
updated each time that struct is changed ANGLE-side.

Note that there will need to be one more change, after Chrome no longer needs to
build with versions of ANGLE before the pending CL, to remove the
pre-version-124 section.

[Shannon Woods, 2014-05-20 21:29:34]

CL rebased to reflect landed ANGLE patch. Also fixed a preexisting typo.

Verified that ShaderTranslatorTest.OptionsString (from gpu_unittests) passes
with both newest (ANGLE_SH_VERSION == 124) and older ANGLE dependencies.

[Shannon Woods, 2014-05-20 21:31:02]

On 2014/05/20 21:29:34, Shannon Woods wrote:
> CL rebased to reflect landed ANGLE patch. Also fixed a preexisting typo.
> 
> Verified that ShaderTranslatorTest.OptionsString (from gpu_unittests) passes
> with both newest (ANGLE_SH_VERSION == 124) and older ANGLE dependencies.

(Er, PTAL.)

[Zhenyao Mo, 2014-05-20 21:48:55]

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:207: DCHECK(compiler_ != NULL);
I think returning "None" instead of DCHECK is safer.

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:212: scoped_ptr<char[]>
resource_str(new char[resource_len]);
this should be scoped_array<char> instead.

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:219: #else
this can be #elif, so you don't need the extra #endif.

[Zhenyao Mo, 2014-05-20 21:50:55]

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:219: #else
On 2014/05/20 21:48:55, Zhenyao Mo wrote:
> this can be #elif, so you don't need the extra #endif.

Never mind this.  I need a pair of glasses apparently.

[Ken Russell (switch to Gerrit), 2014-05-20 21:58:00]

This looks good overall. I defer to Mo's review.

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:207: DCHECK(compiler_ != NULL);
On 2014/05/20 21:48:55, Zhenyao Mo wrote:
> I think returning "None" instead of DCHECK is safer.

Not sure. If we really expect the compiler to be non-NULL then asserting it here
in debug builds seems fine.

[Zhenyao Mo, 2014-05-20 22:22:00]

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:207: DCHECK(compiler_ != NULL);
On 2014/05/20 21:58:00, Ken Russell wrote:
> On 2014/05/20 21:48:55, Zhenyao Mo wrote:
> > I think returning "None" instead of DCHECK is safer.
> 
> Not sure. If we really expect the compiler to be non-NULL then asserting it
here
> in debug builds seems fine.

If we prefer DCHECK, then below we should also DCHECK(resource_len > 1).  As far
as the compiler is initialized successfully, resource string should always be
available.

[Shannon Woods, 2014-05-20 22:24:29]

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:207: DCHECK(compiler_ != NULL);
On 2014/05/20 22:22:01, Zhenyao Mo wrote:
> On 2014/05/20 21:58:00, Ken Russell wrote:
> > On 2014/05/20 21:48:55, Zhenyao Mo wrote:
> > > I think returning "None" instead of DCHECK is safer.
> > 
> > Not sure. If we really expect the compiler to be non-NULL then asserting it
> here
> > in debug builds seems fine.
> 
> If we prefer DCHECK, then below we should also DCHECK(resource_len > 1).  As
far
> as the compiler is initialized successfully, resource string should always be
> available.

From what I can see, all paths to this function lead through Program::Link,
which I think indicates we should expect the compiler to have been initialized.
I'm open to doing it whichever way you think is the better way-- I was mostly
just trying to stick close to established style in the file.

[Zhenyao Mo, 2014-05-20 22:27:17]

On 2014/05/20 22:24:29, Shannon Woods wrote:
>
https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/shader_translator.cc (right):
> 
>
https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/shader_translator.cc:207: DCHECK(compiler_ !=
NULL);
> On 2014/05/20 22:22:01, Zhenyao Mo wrote:
> > On 2014/05/20 21:58:00, Ken Russell wrote:
> > > On 2014/05/20 21:48:55, Zhenyao Mo wrote:
> > > > I think returning "None" instead of DCHECK is safer.
> > > 
> > > Not sure. If we really expect the compiler to be non-NULL then asserting
it
> > here
> > > in debug builds seems fine.
> > 
> > If we prefer DCHECK, then below we should also DCHECK(resource_len > 1).  As
> far
> > as the compiler is initialized successfully, resource string should always
be
> > available.
> 
> From what I can see, all paths to this function lead through Program::Link,
> which I think indicates we should expect the compiler to have been
initialized.
> I'm open to doing it whichever way you think is the better way-- I was mostly
> just trying to stick close to established style in the file.

Then let's do both DCHECK.  That way the code will have less surprise in the
future.

[Zhenyao Mo, 2014-05-20 23:15:23]

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_translator.cc:212: scoped_ptr<char[]>
resource_str(new char[resource_len]);
On 2014/05/20 21:48:55, Zhenyao Mo wrote:
> this should be scoped_array<char> instead.

Oops, http://src.chromium.org/viewvc/chrome?view=revision&revision=194680

scoped_array has depreciated.  This code is correct as is.  Sorry about the
confusion.

[Shannon Woods, 2014-05-20 23:40:56]

On 2014/05/20 22:27:17, Zhenyao Mo wrote:
> On 2014/05/20 22:24:29, Shannon Woods wrote:
> >
>
https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
> > File gpu/command_buffer/service/shader_translator.cc (right):
> > 
> >
>
https://codereview.chromium.org/295803002/diff/20001/gpu/command_buffer/servi...
> > gpu/command_buffer/service/shader_translator.cc:207: DCHECK(compiler_ !=
> NULL);
> > On 2014/05/20 22:22:01, Zhenyao Mo wrote:
> > > On 2014/05/20 21:58:00, Ken Russell wrote:
> > > > On 2014/05/20 21:48:55, Zhenyao Mo wrote:
> > > > > I think returning "None" instead of DCHECK is safer.
> > > > 
> > > > Not sure. If we really expect the compiler to be non-NULL then asserting
> it
> > > here
> > > > in debug builds seems fine.
> > > 
> > > If we prefer DCHECK, then below we should also DCHECK(resource_len > 1). 
As
> > far
> > > as the compiler is initialized successfully, resource string should always
> be
> > > available.
> > 
> > From what I can see, all paths to this function lead through Program::Link,
> > which I think indicates we should expect the compiler to have been
> initialized.
> > I'm open to doing it whichever way you think is the better way-- I was
mostly
> > just trying to stick close to established style in the file.
> 
> Then let's do both DCHECK.  That way the code will have less surprise in the
> future.

Ok, made both DCHECKs. PTAL.

[Zhenyao Mo, 2014-05-20 23:42:25]

LGTM

[Shannon Woods, 2014-05-20 23:44:41]

The CQ bit was checked by shannonwoods@chromium.org

[Shannon Woods, 2014-05-20 23:44:43]

The CQ bit was unchecked by shannonwoods@chromium.org

[Shannon Woods, 2014-05-20 23:47:10]

The CQ bit was checked by shannonwoods@chromium.org

[commit-bot: I haz the power, 2014-05-20 23:49:22]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/shannonwoods@chromium.org/295803002/40001

[commit-bot: I haz the power, 2014-05-21 07:55:52]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-21 20:05:44]

Change committed as 271948