Implement shareResources==true in TestWebGraphicsContext3D

Implement shareResources==true in TestWebGraphicsContext3D

This moves the id namespace (for textures/buffers/images) in a separate struct, that can be shared across instance.
Similarly to the other WGC3D implementations, there is only one global share group, that instances can opt into or be completely segregated (based on whether shareResources==true or not)

BUG=None
R=danakj@chromium.org

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

[piman, 2013-07-15 23:06:50]

This is needed so for a susequent CL, so that TextureLayerClient can create
textures that the compositor context knows about.

[danakj, 2013-07-15 23:15:41]

https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.h:36: attrs.shareResources = true;
this defaults true, right? so won't Create() also be shared?

[piman, 2013-07-15 23:17:55]

On 2013/07/15 23:15:41, danakj wrote:
>
https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
> File cc/test/test_web_graphics_context_3d.h (right):
> 
>
https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
> cc/test/test_web_graphics_context_3d.h:36: attrs.shareResources = true;
> this defaults true, right? so won't Create() also be shared?

Dang, you're right!
Let me revert that part.

[piman, 2013-07-16 00:45:33]

https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.h:36: attrs.shareResources = true;
On 2013/07/15 23:15:41, danakj wrote:
> this defaults true, right? so won't Create() also be shared?

Done.

[piman, 2013-07-16 00:46:26]

On 2013/07/16 00:45:33, piman wrote:
>
https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
> File cc/test/test_web_graphics_context_3d.h (right):
> 
>
https://codereview.chromium.org/18796008/diff/4001/cc/test/test_web_graphics_...
> cc/test/test_web_graphics_context_3d.h:36: attrs.shareResources = true;
> On 2013/07/15 23:15:41, danakj wrote:
> > this defaults true, right? so won't Create() also be shared?
> 
> Done.

(I left the various cleanup I had).

[danakj, 2013-07-16 02:55:50]

Can you update the description to say what this is doing now?

[danakj, 2013-07-16 02:57:02]

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.cc (right):

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:39:
TestWebGraphicsContext3D::shared_namespace_ = NULL;
This will be a leak. Should we refcount and clean it up?

[piman, 2013-07-16 03:06:33]

On 2013/07/16 02:55:50, danakj wrote:
> Can you update the description to say what this is doing now?

Done.

[piman, 2013-07-16 03:06:38]

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.cc (right):

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:39:
TestWebGraphicsContext3D::shared_namespace_ = NULL;
On 2013/07/16 02:57:02, danakj wrote:
> This will be a leak. Should we refcount and clean it up?

It is refcounted, and it cleans itself up :)
See TestWebGraphicsContext3D::Namespace::~Namespace

[danakj, 2013-07-16 03:26:10]

LGTM w nits

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.cc (right):

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:39:
TestWebGraphicsContext3D::shared_namespace_ = NULL;
On 2013/07/16 03:06:39, piman wrote:
> On 2013/07/16 02:57:02, danakj wrote:
> > This will be a leak. Should we refcount and clean it up?
> 
> It is refcounted, and it cleans itself up :)
> See TestWebGraphicsContext3D::Namespace::~Namespace

