Add GenericSharedMemoryId and use w/ GpuMemoryBuffer

Add GenericSharedMemoryId and use w/ GpuMemoryBuffer

Adds a GenericSharedMemoryId type which will be used to track various types of shared memory (SharedBitmap, IOSurface, base::SharedMemory).

Wire up GpuMemoryBufferManager so that GpuMemoryBufferId is just a typedef of this type, setting up for further integration in the future.

Note regarding message changes - We needed a way for all GenericSharedMemoryIds used by a process to be unique. Currently a renderer process sometimes internally allocates shared memory and sometimes asks the browser process to do so. In order to allow for consistent naming, we cause the Renderer process to provide an ID to the browser when asking the browser to allocate on its behalf.

+tsepez for message changes.
+piman for content/gpu changes
+danakj/reveman for other changes.

TBR=sky@chromium.org
BUG=512534
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c9984ebe42daab4cd51fb928c32d7d1f4f14a9c7
Cr-Commit-Position: refs/heads/master@{#343677}

[ericrk, 2015-08-05 23:47:04]

ericrk@chromium.org changed reviewers:
+ danakj@chromium.org, reveman@chromium.org

[ericrk, 2015-08-05 23:47:32]

ericrk@chromium.org changed reviewers:
+ piman@chromium.org

[ericrk, 2015-08-06 00:00:07]

ericrk@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2015-08-06 00:11:09]

Messages LGTM, one comment.

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
File base/memory/generic_shared_memory_id.h (right):

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.h:18: using GenericSharedMemoryId = int;
do we want to use a specific-sized unsigned type like uint32_t?

[reveman, 2015-08-06 04:25:06]

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
File base/memory/generic_shared_memory_id.cc (right):

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.cc:11: 
nit: unnecessary blankline

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.cc:23: 
nit: unnecessary blankline

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
File base/memory/generic_shared_memory_id.h (right):

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.h:18: using GenericSharedMemoryId = int;
On 2015/08/06 at 00:11:08, Tom Sepez wrote:
> do we want to use a specific-sized unsigned type like uint32_t?

+1

https://codereview.chromium.org/1280513002/diff/20001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/20001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:239:
DCHECK(buffers.find(id) == buffers.end());
We need to change this now that the child process controls the ID. This needs to
check if the id already exists and fail in that case. It can't crash as that
allows a child process to crash the browser process.

https://codereview.chromium.org/1280513002/diff/20001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:302: // Create an
additional shared ownership edge with the
Ideally we'd instead add the generic shared memory node on the GPU process side
in the case of GPU process side allocated buffers or ask the
GpuMemoryBufferImplSharedMemory code to create it in case of shared memory.

https://codereview.chromium.org/1280513002/diff/20001/content/common/generic_...
File content/common/generic_shared_memory_id_generator.cc (right):

https://codereview.chromium.org/1280513002/diff/20001/content/common/generic_...
content/common/generic_shared_memory_id_generator.cc:10: 
nit: you can remove this blank line if you like

[ericrk, 2015-08-06 17:45:08]

Patchset #3 (id:40001) has been deleted

[ericrk, 2015-08-06 18:13:25]

Patchset #3 (id:60001) has been deleted

[ericrk, 2015-08-06 18:28:08]

Thanks for the feedback!

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
File base/memory/generic_shared_memory_id.cc (right):

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.cc:11: 
On 2015/08/06 04:25:06, reveman wrote:
> nit: unnecessary blankline

shouldn't have been in memory namespace (that isn't a thing)... was expecting
this to follow folder structure like base::trace_event, but it doesn't

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.cc:23: 
On 2015/08/06 04:25:06, reveman wrote:
> nit: unnecessary blankline

Acknowledged.

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
File base/memory/generic_shared_memory_id.h (right):

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.h:18: using GenericSharedMemoryId = int;
On 2015/08/06 04:25:06, reveman wrote:
> On 2015/08/06 at 00:11:08, Tom Sepez wrote:
> > do we want to use a specific-sized unsigned type like uint32_t?
> 
> +1

The underlying code uses base::StaticAtomicSequenceNumber, which returns an int
(matching existing GpuMemoryBufferId, which was an int itself)... I could switch
to uint32_t, but would need to tweak the generation code. WDYT?

https://codereview.chromium.org/1280513002/diff/20001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/20001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:239:
DCHECK(buffers.find(id) == buffers.end());
On 2015/08/06 04:25:06, reveman wrote:
> We need to change this now that the child process controls the ID. This needs
to
> check if the id already exists and fail in that case. It can't crash as that
> allows a child process to crash the browser process.

Done.

https://codereview.chromium.org/1280513002/diff/20001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:302: // Create an
additional shared ownership edge with the
On 2015/08/06 04:25:06, reveman wrote:
> Ideally we'd instead add the generic shared memory node on the GPU process
side
> in the case of GPU process side allocated buffers or ask the
> GpuMemoryBufferImplSharedMemory code to create it in case of shared memory.

This isn't necessarily ideal, but might be fine as it only applies to
SHARED_MEMORY_BUFFER - could also register each GpuMemoryBufferImplSharedMemory,
but that seems inefficient? Could also just inline code here... let me know what
you think.

https://codereview.chromium.org/1280513002/diff/20001/content/common/generic_...
File content/common/generic_shared_memory_id_generator.cc (right):

https://codereview.chromium.org/1280513002/diff/20001/content/common/generic_...
content/common/generic_shared_memory_id_generator.cc:10: 
On 2015/08/06 04:25:06, reveman wrote:
> nit: you can remove this blank line if you like

Done.

[reveman, 2015-08-06 19:19:36]

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
File base/memory/generic_shared_memory_id.h (right):

