WebSocket Pepper API: WebArrayBuffer support

WebSocket Pepper API: WebArrayBuffer support

- support SetBinaryType()
- support WebArrayBuffer and stop supporting WebData
- use Var instead of PP_Var to hold received messages
- Add C++ interfaces to handle binary types

BUG=87310
TEST=ui_test --gtest_filter='PPAPITest.WebSocket_*'


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118258

[Takashi Toyoshima, 2012-01-13 11:32:26]

This CL is for binary handling improvement by using WebArrayBuffer directly
to communicate between Chromium and WebKit.

After this change, I'll also remove WebData supporting from WebKit side's API
implementation.

Thanks

[dmichael (off chromium), 2012-01-13 17:36:14]

http://codereview.chromium.org/9026007/diff/1/webkit/plugins/ppapi/ppb_websoc...
File webkit/plugins/ppapi/ppb_websocket_impl.h (right):

http://codereview.chromium.org/9026007/diff/1/webkit/plugins/ppapi/ppb_websoc...
webkit/plugins/ppapi/ppb_websocket_impl.h:87: std::queue< ::ppapi::Var*>
received_messages_;
Should this be scoped_refptr< ::ppapi::Var>?

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:200:
websocket_->setBinaryType(static_cast<WebSocket::BinaryType>(binary_type_));
It seems like it would be a good idea to either convert explicitly with a switch
statement or to have assertions somewhere (maybe even statically) so you know
the values match up.

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:422:
static_cast<WebSocket::BinaryType>(binary_type_));
Ditto, some better checking would be good here. Recall that SetBinaryType is
called by the plugin, and is free to send us bad values. It's very easy to send
us an enum to a value that's not actually valid.

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:443: received_messages_.push(new
StringVar(string));
So now the vars in your queue have no ID and no ref-count. That might be okay,
but you have to be careful about your use of the Vars and their ref-counts.

I think you probably need to at least use scoped_refptr to give it an internal
reference. I think as-written that you're leaking vars, since the queue has raw
pointers and you never increment the StringVar's ref count.

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:459: received_messages_.push(var);
Does this compile, to pass a scoped_refptr when a raw pointer is expected? If
so, that seems like a big hole in scoped_refptr... I think in this case, the var
gets deleted when you exit scope, so you probably have use-after-free problems.

[Takashi Toyoshima, 2012-01-17 10:14:58]

Thank you and sorry for careless handling on ref counted objects.

I reflect your comments on Patch Set 5, then rebase to trunk as Patch Set 6.
CcInterfaces tests doesn't work correctly now at Patch Set 6 because it doesn't
set binary type. Unfortunately, I miss to add C++ interfaces to handle binary
types, so I add C++ interfaces and unit tests for them in Patch Set 7.

http://codereview.chromium.org/9026007/diff/1/webkit/plugins/ppapi/ppb_websoc...
File webkit/plugins/ppapi/ppb_websocket_impl.h (right):

http://codereview.chromium.org/9026007/diff/1/webkit/plugins/ppapi/ppb_websoc...
webkit/plugins/ppapi/ppb_websocket_impl.h:87: std::queue< ::ppapi::Var*>
received_messages_;
On 2012/01/13 17:36:14, dmichael wrote:
> Should this be scoped_refptr< ::ppapi::Var>?

Done.

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:200:
websocket_->setBinaryType(static_cast<WebSocket::BinaryType>(binary_type_));
OK, I hold this value as WebSocket::BinaryType, then explicit conversion is done
in Set/GetBinaryType().

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:422:
static_cast<WebSocket::BinaryType>(binary_type_));
On 2012/01/13 17:36:14, dmichael wrote:
> Ditto, some better checking would be good here. Recall that SetBinaryType is
> called by the plugin, and is free to send us bad values. It's very easy to
send
> us an enum to a value that's not actually valid.

Done.

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:443: received_messages_.push(new
StringVar(string));
Oh, sorry. I confused on managing ref counted objects in quieue.
I hold them as scoped_refptr in order to decrement reference at destruction of
queue or pop.

http://codereview.chromium.org/9026007/diff/5005/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:459: received_messages_.push(var);
Fixed.

[dmichael (off chromium), 2012-01-17 22:14:28]

Let me know if you're having trouble understanding scoped_refptr; I can try to
explain a bit if my comments don't make sense.

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:459: received_messages_.push(new
scoped_refptr<Var>(new StringVar(string)));
You don't want to "new" the scoped_refptr; I think you're now leaking both the
Var and the scoped_refptr.

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:475: new scoped_refptr<Var>(new
HostArrayBufferVar(binaryData)));
Ditto

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
File webkit/plugins/ppapi/ppb_websocket_impl.h (right):

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.h:86: std::queue< scoped_refptr<
::ppapi::Var>*> received_messages_;
You don't want the * there

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev.h
File ppapi/cpp/dev/websocket_dev.h (right):

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:192: /// @param[in] binary_type Binary object type
for receibing binary frames
receibing->receiving

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:200: /// GetBinaryType() returns currently
specified binary object type for
nit: returns "the" currently

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:201: /// receiving binary frames representation.
nit: I think "representation" is redundant.

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:204: /// current binary object type.
"the" current

[Takashi Toyoshima, 2012-01-18 12:10:43]

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:459: received_messages_.push(new
scoped_refptr<Var>(new StringVar(string)));
This time, I misunderstand std::queue using with Class.
Now I think I understand correctly.

Here, copy constructor is used to store. In the case of scoped_refptr, reference
counting flips. Finally queue contains copied scoped_refptr having original
StringVar pointer with 1 reference count.

These are deleted or released at the time of queue's destruction.
Could I understand correctly?

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:475: new scoped_refptr<Var>(new
HostArrayBufferVar(binaryData)));
On 2012/01/17 22:14:28, dmichael wrote:
> Ditto

Done.

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
File webkit/plugins/ppapi/ppb_websocket_impl.h (right):

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.h:86: std::queue< scoped_refptr<
::ppapi::Var>*> received_messages_;
On 2012/01/17 22:14:28, dmichael wrote:
> You don't want the * there

Done.

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev.h
File ppapi/cpp/dev/websocket_dev.h (right):

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:192: /// @param[in] binary_type Binary object type
for receibing binary frames
Done here and in api/dev/ppb_websocket_dev_idl.

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:200: /// GetBinaryType() returns currently
specified binary object type for
Ditto.

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:201: /// receiving binary frames representation.
Ditto.

http://codereview.chromium.org/9026007/diff/11002/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:204: /// current binary object type.
Ditto.

[dmichael (off chromium), 2012-01-18 16:20:47]

lgtm

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/9026007/diff/11001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:459: received_messages_.push(new
scoped_refptr<Var>(new StringVar(string)));
On 2012/01/18 12:10:43, toyoshim wrote:
> This time, I misunderstand std::queue using with Class.
> Now I think I understand correctly.
> 
> Here, copy constructor is used to store. In the case of scoped_refptr,
reference
> counting flips. Finally queue contains copied scoped_refptr having original
> StringVar pointer with 1 reference count.
> 
> These are deleted or released at the time of queue's destruction.
> Could I understand correctly?
Yep!

[commit-bot: I haz the power, 2012-01-19 04:03:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9026007/22001

[commit-bot: I haz the power, 2012-01-19 04:16:37]

Try job failure for 9026007-22001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-19 04:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9026007/17003

[commit-bot: I haz the power, 2012-01-19 06:22:52]

Change committed as 118258