Factor out cc::OutputSurface::InitializeAndSetContext3D

Factor out cc::OutputSurface::InitializeAndSetContext3D

This is a protected method so that it can be called by subclasses to set
and initialize a new Context3D post construction time.

BUG=230197
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204976

[boliu, 2013-06-05 18:42:21]

part 2 to the whole webview gl init path

[danakj, 2013-06-05 18:50:22]

LGTM do you think the LTHI stuff to handle this changing should probably land
first?

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface.c...
cc/output/output_surface.cc:86: if (!success)
if (success) client_ = client; and drop the above assignment

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface_u...
File cc/output/output_surface_unittest.cc (right):

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface_u...
cc/output/output_surface_unittest.cc:71:
ASSERT_FALSE(client.did_lose_output_surface_called());
nit: EXPECT instead of ASSERT throughout, this doesn't need to early-out to
prevent a crash

[boliu, 2013-06-05 19:06:31]

On 2013/06/05 18:50:22, danakj wrote:
> LGTM do you think the LTHI stuff to handle this changing should probably land
> first?

Sure, this can wait.

I'm thinking call LTHI DeferredInitialize (or whatever we end up naming it) from
InitializeAndSetContext3D instead of having a subclass do it. Feels more
correct.

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface.c...
cc/output/output_surface.cc:86: if (!success)
On 2013/06/05 18:50:22, danakj wrote:
> if (success) client_ = client; and drop the above assignment

Won't work because client_ is used in InitializeAndSetContext3D to create new
OutputSurfaceCallbacks.

Also context3d_.Pass() and then gets assigned back is a weird pattern as well,
but this is all to not duplicate code.

I'll take any suggestions to make this less weird.

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface_u...
File cc/output/output_surface_unittest.cc (right):

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface_u...
cc/output/output_surface_unittest.cc:71:
ASSERT_FALSE(client.did_lose_output_surface_called());
On 2013/06/05 18:50:22, danakj wrote:
> nit: EXPECT instead of ASSERT throughout, this doesn't need to early-out to
> prevent a crash

All except ASSERT_TRUE(output_surface.context3d()); in the last test.
That one does prevent a crash.

[danakj, 2013-06-05 19:10:12]

> I'm thinking call LTHI DeferredInitialize (or whatever we end up naming it)
from
> InitializeAndSetContext3D instead of having a subclass do it. Feels more
> correct.

I think I agree, it should be the same for everyone.

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/2001/cc/output/output_surface.c...
cc/output/output_surface.cc:86: if (!success)
On 2013/06/05 19:06:31, boliu wrote:
> On 2013/06/05 18:50:22, danakj wrote:
> > if (success) client_ = client; and drop the above assignment
> 
> Won't work because client_ is used in InitializeAndSetContext3D to create new
> OutputSurfaceCallbacks.
> 
> Also context3d_.Pass() and then gets assigned back is a weird pattern as well,
> but this is all to not duplicate code.
> 
> I'll take any suggestions to make this less weird.

Ah okay, I see. Well we have a test for exactly this, so I think it's all good.

[boliu, 2013-06-07 00:01:20]

Rebased onto part 1 and calls DeferredInitialize.

Still need to refactor the test code, but rest is read for another look.

[boliu, 2013-06-07 00:36:56]

Refactored tests. PTAL.

[joth, 2013-06-07 01:02:14]

fly-by commment, but lgtm

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface....
cc/output/output_surface.h:111: bool InitializeAndSetContext3D(
document?
esp. is context_provider for offscreen (impl thread only?) context?

[danakj, 2013-06-07 14:26:09]

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface....
cc/output/output_surface.cc:105: scoped_ptr<WebKit::WebGraphicsContext3D>
original_context3d =
mmm... so you already have a context3d, and now you're passing in a new and
different context3d? I'm not sure I'm understanding why this is so. Why wouldn't
the context3d always be null at this point?

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface....
cc/output/output_surface.h:111: bool InitializeAndSetContext3D(
On 2013/06/07 01:02:14, joth wrote:
> document?
> esp. is context_provider for offscreen (impl thread only?) context?

Agree, offscreen_context_provider is the name used for this variable elsewhere.

[boliu, 2013-06-07 16:03:44]

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface....
cc/output/output_surface.cc:105: scoped_ptr<WebKit::WebGraphicsContext3D>
original_context3d =
On 2013/06/07 14:26:09, danakj wrote:
> mmm... so you already have a context3d, and now you're passing in a new and
> different context3d? I'm not sure I'm understanding why this is so. Why
wouldn't
> the context3d always be null at this point?

It should be null in webview, but I coded the unittests to start with non-null
context3d. Changed tests to start in software mode, dropped this reset on
failure code, and add DCHECK(!context3d_)

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/15647021/diff/15001/cc/output/output_surface....
cc/output/output_surface.h:111: bool InitializeAndSetContext3D(
On 2013/06/07 14:26:09, danakj wrote:
> On 2013/06/07 01:02:14, joth wrote:
> > document?
> > esp. is context_provider for offscreen (impl thread only?) context?
> 
> Agree, offscreen_context_provider is the name used for this variable
elsewhere.

Done.

[danakj, 2013-06-07 17:22:23]

https://codereview.chromium.org/15647021/diff/21002/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/21002/cc/output/output_surface....
cc/output/output_surface.cc:113: context3d_.reset();
reset callbacks_ too? (before the context3d)

https://codereview.chromium.org/15647021/diff/21002/cc/output/output_surface....
cc/output/output_surface.cc:120: DCHECK(context3d);
DCHECK(!context3d_) here too?

[boliu, 2013-06-07 17:27:49]

both done

[danakj, 2013-06-07 17:32:00]

LGTM

https://codereview.chromium.org/15647021/diff/24002/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/24002/cc/output/output_surface....
cc/output/output_surface.cc:114: callbacks_.reset();
nit: since the callbacks use the context3d, i think the order here should be
reversed

[boliu, 2013-06-07 17:34:31]

https://codereview.chromium.org/15647021/diff/24002/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/24002/cc/output/output_surface....
cc/output/output_surface.cc:114: callbacks_.reset();
On 2013/06/07 17:32:00, danakj wrote:
> nit: since the callbacks use the context3d, i think the order here should be
> reversed

Actually context3d uses callbacks, so the order is correct. It's also why I
moved callbacks_ above context3d_ in the header file.

[danakj, 2013-06-07 17:48:07]

https://codereview.chromium.org/15647021/diff/24002/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15647021/diff/24002/cc/output/output_surface....
cc/output/output_surface.cc:114: callbacks_.reset();
On 2013/06/07 17:34:31, boliu wrote:
> On 2013/06/07 17:32:00, danakj wrote:
> > nit: since the callbacks use the context3d, i think the order here should be
> > reversed
> 
> Actually context3d uses callbacks, so the order is correct. It's also why I
> moved callbacks_ above context3d_ in the header file.

Oh, I'm thinking that when callbacks go away they set NULL on the context3d.
You're thinking if the context goes away, it doesn't matter what its pointers
were. I guess the callbacks aren't cleaned up though, I just assumed they were.
LGTM

[commit-bot: I haz the power, 2013-06-07 17:51:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/15647021/24002

[commit-bot: I haz the power, 2013-06-07 23:38:46]

Change committed as 204976