https://codereview.chromium.org/1280513002/diff/20001/base/memory/generic_sha...
base/memory/generic_shared_memory_id.h:18: using GenericSharedMemoryId = int;
On 2015/08/06 at 18:28:08, ericrk wrote:
> On 2015/08/06 04:25:06, reveman wrote:
> > On 2015/08/06 at 00:11:08, Tom Sepez wrote:
> > > do we want to use a specific-sized unsigned type like uint32_t?
> > 
> > +1
> 
> The underlying code uses base::StaticAtomicSequenceNumber, which returns an
int (matching existing GpuMemoryBufferId, which was an int itself)... I could
switch to uint32_t, but would need to tweak the generation code. WDYT?

I think it's fine to keep this as 'int' then unless we change
base::StaticAtomicSequenceNumber.

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:239: if
(buffers.find(id) != buffers.end()) {
nit: please save the iterator and use it to set BufferInfo below to avoid an
unnecessary map lookup in release code.

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:312: // this here.
I was thinking that establishing the link is still up to the GMB manager code
here and all the factory does is make sure there's a guid for whatever memory is
really backing the buffer. Does that make sense?

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:314: pmd,
client_tracing_process_id, buffer_id);
Can we move this into a static function as part
GpuMemoryBufferFactorySharedMemory? that way we can easily have the shared
memory factory that is currently only used for testing purposes do the right
thing if ever use it on the gpu process side.

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:470: if
(buffers.find(id) != buffers.end()) {
same here. can we avoid two lookups?

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc (right):

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc:185:
kImportance);
I think the edge and the gpu_memory_buffer_guid should be created by browser GMB
manager.

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h (left):

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h:45: 
nit: keep this blank line

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h (right):

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h:43:
gfx::GpuMemoryBufferId id);
How about OnMemoryDump? And include size+format so the function can produce a
dump with correct size.

[ericrk, 2015-08-06 21:55:15]

Patchset #4 (id:100001) has been deleted

[ericrk, 2015-08-06 22:05:19]

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:239: if
(buffers.find(id) != buffers.end()) {
On 2015/08/06 19:19:36, reveman wrote:
> nit: please save the iterator and use it to set BufferInfo below to avoid an
> unnecessary map lookup in release code.

At this point iterator shouldn't exist (we expect it to be buffers.end()). That
doesn't really have any re-use value?

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:312: // this here.
On 2015/08/06 19:19:36, reveman wrote:
> I was thinking that establishing the link is still up to the GMB manager code
> here and all the factory does is make sure there's a guid for whatever memory
is
> really backing the buffer. Does that make sense?

Using same GUID for both GpuMemoryBuffers and GenericSharedMemory, so edge is no
longer necessary. pulling this.

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:314: pmd,
client_tracing_process_id, buffer_id);
On 2015/08/06 19:19:36, reveman wrote:
> Can we move this into a static function as part
> GpuMemoryBufferFactorySharedMemory? that way we can easily have the shared
> memory factory that is currently only used for testing purposes do the right
> thing if ever use it on the gpu process side.

Pulled this for now, will re-add generic memory dumping in next CL.

https://codereview.chromium.org/1280513002/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:470: if
(buffers.find(id) != buffers.end()) {
On 2015/08/06 19:19:36, reveman wrote:
> same here. can we avoid two lookups?

Same comment as above.

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc (right):

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc:185:
kImportance);
On 2015/08/06 19:19:36, reveman wrote:
> I think the edge and the gpu_memory_buffer_guid should be created by browser
GMB
> manager.

Acknowledged.

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h (right):

https://codereview.chromium.org/1280513002/diff/80001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h:43:
gfx::GpuMemoryBufferId id);
On 2015/08/06 19:19:36, reveman wrote:
> How about OnMemoryDump? And include size+format so the function can produce a
> dump with correct size.

no longer applies.

[piman, 2015-08-06 23:29:12]

https://codereview.chromium.org/1280513002/diff/120001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/120001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:247: buffers[id] =
BufferInfo(size, gfx::SHARED_MEMORY_BUFFER, format, usage, 0);
To avoid the double-search, what you can do is:
auto result = buffers.insert(std::make_pair(id, BufferInfo(...)));
if (!result.second) {
  ....
  return;
}

(insert fails to insert existing keys).

https://codereview.chromium.org/1280513002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h (left):

https://codereview.chromium.org/1280513002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h:45: 
nit: blank line should stay....

[ericrk, 2015-08-10 16:58:29]

https://codereview.chromium.org/1280513002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h (left):

https://codereview.chromium.org/1280513002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h:45: 
On 2015/08/06 23:29:12, piman (Very slow to review) wrote:
> nit: blank line should stay....

Done.

[reveman, 2015-08-10 18:57:32]

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (left):

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:39: GpuMemoryBufferId buffer_id);
Maybe keep this for single process mode where gpu memory buffer implementations
might not be backed by some type of shared memory. e.g. PBO backed GMBs.

[ericrk, 2015-08-10 21:06:38]

Patchset #5 (id:140001) has been deleted

[ericrk, 2015-08-10 21:12:15]

https://codereview.chromium.org/1280513002/diff/120001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/120001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:247: buffers[id] =
BufferInfo(size, gfx::SHARED_MEMORY_BUFFER, format, usage, 0);
On 2015/08/06 23:29:12, piman (OOO back Aug 17) wrote:
> To avoid the double-search, what you can do is:
> auto result = buffers.insert(std::make_pair(id, BufferInfo(...)));
> if (!result.second) {
>   ....
>   return;
> }
> 
> (insert fails to insert existing keys).

Good call, moved to a helper fun to make this a bit more readable.

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (left):

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:39: GpuMemoryBufferId buffer_id);
On 2015/08/10 18:57:32, reveman (OOO Aug 10 - Aug 11) wrote:
> Maybe keep this for single process mode where gpu memory buffer
implementations
> might not be backed by some type of shared memory. e.g. PBO backed GMBs.

Seems like using GenericSharedMemoryId for these in single process mode is the
most straightforward thing rather than special casing this, espeically as
GpuMemoryBufferId == base::GenericSharedMemoryId? Were you just suggesting just
keeping this function/name as a wrapper?

