Started on Resource class.

Implemented a Resource tracker for Pepper proxy.

BUG=none
TEST=compiles

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=3706

[David Springer, 2010-10-28 23:52:20]

Ok - I see the code now.  But I still don't see any tests?

[sehr (please use chromium), 2010-11-02 16:50:04]

Sorry for the delays.  Some comments.
Btw, please add at least one example that uses AddRef/ReleaseRef RPCs, to show
how the constructor/destructor solution you're providing works.

http://codereview.chromium.org/4210003/diff/11001/12008
File src/shared/ppapi_proxy/plugin_resource.cc (right):

http://codereview.chromium.org/4210003/diff/11001/12008#newcode1
src/shared/ppapi_proxy/plugin_resource.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
ditto on authors, I think.

http://codereview.chromium.org/4210003/diff/11001/12009
File src/shared/ppapi_proxy/plugin_resource.h (right):

http://codereview.chromium.org/4210003/diff/11001/12009#newcode1
src/shared/ppapi_proxy/plugin_resource.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
Native Client Authors, I think.

http://codereview.chromium.org/4210003/diff/11001/12010
File src/shared/ppapi_proxy/plugin_resource_tracker.cc (right):

http://codereview.chromium.org/4210003/diff/11001/12010#newcode43
src/shared/ppapi_proxy/plugin_resource_tracker.cc:43: if (i !=
live_resources_.end()) {
if (!x) { A } else { B } is hard to read.
Use
if (x) { B } else { A }
instead.

http://codereview.chromium.org/4210003/diff/11001/12012
File src/shared/ppapi_proxy/ref_counted.cc (right):

http://codereview.chromium.org/4210003/diff/11001/12012#newcode7
src/shared/ppapi_proxy/ref_counted.cc:7: namespace base {
We don't define namespace base in NaCl, as it causes clashes.  Namespace nacl?

http://codereview.chromium.org/4210003/diff/11001/12013
File src/shared/ppapi_proxy/ref_counted.h (right):

http://codereview.chromium.org/4210003/diff/11001/12013#newcode1
src/shared/ppapi_proxy/ref_counted.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
This file seems more generally useful, and should be in
native_client/src/include/nacl_ref_counted.h.

http://codereview.chromium.org/4210003/diff/11001/12013#newcode3
src/shared/ppapi_proxy/ref_counted.h:3: // found in the LICENSE file.
Wrong include guard, and unnecessary pragma.

[neb, 2010-11-04 00:01:45]

I'm working on 3D which would use this, be a nice example and also a test. I'll
update this CL as I iterate on that use case.

http://codereview.chromium.org/4210003/diff/11001/12008
File src/shared/ppapi_proxy/plugin_resource.cc (right):

http://codereview.chromium.org/4210003/diff/11001/12008#newcode1
src/shared/ppapi_proxy/plugin_resource.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2010/11/02 16:50:04, sehr wrote:
> ditto on authors, I think.

Done.

http://codereview.chromium.org/4210003/diff/11001/12009
File src/shared/ppapi_proxy/plugin_resource.h (right):

http://codereview.chromium.org/4210003/diff/11001/12009#newcode1
src/shared/ppapi_proxy/plugin_resource.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2010/11/02 16:50:04, sehr wrote:
> Native Client Authors, I think.

Done.

http://codereview.chromium.org/4210003/diff/11001/12010
File src/shared/ppapi_proxy/plugin_resource_tracker.cc (right):

http://codereview.chromium.org/4210003/diff/11001/12010#newcode43
src/shared/ppapi_proxy/plugin_resource_tracker.cc:43: if (i !=
live_resources_.end()) {
On 2010/11/02 16:50:04, sehr wrote:
> if (!x) { A } else { B } is hard to read.
> Use
> if (x) { B } else { A }
> instead.

Done.

http://codereview.chromium.org/4210003/diff/11001/12012
File src/shared/ppapi_proxy/ref_counted.cc (right):

http://codereview.chromium.org/4210003/diff/11001/12012#newcode7
src/shared/ppapi_proxy/ref_counted.cc:7: namespace base {
On 2010/11/02 16:50:04, sehr wrote:
> We don't define namespace base in NaCl, as it causes clashes.  Namespace nacl?

Done.

http://codereview.chromium.org/4210003/diff/11001/12013
File src/shared/ppapi_proxy/ref_counted.h (right):

http://codereview.chromium.org/4210003/diff/11001/12013#newcode1
src/shared/ppapi_proxy/ref_counted.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2010/11/02 16:50:04, sehr wrote:
> This file seems more generally useful, and should be in
> native_client/src/include/nacl_ref_counted.h.

Done.

http://codereview.chromium.org/4210003/diff/11001/12013#newcode3
src/shared/ppapi_proxy/ref_counted.h:3: // found in the LICENSE file.
On 2010/11/02 16:50:04, sehr wrote:
> Wrong include guard, and unnecessary pragma.

Done.

[polina, 2010-11-05 01:29:16]

I took a quick look and the new code looks like a copy & paste of Chrome's
src/webkit/glue/plugins/pepper_resource.h/cc and friends.

Is this really what we want? If we are going to do (almost) the same thing as
Chrome, then wouldn't it make sense to somehow reuse their code? Especially
since the proxy is supposed to live within the ppapi world (which is now chrome
world), not native client.

Am I missing something here?

Thanks, 
P.

[sehr (please use chromium), 2010-11-15 16:30:31]

On 2010/11/05 01:29:16, polina wrote:
> I took a quick look and the new code looks like a copy & paste of Chrome's
> src/webkit/glue/plugins/pepper_resource.h/cc and friends.
> 
> Is this really what we want? If we are going to do (almost) the same thing as
> Chrome, then wouldn't it make sense to somehow reuse their code? Especially
> since the proxy is supposed to live within the ppapi world (which is now
chrome
> world), not native client.
> 
> Am I missing something here?
> 
> Thanks, 
> P.

If it's building on the trybots, LGTM.

[neb, 2010-11-16 20:30:17]

Finally I can post replies - for the sake of posterity, yes, this is copied &
pasted Chrome code. Brett's proxy does away wit that by compiling actual Chrome
code, but then we have to port base/. We'll probably do that in the future.