The great shader refactor: Merge fragment shaders into an uber-shader

The great shader refactor: Merge fragment shaders into an uber-shader

None of the fragment shader sub-classes actually need to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[ccameron, 2016-12-30 06:15:27]

Description was changed from

==========
The great shader refactor: Merge all shaders into an uber-shader

None of the shader sub-classes actually needed to exist.

BUG=667966
==========

to

==========
The great shader refactor: Merge all shaders into an uber-shader

None of the shader sub-classes actually needed to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2016-12-30 06:15:59]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-30 06:16:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-30 06:44:44]

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

[commit-bot: I haz the power, 2016-12-30 06:44:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2016-12-30 07:13:58]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-30 07:14:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-30 08:13:56]

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

[commit-bot: I haz the power, 2016-12-30 08:13:57]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-12-30 08:24:55]

Description was changed from

==========
The great shader refactor: Merge all shaders into an uber-shader

None of the shader sub-classes actually needed to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
The great shader refactor: Merge fragment shaders into an uber-shader

None of the fragment shader sub-classes actually need to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2016-12-30 08:24:55]

ccameron@chromium.org changed reviewers:
+ enne@chromium.org, ericrk@chromium.org

[ccameron, 2016-12-30 09:34:55]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-30 09:35:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-30 10:32:50]

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

[commit-bot: I haz the power, 2016-12-30 10:32:50]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-12-30 19:13:25]

PTAL

[enne (OOO), 2017-01-03 21:42:58]

This is super great! Thank you for cleaning this up.  :)

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.cc#new...
cc/output/shader.cc:1174: std::string FragmentShaderBase::GetShaderSource()
const {
Maybe this should be a followup, but it does seem a bit like some of the other
logic might make more sense here, like SetFragmentSamplerType,
SetFragmentTexCoordPrecision, and SetBlendModeFunctions?  Or, do you think those
should stay separate?

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.cc#new...
cc/output/shader.cc:1178: #define HDR(x) header += x + std::string("\n");
Style nit: sure sure, macros are fine, but I like them better when there's no
semicolon in them so that they look like function invocations when used.

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.h
File cc/output/shader.h (right):

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.h#newc...
cc/output/shader.h:583:
DISALLOW_COPY_AND_ASSIGN(FragmentShaderRGBATexAlphaMaskAA);
Tiniest of nits: maybe all or none of these classes should have this?

[ccameron, 2017-01-03 22:12:27]

Thanks - updated.

Btw, can you grab the dependency, too:
https://codereview.chromium.org/2601343002/

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.cc#new...
cc/output/shader.cc:1174: std::string FragmentShaderBase::GetShaderSource()
const {
On 2017/01/03 21:42:58, enne wrote:
> Maybe this should be a followup, but it does seem a bit like some of the other
> logic might make more sense here, like SetFragmentSamplerType,
> SetFragmentTexCoordPrecision, and SetBlendModeFunctions?  Or, do you think
those
> should stay separate?

Yeah, they should definitely be handled in a similar way to this. I'll grab them
in a separate patch -- I'm trying to salami-slice this (to help with bisecting
if there are regressions).

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.cc#new...
cc/output/shader.cc:1178: #define HDR(x) header += x + std::string("\n");
On 2017/01/03 21:42:58, enne wrote:
> Style nit: sure sure, macros are fine, but I like them better when there's no
> semicolon in them so that they look like function invocations when used.

Good call. Wrapped them a do{} while(0) to be more-function-like and also
require a semicolon.

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.h
File cc/output/shader.h (right):

https://codereview.chromium.org/2608863002/diff/60001/cc/output/shader.h#newc...
cc/output/shader.h:583:
DISALLOW_COPY_AND_ASSIGN(FragmentShaderRGBATexAlphaMaskAA);
On 2017/01/03 21:42:58, enne wrote:
> Tiniest of nits: maybe all or none of these classes should have this?

Yeah, I was wondering what to do there. Updated to put a DISALLOW in the base
class and deleted the couple of sub-class instances.

[ccameron, 2017-01-03 22:12:39]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 22:13:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-01-03 22:32:32]

lgtm

https://codereview.chromium.org/2608863002/diff/100001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2608863002/diff/100001/cc/output/shader.cc#ne...
cc/output/shader.cc:1205: if (has_rgba_as_sampler_2d_)
Do you need this conditional here? It seems like TextureLookup handles texture2D
as well.

[ccameron, 2017-01-03 22:47:49]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 22:48:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2017-01-03 22:48:34]

Thanks!

https://codereview.chromium.org/2608863002/diff/100001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2608863002/diff/100001/cc/output/shader.cc#ne...
cc/output/shader.cc:1205: if (has_rgba_as_sampler_2d_)
On 2017/01/03 22:32:31, enne wrote:
> Do you need this conditional here? It seems like TextureLookup handles
texture2D
> as well.

Some of the shaders ignore the SamplerType argument, and just always use
sampler2D+texture2D (e.g, FragmentShaderRGBATexAlphaMask).

That said, you're right, this variable's name didn't make any sense .. I renamed
it to "ignore_sampler_type_".

The full fix for this would to be to update GLRenderer to specify the correct
sampler type ... but that will require more cleanup. Added a TODO in the header.

[commit-bot: I haz the power, 2017-01-03 23:50:08]

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

[commit-bot: I haz the power, 2017-01-03 23:50:09]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-01-04 04:19:41]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-01-04 04:19:42]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2608863002/#ps120001
(title: "Better variable name")

[commit-bot: I haz the power, 2017-01-04 04:19:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 04:25:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483503581919290,
"parent_rev": "8dbe01e3f9950072973e584eeeb01b8d64474761", "commit_rev":
"89317cd02aea67a6ce10de1ad55156a89984d8dd"}

[commit-bot: I haz the power, 2017-01-04 04:26:36]

Description was changed from

==========
The great shader refactor: Merge fragment shaders into an uber-shader

None of the fragment shader sub-classes actually need to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
The great shader refactor: Merge fragment shaders into an uber-shader

None of the fragment shader sub-classes actually need to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2608863002
==========

[commit-bot: I haz the power, 2017-01-04 04:26:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2017-01-04 04:30:14]

Description was changed from

==========
The great shader refactor: Merge fragment shaders into an uber-shader

None of the fragment shader sub-classes actually need to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2608863002
==========

to

==========
The great shader refactor: Merge fragment shaders into an uber-shader

None of the fragment shader sub-classes actually need to exist.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/f9977a81c916aca92d423d9c07c5b01284022d99
Cr-Commit-Position: refs/heads/master@{#441315}
==========

[commit-bot: I haz the power, 2017-01-04 04:30:15]

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