[reveman, 2015-08-10 22:16:14]

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (left):

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:39: GpuMemoryBufferId buffer_id);
On 2015/08/10 at 21:12:14, ericrk wrote:
> On 2015/08/10 18:57:32, reveman (OOO Aug 10 - Aug 11) wrote:
> > Maybe keep this for single process mode where gpu memory buffer
implementations
> > might not be backed by some type of shared memory. e.g. PBO backed GMBs.
> 
> Seems like using GenericSharedMemoryId for these in single process mode is the
most straightforward thing rather than special casing this, espeically as
GpuMemoryBufferId == base::GenericSharedMemoryId? Were you just suggesting just
keeping this function/name as a wrapper?

Yes, maybe just keeping the function as a wrapper for now as the implementation
might change in the future. While using GenericSharedMemoryId for GMBs not
backed by shared memory is probably fine, using a
"genericsharedmemory-x-process/%" guid might be less appropriate.

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:259: LOG(ERROR) <<
"Client process attempted to allocate a GpuMemoryBuffer with "
nit: "Child process..." to be consistent with function argument names

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:260: "an existing ID.
Failing allocation.";
nit: I think "Failing allocation." is implicit from LOG(ERROR)

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:431: request->format,
request->usage, 0)));
Is this DCHECK safe? Is it not possible for a child process to control this ID? 

Also what happens here in a release build without DCHECKs enabled? Please avoid
code that is critical for correct execution in DCHECKs.

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:641: }
I think "if (!buffers_.insert(make_pair(key, value)).second) { ... }" above
instead of this helper function is fine. If you prefer to have this helper
function then can you move it to anonymous namespace or make it static?

[ericrk, 2015-08-11 00:27:42]

Patchset #7 (id:200001) has been deleted

[ericrk, 2015-08-11 00:41:23]

Patchset #7 (id:220001) has been deleted

[ericrk, 2015-08-11 01:26:57]

Patchset #7 (id:240001) has been deleted

[ericrk, 2015-08-11 01:30:52]

Patchset #7 (id:260001) has been deleted

[ericrk, 2015-08-11 01:33:51]

Patchset #7 (id:280001) has been deleted

[ericrk, 2015-08-11 01:38:00]

Patchset #7 (id:250019) has been deleted

[ericrk, 2015-08-11 01:40:07]

Thanks for the feedback.

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (left):

https://codereview.chromium.org/1280513002/diff/120001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:39: GpuMemoryBufferId buffer_id);
On 2015/08/10 22:16:13, reveman (OOO Aug 10 - Aug 11) wrote:
> On 2015/08/10 at 21:12:14, ericrk wrote:
> > On 2015/08/10 18:57:32, reveman (OOO Aug 10 - Aug 11) wrote:
> > > Maybe keep this for single process mode where gpu memory buffer
> implementations
> > > might not be backed by some type of shared memory. e.g. PBO backed GMBs.
> > 
> > Seems like using GenericSharedMemoryId for these in single process mode is
the
> most straightforward thing rather than special casing this, espeically as
> GpuMemoryBufferId == base::GenericSharedMemoryId? Were you just suggesting
just
> keeping this function/name as a wrapper?
> 
> Yes, maybe just keeping the function as a wrapper for now as the
implementation
> might change in the future. While using GenericSharedMemoryId for GMBs not
> backed by shared memory is probably fine, using a
> "genericsharedmemory-x-process/%" guid might be less appropriate.

sounds good.

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:259: LOG(ERROR) <<
"Client process attempted to allocate a GpuMemoryBuffer with "
On 2015/08/10 22:16:14, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: "Child process..." to be consistent with function argument names

Done.

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:260: "an existing ID.
Failing allocation.";
On 2015/08/10 22:16:14, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: I think "Failing allocation." is implicit from LOG(ERROR)

Done.

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:431: request->format,
request->usage, 0)));
On 2015/08/10 22:16:14, reveman (OOO Aug 10 - Aug 11) wrote:
> Is this DCHECK safe? Is it not possible for a child process to control this
ID? 
> 
> Also what happens here in a release build without DCHECKs enabled? Please
avoid
> code that is critical for correct execution in DCHECKs.

The ID here is safe, as it's allocated and controlled by this function (see line
405).. that said, yup, this totally shouldn't be in a DCHECK.

https://codereview.chromium.org/1280513002/diff/180001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:641: }
On 2015/08/10 22:16:14, reveman (OOO Aug 10 - Aug 11) wrote:
> I think "if (!buffers_.insert(make_pair(key, value)).second) { ... }" above
> instead of this helper function is fine. If you prefer to have this helper
> function then can you move it to anonymous namespace or make it static?

removed this... probably not worth the function... was on the fence on this one.

[reveman, 2015-08-11 07:28:03]

lgtm

[ericrk, 2015-08-11 17:54:13]

ericrk@chromium.org changed reviewers:
- piman@chromium.org

[ericrk, 2015-08-11 18:00:26]

ericrk@chromium.org changed reviewers:
+ sievers@chromium.org

[ericrk, 2015-08-11 18:00:27]

+sievers for content/ code. If there's someone more appropriate to review,
please let me know - I usually go to piman, but he's OOO.

danakj - can you take a look at the base/ stuff? I think you have owners there.

Thanks!

[reveman, 2015-08-11 18:15:03]

reveman@chromium.org changed reviewers:
+ avi@chromium.org

[reveman, 2015-08-11 18:15:03]

+avi for content/

[Avi (use Gerrit), 2015-08-11 21:10:25]

lgtm

https://codereview.chromium.org/1280513002/diff/310001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1280513002/diff/310001/content/content_common...
content/content_common.gypi:279: 'common/generic_shared_memory_id_generator.h',
alphabetic

[no sievers, 2015-08-11 21:34:57]

