Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7837022:
WebSocket Pepper C API. In this change, only the C interface is defined and no (Closed)
|
Created:
9 years, 3 months ago by Yuzo Modified:
8 years, 11 months ago CC:
chromium-reviews, piman+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWebSocket Pepper C API. In this change, only the C interface is defined and no
implementation is provided.
TEST=n/a because only the interface is defined in this change.
BUG=87310
Patch Set 1 #
Total comments: 30
Patch Set 2 : Addressed tyoshino's comments #
Total comments: 20
Patch Set 3 : Address Brett's comments #
Messages
Total messages: 7 (0 generated)
Hi, reviewers, I'd appreciate it if you could review this. One thing that I'm unsure about is when to use PP_Var/PP_Bool/... rather than char*/bool/... . For example, should I use PP_Var for a function that returns string / takes string argument? What is the decision factor? I tried to learn from existing interfaces but failed.
http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h File ppapi/c/dev/ppb_websocket_dev.h (right): http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:69: // Connects |web_socket| to null-terminate |url| specifying |protocols|. terminated http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:70: // |protocols| points to an array of null-terminated protocol strings. In case protocol -> sub-protocol http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:96: // otherwise. If successful, it's notified only by callback? PP_OK is never returned even if data is ready? http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:104: // |*message_bytes| up to |message_byte_count| bytes. |message_bytes| can be remove * http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:108: // |remaining_message_byte_count| is not null. The remaining message bytes are I'd suggest s/remaining message bytes/remaining bytes of the message/ http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:109: // returned for subsequent call(s) to this method. Bytes from a message are from -> of? I'm not sure. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:116: // character may have incomplete bytes, and subsequent call my return may http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:116: // character may have incomplete bytes, and subsequent call my return Do you mean part of UTF-8 sequence by "incomplete bytes"? Some rephrasing to make it clear is nice. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:119: // Callers responsibilities: Caller's http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:120: // - Caller must keep |message_bytes| and |message_type| valid until and remaining_message_bytes_count? http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:144: // Gets the connection close code for |web_socket|. It would be nice if you add some text to say when this returns valid value. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:147: // Gets the connection close reason for |web_socket|. ditto http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:150: // Gets if the connection was closed cleanly for |web_socket|. ditto
Thank you for the review. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h File ppapi/c/dev/ppb_websocket_dev.h (right): http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:69: // Connects |web_socket| to null-terminate |url| specifying |protocols|. On 2011/09/07 13:07:58, tyoshino wrote: > terminated Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:70: // |protocols| points to an array of null-terminated protocol strings. In case On 2011/09/07 13:07:58, tyoshino wrote: > protocol -> sub-protocol Done: protocol strings -> sub-protocols. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:96: // otherwise. On 2011/09/07 13:07:58, tyoshino wrote: > If successful, it's notified only by callback? PP_OK is never returned even if > data is ready? Good point. Changed the method such that the number of bytes copied is returned in case a message is already available. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:104: // |*message_bytes| up to |message_byte_count| bytes. |message_bytes| can be On 2011/09/07 13:07:58, tyoshino wrote: > remove * Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:108: // |remaining_message_byte_count| is not null. The remaining message bytes are On 2011/09/07 13:07:58, tyoshino wrote: > I'd suggest > s/remaining message bytes/remaining bytes of the message/ Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:109: // returned for subsequent call(s) to this method. Bytes from a message are On 2011/09/07 13:07:58, tyoshino wrote: > from -> of? I'm not sure. I'm not sure either but changed to "of". http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:116: // character may have incomplete bytes, and subsequent call my return On 2011/09/07 13:07:58, tyoshino wrote: > may Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:116: // character may have incomplete bytes, and subsequent call my return On 2011/09/07 13:07:58, tyoshino wrote: > Do you mean part of UTF-8 sequence by "incomplete bytes"? Some rephrasing to > make it clear is nice. Rewrote as: In copying message to |message_bytes|, UTF-8 byte sequence boundaries are not considered. Hence if the message is larger than |message_bytes|, the last byte sequence may end prematurely. Likewise, the first sequence of the bytes returned for the subsequent call may start in the middle. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:116: // character may have incomplete bytes, and subsequent call my return On 2011/09/07 13:07:58, tyoshino wrote: > may Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:116: // character may have incomplete bytes, and subsequent call my return On 2011/09/07 13:07:58, tyoshino wrote: > may Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:116: // character may have incomplete bytes, and subsequent call my return On 2011/09/07 13:07:58, tyoshino wrote: > may Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:119: // Callers responsibilities: On 2011/09/07 13:07:58, tyoshino wrote: > Caller's Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:120: // - Caller must keep |message_bytes| and |message_type| valid until On 2011/09/07 13:07:58, tyoshino wrote: > and remaining_message_bytes_count? Good catch! Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:120: // - Caller must keep |message_bytes| and |message_type| valid until On 2011/09/07 13:07:58, tyoshino wrote: > and remaining_message_bytes_count? Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:144: // Gets the connection close code for |web_socket|. On 2011/09/07 13:07:58, tyoshino wrote: > It would be nice if you add some text to say when this returns valid value. Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:147: // Gets the connection close reason for |web_socket|. On 2011/09/07 13:07:58, tyoshino wrote: > ditto Done. http://codereview.chromium.org/7837022/diff/1/ppapi/c/dev/ppb_websocket_dev.h... ppapi/c/dev/ppb_websocket_dev.h:150: // Gets if the connection was closed cleanly for |web_socket|. On 2011/09/07 13:07:58, tyoshino wrote: > ditto Done.
LGTM, thanks! I have mostly some comment suggestions and things to think about in subsequent passes. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_dev.h File ppapi/c/dev/ppb_websocket_dev.h (right): http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:30: // Note on the differences from JavaScript WebSocket API This is a nice comment. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:78: const char* const* protocols, I don't want to hold up this patch, but probably the step after you check this in is to convert to IDL. This should be easy, and you can do it in parallel with the implementation. IDL generation has in other places found inconsistencies (like passing a PP_Var by pointer instead of value) that affect the API. In this case, I think it will end up looking like PPP_Instance.DidCreate which will be "const char* protocols[]" You don't have to change this now. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:79: int32_t protocol_count, Can you use uint32_t here? We're using unsigned types for lengths in most of the APIs. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:107: // if any, or 0, if not, is set to |*remaining_message_byte_count| if I think you can remove "or 0, if not," since I think this is implied and makes it kind of difficult to parse this sentence. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:109: // message are returned for subsequent call(s) to this method. Bytes of a I'd rephrase your "Bytes of a message..." sentence as "This function only returns bytes of a single message." http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:112: // matter how small each message is. The message type is set to Can you insert a blank line before "The message type...", this paragraph is getting kind of long. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:114: // is encoded in UTF-8. In copying message to |message_bytes|, UTF-8 byte I'd also add a blank line before "In copying..." since this is kind of a new topic. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:122: // |message_type| valid until |callback| is called. Can you say "...until |callback| is issued or Close() is called." http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:124: int32_t (*ReceiveMessage)(PP_Resource web_socket, This function is kind of complicated to call. I don't see any way around that. One of the next steps after this patch will be to provide a C++ wrapper around this class. I think this should be in two separate layers: the first layer would be a straightforward wrapper around this API using C++ primitives (pp::CompletionCallback). It seems like it would be nice to provide a high-level layer on top of that, where you can imagine what would be most convenient to most callers. It seems like converting the completion callbacks and buffers to some kind of virtual function call that passes an entire message as a vector<char> to the plugin would be most convenient, saving the plugin from having to implement all the buffer and callback management. I would probably implement this in parallel with the Pepper implementation. For testing, I'd write a sample app and just design the high-level C++ wrapper in parallel to be as convenient to use as possible from the perspective of your test plugin. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:151: // Returns the connection close reason for |web_socket|. Returns an empty Can you change this to "Returns a PP_VARTYPE_NULL var if called before the close reason is set, or PP_VARTYPE_UNDEFINED if called on an invalid resource." I think this is also a good rule for the later functions that return PP_Var, rather than returning an empty string.
Brett, thank you for the review and sorry for my late response. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_dev.h File ppapi/c/dev/ppb_websocket_dev.h (right): http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:30: // Note on the differences from JavaScript WebSocket API On 2011/09/08 16:39:36, brettw wrote: > This is a nice comment. Thank you! http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:78: const char* const* protocols, On 2011/09/08 16:39:36, brettw wrote: > I don't want to hold up this patch, but probably the step after you check this > in is to convert to IDL. This should be easy, and you can do it in parallel with > the implementation. > > IDL generation has in other places found inconsistencies (like passing a PP_Var > by pointer instead of value) that affect the API. In this case, I think it will > end up looking like PPP_Instance.DidCreate which will be "const char* > protocols[]" > > You don't have to change this now. Sure, I'll write the IDL later. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:79: int32_t protocol_count, On 2011/09/08 16:39:36, brettw wrote: > Can you use uint32_t here? We're using unsigned types for lengths in most of the > APIs. Done. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:107: // if any, or 0, if not, is set to |*remaining_message_byte_count| if On 2011/09/08 16:39:36, brettw wrote: > I think you can remove "or 0, if not," since I think this is implied and makes > it kind of difficult to parse this sentence. Done. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:109: // message are returned for subsequent call(s) to this method. Bytes of a On 2011/09/08 16:39:36, brettw wrote: > I'd rephrase your "Bytes of a message..." sentence as "This function only > returns bytes of a single message." Done. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:112: // matter how small each message is. The message type is set to On 2011/09/08 16:39:36, brettw wrote: > Can you insert a blank line before "The message type...", this paragraph is > getting kind of long. Done. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:114: // is encoded in UTF-8. In copying message to |message_bytes|, UTF-8 byte On 2011/09/08 16:39:36, brettw wrote: > I'd also add a blank line before "In copying..." since this is kind of a new > topic. Done. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:122: // |message_type| valid until |callback| is called. On 2011/09/08 16:39:36, brettw wrote: > Can you say "...until |callback| is issued or Close() is called." Done. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:124: int32_t (*ReceiveMessage)(PP_Resource web_socket, On 2011/09/08 16:39:36, brettw wrote: > This function is kind of complicated to call. I don't see any way around that. > One of the next steps after this patch will be to provide a C++ wrapper around > this class. I think this should be in two separate layers: the first layer would > be a straightforward wrapper around this API using C++ primitives > (pp::CompletionCallback). > > It seems like it would be nice to provide a high-level layer on top of that, > where you can imagine what would be most convenient to most callers. It seems > like converting the completion callbacks and buffers to some kind of virtual > function call that passes an entire message as a vector<char> to the plugin > would be most convenient, saving the plugin from having to implement all the > buffer and callback management. > > I would probably implement this in parallel with the Pepper implementation. For > testing, I'd write a sample app and just design the high-level C++ wrapper in > parallel to be as convenient to use as possible from the perspective of your > test plugin. I agree that this method should be simplified if possible. http://codereview.chromium.org/7837022/diff/4001/ppapi/c/dev/ppb_websocket_de... ppapi/c/dev/ppb_websocket_dev.h:151: // Returns the connection close reason for |web_socket|. Returns an empty On 2011/09/08 16:39:36, brettw wrote: > Can you change this to "Returns a PP_VARTYPE_NULL var if called before the close > reason is set, or PP_VARTYPE_UNDEFINED if called on an invalid resource." I > think this is also a good rule for the later functions that return PP_Var, > rather than returning an empty string. Done, for all the methods that return PP_Var.
LGTM. Note that we'll have to convert the comments to C-style, but we can do that when we add IDL.
I think this CL is not relevant any more. I will close just close this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
