Collapse the initialize methods in WebGraphicsContext3DCommandBufferImpl

Collapse the initialize methods in WebGraphicsContext3DCommandBufferImpl

There are many such methods that we call through when initializing a
WebGraphicsContext3DCommandBufferImpl, which means when moving things
out of the class I have to add plumbing through all these methods. I
don't intend to replicate this method structure in when moving init
out to ContextProviderCommandBuffer either really.

So collapse all the initialization down to a single helper method
MaybeInitializeGL.

Also remove the "if initialized" check, as BindToCurrentThread()
already does that in ContextProviderCommandBuffer.

Also also make ContextProviderCommandBuffer destroy the
WebGraphicsContext3DCommandBufferImpl if initialization fails in
BindToCurrentThread(). This means we don't need the "Destroy()"
step in WebGraphicsContext3DCommandBufferImpl when it fails to
initialize since it's just going to be deleted anyways. And lets
us move the "if initialize failed" check out to BindToCurrentThread().

R=piman@chromium.org, sievers
BUG=584497
Committed: https://crrev.com/ea18f360da40ebcd2ec516d49e9b0918123eee1b
Cr-Commit-Position: refs/heads/master@{#391125}

[danakj, 2016-04-30 01:47:37]

Patchset #2 (id:20001) has been deleted

[danakj, 2016-04-30 01:47:57]

Patchset #1 (id:1) has been deleted

[danakj, 2016-04-30 01:48:02]

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

[commit-bot: I haz the power, 2016-04-30 01:48:28]

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

[danakj, 2016-04-30 01:49:43]

Patchset #1 (id:40001) has been deleted

[danakj, 2016-04-30 01:49:47]

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

[commit-bot: I haz the power, 2016-04-30 01:49:57]

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

[danakj, 2016-04-30 01:56:40]

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(left):

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:79: //
TODO(vadimt): Remove ScopedTracker below once crbug.com/125248 is fixed.
Moved this out to GpuProcessTransportFactory, since this only applies to the
browser compositor and we already know it's tracking the time of
BindToCurrentThread, most of which is in the sync IPCs creating the
CommandBufferProxyImpl.

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:128:
DVLOG_IF(1, gpu::error::IsError(command_buffer_->GetLastError()))
I can't see a way for last_state_ to be set to anything interesting after
creating the CommandBuffer. We do a sync call but don't do pass it a last_state_
pointer like we do in WaitForToken, so I dropped this print.

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:222:
WebGraphicsContext3DCommandBufferImpl::GetContextSupport() {
This is just collatoral damage, as it's pointless since we return the
GLES2Implementation already. So just use that in the provider.

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:95:
gles2_helper_->SetAutomaticFlushes(automatic_flushes_);
I thot it was a bit weird that we set this after calling Initialize which
potentially makes it do more work, so I put this above Initialize so it's just
always true. It seems like this should be ok since Init calls AllocateRingBuffer
which does CalcImmediateEntries, and that's the only place this value is used.
And that's what SetAutomaticFlushes would do also after Initialize (tho here it
would just early out now so it's cheaper?) I also considered making it part of
the constructor. WDYT, is this ok?

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:97:
DLOG(ERROR) << "Failed to initialize GLES2CmdHelper.";
I turned all the LOGs into DLOGs which print when DCHECK_IS_ON so it's there on
the gpu bots.

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(left):

https://codereview.chromium.org/1936503003/diff/60001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:144:
base::WeakPtrFactory<WebGraphicsContext3DCommandBufferImpl> weak_ptr_factory_;
This was just unused.

[piman, 2016-05-02 20:45:17]

LGTM except for 1 thing

https://codereview.chromium.org/1936503003/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (left):

https://codereview.chromium.org/1936503003/diff/80001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:82:
context3d_->GetCommandBufferProxy()->SetLock(nullptr);
See buildbots. If we don't reset the lock on the CBPI, it needs to be held
during teardown.

[danakj, 2016-05-02 21:11:36]

https://codereview.chromium.org/1936503003/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/context_provider_command_buffer.cc (left):

https://codereview.chromium.org/1936503003/diff/80001/content/common/gpu/clie...
content/common/gpu/client/context_provider_command_buffer.cc:82:
context3d_->GetCommandBufferProxy()->SetLock(nullptr);
On 2016/05/02 20:45:17, piman wrote:
> See buildbots. If we don't reset the lock on the CBPI, it needs to be held
> during teardown.

Ah okay.. how about I set the lock to null, but avoid branching on
GetCommandBufferProxy() and just do this if we were previously bound. (moved
this into the block below and added a comment)

[piman, 2016-05-02 22:08:11]

lgtm

[danakj, 2016-05-02 23:10:43]

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

[commit-bot: I haz the power, 2016-05-02 23:11:21]

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

[commit-bot: I haz the power, 2016-05-03 00:20:12]

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

[commit-bot: I haz the power, 2016-05-03 00:20:13]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-05-03 00:33:40]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-05-03 00:33:40]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1936503003/#ps120001
(title: "collapse: rebase")

[commit-bot: I haz the power, 2016-05-03 00:34:13]

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

[commit-bot: I haz the power, 2016-05-03 00:40:39]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-05-03 00:42:44]

Description was changed from

==========
Collapse the initialize methods in WebGraphicsContext3DCommandBufferImpl

There are many such methods that we call through when initializing a
WebGraphicsContext3DCommandBufferImpl, which means when moving things
out of the class I have to add plumbing through all these methods. I
don't intend to replicate this method structure in when moving init
out to ContextProviderCommandBuffer either really.

So collapse all the initialization down to a single helper method
MaybeInitializeGL.

Also remove the "if initialized" check, as BindToCurrentThread()
already does that in ContextProviderCommandBuffer.

Also also make ContextProviderCommandBuffer destroy the
WebGraphicsContext3DCommandBufferImpl if initialization fails in
BindToCurrentThread(). This means we don't need the "Destroy()"
step in WebGraphicsContext3DCommandBufferImpl when it fails to
initialize since it's just going to be deleted anyways. And lets
us move the "if initialize failed" check out to BindToCurrentThread().

R=piman@chromium.org, sievers
BUG=584497
==========

to

==========
Collapse the initialize methods in WebGraphicsContext3DCommandBufferImpl

There are many such methods that we call through when initializing a
WebGraphicsContext3DCommandBufferImpl, which means when moving things
out of the class I have to add plumbing through all these methods. I
don't intend to replicate this method structure in when moving init
out to ContextProviderCommandBuffer either really.

So collapse all the initialization down to a single helper method
MaybeInitializeGL.

Also remove the "if initialized" check, as BindToCurrentThread()
already does that in ContextProviderCommandBuffer.

Also also make ContextProviderCommandBuffer destroy the
WebGraphicsContext3DCommandBufferImpl if initialization fails in
BindToCurrentThread(). This means we don't need the "Destroy()"
step in WebGraphicsContext3DCommandBufferImpl when it fails to
initialize since it's just going to be deleted anyways. And lets
us move the "if initialize failed" check out to BindToCurrentThread().

R=piman@chromium.org, sievers
BUG=584497

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

[commit-bot: I haz the power, 2016-05-03 00:42:45]

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