On 2015/08/11 18:00:27, ericrk wrote:
> +sievers for content/ code. If there's someone more appropriate to review,
> please let me know - I usually go to piman, but he's OOO.
> 

content lgtm with avi's nit addressed

[ericrk, 2015-08-11 21:36:36]

thanks!

https://codereview.chromium.org/1280513002/diff/310001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1280513002/diff/310001/content/content_common...
content/content_common.gypi:279: 'common/generic_shared_memory_id_generator.h',
On 2015/08/11 21:10:24, Avi wrote:
> alphabetic

Done.

[ericrk, 2015-08-12 23:29:19]

Patchset #9 (id:350001) has been deleted

[ericrk, 2015-08-12 23:43:10]

Patchset #9 (id:370001) has been deleted

[ericrk, 2015-08-12 23:46:35]

Incorporates feedback from danakj:

- base::GenericSharedMemoryId > gfx::GenericSharedMemoryTracingId

- gfx::GenericSharedMemoryTracingId is a class to prevent accidental/incorrect
conversion to/from int.

[danakj, 2015-08-12 23:49:02]

Going to review the ui/gfx stuff but from earlier patch here's a few comments
mostly style.

https://codereview.chromium.org/1280513002/diff/310001/base/memory/generic_sh...
File base/memory/generic_shared_memory_id.h (right):

