Refactor CommandBufferProxy -> CommandBufferProxy (interface) + CommandBufferProxyImpl.

Refactor CommandBufferProxy -> CommandBufferProxy (interface) + CommandBufferProxyImpl.
No change in functionality.

PpapiCommandBufferProxy will then be updated to use the CommandBufferProxy interface. 

Ultimately, the goal is to enable the browser plugin to reuse WebGraphicsContext3DCommandBufferImpl, instead of requiring a new WebGraphicsContext3DGuest.

BUG=119855
TEST=manually


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129358

[piman, 2012-03-23 22:52:47]

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy.h (right):

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy.h:25: public
IPC::Channel::Listener,
I think this is only needed on the implementation.

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy.h:26: public
base::SupportsWeakPtr<CommandBufferProxy> {
If you actually need the weak ptr semantics on the interface, better is to add a
pure virtual AsWeakPtr and let the implementations implement it (with their own
SupportsWeakPtr). Otherwise you get multiple implementation inheritance, which
can give you very unexpected results.

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy.h:35: virtual int route_id()
const = 0;
nit: Rename to GetRouteID() because it's not inline anymore.

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:190: int32
last_known_get) {
nit: indentation

https://chromiumcodereview.appspot.com/9835069/diff/1/content/content_common....
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/9835069/diff/1/content/content_common....
content/content_common.gypi:165:
'common/gpu/client/command_buffer_proxy_impl.h',
keep common/gpu/client/command_buffer_proxy.h in so that windows people get it
in their IDE.

[Fady Samuel, 2012-03-23 23:28:21]

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy.h (right):

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy.h:25: public
IPC::Channel::Listener,
On 2012/03/23 22:52:47, piman wrote:
> I think this is only needed on the implementation.

Done.

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy.h:26: public
base::SupportsWeakPtr<CommandBufferProxy> {
On 2012/03/23 22:52:47, piman wrote:
> If you actually need the weak ptr semantics on the interface, better is to add
a
> pure virtual AsWeakPtr and let the implementations implement it (with their
own
> SupportsWeakPtr). Otherwise you get multiple implementation inheritance, which
> can give you very unexpected results.

Done. We don't need the Interface to be a weak pointer...at least not yet, as
far as I can tell. Moved it to the implementation.

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy.h:35: virtual int route_id()
const = 0;
On 2012/03/23 22:52:47, piman wrote:
> nit: Rename to GetRouteID() because it's not inline anymore.

Done.

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://chromiumcodereview.appspot.com/9835069/diff/1/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:190: int32
last_known_get) {
On 2012/03/23 22:52:47, piman wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/9835069/diff/1/content/content_common....
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/9835069/diff/1/content/content_common....
content/content_common.gypi:165:
'common/gpu/client/command_buffer_proxy_impl.h',
On 2012/03/23 22:52:47, piman wrote:
> keep common/gpu/client/command_buffer_proxy.h in so that windows people get it
> in their IDE.

Done.

[piman, 2012-03-27 01:10:03]

you're missing command_buffer_proxy_impl.h from the CL!

https://chromiumcodereview.appspot.com/9835069/diff/5001/content/common/gpu/c...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://chromiumcodereview.appspot.com/9835069/diff/5001/content/common/gpu/c...
content/common/gpu/client/gpu_channel_host.cc:277: if
(proxies_.find(command_buffer->GetRouteID()) != proxies_.end())
nit, while you're here: command_buffer->GetRouteID() -> route_id since there's
no point in getting it twice.
Also, move line 276 above 273 and use route_id there as well.

[Fady Samuel, 2012-03-27 01:38:28]

Ooops thanks for catching the missing file! Made the changes you requested.

http://codereview.chromium.org/9835069/diff/5001/content/common/gpu/client/gp...
File content/common/gpu/client/gpu_channel_host.cc (right):

http://codereview.chromium.org/9835069/diff/5001/content/common/gpu/client/gp...
content/common/gpu/client/gpu_channel_host.cc:277: if
(proxies_.find(command_buffer->GetRouteID()) != proxies_.end())
On 2012/03/27 01:10:03, piman wrote:
> nit, while you're here: command_buffer->GetRouteID() -> route_id since there's
> no point in getting it twice.
> Also, move line 276 above 273 and use route_id there as well.

Done.

[piman, 2012-03-27 01:45:29]

LGTM+nit

http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
File content/common/gpu/client/command_buffer_proxy_impl.h (right):

http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
content/common/gpu/client/command_buffer_proxy_impl.h:40: public
base::SupportsWeakPtr<CommandBufferProxyImpl> {
nit: BTW, looking at the implementation, it looks like you don't need the weak
pointer either! It was probably a leftover from previous refactoring. If it
still compiles without it, I'd take it out.

[Fady Samuel, 2012-03-27 14:48:38]

On 2012/03/27 01:45:29, piman wrote:
> LGTM+nit
> 
>
http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
> File content/common/gpu/client/command_buffer_proxy_impl.h (right):
> 
>
http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
> content/common/gpu/client/command_buffer_proxy_impl.h:40: public
> base::SupportsWeakPtr<CommandBufferProxyImpl> {
> nit: BTW, looking at the implementation, it looks like you don't need the weak
> pointer either! It was probably a leftover from previous refactoring. If it
> still compiles without it, I'd take it out.

GpuChannelHost:262 AddRoute(route_id, command_buffer->AsWeakPtr());

I'm committing without any changes because we need it to be a weak pointer.

[commit-bot: I haz the power, 2012-03-27 14:48:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/9835069/8001

[commit-bot: I haz the power, 2012-03-27 14:49:04]

Presubmit check for 9835069-8001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 1.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
[email protected]!

[Fady Samuel, 2012-03-27 14:51:42]

Adding jam@ as an OWNER of content.

Antoine, I made no changes since the last cl because it turns out we need
CommandBufferProxyImpl to be a weak pointer: please see my reply to your comment

[piman, 2012-03-27 15:46:13]

Ok, LGTM still.

[jam, 2012-03-27 18:15:16]

http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
File content/common/gpu/client/command_buffer_proxy_impl.h (right):

http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
content/common/gpu/client/command_buffer_proxy_impl.h:52: virtual int
GetRouteID() const { return route_id_; }
all the CommandBufferProxy methods should be beside each other, and should say
"CommandBufferProxy implementation:" in a comment before them like we usually do

[Fady Samuel, 2012-03-27 18:31:28]

Hi John,

I've reordered command_buffer_proxy_impl.h as you pointed out. 

Thanks,

Fady

http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
File content/common/gpu/client/command_buffer_proxy_impl.h (right):

http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
content/common/gpu/client/command_buffer_proxy_impl.h:40: public
base::SupportsWeakPtr<CommandBufferProxyImpl> {
On 2012/03/27 01:45:29, piman wrote:
> nit: BTW, looking at the implementation, it looks like you don't need the weak
> pointer either! It was probably a leftover from previous refactoring. If it
> still compiles without it, I'd take it out.

Done.

http://codereview.chromium.org/9835069/diff/8001/content/common/gpu/client/co...
content/common/gpu/client/command_buffer_proxy_impl.h:52: virtual int
GetRouteID() const { return route_id_; }
On 2012/03/27 18:15:16, John Abd-El-Malek wrote:
> all the CommandBufferProxy methods should be beside each other, and should say
> "CommandBufferProxy implementation:" in a comment before them like we usually
do

Done.

[jam, 2012-03-27 18:55:15]

lgtm

[commit-bot: I haz the power, 2012-03-27 18:56:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/9835069/7004

[commit-bot: I haz the power, 2012-03-27 19:29:35]

Try job failure for 9835069-7004 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-03-27 19:44:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/9835069/10004

[commit-bot: I haz the power, 2012-03-27 20:38:31]

Try job failure for 9835069-10004 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-28 00:19:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/9835069/10004

[commit-bot: I haz the power, 2012-03-28 00:41:31]

Try job failure for 9835069-10004 (previous was lost) on win_rel for step
"update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-03-28 02:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/9835069/10004

[commit-bot: I haz the power, 2012-03-28 04:01:57]

Change committed as 129358