DrawingBuffer clean-up: Aesthetic changes

DrawingBuffer clean-up: Aesthetic changes

Rename several variables to reflect that they are based on ColorBuffers.

Re-organize createColorBuffer to be in a single function with multiple
steps, rather than several functions that call each other.

Re-organize prepareTextureMailboxInternal to branch off into separate
gpu and software implementations.

This patch should have no functional effect.

BUG=648707
Committed: https://crrev.com/8b7a69f6544da9a4b1fa4c0719e814269115acbd
Cr-Commit-Position: refs/heads/master@{#424006}

[ccameron, 2016-10-06 21:11:05]

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

[commit-bot: I haz the power, 2016-10-06 21:11:47]

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-10-06 21:15:21]

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

[commit-bot: I haz the power, 2016-10-06 21:15:22]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-10-07 01:07:01]

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

[commit-bot: I haz the power, 2016-10-07 01:07:23]

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

[ccameron, 2016-10-07 01:09:57]

ccameron@chromium.org changed reviewers:
+ erikchen@chromium.org, kbr@chromium.org

[ccameron, 2016-10-07 01:09:58]

This rearranges code and renames variables.

In the instance of ColorBuffer allocation, I replaced the inter-calling network
of functions with one function which, although long, has very clear steps (and
I've broken them into blocks).

In the instance of populating mailboxes, I've split the function into the
software and GPU flavors.

[commit-bot: I haz the power, 2016-10-07 02:23:18]

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

[commit-bot: I haz the power, 2016-10-07 02:23:19]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-10-07 03:33:53]

Thank you for cleaning up this code. Your reorganization is excellent and makes
the code much more comprehensible. LGTM. One comment/question.

https://codereview.chromium.org/2397713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2397713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1177:
!m_wantAlphaChannel) {
Should this code path also be gated on shouldUseChromiumImage()? It looks like
it used to implicitly be in the old
DrawingBuffer::createTextureAndAllocateMemory.

[ccameron, 2016-10-07 08:28:14]

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

[commit-bot: I haz the power, 2016-10-07 08:28:21]

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

[ccameron, 2016-10-07 08:28:23]

Thanks -- good catch!

https://codereview.chromium.org/2397713005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2397713005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1177:
!m_wantAlphaChannel) {
On 2016/10/07 03:33:53, Ken Russell wrote:
> Should this code path also be gated on shouldUseChromiumImage()? It looks like
> it used to implicitly be in the old
> DrawingBuffer::createTextureAndAllocateMemory.

Yes, good catch. I've conditionalized this on having allocated an imageId.

[commit-bot: I haz the power, 2016-10-07 12:10:21]

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

[commit-bot: I haz the power, 2016-10-07 12:10:22]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-10-07 16:25:09]

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:319: {
why the new scope?

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1116:
std::vector<uint8_t> scanline(width * 4);
if this gets called frequently, the new logic seems like a performance
regression?

[Ken Russell (switch to Gerrit), 2016-10-07 18:34:41]

Thanks. Still LGTM.

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1116:
std::vector<uint8_t> scanline(width * 4);
On 2016/10/07 16:25:09, erikchen wrote:
> if this gets called frequently, the new logic seems like a performance
> regression?

This is a pretty slow path anyway. I'm personally not concerned about moving the
allocation here, and it'll reduce the bookkeeping involved.

[ccameron, 2016-10-07 20:36:30]

Thanks!

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:319: {
On 2016/10/07 16:25:09, erikchen wrote:
> why the new scope?

Two reasons:
- it makes it clear what the steps in the function are (without creating actual
separate functions)
- it makes it clear what the scope of use of the temporary variables are
(needPremultiply, pixels, etc)

I added lots of block like this to finishPrepareTextureMailboxGpu for the same
reasons, and I think it really helps with the readability of the function.

This particular function isn't very long, so it doesn't make too much of a
difference. I was thinking to put the *outReleaseCallback and its "func"
variable into a block as well, but that seemed excessive.

https://codereview.chromium.org/2397713005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1116:
std::vector<uint8_t> scanline(width * 4);
On 2016/10/07 18:34:41, Ken Russell wrote:
> On 2016/10/07 16:25:09, erikchen wrote:
> > if this gets called frequently, the new logic seems like a performance
> > regression?
> 
> This is a pretty slow path anyway. I'm personally not concerned about moving
the
> allocation here, and it'll reduce the bookkeeping involved.

Yeah, I suspect that this was premature optimization. Reading back from GL is a
very painful path (it includes an implicit GL finish, depending how you do it).

[ccameron, 2016-10-07 23:11:59]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-10-07 23:12:34]

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-10-07 23:17:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-07 23:20:51]

Description was changed from

==========
DrawingBuffer clean-up: Aesthetic changes

Rename several variables to reflect that they are based on ColorBuffers.

Re-organize createColorBuffer to be in a single function with multiple
steps, rather than several functions that call each other.

Re-organize prepareTextureMailboxInternal to branch off into separate
gpu and software implementations.

This patch should have no functional effect.

BUG=648707
==========

to

==========
DrawingBuffer clean-up: Aesthetic changes

Rename several variables to reflect that they are based on ColorBuffers.

Re-organize createColorBuffer to be in a single function with multiple
steps, rather than several functions that call each other.

Re-organize prepareTextureMailboxInternal to branch off into separate
gpu and software implementations.

This patch should have no functional effect.

BUG=648707

Committed: https://crrev.com/8b7a69f6544da9a4b1fa4c0719e814269115acbd
Cr-Commit-Position: refs/heads/master@{#424006}
==========

[commit-bot: I haz the power, 2016-10-07 23:20:53]

Patchset 3 (id:??) landed as
https://crrev.com/8b7a69f6544da9a4b1fa4c0719e814269115acbd
Cr-Commit-Position: refs/heads/master@{#424006}