Implemented software output device for Aura.

Implemented software output device for Aura.


BUG=124671, 161008

[piman, 2013-03-27 21:49:28]

It would be nice to have a unit test for this class. I don't think it needs much
infrastructure if at all - and it would be nice to have a test for some of the
problems I evoke below.

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
File content/renderer/gpu/compositor_software_output_device.cc (right):

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:36: last_buffer_(-1),
nit: indent (all initializers should be aligned)

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:53: if (dibs_.empty()
|| dibs_[0]->size() < ViewportSizeInBytes()) {
You may want to reallocate always, not just if you grow. Otherwise you can waste
a lot of memory.

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:54: dibs_.clear();
Is it ok to clear here if the handle may still be in flight to the browser? I.e.
is it possible that we end up closing the Handle/FD before the send over IPC
(which happens on another thread) is done, causing all sorts of bad things?

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:56:
dibs_.push_back(CreateDIB());
Is it needed to pre-allocate the TransportDIBs? It looks like they will be
allocated on demand in BeginPaint.

Also, shouldn't you reset front_buffer_ to 0? What happens if, say, front_buffer
was 2?

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:99:
DCHECK_GE(int(dibs_.size()), kNumBuffers);
nit: Make kNumBuffers a size_t and you don't need a cast (which doesn't follow
the chrome style btw).

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:101:
DCHECK_LT(front_buffer_, int(dibs_.size()));
well, you can make front_buffer_ a size_t too. (or use static_cast)

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:118: // active dibs if
we got a resize event in the mean time.
What if the handle got recycled?

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.cc:124:
DCHECK_LE(num_free_buffers_, int(dibs_.size()));
make num_free_buffers_ a size_t.

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
File content/renderer/gpu/compositor_software_output_device.h (right):

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.h:8: #include
"base/memory/scoped_ptr.h"
I don't think you need this.

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.h:18: // to a fixed
thread when bindToClient is called.
nit: BindToClient

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.h:19: class
CompositorSoftwareOutputDevice
I don't see this class instantiated anywhere yet.

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.h:21:
NON_EXPORTED_BASE(public base::NonThreadSafe) {
If the above comment is true, you should see asserts in the destructor since you
don't DetachFromThread. Also, NonThreadSafe doesn't give you a lot of guarantees
if you don't check CalledOnValidThread()

I think you want to call DetachFromThread in the constructor, and then
DCHECK(CalledOnValidThread()) in each method.

https://codereview.chromium.org/13042022/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_software_output_device.h:38: TransportDIB*
CreateDIB() {
This method is non-trivial, move to the .cc file? There's no advantage to
putting it in the header since no outside class has an opportunity to inline
this.

Without it, you should even be able to remove the ui/surface/transport_dib.h
header, just forward-declare (and only include in the .cc file).