content: Cleanup GpuMemoryBuffer allocation.

content: Cleanup GpuMemoryBuffer allocation.

This moves implementation of GpuMemoryBuffer allocation
to each specific gpu_memory_buffer_impl_|TYPE| file and
the platform specific decision about what implementation
to use into gpu_memory_buffer_impl_|PLATFORM| files.

Android and MacOSX code is currently tied to the browser
and can't be moved to each specific GpuMemoryBuffer
implementation. This will be fixed by follow up changes.

BUG=

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

[piman, 2014-05-02 22:26:51]

https://codereview.chromium.org/263553009/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/263553009/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:31:
static_cast<uint64>(size.width()) * BytesPerPixel(internalformat);
use base::CheckedNumeric here and below

https://codereview.chromium.org/263553009/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:44:
ChildProcessHostImpl::AllocateSharedMemory(
This /looks/ wrong, because this class is in a directory that's on the client
side, and we're calling a host side function. I guess because
ChildProcessHostImpl is in content/common/ this doesn't cause deps issues?

In the future I was hoping to move some of the content/common/gpu/client classes
out of content/ and into gpu/ipc or something - so that I can use them from
ppapi/proxy, that also runs in NaCl and can't depend on content/.

It's kinda unfortunate to have both the host and client side in the same file,
because that prevents us from doing it (the host side will never work in nacl).
Do you have an idea how we should do that?

[reveman, 2014-05-03 00:59:48]

PTAL

https://codereview.chromium.org/263553009/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/263553009/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:31:
static_cast<uint64>(size.width()) * BytesPerPixel(internalformat);
On 2014/05/02 22:26:51, piman wrote:
> use base::CheckedNumeric here and below

Done. And moved some of these checks to where I think they really belong.

https://codereview.chromium.org/263553009/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:44:
ChildProcessHostImpl::AllocateSharedMemory(
On 2014/05/02 22:26:51, piman wrote:
> This /looks/ wrong, because this class is in a directory that's on the client
> side, and we're calling a host side function. I guess because
> ChildProcessHostImpl is in content/common/ this doesn't cause deps issues?

Yes, we shouldn't be using ChildProcessHostImpl here. I removed it from latest
patch.

> 
> In the future I was hoping to move some of the content/common/gpu/client
classes
> out of content/ and into gpu/ipc or something - so that I can use them from
> ppapi/proxy, that also runs in NaCl and can't depend on content/.
> 
> It's kinda unfortunate to have both the host and client side in the same file,
> because that prevents us from doing it (the host side will never work in
nacl).
> Do you have an idea how we should do that?

Makes sense. I'll probably add some host interface to support GPU process side
allocation. I'm not sure exactly what that will look like but I'll make sure all
dependencies on content/ are properly separated from common/client code.

[piman, 2014-05-05 19:23:50]

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:23:
base::CheckedNumeric<size_t> buffer_size = size.GetArea();
Actually, GetArea can overflow too (int*int -> int).

[danakj, 2014-05-05 19:27:24]

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:23:
base::CheckedNumeric<size_t> buffer_size = size.GetArea();
On 2014/05/05 19:23:51, piman wrote:
> Actually, GetArea can overflow too (int*int -> int).

We have some various helpers here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/share...

[reveman, 2014-05-05 20:41:23]

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:23:
base::CheckedNumeric<size_t> buffer_size = size.GetArea();
On 2014/05/05 19:23:51, piman wrote:
> Actually, GetArea can overflow too (int*int -> int).

I made the caller responsible for passing a valid gfx::Size (where GetArea
doesn't overflow) to GpuMemoryBufferImpl::AllocateForChildProcess. See line 1265
in render_message_filter.cc. I think this more appropriate than having to check
it here.

[piman, 2014-05-05 21:02:14]

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:23:
base::CheckedNumeric<size_t> buffer_size = size.GetArea();
On 2014/05/05 20:41:23, reveman wrote:
> On 2014/05/05 19:23:51, piman wrote:
> > Actually, GetArea can overflow too (int*int -> int).
> 
> I made the caller responsible for passing a valid gfx::Size (where GetArea
> doesn't overflow) to GpuMemoryBufferImpl::AllocateForChildProcess. See line
1265
> in render_message_filter.cc. I think this more appropriate than having to
check
> it here.

It's not obvious that it's the only caller (now, future...). Is there a way to
add a DCHECK to make it an explicit precondition?

[reveman, 2014-05-06 12:26:26]

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_shm.cc (right):

https://codereview.chromium.org/263553009/diff/70001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_shm.cc:23:
base::CheckedNumeric<size_t> buffer_size = size.GetArea();
On 2014/05/05 21:02:16, piman wrote:
> On 2014/05/05 20:41:23, reveman wrote:
> > On 2014/05/05 19:23:51, piman wrote:
> > > Actually, GetArea can overflow too (int*int -> int).
> > 
> > I made the caller responsible for passing a valid gfx::Size (where GetArea
> > doesn't overflow) to GpuMemoryBufferImpl::AllocateForChildProcess. See line
> 1265
> > in render_message_filter.cc. I think this more appropriate than having to
> check
> > it here.
> 
> It's not obvious that it's the only caller (now, future...). Is there a way to
> add a DCHECK to make it an explicit precondition?

Added a IsConfigurationSupported() function to latest patch and I'm now
verifying and failing properly in both AllocateForChildProcess() and
Initialize() instead of making the assumption that only one might be used in the
browser. IsConfigurationSupported doesn't make the assumption that GetArea will
not overflow.

[piman, 2014-05-06 18:04:42]

lgtm

[reveman, 2014-05-08 15:07:10]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-05-08 15:09:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/263553009/150001

[commit-bot: I haz the power, 2014-05-08 19:26:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[reveman, 2014-05-08 20:22:30]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-05-08 20:29:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/263553009/170001

[commit-bot: I haz the power, 2014-05-09 16:29:17]

Change committed as 269327