WebSocket Pepper API: C++ bindings implementation.

WebSocket Pepper API: C++ bindings implementation.

This change contains simple straightforward C++ interfaces.
JS like bindings will be implemented as a helper class in another change.

BUG=87310
TEST=ui_tests --gtest_filter='PPAPITest.WebSocket_CcInterfaces'


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

[Takashi Toyoshima, 2011-12-06 16:33:34]

Hi, ppapi OWNERS and Darin.

I almost finish the implementation of WebSocket C bindings for NaCl except for
binary support. Now, I'd like to go back on discussion of C++ bindings.

For now, chromium tree contains a header file for WebSocket in ppapi/cpp/dev/.
But doesn't have its implementation.

This change contains the implementation. But I think there might be room to
discuss on API  itself and its implementation. That's the reason why I add
Darin into the reviewers.

[Takashi Toyoshima, 2011-12-06 19:00:00]

> Depends on following bug fix,
> - http://codereview.chromium.org/8821008/
> and contains it as changes in webkit/plugins/ppapi/ppb_websocket_impl.cc.

That change is landed and now I uploaded new patch set 5.
That is rebased to the latest master branch containing the landed change.

[dmichael (off chromium), 2011-12-07 23:31:28]

I'm sorry, I haven't gotten too far yet, but I wanted you to see what I have so
far.

Can you elaborate on what concerns you have with the API now that you've written
this implementation? Maybe it will be obvious once I get further, but it would
help to know what you think about it.

http://codereview.chromium.org/8821010/diff/4001/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/4001/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.cc:29: : connect_callback_(new
CompletionCallback(DidConnectCallback, this,
You'll probably need PP_ALLOW_THIS_IN_INITIALIZER_LIST. You can move it from
ppapi/tests/test_utils.h to ppapi/api/pp_macros.idl and re-gen pp_macros.h.

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

http://codereview.chromium.org/8821010/diff/4001/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.h:137: WebSocketPrivate* impl;
nit: impl->impl_

http://codereview.chromium.org/8821010/diff/3011/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/3011/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.cc:32: receive_callback_(new
CompletionCallback(DidReceiveCallback, this)),
You should probably just use CompletionCallbackFactory, to protect from
callbacks firing after WebSocketPrivate has been deleted. It's probably simpler
than the static set of live instances you're using below.

http://codereview.chromium.org/8821010/diff/3011/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.cc:45: receive_callback_->pp_completion_callback());
I think you need to set the PP_COMPLETIONCALLBACK_FLAG_OPTIONAL for the
receive_callback_, otherwise this will always fire asynchronously

[Takashi Toyoshima, 2011-12-08 14:40:09]

Sorry for my vague review request.

I think this API set is good for users who are familier with WebSocket JS
bindings.
But, it seems to be a little unusual way for Pepper.
Other APIs provide C++ interfaces as a simple bridge to C APIs.
For example, if a C API requires CompletionCallback, C++ API also has
CompletionCallback, etc.
But, my API set make all callbacks hidden behind the API. Is this acceptable as
a Pepper API?

http://codereview.chromium.org/8821010/diff/4001/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/4001/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.cc:29: : connect_callback_(new
CompletionCallback(DidConnectCallback, this,
On 2011/12/07 23:31:28, dmichael wrote:
> You'll probably need PP_ALLOW_THIS_IN_INITIALIZER_LIST. You can move it from
> ppapi/tests/test_utils.h to ppapi/api/pp_macros.idl and re-gen pp_macros.h.

Done.

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

http://codereview.chromium.org/8821010/diff/4001/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.h:137: WebSocketPrivate* impl;
On 2011/12/07 23:31:28, dmichael wrote:
> nit: impl->impl_

Done.

http://codereview.chromium.org/8821010/diff/3011/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/3011/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.cc:32: receive_callback_(new
CompletionCallback(DidReceiveCallback, this)),
On 2011/12/07 23:31:28, dmichael wrote:
> You should probably just use CompletionCallbackFactory, to protect from
> callbacks firing after WebSocketPrivate has been deleted. It's probably
simpler
> than the static set of live instances you're using below.