https://codereview.chromium.org/1280513002/diff/310001/base/memory/generic_sh...
base/memory/generic_shared_memory_id.h:13: // Defines an ID type which is used
accross all types of shared memory
across

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:257: if
(!buffers.insert(
is is possible to split this up into multiple lines so this code formatting
isn't so intense?

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:261: LOG(ERROR) <<
"Child process attempted to allocate a GpuMemoryBuffer with "
Can we use DLOG or VLOG? (prefer former so we don't ship it)

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:430: bool
insert_succeeded =
is is possible to split this up into multiple lines so this code formatting
isn't so intense?

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:513: if
(!buffers.insert(std::make_pair(id, BufferInfo(size, gfx::EMPTY_BUFFER,
is is possible to split this up into multiple lines so this code formatting
isn't so intense?

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:516: LOG(ERROR) <<
"Child process attempted to allocate a GpuMemoryBuffer with "
DLOG? VLOD?

https://codereview.chromium.org/1280513002/diff/310001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.cc (right):

https://codereview.chromium.org/1280513002/diff/310001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.cc:13: // GetGenericSahredMemoryGUIDForTracing, we may
want to special case this if
Shared

[danakj, 2015-08-13 00:05:11]

ui/gfx LGTM

https://codereview.chromium.org/1280513002/diff/390001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/390001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:28: bool operator==(const
GenericSharedMemoryTracingId& other) const {
Can you make an Equals method instead? Style guide says don't make operators if
you don't have to.

[ericrk, 2015-08-13 00:13:55]

ericrk@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[ericrk, 2015-08-13 00:13:56]

Thanks for the feedback!

+dalecurtis for media/renderers/mock_gpu_video_accelerator_factories.cc - should
be a pretty non-interesting change to a mock class.

https://codereview.chromium.org/1280513002/diff/310001/base/memory/generic_sh...
File base/memory/generic_shared_memory_id.h (right):

https://codereview.chromium.org/1280513002/diff/310001/base/memory/generic_sh...
base/memory/generic_shared_memory_id.h:13: // Defines an ID type which is used
accross all types of shared memory
On 2015/08/12 23:49:01, danakj wrote:
> across

Done.

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:257: if
(!buffers.insert(
On 2015/08/12 23:49:02, danakj wrote:
> is is possible to split this up into multiple lines so this code formatting
> isn't so intense?

Done.

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:261: LOG(ERROR) <<
"Child process attempted to allocate a GpuMemoryBuffer with "
On 2015/08/12 23:49:02, danakj wrote:
> Can we use DLOG or VLOG? (prefer former so we don't ship it)

Done.

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:430: bool
insert_succeeded =
On 2015/08/12 23:49:01, danakj wrote:
> is is possible to split this up into multiple lines so this code formatting
> isn't so intense?

I think that after removing the .second things look a lot better. Let me know if
you think this should be broken up further.

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:513: if
(!buffers.insert(std::make_pair(id, BufferInfo(size, gfx::EMPTY_BUFFER,
On 2015/08/12 23:49:01, danakj wrote:
> is is possible to split this up into multiple lines so this code formatting
> isn't so intense?

Done.

https://codereview.chromium.org/1280513002/diff/310001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:516: LOG(ERROR) <<
"Child process attempted to allocate a GpuMemoryBuffer with "
On 2015/08/12 23:49:01, danakj wrote:
> DLOG? VLOD?

Done.

https://codereview.chromium.org/1280513002/diff/310001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.cc (right):

https://codereview.chromium.org/1280513002/diff/310001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.cc:13: // GetGenericSahredMemoryGUIDForTracing, we may
want to special case this if
On 2015/08/12 23:49:02, danakj wrote:
> Shared

Done.

https://codereview.chromium.org/1280513002/diff/390001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/390001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:28: bool operator==(const
GenericSharedMemoryTracingId& other) const {
On 2015/08/13 00:05:11, danakj wrote:
> Can you make an Equals method instead? Style guide says don't make operators
if
> you don't have to.

Need this for hash_map... I could also specialize std::EqualsTo for this... not
sure if that's better?

[danakj, 2015-08-13 00:25:06]

Thanks

https://codereview.chromium.org/1280513002/diff/390001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/390001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:28: bool operator==(const
GenericSharedMemoryTracingId& other) const {
On 2015/08/13 00:13:56, ericrk wrote:
> On 2015/08/13 00:05:11, danakj wrote:
> > Can you make an Equals method instead? Style guide says don't make operators
> if
> > you don't have to.
> 
> Need this for hash_map... I could also specialize std::EqualsTo for this...
not
> sure if that's better?

Oh.. okay.

https://codereview.chromium.org/1280513002/diff/410001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/410001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:257: auto insert_result
= buffers.insert(std::make_pair(
Looks good.

[DaleCurtis, 2015-08-13 00:28:05]

media/ lgtm

[ericrk, 2015-08-13 00:35:24]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-13 00:35:24]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
avi@chromium.org, reveman@chromium.org, sievers@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1280513002/#ps410001
(title: "Feedback")

[commit-bot: I haz the power, 2015-08-13 00:35:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280513002/410001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280513002/410001

[commit-bot: I haz the power, 2015-08-13 00:46:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 00:46:39]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[ericrk, 2015-08-13 03:34:40]

Patchset #11 (id:430001) has been deleted

[reveman, 2015-08-13 08:24:44]

https://codereview.chromium.org/1280513002/diff/410001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1280513002/diff/410001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:513: DLOG(ERROR) <<
"Child process attempted to allocate a GpuMemoryBuffer with "
If using DLOG here then please switch to DLOG for similar errors in
::DestroyGpuMemoryBufferOnIO.

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:11: namespace gfx {
This is OK but FYI, I was hoping to use this for discardable shared memory but
that's not really appropriate now that this lives in ui/gfx/. I guess I'll just
move it to base/ as part of changing discardable.

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:16: class GFX_EXPORT
GenericSharedMemoryTracingId {
What are some examples of usage that we're trying to protect against by making
this a class? How is it different from tracing_process_id, child_process_id,
client_id, discardable_memory_id, etc. that are just simple typedefs?

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:28: using GpuMemoryBufferId =
gfx::GenericSharedMemoryTracingId;
I'm not a fan of using something called a "tracing" Id for something non-tracing
related. GpuMemoryBufferId is used to identify GMBs. We need it independent of
tracing.

[danakj, 2015-08-13 17:29:50]

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:11: namespace gfx {
On 2015/08/13 08:24:44, reveman wrote:
> This is OK but FYI, I was hoping to use this for discardable shared memory but
> that's not really appropriate now that this lives in ui/gfx/. I guess I'll
just
> move it to base/ as part of changing discardable.

Yep. We can move it when we need it.

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:16: class GFX_EXPORT
GenericSharedMemoryTracingId {
On 2015/08/13 08:24:44, reveman wrote:
> What are some examples of usage that we're trying to protect against by making
> this a class? How is it different from tracing_process_id, child_process_id,
> client_id, discardable_memory_id, etc. that are just simple typedefs?

This was my idea. If we're going to use a typedef we're trying to name the
thing, but given these are being passed around amongst many different systems,
it seems easy to mix up ids/ints somewhere and pass the wrong thing. And you
wouldn't really know cuz it would just show different data in tracing. This
gives us some nice type safety.

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:28: using GpuMemoryBufferId =
gfx::GenericSharedMemoryTracingId;
On 2015/08/13 08:24:44, reveman wrote:
> I'm not a fan of using something called a "tracing" Id for something
non-tracing
> related. GpuMemoryBufferId is used to identify GMBs. We need it independent of
> tracing.

It is a tracing id though. For GpuMemoryBuffers we happen to use that tracing id
as a main id. In other places we don't. I thought this captures it really well?

[reveman, 2015-08-13 20:12:23]

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:16: class GFX_EXPORT
GenericSharedMemoryTracingId {
On 2015/08/13 at 17:29:50, danakj wrote:
> On 2015/08/13 08:24:44, reveman wrote:
> > What are some examples of usage that we're trying to protect against by
making
> > this a class? How is it different from tracing_process_id, child_process_id,
> > client_id, discardable_memory_id, etc. that are just simple typedefs?
> 
> This was my idea. If we're going to use a typedef we're trying to name the
thing, but given these are being passed around amongst many different systems,
it seems easy to mix up ids/ints somewhere and pass the wrong thing. And you
wouldn't really know cuz it would just show different data in tracing. This
gives us some nice type safety.

We would see a difference. We'd not render anything and most likely crash if GMB
ids are messed up. Tracing will be the least of our concerns if the ids are not
correct. While some type safety is nice and I'm not against that, I'm still
struggling to see the difference and the need here compared to other ids. If
this is now what we prefer for all type of ids then I'd understand. I guess my
concern is that people will have trouble understanding why this is different
than other ids. An example where this class helps us would be useful for me to
understand.

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:28: using GpuMemoryBufferId =
gfx::GenericSharedMemoryTracingId;
On 2015/08/13 at 17:29:50, danakj wrote:
> On 2015/08/13 08:24:44, reveman wrote:
> > I'm not a fan of using something called a "tracing" Id for something
non-tracing
> > related. GpuMemoryBufferId is used to identify GMBs. We need it independent
of
> > tracing.
> 
> It is a tracing id though. For GpuMemoryBuffers we happen to use that tracing
id as a main id. In other places we don't. I thought this captures it really
well?

I don't know what it means to be a tracing id. This is the main id in all cases
I'm currently aware that we intend to use it. We're just interested in making
the use of ids for various types of shared memory consistent and allocated in a
way that allows them to optionally be used for tracing. I think we should be
able to use it for some shared memory without having to add tracing support for
the memory.

By naming this a "tracing" id, we're making all the systems -including the
GpuMemoryBuffer framework- that use it as its main id depend on tracing
implicitly. I think that's incorrect and confusing.

[danakj, 2015-08-13 20:30:37]

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:28: using GpuMemoryBufferId =
gfx::GenericSharedMemoryTracingId;
On 2015/08/13 20:12:23, reveman wrote:
> On 2015/08/13 at 17:29:50, danakj wrote:
> > On 2015/08/13 08:24:44, reveman wrote:
> > > I'm not a fan of using something called a "tracing" Id for something
> non-tracing
> > > related. GpuMemoryBufferId is used to identify GMBs. We need it
independent
> of
> > > tracing.
> > 
> > It is a tracing id though. For GpuMemoryBuffers we happen to use that
tracing
> id as a main id. In other places we don't. I thought this captures it really
> well?
> 
> I don't know what it means to be a tracing id. This is the main id in all
cases
> I'm currently aware that we intend to use it. We're just interested in making
> the use of ids for various types of shared memory consistent and allocated in
a
> way that allows them to optionally be used for tracing. I think we should be
> able to use it for some shared memory without having to add tracing support
for
> the memory.
> 
> By naming this a "tracing" id, we're making all the systems -including the
> GpuMemoryBuffer framework- that use it as its main id depend on tracing
> implicitly. I think that's incorrect and confusing.

My understanding is that there are shared memory types that will have this id
attached to them for no reason other than for tracing. And that GpuMemoryBuffer
just happens to use this for other things too, as it's main id.

[reveman, 2015-08-13 20:52:23]

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:28: using GpuMemoryBufferId =
gfx::GenericSharedMemoryTracingId;
On 2015/08/13 at 20:30:37, danakj wrote:
> On 2015/08/13 20:12:23, reveman wrote:
> > On 2015/08/13 at 17:29:50, danakj wrote:
> > > On 2015/08/13 08:24:44, reveman wrote:
> > > > I'm not a fan of using something called a "tracing" Id for something
> > non-tracing
> > > > related. GpuMemoryBufferId is used to identify GMBs. We need it
independent
> > of
> > > > tracing.
> > > 
> > > It is a tracing id though. For GpuMemoryBuffers we happen to use that
tracing
> > id as a main id. In other places we don't. I thought this captures it really
> > well?
> > 
> > I don't know what it means to be a tracing id. This is the main id in all
cases
> > I'm currently aware that we intend to use it. We're just interested in
making
> > the use of ids for various types of shared memory consistent and allocated
in a
> > way that allows them to optionally be used for tracing. I think we should be
> > able to use it for some shared memory without having to add tracing support
for
> > the memory.
> > 
> > By naming this a "tracing" id, we're making all the systems -including the
> > GpuMemoryBuffer framework- that use it as its main id depend on tracing
> > implicitly. I think that's incorrect and confusing.
> 
> My understanding is that there are shared memory types that will have this id
attached to them for no reason other than for tracing. And that GpuMemoryBuffer
just happens to use this for other things too, as it's main id.

GMBs, DiscardableMemory, IOSurfaces, SurfaceTextures, etc. will use this as a
real id that is required for these systems to work. Maybe we'll use it for
something that is currently not tracked by ids just to add tracing support in
the future but we can always "using XResourceTracingId =
gfx::GenericSharedMemoryId" if we want to make it explicit that it's only used
for tracing in those cases.

[danakj, 2015-08-13 21:20:26]

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:16: class GFX_EXPORT
GenericSharedMemoryTracingId {
On 2015/08/13 20:12:23, reveman wrote:
> On 2015/08/13 at 17:29:50, danakj wrote:
> > On 2015/08/13 08:24:44, reveman wrote:
> > > What are some examples of usage that we're trying to protect against by
> making
> > > this a class? How is it different from tracing_process_id,
child_process_id,
> > > client_id, discardable_memory_id, etc. that are just simple typedefs?
> > 
> > This was my idea. If we're going to use a typedef we're trying to name the
> thing, but given these are being passed around amongst many different systems,
> it seems easy to mix up ids/ints somewhere and pass the wrong thing. And you
> wouldn't really know cuz it would just show different data in tracing. This
> gives us some nice type safety.
> 
> We would see a difference. We'd not render anything and most likely crash if
GMB
> ids are messed up. Tracing will be the least of our concerns if the ids are
not
> correct. While some type safety is nice and I'm not against that, I'm still
> struggling to see the difference and the need here compared to other ids. If
> this is now what we prefer for all type of ids then I'd understand. I guess my
> concern is that people will have trouble understanding why this is different
> than other ids. An example where this class helps us would be useful for me to
> understand.

Most ids are a part of a single small system. This was proposed as a very
general thing used all over the place. So my level of caution went up. If you're
going to put something in a very general place like base (or even ui/gfx),
people will (mis)use it. The safer the better.

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.h:28: using GpuMemoryBufferId =
gfx::GenericSharedMemoryTracingId;
On 2015/08/13 20:52:23, reveman wrote:
> On 2015/08/13 at 20:30:37, danakj wrote:
> > On 2015/08/13 20:12:23, reveman wrote:
> > > On 2015/08/13 at 17:29:50, danakj wrote:
> > > > On 2015/08/13 08:24:44, reveman wrote:
> > > > > I'm not a fan of using something called a "tracing" Id for something
> > > non-tracing
> > > > > related. GpuMemoryBufferId is used to identify GMBs. We need it
> independent
> > > of
> > > > > tracing.
> > > > 
> > > > It is a tracing id though. For GpuMemoryBuffers we happen to use that
> tracing
> > > id as a main id. In other places we don't. I thought this captures it
really
> > > well?
> > > 
> > > I don't know what it means to be a tracing id. This is the main id in all
> cases
> > > I'm currently aware that we intend to use it. We're just interested in
> making
> > > the use of ids for various types of shared memory consistent and allocated
> in a
> > > way that allows them to optionally be used for tracing. I think we should
be
> > > able to use it for some shared memory without having to add tracing
support
> for
> > > the memory.
> > > 
> > > By naming this a "tracing" id, we're making all the systems -including the
> > > GpuMemoryBuffer framework- that use it as its main id depend on tracing
> > > implicitly. I think that's incorrect and confusing.
> > 
> > My understanding is that there are shared memory types that will have this
id
> attached to them for no reason other than for tracing. And that
GpuMemoryBuffer
> just happens to use this for other things too, as it's main id.
> 
> GMBs, DiscardableMemory, IOSurfaces, SurfaceTextures, etc. will use this as a
> real id that is required for these systems to work. Maybe we'll use it for
> something that is currently not tracked by ids just to add tracing support in
> the future but we can always "using XResourceTracingId =
> gfx::GenericSharedMemoryId" if we want to make it explicit that it's only used
> for tracing in those cases.

OK if that's the case then I guess you're right and naming it for Tracing
doesn't make sense.

[ericrk, 2015-08-13 23:21:55]

+cblume - FYI, this is the latest CL for memory infra tracking for GPU resources
- will CC you on others in the future.

[ericrk, 2015-08-14 00:24:18]

On 2015/08/13 21:20:26, danakj wrote:
>
https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
> File ui/gfx/generic_shared_memory_tracing_id.h (right):
> 
>
https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
> ui/gfx/generic_shared_memory_tracing_id.h:16: class GFX_EXPORT
> GenericSharedMemoryTracingId {
> On 2015/08/13 20:12:23, reveman wrote:
> > On 2015/08/13 at 17:29:50, danakj wrote:
> > > On 2015/08/13 08:24:44, reveman wrote:
> > > > What are some examples of usage that we're trying to protect against by
> > making
> > > > this a class? How is it different from tracing_process_id,
> child_process_id,
> > > > client_id, discardable_memory_id, etc. that are just simple typedefs?
> > > 
> > > This was my idea. If we're going to use a typedef we're trying to name the
> > thing, but given these are being passed around amongst many different
systems,
> > it seems easy to mix up ids/ints somewhere and pass the wrong thing. And you
> > wouldn't really know cuz it would just show different data in tracing. This
> > gives us some nice type safety.
> > 
> > We would see a difference. We'd not render anything and most likely crash if
> GMB
> > ids are messed up. Tracing will be the least of our concerns if the ids are
> not
> > correct. While some type safety is nice and I'm not against that, I'm still
> > struggling to see the difference and the need here compared to other ids. If
> > this is now what we prefer for all type of ids then I'd understand. I guess
my
> > concern is that people will have trouble understanding why this is different
> > than other ids. An example where this class helps us would be useful for me
to
> > understand.
> 
> Most ids are a part of a single small system. This was proposed as a very
> general thing used all over the place. So my level of caution went up. If
you're
> going to put something in a very general place like base (or even ui/gfx),
> people will (mis)use it. The safer the better.
> 
>
https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
> File ui/gfx/gpu_memory_buffer.h (right):
> 
>
https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/gpu_memory_buff...
> ui/gfx/gpu_memory_buffer.h:28: using GpuMemoryBufferId =
> gfx::GenericSharedMemoryTracingId;
> On 2015/08/13 20:52:23, reveman wrote:
> > On 2015/08/13 at 20:30:37, danakj wrote:
> > > On 2015/08/13 20:12:23, reveman wrote:
> > > > On 2015/08/13 at 17:29:50, danakj wrote:
> > > > > On 2015/08/13 08:24:44, reveman wrote:
> > > > > > I'm not a fan of using something called a "tracing" Id for something
> > > > non-tracing
> > > > > > related. GpuMemoryBufferId is used to identify GMBs. We need it
> > independent
> > > > of
> > > > > > tracing.
> > > > > 
> > > > > It is a tracing id though. For GpuMemoryBuffers we happen to use that
> > tracing
> > > > id as a main id. In other places we don't. I thought this captures it
> really
> > > > well?
> > > > 
> > > > I don't know what it means to be a tracing id. This is the main id in
all
> > cases
> > > > I'm currently aware that we intend to use it. We're just interested in
> > making
> > > > the use of ids for various types of shared memory consistent and
allocated
> > in a
> > > > way that allows them to optionally be used for tracing. I think we
should
> be
> > > > able to use it for some shared memory without having to add tracing
> support
> > for
> > > > the memory.
> > > > 
> > > > By naming this a "tracing" id, we're making all the systems -including
the
> > > > GpuMemoryBuffer framework- that use it as its main id depend on tracing
> > > > implicitly. I think that's incorrect and confusing.
> > > 
> > > My understanding is that there are shared memory types that will have this
> id
> > attached to them for no reason other than for tracing. And that
> GpuMemoryBuffer
> > just happens to use this for other things too, as it's main id.
> > 
> > GMBs, DiscardableMemory, IOSurfaces, SurfaceTextures, etc. will use this as
a
> > real id that is required for these systems to work. Maybe we'll use it for
> > something that is currently not tracked by ids just to add tracing support
in
> > the future but we can always "using XResourceTracingId =
> > gfx::GenericSharedMemoryId" if we want to make it explicit that it's only
used
> > for tracing in those cases.
> 
> OK if that's the case then I guess you're right and naming it for Tracing
> doesn't make sense.

OK - sorry for the confusion - I was thinking that on GLImages these would just
be used for tracing, inherited from the GpuMemoryBufferID, but looking at the
underlying systems more carefully I guess we also have some tracking IDs on the
various types of shared memory which this will be replacing, so yeah, I guess
it's not tracing really. Will remove that.

As far as struct vs. simple typedef, I don't think it matters hugely - it is
nice that we can't accidentally pass a client_id or something for one of these.
Does add a bit of extra code here and there, but nothing really complicated.
Once we finish refactoring the underlying types to use these IDs most of the
ugliness (id.id) will go away, and we'll only be doing stuff like this for cases
where we want to log the value. I'm tempted to keep the struct.

[reveman, 2015-08-14 12:36:32]

lgtm after removing tracing from the name but you can keep the struct (maybe add
an int operator)

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
File ui/gfx/generic_shared_memory_tracing_id.h (right):

https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
ui/gfx/generic_shared_memory_tracing_id.h:16: class GFX_EXPORT
GenericSharedMemoryTracingId {
On 2015/08/13 at 21:20:26, danakj wrote:
> On 2015/08/13 20:12:23, reveman wrote:
> > On 2015/08/13 at 17:29:50, danakj wrote:
> > > On 2015/08/13 08:24:44, reveman wrote:
> > > > What are some examples of usage that we're trying to protect against by
> > making
> > > > this a class? How is it different from tracing_process_id,
child_process_id,
> > > > client_id, discardable_memory_id, etc. that are just simple typedefs?
> > > 
> > > This was my idea. If we're going to use a typedef we're trying to name the
> > thing, but given these are being passed around amongst many different
systems,
> > it seems easy to mix up ids/ints somewhere and pass the wrong thing. And you
> > wouldn't really know cuz it would just show different data in tracing. This
> > gives us some nice type safety.
> > 
> > We would see a difference. We'd not render anything and most likely crash if
GMB
> > ids are messed up. Tracing will be the least of our concerns if the ids are
not
> > correct. While some type safety is nice and I'm not against that, I'm still
> > struggling to see the difference and the need here compared to other ids. If
> > this is now what we prefer for all type of ids then I'd understand. I guess
my
> > concern is that people will have trouble understanding why this is different
> > than other ids. An example where this class helps us would be useful for me
to
> > understand.
> 
> Most ids are a part of a single small system. This was proposed as a very
general thing used all over the place. So my level of caution went up. If you're
going to put something in a very general place like base (or even ui/gfx),
people will (mis)use it. The safer the better.

Ok, I buy that.

How about we add "operator int() const { return id; }" so we don't have to type
id.id all over the place?

[danakj, 2015-08-14 17:28:37]

On Fri, Aug 14, 2015 at 5:36 AM, <reveman@chromium.org> wrote:

> lgtm after removing tracing from the name but you can keep the struct
> (maybe add
> an int operator)
>
>
>
>
https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
> File ui/gfx/generic_shared_memory_tracing_id.h (right):
>
>
>
https://codereview.chromium.org/1280513002/diff/410001/ui/gfx/generic_shared_...
> ui/gfx/generic_shared_memory_tracing_id.h:16: class GFX_EXPORT
> GenericSharedMemoryTracingId {
> On 2015/08/13 at 21:20:26, danakj wrote:
>
>> On 2015/08/13 20:12:23, reveman wrote:
>> > On 2015/08/13 at 17:29:50, danakj wrote:
>> > > On 2015/08/13 08:24:44, reveman wrote:
>> > > > What are some examples of usage that we're trying to protect
>>
> against by
>
>> > making
>> > > > this a class? How is it different from tracing_process_id,
>>
> child_process_id,
>
>> > > > client_id, discardable_memory_id, etc. that are just simple
>>
> typedefs?
>
>> > >
>> > > This was my idea. If we're going to use a typedef we're trying to
>>
> name the
>
>> > thing, but given these are being passed around amongst many
>>
> different systems,
>
>> > it seems easy to mix up ids/ints somewhere and pass the wrong thing.
>>
> And you
>
>> > wouldn't really know cuz it would just show different data in
>>
> tracing. This
>
>> > gives us some nice type safety.
>> >
>> > We would see a difference. We'd not render anything and most likely
>>
> crash if GMB
>
>> > ids are messed up. Tracing will be the least of our concerns if the
>>
> ids are not
>
>> > correct. While some type safety is nice and I'm not against that,
>>
> I'm still
>
>> > struggling to see the difference and the need here compared to other
>>
> ids. If
>
>> > this is now what we prefer for all type of ids then I'd understand.
>>
> I guess my
>
>> > concern is that people will have trouble understanding why this is
>>
> different
>
>> > than other ids. An example where this class helps us would be useful
>>
> for me to
>
>> > understand.
>>
>
> Most ids are a part of a single small system. This was proposed as a
>>
> very general thing used all over the place. So my level of caution went
> up. If you're going to put something in a very general place like base
> (or even ui/gfx), people will (mis)use it. The safer the better.
>
> Ok, I buy that.
>
> How about we add "operator int() const { return id; }" so we don't have
> to type id.id all over the place?
>

Style guide doesn't want us to use operators generally, except for special
cases. I don't think this should be one of them, esp given the id.id stuff
will just be in tracing code eventually.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ericrk, 2015-08-14 19:51:27]

ericrk@chromium.org changed reviewers:
+ erg@chromium.org

[ericrk, 2015-08-14 19:51:28]

+erg for the changes components/view_manager. Changes are pretty much no-op, so
should be straightforward.

If someone else is better to take a look, let me know.

Thanks!

[ericrk, 2015-08-17 11:00:54]

ericrk@chromium.org changed reviewers:
- erg@chromium.org

[ericrk, 2015-08-17 11:01:37]

ericrk@chromium.org changed reviewers:
+ sky@chromium.org

[ericrk, 2015-08-17 11:24:50]

+sky for the changes components/view_manager. Changes are pretty much no-op, so
should be straightforward.

Thanks!

[ericrk, 2015-08-17 13:13:27]

On 2015/08/17 11:24:50, ericrk wrote:
> +sky for the changes components/view_manager. Changes are pretty much no-op,
so
> should be straightforward.
> 
> Thanks!

Given that the components/view_manager changes are truly trivial, TBR to get
things going. Thanks!

[ericrk, 2015-08-17 13:13:33]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-17 13:13:33]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
danakj@chromium.org, sievers@chromium.org, tsepez@chromium.org,
dalecurtis@chromium.org, reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1280513002/#ps470001
(title: "remove "tracing" from name")

[commit-bot: I haz the power, 2015-08-17 13:13:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280513002/470001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280513002/470001

[commit-bot: I haz the power, 2015-08-17 14:22:44]

Committed patchset #12 (id:470001)

[commit-bot: I haz the power, 2015-08-17 14:23:21]

Patchset 12 (id:??) landed as
https://crrev.com/c9984ebe42daab4cd51fb928c32d7d1f4f14a9c7
Cr-Commit-Position: refs/heads/master@{#343677}

[sky, 2015-08-17 18:01:37]

viewmanager LGTM