Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.

For this change, we just need to make sure that all bots are green.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/11401539431e6ff15901f90f4d1e87033dc77ba5
Cr-Commit-Position: refs/heads/master@{#398286}

[xidachen, 2016-05-30 15:54:44]

Description was changed from

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.
==========

to

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xidachen, 2016-05-30 15:55:49]

Description was changed from

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.

For this change, we just need to make sure that all bots are green.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xidachen, 2016-05-30 15:55:49]

xidachen@chromium.org changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[xidachen, 2016-05-30 17:30:33]

PTAL.

[Zhenyao Mo, 2016-05-31 02:07:57]

On 2016/05/30 17:30:33, xidachen wrote:
> PTAL.

This doesn't compile...

[xidachen, 2016-05-31 02:11:41]

https://codereview.chromium.org/2025703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4062: if
((!strcmp(funcType, "texImage3D") || !strcmp(funcType, "texSubImage3D")) &&
!validateTexture3DBinding(funcType, target))
Yes, the error is because validateTexture3DBinding() is defined in
WebGL2RenderingContext. Would it be acceptable to define this as virtual in
WebGLRenderingContext and override it in WebGL2?

[Zhenyao Mo, 2016-05-31 16:37:33]

https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4054: void
WebGLRenderingContextBase::texImageHelperDOMArrayBufferView(const char*
funcType, GLenum target, GLint level, GLint internalformat,
This should be named funcName to be consistent with other functions.

https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4060: if
((!strcmp(funcType, "texImage2D") || !strcmp(funcType, "texSubImage2D")) &&
!validateTexture2DBinding(funcType, target))
To me, you can make a virtual function validateTextureBinding(), and in WebGL 1,
you only deal the 2D binding, and in WebGL 2, you deal with both 2D and 3D
bindings, depending on the function. This way, we don't have to expose
validateTexture3DBinding in the WebGL 1 context, which to me is really ugly.

https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4072: if
(!strcmp(funcType, "texImage2D") || !strcmp(funcType, "texSubImage2D"))
Can you find a way not to do all these duplicated strcmp? Even though it may not
be heavy, but we should still strive to optimize the code, even a small piece.

[xidachen, 2016-05-31 17:15:43]

https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4072: if
(!strcmp(funcType, "texImage2D") || !strcmp(funcType, "texSubImage2D"))
On 2016/05/31 16:37:33, Zhenyao Mo wrote:
> Can you find a way not to do all these duplicated strcmp? Even though it may
not
> be heavy, but we should still strive to optimize the code, even a small piece.

Thank you for pointing it out. Just realized that I have injected a whole bunch
of strcmp here.

What about instead of passing a const char*, I pass an enum, and use switch
here? Would that be better?

[Zhenyao Mo, 2016-05-31 17:16:34]

On 2016/05/31 17:15:43, xidachen wrote:
>
https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2025703002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4072: if
> (!strcmp(funcType, "texImage2D") || !strcmp(funcType, "texSubImage2D"))
> On 2016/05/31 16:37:33, Zhenyao Mo wrote:
> > Can you find a way not to do all these duplicated strcmp? Even though it may
> not
> > be heavy, but we should still strive to optimize the code, even a small
piece.
> 
> Thank you for pointing it out. Just realized that I have injected a whole
bunch
> of strcmp here.
> 
> What about instead of passing a const char*, I pass an enum, and use switch
> here? Would that be better?

Yeah, that should be fine.

[Ken Russell (switch to Gerrit), 2016-06-01 00:34:03]

Thanks for thinking about this and working on sharing more code. There are some
aspects of these helper functions that need more thought. Could you please spend
some more time thinking about how to make the design more clear and consistent?

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1016:
bool succeed = texImageHelperHTMLImageElement(TexSubImage3D, target, level, 0,
format, type, xoffset, yoffset, zoffset, image, exceptionState);
It's very confusing that this is expected to fall through here and execute the
code after the helper function in order to do the 3D texture upload.

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4601:
WebGLTexture* texture = texImageHelperHTMLCanvasElement(TexSubImage2D, target,
level, 0, format, type, xoffset, yoffset, 0, canvas, exceptionState);
It's confusing that some of the new helpers are expected to do all of the work
inside themselves, but this one essentially only does the validation, leaving
the caller to do the actual upload. Same for texImageHelperHTMLVideoElement.

[xidachen, 2016-06-01 19:59:15]

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1016:
bool succeed = texImageHelperHTMLImageElement(TexSubImage3D, target, level, 0,
format, type, xoffset, yoffset, zoffset, image, exceptionState);
On 2016/06/01 00:34:03, Ken Russell wrote:
> It's very confusing that this is expected to fall through here and execute the
> code after the helper function in order to do the 3D texture upload.

I agree. It seems that the only reason that there are still some code after this
Helper function is that it calls texSubImage3DImpl, while texSubImage3DImpl is
not accessible from WebGL 1.

I looked deeper into it, and I found that texSubImage3DImpl is almost identical
with texSubImage2DImpl, which is also very similar to texImage2DImpl. It feels
like a lot of code can be shared here. But if I do everything in one patch, it
will be too big and hard to manage. I will probably break things into small CLs.

Also, I found that this if statement is duplicated in several places. I will try
to clean it up.
if (type == GL_UNSIGNED_INT_10F_11F_11F_REV) {
        // The UNSIGNED_INT_10F_11F_11F_REV type pack/unpack isn't implemented.
        type = GL_FLOAT;
}

Does the above makes sense?

[Ken Russell (switch to Gerrit), 2016-06-02 00:58:50]

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1016:
bool succeed = texImageHelperHTMLImageElement(TexSubImage3D, target, level, 0,
format, type, xoffset, yoffset, zoffset, image, exceptionState);
On 2016/06/01 19:59:15, xidachen wrote:
> On 2016/06/01 00:34:03, Ken Russell wrote:
> > It's very confusing that this is expected to fall through here and execute
the
> > code after the helper function in order to do the 3D texture upload.
> 
> I agree. It seems that the only reason that there are still some code after
this
> Helper function is that it calls texSubImage3DImpl, while texSubImage3DImpl is
> not accessible from WebGL 1.
> 
> I looked deeper into it, and I found that texSubImage3DImpl is almost
identical
> with texSubImage2DImpl, which is also very similar to texImage2DImpl. It feels
> like a lot of code can be shared here. But if I do everything in one patch, it
> will be too big and hard to manage. I will probably break things into small
CLs.
> 
> Also, I found that this if statement is duplicated in several places. I will
try
> to clean it up.
> if (type == GL_UNSIGNED_INT_10F_11F_11F_REV) {
>         // The UNSIGNED_INT_10F_11F_11F_REV type pack/unpack isn't
implemented.
>         type = GL_FLOAT;
> }
> 
> Does the above makes sense?

Yes, but as we discussed offline, the very different behavior of the various
helpers is too confusing in this CL. It looks like there are three categories of
behaviors of the new helper functions; those which return void, those which
return bool, and those which return an object (WebGLTexture*) for later
consumption. Per our discussion, I'll wait for a revised CL that unifies these
behaviors somewhat.

[xidachen, 2016-06-02 21:37:15]

PTAL.

Right now all the tex(Sub)Image2D(3D) calls its own helper function and nothing
more.

[Zhenyao Mo, 2016-06-03 17:49:51]

lgtm with some minor suggestions.

but let kbr takes a final look.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4083: if
(!validateTexImageBinding(funcName, functionName, target))
This funcName and functionName is very confusing. Maybe just call one enum
functionID? Here and a bunch other places.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4103: if
(functionName == TexImage2D || functionName == TexSubImage2D) {
sourceType == Tex2D

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4110:
return;
For now let's not worry about flipY and premultiplyAlpha for 3D tex and texSub
functions, so no need for this logic.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4126:
contextGL()->TexSubImage3D(target, level, xoffset, yoffset, zoffset, width,
height, depth, format, type, data);
Same here, let's move this before the resetUnpackParameters(), with a comment
FIXME comment that we might implement flipY and premultiplyAlpha for 3D tex
functions.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4183: else
// must be "texSubImage3D"
Can you ASSERT? here and other places.

[xidachen, 2016-06-03 18:28:30]

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4083: if
(!validateTexImageBinding(funcName, functionName, target))
On 2016/06/03 17:49:51, Zhenyao Mo wrote:
> This funcName and functionName is very confusing. Maybe just call one enum
> functionID? Here and a bunch other places.

functionID is much clearer. Changed all over the places.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4103: if
(functionName == TexImage2D || functionName == TexSubImage2D) {
On 2016/06/03 17:49:51, Zhenyao Mo wrote:
> sourceType == Tex2D

Done.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4110:
return;
On 2016/06/03 17:49:51, Zhenyao Mo wrote:
> For now let's not worry about flipY and premultiplyAlpha for 3D tex and texSub
> functions, so no need for this logic.

Done.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4126:
contextGL()->TexSubImage3D(target, level, xoffset, yoffset, zoffset, width,
height, depth, format, type, data);
On 2016/06/03 17:49:51, Zhenyao Mo wrote:
> Same here, let's move this before the resetUnpackParameters(), with a comment
> FIXME comment that we might implement flipY and premultiplyAlpha for 3D tex
> functions.

Done.

https://codereview.chromium.org/2025703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4183: else
// must be "texSubImage3D"
On 2016/06/03 17:49:51, Zhenyao Mo wrote:
> Can you ASSERT? here and other places.

Done.

[xidachen, 2016-06-06 15:42:33]

kbr@: as suggested by zmo@, could you take a final look?

[Ken Russell (switch to Gerrit), 2016-06-07 04:06:53]

Very nice Xida. Thanks for persisting with this refactoring. I didn't have time
to compare the helper functions line-by-line with their originals but trust that
you've been careful and, further, that the tests sufficiently cover these code
paths. LGTM

https://codereview.chromium.org/2025703002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2025703002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4075:
return "texImage3D";
Based on my own experience with this, it's more future-proof to switch on
funcName and have four case labels with no default label. This will cause a
compile error if a new entry is added to TexImageFunctionID. Would you make that
minor change?

[xidachen, 2016-06-07 11:17:26]

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

[commit-bot: I haz the power, 2016-06-07 11:17:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2025703002/220001

[commit-bot: I haz the power, 2016-06-07 11:28:37]

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

[commit-bot: I haz the power, 2016-06-07 11:28:38]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[xidachen, 2016-06-07 11:56:05]

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

[commit-bot: I haz the power, 2016-06-07 11:56:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2025703002/240001

[commit-bot: I haz the power, 2016-06-07 13:12:58]

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

[commit-bot: I haz the power, 2016-06-07 13:12:59]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-06-07 13:13:48]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-06-07 13:13:48]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org,
kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2025703002/#ps240001
(title: "add a default case to prevent compile error")

[commit-bot: I haz the power, 2016-06-07 13:13:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2025703002/240001

[commit-bot: I haz the power, 2016-06-07 13:17:21]

Description was changed from

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.

For this change, we just need to make sure that all bots are green.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.

For this change, we just need to make sure that all bots are green.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-06-07 13:17:22]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-06-07 13:19:04]

Description was changed from

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.

For this change, we just need to make sure that all bots are green.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Pack repeated code in tex(Sub)Image2D and texSubImage3D into helper func

There are some repeated code in these functions and the only diff amongest
them is whether it gives "texImage2D" or "texSubImage2D" or "texSubImage3D".

This CL packs the repeated part into helper function. After this, when
we make changes to these functions in the future, we would only need to
change one place instead of 3 places. Also, this will reduce potential
bugs in the code. For example, at this moment, line 4131 in
WebGLRenderingContextBase.cpp (texImage2D) does this:
if (imageForRender && imageForRender->isSVGImage())

while line 1095 in WebGL2RenderingContextBase.cpp (texSubImage3D) does:
if (imageForRender->isSVGImage())
which doesn't do a null check on imageForRender.

This change can certainly prevent bugs like this.

For this change, we just need to make sure that all bots are green.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/11401539431e6ff15901f90f4d1e87033dc77ba5
Cr-Commit-Position: refs/heads/master@{#398286}
==========

[commit-bot: I haz the power, 2016-06-07 13:19:05]

Patchset 13 (id:??) landed as
https://crrev.com/11401539431e6ff15901f90f4d1e87033dc77ba5
Cr-Commit-Position: refs/heads/master@{#398286}