Remove the "target" argument from CopyTextureChromium.

Remove the "target" argument from CopyTextureChromium.

The argument is meaningless, since the function also takes a source_id and
dest_id, each of which is already bound to a target. This is a refactor and has
no intended behavior change.

BUG=533617
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOPRESUBMIT=true

Committed: https://crrev.com/50b7660fa4b9aeb05d7e9b8919febeea5ccc1374
Cr-Commit-Position: refs/heads/master@{#367878}

[erikchen, 2015-12-31 00:32:47]

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

[commit-bot: I haz the power, 2015-12-31 00:33:38]

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

[commit-bot: I haz the power, 2015-12-31 00:41:51]

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

[commit-bot: I haz the power, 2015-12-31 00:41:52]

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

[erikchen, 2015-12-31 00:56:00]

Description was changed from

==========
Remove the "target" argument from CopyTextureChromium.

The argument is meaningless, since the function also takes a source_id and
dest_id, each of which is already bound to a target. This is a refactor and has
no intended behavior change.

BUG=533617
==========

to

==========
Remove the "target" argument from CopyTextureChromium.

The argument is meaningless, since the function also takes a source_id and
dest_id, each of which is already bound to a target. This is a refactor and has
no intended behavior change.

BUG=533617
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2015-12-31 00:56:01]

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

[commit-bot: I haz the power, 2015-12-31 00:56:21]

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

[commit-bot: I haz the power, 2015-12-31 01:06:09]

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

[commit-bot: I haz the power, 2015-12-31 01:06:10]

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

[erikchen, 2015-12-31 01:37:17]

erikchen@chromium.org changed reviewers:
+ zmo@chromium.org

[erikchen, 2015-12-31 01:37:18]

zmo: Please review.

What's up with the presubmit error?

[Zhenyao Mo, 2016-01-04 18:12:33]

On 2015/12/31 01:37:18, erikchen wrote:
> zmo: Please review.
> 
> What's up with the presubmit error?

lgtm

You will need to add ppapi/lib/gl/include/GLES2/gl2ext.h needs to
src/tools/copyright_scanner/third_party_files_whitelist.txt

[Zhenyao Mo, 2016-01-04 18:12:44]

