mojo/gpu: use SharedBuffer instead of base::SharedMemory with hacks

mojo/gpu: use SharedBuffer instead of base::SharedMemory with hacks

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

[piman, 2014-04-02 01:47:20]

needs https://codereview.chromium.org/221663004/ and
https://codereview.chromium.org/213353005/ first.

[viettrungluu, 2014-04-02 20:15:59]

LGTM w/comments.

https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
mojo/gles2/command_buffer_client_impl.cc:38: return false;
Note that on failure, you leak the mapping.

Arguably, you should do the duplicate first to avoid that.

Possibly, I should also improve the C++ wrappers to make handling this easier.

(In Chromium code, I guess you could also use a scoped_ptr<> with a suitable
deleter (namely, one that calls UnmapBuffer()).)

[piman, 2014-04-02 20:22:53]

https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
File mojo/gles2/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
mojo/gles2/command_buffer_client_impl.cc:38: return false;
On 2014/04/02 20:15:59, viettrungluu wrote:
> Note that on failure, you leak the mapping.
> 
> Arguably, you should do the duplicate first to avoid that.
> 
> Possibly, I should also improve the C++ wrappers to make handling this easier.
> 
> (In Chromium code, I guess you could also use a scoped_ptr<> with a suitable
> deleter (namely, one that calls UnmapBuffer()).)

Good point. Because the other calls return into Scoped things, the easiest thing
is to do the mapping last.

Re: c++ wrappers, agreed, we may want something to the level of
base::SharedMemory that lets you encapsulate the handle and provide you with
Map/Unmap/Duplicate.
Should mojo::SharedBuffer be that?

[piman, 2014-04-02 23:59:58]

The CQ bit was checked by piman@chromium.org

[piman, 2014-04-03 00:00:49]

On 2014/04/02 20:22:53, piman wrote:
>
https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
> File mojo/gles2/command_buffer_client_impl.cc (right):
> 
>
https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
> mojo/gles2/command_buffer_client_impl.cc:38: return false;
> On 2014/04/02 20:15:59, viettrungluu wrote:
> > Note that on failure, you leak the mapping.
> > 
> > Arguably, you should do the duplicate first to avoid that.
> > 
> > Possibly, I should also improve the C++ wrappers to make handling this
easier.
> > 
> > (In Chromium code, I guess you could also use a scoped_ptr<> with a suitable
> > deleter (namely, one that calls UnmapBuffer()).)
> 
> Good point. Because the other calls return into Scoped things, the easiest
thing
> is to do the mapping last.

Done.

[commit-bot: I haz the power, 2014-04-03 00:02:09]

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

[viettrungluu, 2014-04-03 00:11:39]

On 2014/04/02 20:22:53, piman wrote:
>
https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
> File mojo/gles2/command_buffer_client_impl.cc (right):
> 
>
https://codereview.chromium.org/221453007/diff/1/mojo/gles2/command_buffer_cl...
> mojo/gles2/command_buffer_client_impl.cc:38: return false;
> On 2014/04/02 20:15:59, viettrungluu wrote:
> > Note that on failure, you leak the mapping.
> > 
> > Arguably, you should do the duplicate first to avoid that.
> > 
> > Possibly, I should also improve the C++ wrappers to make handling this
easier.
> > 
> > (In Chromium code, I guess you could also use a scoped_ptr<> with a suitable
> > deleter (namely, one that calls UnmapBuffer()).)
> 
> Good point. Because the other calls return into Scoped things, the easiest
thing
> is to do the mapping last.
> 
> Re: c++ wrappers, agreed, we may want something to the level of
> base::SharedMemory that lets you encapsulate the handle and provide you with
> Map/Unmap/Duplicate.
> Should mojo::SharedBuffer be that?

Let me think about that. (It'd be natural for mojo::SharedBuffer to support
Duplicate[*]. On the other hand, I'm not sure how I feel about it supporting
Map, since probably being like base::SharedMemory would mean that the mapping
would go away on its destruction and it would support only one mapping at a
time.)

[*] Although would Duplicate produce a mojo::SharedBuffer or just the handle?
Hrm.

[commit-bot: I haz the power, 2014-04-03 00:25:57]

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

[commit-bot: I haz the power, 2014-04-03 00:25:57]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[piman, 2014-04-03 00:36:42]

On 2014/04/03 00:25:57, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on mac_chromium_compile_dbg

Dear CQ, If I were you and I failed to apply the patch 'cause I'm out of date, I
wouldn't try to compile it. Up to you though.

[piman, 2014-04-03 00:49:10]

The CQ bit was checked by piman@chromium.org

[piman, 2014-04-03 00:49:34]

Also, blame Trung for asserting things that aren't bools (and myself for not
compiling in debug).

[commit-bot: I haz the power, 2014-04-03 00:53:54]

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

[commit-bot: I haz the power, 2014-04-03 02:44:01]

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

[commit-bot: I haz the power, 2014-04-03 02:44:01]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[piman, 2014-04-03 19:55:25]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-04-03 19:55:30]

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

[commit-bot: I haz the power, 2014-04-03 20:02:22]

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

[commit-bot: I haz the power, 2014-04-03 20:02:23]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[piman, 2014-04-03 20:03:48]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-04-03 20:20:55]

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

[commit-bot: I haz the power, 2014-04-03 23:44:30]

Change committed as 261578