WebGL Code Refactoring: remove redundant validations for texImage2D and texSubImage2D

WebGL Code Refactoring: remove redundant validations for texImage2D and texSubImage2D.

Almost all validations in texImage2DBase() and texSubImage2DBase() have been done in 
their callers. These validations are executed twice. It is not necessary. For example, 
validateTexFuncParameters() is not very cheap. it is called in texImage2DBase(). This 
function is also called by validateTexFunc(). But all variants of texImage2D() calls 
both validateTexFunc() and texImage2DBase(). 

No WebGL 1.0 conformance test regression with this change.

BUG=528180
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201886

[yunchao, 2015-09-02 03:13:08]

Patchset #1 (id:1) has been deleted

[yunchao, 2015-09-04 08:35:58]

yunchao.he@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[yunchao, 2015-09-04 08:37:22]

A small change, PTAL. Thanks!

[bajones, 2015-09-04 17:33:39]

On 2015/09/04 08:37:22, yunchao wrote:
> A small change, PTAL. Thanks!

LGTM, thanks for the cleanup!

A general word of advice for these sorts of patches: The conformance tests can
be sensitive to changing the order that validations are performed in. For
example, if you pass both a deleted texture and a bad size to texImage2D the
tests may expect and INVALID_OPERATION (bad buffer) instead of a INVALID_VALUE
(bad size). I've inadvertently broken this before. As a result it's a good habit
to ensure that there's zero conformance regressions when doing these tests, even
with tests that seem unrelated.

[Zhenyao Mo, 2015-09-04 18:32:24]

On 2015/09/04 17:33:39, bajones wrote:
> On 2015/09/04 08:37:22, yunchao wrote:
> > A small change, PTAL. Thanks!
> 
> LGTM, thanks for the cleanup!
> 
> A general word of advice for these sorts of patches: The conformance tests can
> be sensitive to changing the order that validations are performed in. For
> example, if you pass both a deleted texture and a bad size to texImage2D the
> tests may expect and INVALID_OPERATION (bad buffer) instead of a INVALID_VALUE
> (bad size). I've inadvertently broken this before. As a result it's a good
habit
> to ensure that there's zero conformance regressions when doing these tests,
even
> with tests that seem unrelated.

It could also be the case the conformance tests assumes too much (say two error
cases, the spec usually doesn't say which one should take priority). In such
cases, we should relax conformance tests to != GL_NO_ERROR.

[yunchao, 2015-09-06 01:58:22]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-06 01:58:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310003006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310003006/20001

[commit-bot: I haz the power, 2015-09-06 01:59:32]

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

[commit-bot: I haz the power, 2015-09-06 01:59:33]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yunchao, 2015-09-07 14:29:43]

On 2015/09/04 17:33:39, bajones wrote:
> On 2015/09/04 08:37:22, yunchao wrote:
> > A small change, PTAL. Thanks!
> 
> LGTM, thanks for the cleanup!
> 
> A general word of advice for these sorts of patches: The conformance tests can
> be sensitive to changing the order that validations are performed in. For
> example, if you pass both a deleted texture and a bad size to texImage2D the
> tests may expect and INVALID_OPERATION (bad buffer) instead of a INVALID_VALUE
> (bad size). I've inadvertently broken this before. As a result it's a good
habit
> to ensure that there's zero conformance regressions when doing these tests,
even
> with tests that seem unrelated.

Yeah. Thanks for your advice, Brandon. I also came across this kind of failures
before. After that, I run all WebGL 1.0 conformance test to avoid potential
failures for my CL. But for WebGL 2.0 test cases, I just run the related cases,
because there are many failures If I run all tests. It is not easy to find out
the difference w/ my CL or not for webgl 2.0 test cases.

[yunchao, 2015-09-07 14:32:24]

On 2015/09/04 18:32:24, Zhenyao Mo wrote:
> On 2015/09/04 17:33:39, bajones wrote:
> > On 2015/09/04 08:37:22, yunchao wrote:
> > > A small change, PTAL. Thanks!
> > 
> > LGTM, thanks for the cleanup!
> > 
> > A general word of advice for these sorts of patches: The conformance tests
can
> > be sensitive to changing the order that validations are performed in. For
> > example, if you pass both a deleted texture and a bad size to texImage2D the
> > tests may expect and INVALID_OPERATION (bad buffer) instead of a
INVALID_VALUE
> > (bad size). I've inadvertently broken this before. As a result it's a good
> habit
> > to ensure that there's zero conformance regressions when doing these tests,
> even
> > with tests that seem unrelated.
> 
> It could also be the case the conformance tests assumes too much (say two
error
> cases, the spec usually doesn't say which one should take priority). In such
> cases, we should relax conformance tests to != GL_NO_ERROR.

Thanks for your suggestion, zmo@. It makes sense. 

bajones@ and zmo@, could you have a double check? I have updated the code to
remove unnecessary validation in both texSubImage2D and texImage2D. There is no
regression in WebGL 1.0 conformance tests suite.

[Zhenyao Mo, 2015-09-07 20:56:28]

LGTM

[yunchao, 2015-09-08 01:44:16]

The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-08 01:44:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310003006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310003006/60001

[commit-bot: I haz the power, 2015-09-08 02:47:19]

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

[commit-bot: I haz the power, 2015-09-08 02:47:19]

Dry run: This issue passed the CQ dry run.

[yunchao, 2015-09-08 02:48:28]

The CQ bit was checked by yunchao.he@intel.com

[yunchao, 2015-09-08 02:48:28]

The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/1310003006/#ps60001
(title: "remove unnecessary validations in texImage2D/texImage2DBase")

[commit-bot: I haz the power, 2015-09-08 02:48:36]

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

[commit-bot: I haz the power, 2015-09-08 02:52:58]

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201886