https://codereview.chromium.org/1551143002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1551143002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13500: void
GLES2DecoderImpl::DoCompressedCopyTextureCHROMIUM(GLenum target,
We probably should get rid of this target arg also.

Maybe in a separate CL?

https://codereview.chromium.org/1551143002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13676: void
GLES2DecoderImpl::DoCompressedCopySubTextureCHROMIUM(GLenum target,
Same here, get rid of this target in a separate CL?

[erikchen, 2016-01-04 18:50:40]

Description was changed from

==========
Remove the "target" argument from CopyTextureChromium.

The argument is meaningless, since the function also takes a source_id and
dest_id, each of which is already bound to a target. This is a refactor and has
no intended behavior change.

BUG=533617
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove the "target" argument from CopyTextureChromium.

The argument is meaningless, since the function also takes a source_id and
dest_id, each of which is already bound to a target. This is a refactor and has
no intended behavior change.

BUG=533617
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOPRESUBMIT=true
==========

[erikchen, 2016-01-04 18:52:04]

On 2016/01/04 18:12:33, Zhenyao Mo wrote:
> On 2015/12/31 01:37:18, erikchen wrote:
> > zmo: Please review.
> > 
> > What's up with the presubmit error?
> 
> lgtm
> 
> You will need to add ppapi/lib/gl/include/GLES2/gl2ext.h needs to
> src/tools/copyright_scanner/third_party_files_whitelist.txt

Actually, that doesn't help. Looking at the commit history for
ppapi/lib/gl/include/GLES2/gl2ext.h, it looks like the standard technique is to
add NOPRESUBMIT=true, which I've done.

[erikchen, 2016-01-04 18:52:10]

https://codereview.chromium.org/1551143002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1551143002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13500: void
GLES2DecoderImpl::DoCompressedCopyTextureCHROMIUM(GLenum target,
On 2016/01/04 18:12:43, Zhenyao Mo wrote:
> We probably should get rid of this target arg also.
> 
> Maybe in a separate CL?

yup

https://codereview.chromium.org/1551143002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13676: void
GLES2DecoderImpl::DoCompressedCopySubTextureCHROMIUM(GLenum target,
On 2016/01/04 18:12:44, Zhenyao Mo wrote:
> Same here, get rid of this target in a separate CL?

can do.

[erikchen, 2016-01-04 19:02:37]

erikchen@chromium.org changed reviewers:
+ bbudge@chromium.org, danakj@chromium.org, esprehn@chromium.org,
piman@chromium.org, qinmin@chromium.org, xhwang@chromium.org,
yzshen@chromium.org

[erikchen, 2016-01-04 19:02:37]

qinmin: Please review content/renderer/media/android/webmediaplayer_android.cc

danakj: Please review cc/

piman: Please review gpu/blink/

xhwang: Please review media/

yzshen: Please review mojo/public/

bbudge: Please review ppapi/

esprehn: Please review third_party/WebKit/Source/platform and
third_party/WebKit/public

[xhwang, 2016-01-04 19:08:51]

media/ rs lgtm

[qinmin, 2016-01-04 19:12:00]

lgtm

[bbudge, 2016-01-04 20:00:31]

ppapi LGTM

[yzshen1, 2016-01-04 20:54:11]

mojo/public LGTM

[esprehn, 2016-01-04 21:01:37]

lgtm

[Zhenyao Mo, 2016-01-04 22:47:59]

still lgtm, but could you add the revision history?

https://codereview.chromium.org/1551143002/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/1551143002/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:166:
unpack_unmultiply_alpha to both commands.
Just realized. I think it's worth an entry in the revision history since it's a
function signature change.

[erikchen, 2016-01-04 23:13:52]

https://codereview.chromium.org/1551143002/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/1551143002/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:166:
unpack_unmultiply_alpha to both commands.
On 2016/01/04 22:47:59, Zhenyao Mo wrote:
> Just realized. I think it's worth an entry in the revision history since it's
a
> function signature change.

Done.

[piman, 2016-01-05 00:21:10]

https://codereview.chromium.org/1551143002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1551143002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13196: GLenum dest_target =
dest_texture->target();
These can be 0 if this is used before the texture is ever bound (the
TextureRefs/Textures could still be valid).

Can you add an explicit error path (GL_INVALID_OPERATION) for that case?

https://codereview.chromium.org/1551143002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13361: GLenum dest_target =
dest_texture->target();
Ditto.

[erikchen, 2016-01-05 00:34:15]

piman: PTAL

https://codereview.chromium.org/1551143002/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1551143002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13196: GLenum dest_target =
dest_texture->target();
On 2016/01/05 00:21:09, piman (OOO until 2016-1-4) wrote:
> These can be 0 if this is used before the texture is ever bound (the
> TextureRefs/Textures could still be valid).
> 
> Can you add an explicit error path (GL_INVALID_OPERATION) for that case?

This is checked by ValidateCopyTextureChromium(). I've moved the check closer to
the top of this function. I also got rid of the (!source_texture_ref ||
!dest_texture_ref) conditional, since that's also checked in
ValidateCopyTextureChromium().

https://codereview.chromium.org/1551143002/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:13361: GLenum dest_target =
dest_texture->target();
On 2016/01/05 00:21:10, piman (OOO until 2016-1-4) wrote:
> Ditto.

Done.

[danakj, 2016-01-06 00:00:14]

cc LGTM

[piman, 2016-01-06 07:28:27]

lgtm

[erikchen, 2016-01-06 18:05:12]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-01-06 18:05:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
esprehn@chromium.org, zmo@chromium.org, qinmin@chromium.org,
yzshen@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1551143002/#ps120001
(title: "Fix compile error.")

[commit-bot: I haz the power, 2016-01-06 18:05:34]

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

[commit-bot: I haz the power, 2016-01-06 19:35:45]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-06 19:36:30]

Description was changed from

==========
Remove the "target" argument from CopyTextureChromium.

The argument is meaningless, since the function also takes a source_id and
dest_id, each of which is already bound to a target. This is a refactor and has
no intended behavior change.

BUG=533617
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOPRESUBMIT=true
==========

to

==========
Remove the "target" argument from CopyTextureChromium.

The argument is meaningless, since the function also takes a source_id and
dest_id, each of which is already bound to a target. This is a refactor and has
no intended behavior change.

BUG=533617
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOPRESUBMIT=true

Committed: https://crrev.com/50b7660fa4b9aeb05d7e9b8919febeea5ccc1374
Cr-Commit-Position: refs/heads/master@{#367878}
==========

[commit-bot: I haz the power, 2016-01-06 19:36:32]

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