android_webview: changes to support Android GraphicBuffers

cc/resources/graphic_buffer.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CC_RESOURCES_GRAPHIC_BUFFER_H_
+#define CC_RESOURCES_GRAPHIC_BUFFER_H_
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+
+namespace gfx {
+class Size;
+}
+
+namespace cc {
+
+// Interface for creating and accessing a zero-copy buffer such as GraphicBuffer
+// of Android framework.
+//
+// THREADING CONSIDERATIONS:
+//  
+// It is legal for several different threads to lock a buffer from 
+// read access, none of the threads are blocked.
+// However, locking a buffer simultaneously for write or read/write is
+// undefined, but:
+// - shall not result in termination of the process
+// - shall not block the caller
+// It is acceptable to return an error or to leave the buffer's content
+// into an indeterminate state.
+class GraphicBuffer {
+ public:
+  // The factory is set by the ContentRendererClient::GetGraphicBufferFactory()
+  // method.
+  typedef base::Callback<scoped_ptr<cc::GraphicBuffer>(const gfx::Size&)>

[jamesr, 2013/04/03 03:14:38]

gfx::Size should be passed by value, not const ref. That might make the line
wrapping easier

[kaanb, 2013/04/03 04:39:53]

Done.

+      Factory;

[jamesr, 2013/04/03 03:14:38]

call this Create, not Factory

[kaanb, 2013/04/03 04:39:53]

Done.

+  virtual ~GraphicBuffer() {}
+
+  // Locks the buffer for the usage specified so the client

[jamesr, 2013/04/03 03:14:38]

if these are analogous to map/unmap in other parts of the code, why aren't they
just called map/unmap?

[kaanb, 2013/04/03 04:39:53]

Done.

+  // can read or write the bitmap data in |*vaddr| subsequently.
+  // This call may block, for instance if the h/w needs
+  // to finish rendering or if CPU caches need to be synchronized.
+  // Returns true on success, false on failure.

[jamesr, 2013/04/03 03:14:38]

returning a bool on this API is useless unless the user of this API can respond
in a useful way to an error.  If the only useful thing a caller can do is
DCHECK, you should that check further up the stack.

[kaanb, 2013/04/03 04:39:53]

Done.

+  virtual bool LockForRead(void** vaddr) = 0;

[jamesr, 2013/04/03 03:14:38]

LockForRead() isn't used at all in the big patch and I can't imagine how it
would be.  Drop it if unless there's a need for this.

[kaanb, 2013/04/03 04:39:53]

Done.

[kaanb, 2013/04/03 04:39:53]

Done.

+  virtual bool LockForWrite(void** vaddr) = 0;
+  // Unlocks the buffer. Returns true on success, false otherwise.

[jamesr, 2013/04/03 03:14:38]

leave a blank line between functions.

how could unlock fail?  What would that even mean?

[kaanb, 2013/04/03 04:39:53]

Done.

+  virtual bool Unlock() = 0;
+  // Returns the size of the buffer

[jamesr, 2013/04/03 03:14:38]

do you need a size getter?  whoever allocates this GraphicsBuffer must know the
size of the buffer created and probably has to store the size anyway, so this
seems redundant.

[kaanb, 2013/04/03 04:39:53]

Done.

+  virtual gfx::Size GetSize() = 0;
+  // Returns the native pointer for the graphic buffer.
+  virtual void* GetNativeBuffer() = 0;
+  // Returns the stride for the graphic buffer.
+  virtual uint32 GetStride() = 0;
+};
+
+}  // namespace cc
+
+#endif  // CC_RESOURCES_GRAPHIC_BUFFER_H_