Done.

http://codereview.chromium.org/8821010/diff/3011/ppapi/cpp/dev/websocket_dev....
ppapi/cpp/dev/websocket_dev.cc:45: receive_callback_->pp_completion_callback());
On 2011/12/07 23:31:28, dmichael wrote:
> I think you need to set the PP_COMPLETIONCALLBACK_FLAG_OPTIONAL for the
> receive_callback_, otherwise this will always fire asynchronously

Done.

[dmichael (off chromium), 2011-12-08 21:26:48]

Thanks for explaining why you wanted to revisit the API discussion. I think that
was the same concern I had at the original API review. The C++ is made to look
like the JS API, while the C API uses completion callbacks instead. The C++
implementation here might have been more straightforward if the C API used a
PPP_WebSocket interface for OnOpen, etc, instead of completion callbacks. Or if
the C++ interface was a direct translation of the C interface, of course, that
would be simpler too.

That said, it didn't turn out too complex and it seems fine to me. Both
interfaces seem good, just different. This C++ one does protect the user a bit
from using ReceiveMessage in a slow way.

LGTM, but I wouldn't mind a 2nd opinion from brettw, darin, or piman.

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.cc:36: 
nit: I think everything below here except the 'New...Callback' functions could
probably be private

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.cc:109: delete impl_;
Could you use scoped_ptr?

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.cc:151: return PP_ERROR_BADRESOURCE;
Your return type is uint64_t; you probably want to just return 0.

http://codereview.chromium.org/8821010/diff/12007/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8821010/diff/12007/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:58: uint16_t code;
nit: close_code?

http://codereview.chromium.org/8821010/diff/12007/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:64: TestWebSocketClient(pp::Instance* instance)
nit: explicit

[brettw, 2011-12-08 23:00:00]

Here's my advice:

I think it's best to keep the base C++ layer a very simple wrapper around the C
layer. Otherwise it will get confusing with every component author deciding
something slightly different.

