Fixing gpu_ipc_browsertests.cc

Fixing gpu_ipc_browsertests.cc
(Turns out they were not running before.)

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

[piman, 2013-06-26 21:41:03]

2 nits

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
File content/browser/gpu/gpu_context_tests.h (right):

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
content/browser/gpu/gpu_context_tests.h:29: context_->flush();
nit: why the need for flush here?
The insertSyncPoint should have flushed, and whatever happens afterwards should
have no effect on the signalSyncPoint behavior...

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_ip...
File content/browser/gpu/gpu_ipc_gputests.cc (right):

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_ip...
content/browser/gpu/gpu_ipc_gputests.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Nit: this test doesn't do IPCs, the file name seems wrong. How about:
gpu_in_process_context_tests.cc?

Also, I think this file can move to content/common/gpu/client, along with
gpu_content_tests.h

[hubbe, 2013-06-27 20:42:40]

Still working on figuring out why the tests fail on some platforms. (But works
on my desktop.)

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
File content/browser/gpu/gpu_context_tests.h (right):

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
content/browser/gpu/gpu_context_tests.h:29: context_->flush();
On 2013/06/26 21:41:03, piman wrote:
> nit: why the need for flush here?
> The insertSyncPoint should have flushed, and whatever happens afterwards
should
> have no effect on the signalSyncPoint behavior...

But it does. Without the flush, this test does not finish when running on an
in-process context.

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_ip...
File content/browser/gpu/gpu_ipc_gputests.cc (right):

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_ip...
content/browser/gpu/gpu_ipc_gputests.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/06/26 21:41:03, piman wrote:
> Nit: this test doesn't do IPCs, the file name seems wrong. How about:
> gpu_in_process_context_tests.cc?
> 
> Also, I think this file can move to content/common/gpu/client, along with
> gpu_content_tests.h

Done.

[piman, 2013-06-27 20:51:33]

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
File content/browser/gpu/gpu_context_tests.h (right):

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
content/browser/gpu/gpu_context_tests.h:29: context_->flush();
On 2013/06/27 20:42:40, hubbe wrote:
> On 2013/06/26 21:41:03, piman wrote:
> > nit: why the need for flush here?
> > The insertSyncPoint should have flushed, and whatever happens afterwards
> should
> > have no effect on the signalSyncPoint behavior...
> 
> But it does. Without the flush, this test does not finish when running on an
> in-process context.

It sounds like the missing flush would be in the in-process context then. An
explicit flush should never be required for completion.

Still, I don't understand why it's needed.
WebGraphicsContext3DInProcessCommandBufferImpl::signalSyncPoint does a flush
(albeit a shallow one, which means we don't issue a glFlush, but that's fully
orthogonal to sync points).

[hubbe, 2013-06-27 22:52:10]

I think the tests work now.
(Waiting for trybots to confirm.)

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
File content/browser/gpu/gpu_context_tests.h (right):

https://codereview.chromium.org/17904008/diff/5001/content/browser/gpu/gpu_co...
content/browser/gpu/gpu_context_tests.h:29: context_->flush();
On 2013/06/27 20:51:34, piman wrote:
> On 2013/06/27 20:42:40, hubbe wrote:
> > On 2013/06/26 21:41:03, piman wrote:
> > > nit: why the need for flush here?
> > > The insertSyncPoint should have flushed, and whatever happens afterwards
> > should
> > > have no effect on the signalSyncPoint behavior...
> > 
> > But it does. Without the flush, this test does not finish when running on an
> > in-process context.
> 
> It sounds like the missing flush would be in the in-process context then. An
> explicit flush should never be required for completion.
> 
> Still, I don't understand why it's needed.
> WebGraphicsContext3DInProcessCommandBufferImpl::signalSyncPoint does a flush
> (albeit a shallow one, which means we don't issue a glFlush, but that's fully
> orthogonal to sync points).

Fix implemented as per discussion.

[piman, 2013-06-27 22:54:40]

LGTM, thanks!

[commit-bot: I haz the power, 2013-07-01 07:47:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/17904008/46001

[commit-bot: I haz the power, 2013-07-01 11:01:50]

Change committed as 209404