Oh right, cool.

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:48: if (shared_namespace_ == this)
Do we need a lock for accessing the shared_namespace_ ?

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:73: if (shared_namespace_) {
can you DCHECK(attributes.shareResources) here so we dont break this in the
future somehow? or have this code also be conditional (maybe shared method).

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:77: shared_namespace_ = namespace_;
.get()

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:105: namespace_ = shared_namespace_;
cast from raw pointer to scoped_refptr is kinda icky, but i guess that will
remain valid for the forseeable future?

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:108: shared_namespace_ = namespace_;
.get()

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:522: ScopedPtrHashMap<unsigned, Image>
&images = namespace_->images;
& images

(cpplint fail)

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:550: ScopedPtrHashMap<unsigned, Image>
&images = namespace_->images;
& images

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.h:233: // Protects all fields;
nit: s/;/./

[piman, 2013-07-16 04:50:55]

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.cc (right):

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:48: if (shared_namespace_ == this)
On 2013/07/16 03:26:11, danakj wrote:
> Do we need a lock for accessing the shared_namespace_ ?

Yes, we do! Thanks!

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:73: if (shared_namespace_) {
On 2013/07/16 03:26:11, danakj wrote:
> can you DCHECK(attributes.shareResources) here so we dont break this in the
> future somehow? or have this code also be conditional (maybe shared method).

Created helper function (made conditional).

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:77: shared_namespace_ = namespace_;
On 2013/07/16 03:26:11, danakj wrote:
> .get()

Done.

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:105: namespace_ = shared_namespace_;
On 2013/07/16 03:26:11, danakj wrote:
> cast from raw pointer to scoped_refptr is kinda icky, but i guess that will
> remain valid for the forseeable future?

It's not a cast but an assignment.. What bothers you? It's a somewhat common
pattern (e.g. refcounted field, accessor returns a raw pointer, passed to
another method that takes a pointer, assigned to a field that's a refptr).

In this case, shared_namespace_ is really a weakptr, i.e. it's either NULL or
someone has a ref on it, so I don't know how it could go wrong? refs are
taken/released under the lock.

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:108: shared_namespace_ = namespace_;
On 2013/07/16 03:26:11, danakj wrote:
> .get()

Done.

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:522: ScopedPtrHashMap<unsigned, Image>
&images = namespace_->images;
On 2013/07/16 03:26:11, danakj wrote:
> & images
> 
> (cpplint fail)

Done.

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:550: ScopedPtrHashMap<unsigned, Image>
&images = namespace_->images;
On 2013/07/16 03:26:11, danakj wrote:
> & images

Done.

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.h:233: // Protects all fields;
On 2013/07/16 03:26:11, danakj wrote:
> nit: s/;/./

Done.

[piman, 2013-07-16 04:51:19]

(PTAL for the lock logic)

[danakj, 2013-07-16 05:39:00]

LGTM

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.cc (right):

https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.cc:105: namespace_ = shared_namespace_;
On 2013/07/16 04:50:55, piman wrote:
> On 2013/07/16 03:26:11, danakj wrote:
> > cast from raw pointer to scoped_refptr is kinda icky, but i guess that will
> > remain valid for the forseeable future?
> 
> It's not a cast but an assignment.. What bothers you? It's a somewhat common
> pattern (e.g. refcounted field, accessor returns a raw pointer, passed to
> another method that takes a pointer, assigned to a field that's a refptr).
> 
> In this case, shared_namespace_ is really a weakptr, i.e. it's either NULL or
> someone has a ref on it, so I don't know how it could go wrong? refs are
> taken/released under the lock.

Yeh, I've seen this used in chromium before as well. It seems to me that a raw
pointer should signify no ownership changing abilties, and should be able to be
assumed it is just a raw pointer forever. Like, with scoped_ptr you would never
take a raw pointer and turn it into a scoped_ptr. I feel like things would be
more clear if you couldn't do the same with a refptr either, that going to a raw
pointer would be a one-way change. I guess if this were to change though, it
would require some kind of patterned adjustment across the codebase.

[commit-bot: I haz the power, 2013-07-16 18:04:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/18796008/37006

[piman, 2013-07-16 18:58:28]

On Mon, Jul 15, 2013 at 10:39 PM, <danakj@chromium.org> wrote:

> LGTM
>
>
>
> https://codereview.chromium.**org/18796008/diff/5013/cc/**
>
test/test_web_graphics_**context_3d.cc<https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_context_3d.cc>
> File cc/test/test_web_graphics_**context_3d.cc (right):
>
> https://codereview.chromium.**org/18796008/diff/5013/cc/**
>
test/test_web_graphics_**context_3d.cc#newcode105<https://codereview.chromium.org/18796008/diff/5013/cc/test/test_web_graphics_context_3d.cc#newcode105>
> cc/test/test_web_graphics_**context_3d.cc:105: namespace_ =
> shared_namespace_;
> On 2013/07/16 04:50:55, piman wrote:
>
>> On 2013/07/16 03:26:11, danakj wrote:
>> > cast from raw pointer to scoped_refptr is kinda icky, but i guess
>>
> that will
>
>> > remain valid for the forseeable future?
>>
>
>  It's not a cast but an assignment.. What bothers you? It's a somewhat
>>
> common
>
>> pattern (e.g. refcounted field, accessor returns a raw pointer, passed
>>
> to
>
>> another method that takes a pointer, assigned to a field that's a
>>
> refptr).
>
>  In this case, shared_namespace_ is really a weakptr, i.e. it's either
>>
> NULL or
>
>> someone has a ref on it, so I don't know how it could go wrong? refs
>>
> are
>
>> taken/released under the lock.
>>
>
> Yeh, I've seen this used in chromium before as well. It seems to me that
> a raw pointer should signify no ownership changing abilties, and should
> be able to be assumed it is just a raw pointer forever. Like, with
> scoped_ptr you would never take a raw pointer and turn it into a
> scoped_ptr. I feel like things would be more clear if you couldn't do
> the same with a refptr either, that going to a raw pointer would be a
> one-way change. I guess if this were to change though, it would require
> some kind of patterned adjustment across the codebase.
>

I guess the difference between scoped_ptr and refcounted types is that in
the former case, the type of the smart pointer indicates the ownership
semantics whereas in the second case the type of the instance indicates it.
There's no doubt about it, it's illegal to put a RefCounted object in a
scoped_ptr. A non-RefCounted object though can go in a scoped_ptr, a
linked_ptr, etc. on a per-instance basis. That's how they're different.

There are patterns that you can't simply fix the way you describe it - for
example, 'this' is not a scoped_refptr, so your suggestion would preclude
us from giving it to anything. Also, there are many cases where we *don't*
want to put the pointer in a scoped_refptr, because that would cause cycles
and leaks (this is a case here - if we made shared_namespace_ a
scoped_refptr, it would cause a leak).


>
>
https://codereview.chromium.**org/18796008/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-07-16 20:13:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-16 20:22:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/18796008/37006

[commit-bot: I haz the power, 2013-07-17 00:53:34]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[piman, 2013-07-17 01:03:30]

Committed patchset #7 manually as r211886 (presubmit successful).