For many components (possibly this one, though I didn't look carefully) it might
be challenging to use the base API. I think in these cases we should add helper
classes that give the API you want, and may do nice C++ things for the caller.
So if you can make an API similar to WebSockets

[darin (slow to review), 2011-12-08 23:21:56]

+1... I think we should eventually go even further.  Just auto-gen the
basic C++ bindings.
-Darin

On Thu, Dec 8, 2011 at 3:00 PM, <brettw@chromium.org> wrote:

> Here's my advice:
>
> I think it's best to keep the base C++ layer a very simple wrapper around
> the C
> layer. Otherwise it will get confusing with every component author deciding
> something slightly different.
>
> For many components (possibly this one, though I didn't look carefully) it
> might
> be challenging to use the base API. I think in these cases we should add
> helper
> classes that give the API you want, and may do nice C++ things for the
> caller.
> So if you can make an API similar to WebSockets
>
>
http://codereview.chromium.**org/8821010/<http://codereview.chromium.org/8821...
>

[brettw, 2011-12-08 23:56:17]

Sorry, I ended mid-sentence...

On Thu, Dec 8, 2011 at 3:00 PM,  <brettw@chromium.org> wrote:
> Here's my advice:
>
> I think it's best to keep the base C++ layer a very simple wrapper around
> the C
> layer. Otherwise it will get confusing with every component author deciding
> something slightly different.
>
> For many components (possibly this one, though I didn't look carefully) it
> might
> be challenging to use the base API. I think in these cases we should add
> helper
> classes that give the API you want, and may do nice C++ things for the
> caller.
> So if you can make an API similar to WebSockets

...in a helper class, please do so :)

Brett

[dmichael (off chromium), 2011-12-09 02:47:39]

On 2011/12/08 23:56:17, brettw wrote:
> Sorry, I ended mid-sentence...
> 
> On Thu, Dec 8, 2011 at 3:00 PM,  <mailto:brettw@chromium.org> wrote:
> > Here's my advice:
> >
> > I think it's best to keep the base C++ layer a very simple wrapper around
> > the C
> > layer. Otherwise it will get confusing with every component author deciding
> > something slightly different.
> >
> > For many components (possibly this one, though I didn't look carefully) it
> > might
> > be challenging to use the base API. I think in these cases we should add
> > helper
> > classes that give the API you want, and may do nice C++ things for the
> > caller.
> > So if you can make an API similar to WebSockets
> 
> ...in a helper class, please do so :)
> 
> Brett
So are you suggesting having a simple C++ version of the PPB interface that is
the direct translation (using completion callbacks in this case), and then
providing a separate wrapper class to provide the WebSockets-like interface?

SGTM

[Takashi Toyoshima, 2011-12-09 05:41:46]

Thank you guys for helpful advices.

> David
Yeah, now I understand that I was not so familier with pepper API at that time
;-)

> Brett, Darin
Your suggestion sounds good to me.
Actually, I hit on the same idea when I hear about the plan of layer separation
into bindings and helpers.

OK, I'll make a change of straightforward C++ bindings, then I implement this
change again by using them.
The points David make comments wil be fixed in the new implementation.

For now, I have a question on one comments. Please see following comments.

Thanks,

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.cc:109: delete impl_;
Can I use scoped_ptr in NaCl world?

[brettw, 2011-12-09 05:58:26]

I would probably recommend doing your helper class as a separate patch. I think
the straight C++ wrapper will be much simpler and you can check it in faster,
which will give you more time to work on a good helper class.

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.cc:109: delete impl_;
We don't use base from the C++ layer, so I'd avoid this.

[dmichael (off chromium), 2011-12-09 13:44:42]

right, I just wasn't thinking straight when I suggested scoped_ptr, sorry.
TR1 has a scoped ptr and auto_ptr would work here, but they have draw
backs. Raw pointer is good.
On Dec 8, 2011 10:58 PM, <brettw@chromium.org> wrote:

> I would probably recommend doing your helper class as a separate patch. I
> think
> the straight C++ wrapper will be much simpler and you can check it in
> faster,
> which will give you more time to work on a good helper class.
>
>
> http://codereview.chromium.**org/8821010/diff/12007/ppapi/**
>
cpp/dev/websocket_dev.cc<http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev.cc>
> File ppapi/cpp/dev/websocket_dev.cc (right):
>
> http://codereview.chromium.**org/8821010/diff/12007/ppapi/**
>
cpp/dev/websocket_dev.cc#**newcode109<http://codereview.chromium.org/8821010/diff/12007/ppapi/cpp/dev/websocket_dev.cc#newcode109>
> ppapi/cpp/dev/websocket_dev.**cc:109: delete impl_;
> We don't use base from the C++ layer, so I'd avoid this.
>
>
http://codereview.chromium.**org/8821010/<http://codereview.chromium.org/8821...
>

[Takashi Toyoshima, 2011-12-09 16:36:01]

Hi, David.

I prepare another straightforward C++ bindings.
Could you review this?

Implementing it, I found miss of incrementing PP_Var's reference counts on C
APIs
which return PP_Var values. I request another review on that by 8872065.

Thanks

[dmichael (off chromium), 2011-12-09 23:05:25]

mostly just nits.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:183: ///         :
factory_(PP_ALLOW_THIS_IN_INITIALIZER_LIST(this)), offset_(0) {
Actually, this macro is really just a way around a warning on the Windows
compiler. Maybe leave it alone, but note below that some compilers may warn
about using "this" in an initializer list, and that PP_ALLOW... can be used to
work around the warning?

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.cc:114: return result == PP_TRUE;
We usually use
return PP_ToBool(result);
for these

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

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:37: /// argument can be null only if
<code>protocol_count</code> is 0.
Is there a reason you removed this comment on the C but left it here? Seems like
it could be true for both?

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:42: /// connection.
"the" connection ?

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:44: /// @return Returns
<code>PP_OK_COMPLETIONPENDING</code> then callback is
Maybe this should be worded differently. It can return an error immediately in
some cases, I think. And eventually we will support synchronous behavior off the
main thread.A lot of our other completion-callback functions just say something
like:
@return An int32_t containing an error code from <code>pp_errors.h</code>.
More detail is helpful to explain the error codes, but it should probably not
say that PP_OK_COMPLETIONPENDING is always returned.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:46: /// <code>PP_ERROR_BADARGUMENT</code>
corresponds to JavaScript SyntaxError,
I think it would be better to explain what the error code means rather than map
it to a JavaScript exception. Some pepper users won't be familiar with the
WebSockets spec.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:50: int32_t Connect(const Var& url, const Var*
const protocols,
Why did you change from const Var protocols[]? In any case, the latter 'const'
isn't really meaningful here (it's similar to having |const uint32_t| for the
next parameter). I'd omit that const.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:60: /// when the connection is closed or an error
occurs in closing connection.
closing 'the' connection?

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:72: /// This interface only returns bytes of a
single message. That is, this
I would remove 'bytes of '

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:77: /// <code>message</code>.
Maybe it's worth noting that the Var passed for |message| must remain valid
until the ReceiveMessage operation completes? Or maybe that's obvious.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:79: /// when the receiving message is completed.
It is ignored when the function
It might be clearer to say something like:
"It is ignored if ReceiveMessage completes synchronously and returns PP_OK."

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:122: /// close reason is set, it contains empty
string.
contains 'an' empty string?

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:131: /// <code>false</code> if the connection was
closed by abnormal reasons.
by->for

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:139: /// Currently its data is always empty
string.
'an' empty string?

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:146: /// connection is established, it containss
empty string.
"containss"->"contains the"

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:159: /// connection is established, it contains
empty string.
contains "the"?

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

http://codereview.chromium.org/8821010/diff/23001/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:379: return
ReturnVar(url_->GetPPVar());
It looks like this got polluted from that other CL that makes this change?

[Takashi Toyoshima, 2011-12-13 14:08:23]

Thank you for many useful advice on documents.
I refined the document again.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/completion_callba...
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/completion_callba...
ppapi/cpp/completion_callback.h:183: ///         :
factory_(PP_ALLOW_THIS_IN_INITIALIZER_LIST(this)), offset_(0) {
On 2011/12/09 23:05:26, dmichael wrote:
> Actually, this macro is really just a way around a warning on the Windows
> compiler. Maybe leave it alone, but note below that some compilers may warn
> about using "this" in an initializer list, and that PP_ALLOW... can be used to
> work around the warning?

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev.cc
File ppapi/cpp/dev/websocket_dev.cc (right):

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.cc:114: return result == PP_TRUE;
On 2011/12/09 23:05:26, dmichael wrote:
> We usually use
> return PP_ToBool(result);
> for these

Done.

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

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:37: /// argument can be null only if
<code>protocol_count</code> is 0.
Oh, sorry.
I misunderstand on array like that foo[] could not accept NULL. But it's wrong.
As you said, C interface also need this comment, and I'll revert following
interface change from pointer to [].

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:42: /// connection.
s/the/a/. on this and idl definition.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:44: /// @return Returns
<code>PP_OK_COMPLETIONPENDING</code> then callback is
Thank you.
I fixed all description on C/C++ APIs which have an callback.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:46: /// <code>PP_ERROR_BADARGUMENT</code>
corresponds to JavaScript SyntaxError,
The error detection rule is complicated, but I try on IDL and C++ description.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:50: int32_t Connect(const Var& url, const Var*
const protocols,
On 2011/12/09 23:05:26, dmichael wrote:
> Why did you change from const Var protocols[]? In any case, the latter 'const'
> isn't really meaningful here (it's similar to having |const uint32_t| for the
> next parameter). I'd omit that const.

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:60: /// when the connection is closed or an error
occurs in closing connection.
On 2011/12/09 23:05:26, dmichael wrote:
> closing 'the' connection?

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:72: /// This interface only returns bytes of a
single message. That is, this
On 2011/12/09 23:05:26, dmichael wrote:
> I would remove 'bytes of '

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:77: /// <code>message</code>.
On 2011/12/09 23:05:26, dmichael wrote:
> Maybe it's worth noting that the Var passed for |message| must remain valid
> until the ReceiveMessage operation completes? Or maybe that's obvious.

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:79: /// when the receiving message is completed.
It is ignored when the function
Thank you for good suggestion.
Fixed.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:122: /// close reason is set, it contains empty
string.
On 2011/12/09 23:05:26, dmichael wrote:
> contains 'an' empty string?

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:131: /// <code>false</code> if the connection was
closed by abnormal reasons.
On 2011/12/09 23:05:26, dmichael wrote:
> by->for

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:139: /// Currently its data is always empty
string.
On 2011/12/09 23:05:26, dmichael wrote:
> 'an' empty string?

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:146: /// connection is established, it containss
empty string.
On 2011/12/09 23:05:26, dmichael wrote:
> "containss"->"contains the"

Done.

http://codereview.chromium.org/8821010/diff/23001/ppapi/cpp/dev/websocket_dev...
ppapi/cpp/dev/websocket_dev.h:159: /// connection is established, it contains
empty string.
On 2011/12/09 23:05:26, dmichael wrote:
> contains "the"?

Done.

[Takashi Toyoshima, 2011-12-16 02:13:42]

ping?

This review is suspended.
Could you resume please?

[dmichael (off chromium), 2011-12-16 03:04:28]

lgtm

http://codereview.chromium.org/8821010/diff/30013/ppapi/api/dev/ppb_websocket...
File ppapi/api/dev/ppb_websocket_dev.idl (right):

http://codereview.chromium.org/8821010/diff/30013/ppapi/api/dev/ppb_websocket...
ppapi/api/dev/ppb_websocket_dev.idl:122: * <code>url</code> is not secure
protocol, but the origin of the caller
nit: "not secure" -> "not a secure"

http://codereview.chromium.org/8821010/diff/30013/ppapi/api/dev/ppb_websocket...
ppapi/api/dev/ppb_websocket_dev.idl:125: * access because the port is well-known
ports like SMTP, or so. It
nit: "is well"->"is a well"
nit: "ports"->"port"
nit: Not sure what ", or so." was supposed to say. It could probably just be
removed?

http://codereview.chromium.org/8821010/diff/30013/ppapi/shared_impl/var_track...
File ppapi/shared_impl/var_tracker.cc (right):

http://codereview.chromium.org/8821010/diff/30013/ppapi/shared_impl/var_track...
ppapi/shared_impl/var_tracker.cc:103: info.var->AssignVarID(0);
This landed with the other patch, right? So I'll just assume it won't end up
part of this CL after you merge.

[Takashi Toyoshima, 2011-12-16 05:36:37]

Thanks.
I'll apply these editorial improvements into helper class document, too.

http://codereview.chromium.org/8821010/diff/30013/ppapi/api/dev/ppb_websocket...
File ppapi/api/dev/ppb_websocket_dev.idl (right):

http://codereview.chromium.org/8821010/diff/30013/ppapi/api/dev/ppb_websocket...
ppapi/api/dev/ppb_websocket_dev.idl:122: * <code>url</code> is not secure
protocol, but the origin of the caller
On 2011/12/16 03:04:29, dmichael wrote:
> nit: "not secure" -> "not a secure"

Done.

http://codereview.chromium.org/8821010/diff/30013/ppapi/api/dev/ppb_websocket...
ppapi/api/dev/ppb_websocket_dev.idl:125: * access because the port is well-known
ports like SMTP, or so. It
On 2011/12/16 03:04:29, dmichael wrote:
> nit: "is well"->"is a well"
> nit: "ports"->"port"
> nit: Not sure what ", or so." was supposed to say. It could probably just be
> removed?

Done.

http://codereview.chromium.org/8821010/diff/30013/ppapi/shared_impl/var_track...
File ppapi/shared_impl/var_tracker.cc (right):

http://codereview.chromium.org/8821010/diff/30013/ppapi/shared_impl/var_track...
ppapi/shared_impl/var_tracker.cc:103: info.var->AssignVarID(0);
This change was landed as another CL, and was removed at the next patch set.

[commit-bot: I haz the power, 2011-12-16 05:36:59]

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

[commit-bot: I haz the power, 2011-12-16 07:28:24]

Change committed as 114779