Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8395037:
IDL for WebSocket Pepper API. (Closed)
|
Created:
9 years, 2 months ago by Takashi Toyoshima Modified:
9 years, 1 month ago Reviewers:
tyoshino (SeeGerritForStatus), Yuzo, cstefansen, dmichael (off chromium), darin (slow to review), brettw CC:
chromium-reviews, piman+watch_chromium.org, darin-cc_chromium.org, ihf+watch_chromium.org, Yuta Kitamura Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIDL for WebSocket Pepper API.
Define WebSocket Pepper API as IDL and generate the C interface from it.
This IDL takes over from http://codereview.chromium.org/7837022/
BUG=87310
TEST=n/a because only the interface is defined in this change.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109592
Patch Set 1 : use PP_Var #Patch Set 2 : Add closure callback setter #
Total comments: 6
Patch Set 3 : revised #
Total comments: 1
Patch Set 4 : handler registration model as Christian said #
Total comments: 54
Patch Set 5 : revise #Patch Set 6 : Add C++ API sample #
Total comments: 2
Patch Set 7 : rebase (omit generator related change) #Patch Set 8 : rebase again (omit generator related change) #Patch Set 9 : remove delegate version of C++ API #Patch Set 10 : update C++ API #
Total comments: 4
Messages
Total messages: 25 (0 generated)
Share a draft for the final review with Yuzo and other WebSocket guys.
Now header for C was generated by ppapi/generators/generator.py. I reorganized comments because of document format differences. Major changes from Yuzo-san's C interfaces are following two points. - use PP_Var insteads of char* - Add SetCloseCallback() interface
http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... ppapi/api/dev/ppb_websocket_dev.idl:33: * PP_ERROR_BADARGUMENT: SYNTAX_ERR Use new style exception symbols? e.g. here, SyntaxError http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... ppapi/api/dev/ppb_websocket_dev.idl:96: * Create() Creates a WebSocket instance. creates http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... ppapi/api/dev/ppb_websocket_dev.idl:123: * Closure code and reason should be queried via each interface. how about using the same term as the spec? closure status code -> close code closure reason -> close reason
http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... ppapi/api/dev/ppb_websocket_dev.idl:33: * PP_ERROR_BADARGUMENT: SYNTAX_ERR On 2011/10/26 17:56:35, tyoshino wrote: > Use new style exception symbols? e.g. here, SyntaxError Done. http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... ppapi/api/dev/ppb_websocket_dev.idl:96: * Create() Creates a WebSocket instance. On 2011/10/26 17:56:35, tyoshino wrote: > creates Done. http://codereview.chromium.org/8395037/diff/2001/ppapi/api/dev/ppb_websocket_... ppapi/api/dev/ppb_websocket_dev.idl:123: * Closure code and reason should be queried via each interface. On 2011/10/26 17:56:35, tyoshino wrote: > how about using the same term as the spec? closure status code -> close code > closure reason -> close reason Done. http://codereview.chromium.org/8395037/diff/5001/ppapi/api/dev/ppb_websocket_... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/5001/ppapi/api/dev/ppb_websocket_... ppapi/api/dev/ppb_websocket_dev.idl:189: * SyntaxError and <code>PP_ERROR_NOACCESS</code> corresponding to JavaScript fixed as spec says
+Christain and Brett.
Revised as an event handler registration model. It is more compatible with JS API.
http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:103: int32_t SetOpenCallback([in] PP_Resource web_socket, How about just passing a PP_CompletionCallback to Connect, and then eliminate this SetOpenCallback method altogether? http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:121: int32_t SetMessageCallback([in] PP_Resource web_socket, Have you considered just passing a PP_CompletionCallback parameter to ReceiveMessage? Maybe you are concerned about the fact that ReceiveMessage takes out-parameters? Those would need to remain valid until the completion callback runs. Note: completion callbacks are one-shot. Also, by placing the completion callback on the method that might take time to complete, you create an API that can either work asynchronously on the main thread (completion callback is non-null) or synchronously on a background thread (completion callback is null). http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:138: int32_t SetErrorCallback([in] PP_Resource web_socket, It seems like this should not be necessary. errors can be reported by making Connect, ReceiveMessage or SendMessage generate an error, right? There could also be a way to poll for the current connection state if being able to notice errors after connect and before trying to read/write a message is useful. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:156: int32_t SetCloseCallback([in] PP_Resource web_socket, Ditto. I'd just get rid of this in favor of using different error codes to distinguish between different flavors of closing. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:259: [out,size_as=message_byte_count] void message_bytes, in the comments, the out param for the message is represented as a PP_Var. that seems like a pretty good thing to me. why did you decide to go with a raw byte array instead? http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:292: [in] int32_t message_byte_count, ditto. using PP_Var for the message seems like a good thing. is this about the fact that PP_Var does not yet support byte arrays? we plan to make it support byte arrays. maybe we should increase the priority of that work. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:308: uint64_t GetBufferedAmount([in] PP_Resource web_socket); be mindful that all of these getters will need to be implemented using a cache in the plugin process. this is because we cannot have the plugin send synchronous IPCs to the webkit main thread. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:396: PP_Var GetUrl([in] PP_Resource web_socket); nit: GetURL
A couple of comments and questions. Darin, Dave, it would be great if you could have a look. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:10: M16 = 0.1 Note: We should plan to pull it out of dev for M17, but a different CL can do that. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... File ppapi/c/dev/ppb_websocket_dev.h (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:239: * <code>message_byte_count</code> is 0. What would be the purpose of calling ReceiveMessage with message_byte_count = 0? Is this just meant as a convenience to avoid special casing? http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:243: * Likewise, the first sequence of the bytes returned for the subsequent call This sounds a bit inconvenient for the API user. Is there another reason for doing this than simply convenience for the implementor? http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:272: PP_WebSocketMessageType_Dev* message_type); To receive a message one would have to (a) receive a callback on the register callback (see SetMessageCallback) and (b) call ReceiveMessage. I think this is fine, but I'd like us to make sure that we can perform these two steps without having to make several RPC calls in the out-of-process case, as this would adversely affect performance. Thoughts? http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:293: * Returns <code>PP_ERROR_FAILED</code> corresponding JavaScript Nit: corresponding -> corresponding to http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:294: * InvalidStateError and <code>PP_ERROR_BADARGUMENT</code> corresponding Same here. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:309: * release. Could you elaborate?
More questions and comments to Darin's thoughts. :-) http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:121: int32_t SetMessageCallback([in] PP_Resource web_socket, This slightly incongruent with the JS version which has you register event handlers (e.g., onmessage), but I think I like it – particularly given that callbacks are one-shot, as you mention, so it won't work to register a general onmessage callback. On 2011/11/01 17:40:53, darin wrote: > Have you considered just passing a PP_CompletionCallback parameter to > ReceiveMessage? Maybe you are concerned about the fact that ReceiveMessage > takes out-parameters? Those would need to remain valid until the completion > callback runs. > > Note: completion callbacks are one-shot. Also, by placing the completion > callback on the method that might take time to complete, you create an API that > can either work asynchronously on the main thread (completion callback is > non-null) or synchronously on a background thread (completion callback is null). http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:156: int32_t SetCloseCallback([in] PP_Resource web_socket, How would you know that the connection closed if you didn't initiate the closure? Polling for status is not desirable, and you will often keep the connection open for some time without sending or receiving, so you are not necessarily calling those functions either. On 2011/11/01 17:40:53, darin wrote: > Ditto. I'd just get rid of this in favor of using different error codes to > distinguish between different flavors of closing. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:308: uint64_t GetBufferedAmount([in] PP_Resource web_socket); That doesn't seem very performant. Is there an alternative? On 2011/11/01 17:40:53, darin wrote: > be mindful that all of these getters will need to be implemented using a cache > in the plugin process. this is because we cannot have the plugin send > synchronous IPCs to the webkit main thread.
On Tue, Nov 1, 2011 at 11:08 AM, <cstefansen@google.com> wrote: > More questions and comments to Darin's thoughts. :-) > > > > http://codereview.chromium.**org/8395037/diff/10001/ppapi/** > api/dev/ppb_websocket_dev.idl<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl> > File ppapi/api/dev/ppb_websocket_**dev.idl (right): > > http://codereview.chromium.**org/8395037/diff/10001/ppapi/** > api/dev/ppb_websocket_dev.idl#**newcode121<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl#newcode121> > ppapi/api/dev/ppb_websocket_**dev.idl:121: int32_t SetMessageCallback([in] > PP_Resource web_socket, > This slightly incongruent with the JS version which has you register > event handlers (e.g., onmessage), but I think I like it – particularly > given that callbacks are one-shot, as you mention, so it won't work to > register a general onmessage callback. > > > On 2011/11/01 17:40:53, darin wrote: > >> Have you considered just passing a PP_CompletionCallback parameter to >> ReceiveMessage? Maybe you are concerned about the fact that >> > ReceiveMessage > >> takes out-parameters? Those would need to remain valid until the >> > completion > >> callback runs. >> > > Note: completion callbacks are one-shot. Also, by placing the >> > completion > >> callback on the method that might take time to complete, you create an >> > API that > >> can either work asynchronously on the main thread (completion callback >> > is > >> non-null) or synchronously on a background thread (completion callback >> > is null). > > http://codereview.chromium.**org/8395037/diff/10001/ppapi/** > api/dev/ppb_websocket_dev.idl#**newcode156<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl#newcode156> > ppapi/api/dev/ppb_websocket_**dev.idl:156: int32_t SetCloseCallback([in] > PP_Resource web_socket, > How would you know that the connection closed if you didn't initiate the > closure? Polling for status is not desirable, and you will often keep > the connection open for some time without sending or receiving, so you > are not necessarily calling those functions either. Hmm... PPB_URLLoader has the same "problem"... You cannot find out if it is closed until you try to use it. It seems like many users of a websocket will have a call to ReceiveMessage(..., completion_callback) in flight. This would fail when the websocket is closed. Perhaps some users will only call ReceiveMessage after calling SendMessage, and perhaps SendMessage will not be frequent. In that case, if the websocket is closed, Chrome would be able to cleanup resources associated with the websocket, but the plugin would not (unless it did some kind of polling to check the status). One idea might be to add some kind of function that only completes when the ready-state changes or when the close status changes. -Darin > > > On 2011/11/01 17:40:53, darin wrote: > >> Ditto. I'd just get rid of this in favor of using different error >> > codes to > >> distinguish between different flavors of closing. >> > > http://codereview.chromium.**org/8395037/diff/10001/ppapi/** > api/dev/ppb_websocket_dev.idl#**newcode308<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl#newcode308> > ppapi/api/dev/ppb_websocket_**dev.idl:308: uint64_t GetBufferedAmount([in] > PP_Resource web_socket); > That doesn't seem very performant. Is there an alternative? > > > On 2011/11/01 17:40:53, darin wrote: > >> be mindful that all of these getters will need to be implemented using >> > a cache > >> in the plugin process. this is because we cannot have the plugin send >> synchronous IPCs to the webkit main thread. >> > > http://codereview.chromium.**org/8395037/<http://codereview.chromium.org/8395... >
Inline... On Tue, Nov 1, 2011 at 11:21 AM, Darin Fisher <darin@chromium.org> wrote: > > > On Tue, Nov 1, 2011 at 11:08 AM, <cstefansen@google.com> wrote: > >> More questions and comments to Darin's thoughts. :-) >> >> >> >> http://codereview.chromium.**org/8395037/diff/10001/ppapi/** >> api/dev/ppb_websocket_dev.idl<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl> >> File ppapi/api/dev/ppb_websocket_**dev.idl (right): >> >> http://codereview.chromium.**org/8395037/diff/10001/ppapi/** >> api/dev/ppb_websocket_dev.idl#**newcode121<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl#newcode121> >> ppapi/api/dev/ppb_websocket_**dev.idl:121: int32_t >> SetMessageCallback([in] >> PP_Resource web_socket, >> This slightly incongruent with the JS version which has you register >> event handlers (e.g., onmessage), but I think I like it – particularly >> given that callbacks are one-shot, as you mention, so it won't work to >> register a general onmessage callback. >> >> >> On 2011/11/01 17:40:53, darin wrote: >> >>> Have you considered just passing a PP_CompletionCallback parameter to >>> ReceiveMessage? Maybe you are concerned about the fact that >>> >> ReceiveMessage >> >>> takes out-parameters? Those would need to remain valid until the >>> >> completion >> >>> callback runs. >>> >> >> Note: completion callbacks are one-shot. Also, by placing the >>> >> completion >> >>> callback on the method that might take time to complete, you create an >>> >> API that >> >>> can either work asynchronously on the main thread (completion callback >>> >> is >> >>> non-null) or synchronously on a background thread (completion callback >>> >> is null). >> >> http://codereview.chromium.**org/8395037/diff/10001/ppapi/** >> api/dev/ppb_websocket_dev.idl#**newcode156<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl#newcode156> >> ppapi/api/dev/ppb_websocket_**dev.idl:156: int32_t SetCloseCallback([in] >> PP_Resource web_socket, >> How would you know that the connection closed if you didn't initiate the >> closure? Polling for status is not desirable, and you will often keep >> the connection open for some time without sending or receiving, so you >> are not necessarily calling those functions either. > > > Hmm... PPB_URLLoader has the same "problem"... You cannot find out if it > is closed until you try to use it. > My worry is that some apps would be sensitive to a delay in notification. E.g., if my connection fails because of a hiccup, I want the app (think a networked game or streaming app) to try to reestablish the connection immediately, rather than waiting until I need it and thus potentially incurring a noticeable TCP establishment delay. In this respect the protocol may be more sensitive that the URLLoader. > > It seems like many users of a websocket will have a call to > ReceiveMessage(..., completion_callback) in flight. This would fail when > the websocket is closed. > Right, that probably covers the majority of the use cases. > > Perhaps some users will only call ReceiveMessage after calling > SendMessage, and perhaps SendMessage will not be frequent. In that case, > if the websocket is closed, Chrome would be able to cleanup resources > associated with the websocket, but the plugin would not (unless it did some > kind of polling to check the status). > > One idea might be to add some kind of function that only completes when > the ready-state changes or when the close status changes. > That could work. Alternatively – and a bit ugly maybe – one should just always have a ReceiveMessage call in flight, even if the app never actually does anything other than SendMessage. Cheers, Christian > -Darin > > > >> >> >> On 2011/11/01 17:40:53, darin wrote: >> >>> Ditto. I'd just get rid of this in favor of using different error >>> >> codes to >> >>> distinguish between different flavors of closing. >>> >> >> http://codereview.chromium.**org/8395037/diff/10001/ppapi/** >> api/dev/ppb_websocket_dev.idl#**newcode308<http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket_dev.idl#newcode308> >> ppapi/api/dev/ppb_websocket_**dev.idl:308: uint64_t >> GetBufferedAmount([in] >> PP_Resource web_socket); >> That doesn't seem very performant. Is there an alternative? >> >> >> On 2011/11/01 17:40:53, darin wrote: >> >>> be mindful that all of these getters will need to be implemented using >>> >> a cache >> >>> in the plugin process. this is because we cannot have the plugin send >>> synchronous IPCs to the webkit main thread. >>> >> >> http://codereview.chromium.**org/8395037/<http://codereview.chromium.org/8395... >> > > -- Christian Stefansen Product Manager Google If you received this communication by mistake, please don't forward it to anyone else (it may contain confidential or privileged information), please erase all copies of it, including all attachments, and please let the sender know it went to the wrong person. Thanks.
http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:24: PP_WEBSOCKETREADYSTATE_CONNECTING = 0, We've had problems in the past when transitioning out of Dev because enum names collide. Maybe we should append _DEV to the enum names? http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:156: int32_t SetCloseCallback([in] PP_Resource web_socket, On 2011/11/01 17:40:53, darin wrote: > Ditto. I'd just get rid of this in favor of using different error codes to > distinguish between different flavors of closing. Another thing to note, though, is that it doesn't make much sense to have >1 completion callback in response to a particular PPB call. At the very least, it would make the return value ambiguous and give the caller several ways to mess up. Are you suggesting having both a 'Connected' and 'Close' callback passed to 'Connect'? I don't think we should. It might make sense to have a PPP interface. That would make it easier to map to the JavaScript API. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:259: [out,size_as=message_byte_count] void message_bytes, Right. The JS spec uses MessageEvent, of which we probably only care about the 'data' field. But it's an 'any'. So it seems like it will be cleaner to use PP_Var, so you can support the same kind of behavior as JS. The buffer of bytes forces the recipient to know how to deserialize the data. It's probably straightforward for Blob or ArrayBuffer, but for anything else, we'd have to specify how we serialized it. Of course, PP_Vars don't actually support ArrayBuffer, Blob, or objects yet :-). But if it's important to get done soon, I can put that on the head of my queue. I don't think it will take too long if I can focus on it. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:262: [out] PP_WebSocketMessageType_Dev message_type); I think based on how I'm reading the spec, we need to provide the origin to the plugin here as well. 5.3: """ Initialize event's origin attribute to the Unicode serialization of the origin of the URL that was passed to the WebSocket object's constructor. """ For PPx_Messaging, we talked about providing origin, but decided not to (at least for now) since we can only load nexes from same-origin. In this case, it appears that WebSocket can open a connection to any server, regardless of origin (or am I reading it wrong?). Since the origin is specified to be the same as what was passed to the constructor, maybe we can omit it? http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:350: * specified WebSocket connection. It would probably be good to mention that this is always a string. You might also mention that it will currently always be the empty string (as the WebSocket spec states) http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:355: * @return Returns a <code>PP_VARTYPE_NULL</code> var if called before the The spec says "must initially return the empty string" before the connection is established. We should probably do the same, right? http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:368: * @return Returns a <code>PP_VARTYPE_NULL</code> var if called before the Same here, I would suggest returning the empty string to match the spec.
Thank you for comments. I'll upload revised API definition, soon. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:10: M16 = 0.1 I guess IDL generator doesn't support M17 yet. First, I try to define it as M17, but generator failed to generate C header. Who should I ping about generator's M17 support? http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:24: PP_WEBSOCKETREADYSTATE_CONNECTING = 0, On 2011/11/01 21:03:24, dmichael wrote: > We've had problems in the past when transitioning out of Dev because enum names > collide. Maybe we should append _DEV to the enum names? Done. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:103: int32_t SetOpenCallback([in] PP_Resource web_socket, I think Darin's suggest is good for IDL and C interface. I could build JS like API on C++ using these C interfaces and it will satisfy Christian's requirement. So I'll revert to previous API definition which didn't have these Set*Callback APIs. And I'll add CompletionCallback to Connect. I guess I can use ReceiveMessage's completion callback for C++ API to provide JS like interfaces. Continuously call ReceiveMessage as async on background. In this manner, we can notice frame receiving and unexpected close just in time. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:121: int32_t SetMessageCallback([in] PP_Resource web_socket, See comment at SetOpenCallback. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:138: int32_t SetErrorCallback([in] PP_Resource web_socket, See comment at SetOpenCallback. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:156: int32_t SetCloseCallback([in] PP_Resource web_socket, See comment at SetOpenCallback. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:259: [out,size_as=message_byte_count] void message_bytes, Darin, The only reason why I avoid to use PP_Var is that PP_Var currently doesn't support ArrayBuffer and Blob. But, if Dave can fix it, I'll decide to use PP_Var. Dave, I'm happy if you focus on updating PP_Var implementation. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:262: [out] PP_WebSocketMessageType_Dev message_type); In JS API, Chrome/WebKit appends origin field to the opening handshake automatically. Then server decides if the specified origin is acceptable or not. Only if server accept, the connection is established. For Chrome Extension, origin will be the extension ID in URI format. So, the origin field of MessageEvent is independent from connection limitation. I think this field exists just for compatibility to HTML5's common MessageEvent definition. We can omit it. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:292: [in] int32_t message_byte_count, Yes. I'd like to use PP_Var. Please increase the priority. Thanks! http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:308: uint64_t GetBufferedAmount([in] PP_Resource web_socket); I see. Do you have any plan to support synchronous IPC? I need time to consider on following synchronous APIs. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:350: * specified WebSocket connection. Oh, I see. That will be better. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:355: * @return Returns a <code>PP_VARTYPE_NULL</code> var if called before the Yes. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:368: * @return Returns a <code>PP_VARTYPE_NULL</code> var if called before the Ditto. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:396: PP_Var GetUrl([in] PP_Resource web_socket); Done. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... File ppapi/c/dev/ppb_websocket_dev.h (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:239: * <code>message_byte_count</code> is 0. This header file is generated from IDL automatically, so I think it would be better to comment to IDL itself. Anyway, I make responses here. Calling with message_byte_count = 0 can be used to know the incoming next frame size. First, call it with empty buffer to know the size of frame, then allocate a buffer with the size. Finally, we can get the whole one frame data by one call. Win32 API sometime prefer such kind of API to receive unknown size data via API. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:243: * Likewise, the first sequence of the bytes returned for the subsequent call Now, I think we must define minimum C API which is necessary and sufficient. We can define convenient C++ API over them. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:272: PP_WebSocketMessageType_Dev* message_type); OK. I'll take care on it for next revision. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:293: * Returns <code>PP_ERROR_FAILED</code> corresponding JavaScript On 2011/11/01 17:58:15, cstefansen wrote: > Nit: corresponding -> corresponding to Done. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:294: * InvalidStateError and <code>PP_ERROR_BADARGUMENT</code> corresponding On 2011/11/01 17:58:15, cstefansen wrote: > Same here. Done. http://codereview.chromium.org/8395037/diff/10001/ppapi/c/dev/ppb_websocket_d... ppapi/c/dev/ppb_websocket_dev.h:309: * release. On 2011/11/01 17:58:15, cstefansen wrote: > Could you elaborate? Done.
http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:10: M16 = 0.1 On 2011/11/02 09:23:42, toyoshim wrote: > I guess IDL generator doesn't support M17 yet. > First, I try to define it as M17, but generator failed to generate C header. > Who should I ping about generator's M17 support? noelallen http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:103: int32_t SetOpenCallback([in] PP_Resource web_socket, On 2011/11/02 09:23:42, toyoshim wrote: > I think Darin's suggest is good for IDL and C interface. > I could build JS like API on C++ using these C interfaces and it will satisfy > Christian's requirement. I'm not sure what you mean, but it might be interesting to give it a shot. You could also make the C++ look a lot like the JavaScript (if desired) by adding a PPP interface with something like DidClose, DidReceiveMessage, DidReceiveError. The C++ class could then allow end-developers to override appropriate virtual methods. It might be more straightforward, if mirroring the JS API is important to you. > > So I'll revert to previous API definition which didn't have these Set*Callback > APIs. > And I'll add CompletionCallback to Connect. I think that's a good approach too, though it has the downside that Christian mentioned, and it might be more difficult to make the C++ look like the JavaScript API. > I guess I can use ReceiveMessage's completion callback for C++ API to provide JS > like interfaces. Continuously call ReceiveMessage as async on background. In > this manner, we can notice frame receiving and unexpected close just in time. What do you mean by 'async on background'? Are you going to chain ReceiveMessage calls each time the completion callback is invoked? Bear in mind it might be tricky to make completion callbacks in a C++ wrapper in a thread-safe way. We currently don't have threading or synchronization primitives available to the C++ layer. We might need to add them anyway, but it feels like overkill for this if there are other ways. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:262: [out] PP_WebSocketMessageType_Dev message_type); On 2011/11/02 09:23:42, toyoshim wrote: > So, the origin field of MessageEvent is independent from connection limitation. > I think this field exists just for compatibility to HTML5's common MessageEvent > definition. We can omit it. I suspect you're right, that it's not needed. I bring it up because the MessageEvent definition in the Communications spec requires origin to be provided for server-sent events: http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#dom-me... Message events sent to Workers do leave origin as the empty string, but that's what the spec requires for events that are neither server-sent nor cross-document. So... we can consider deviating from the spec, since it really does seem redundant in this case. But I want to get a third opinion (darin?) http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:308: uint64_t GetBufferedAmount([in] PP_Resource web_socket); On 2011/11/02 09:23:42, toyoshim wrote: > I see. > Do you have any plan to support synchronous IPC? > > I need time to consider on following synchronous APIs. We try to avoid using synchronous IPC in the proxy. But it seems in this case that you don't need it, and the performance should be fine. What you want to do is have the host side push data to the plugin side, where the plugin side of the proxy will cache it. When a plugin calls GetBufferedAmount, the proxy just asks its local buffer and returns without using IPC. I think it should be fine.
http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:103: int32_t SetOpenCallback([in] PP_Resource web_socket, Personally, I think having an API that is natural and idiomatic in C and C++ is more important than mirroring JS exactly. On 2011/11/02 16:14:40, dmichael wrote: > On 2011/11/02 09:23:42, toyoshim wrote: > > I think Darin's suggest is good for IDL and C interface. > > I could build JS like API on C++ using these C interfaces and it will satisfy > > Christian's requirement. > I'm not sure what you mean, but it might be interesting to give it a shot. You > could also make the C++ look a lot like the JavaScript (if desired) by adding a > PPP interface with something like DidClose, DidReceiveMessage, DidReceiveError. > The C++ class could then allow end-developers to override appropriate virtual > methods. It might be more straightforward, if mirroring the JS API is important > to you. > > > > So I'll revert to previous API definition which didn't have these Set*Callback > > APIs. > > And I'll add CompletionCallback to Connect. > I think that's a good approach too, though it has the downside that Christian > mentioned, and it might be more difficult to make the C++ look like the > JavaScript API. > > I guess I can use ReceiveMessage's completion callback for C++ API to provide > JS > > like interfaces. Continuously call ReceiveMessage as async on background. In > > this manner, we can notice frame receiving and unexpected close just in time. > What do you mean by 'async on background'? Are you going to chain ReceiveMessage > calls each time the completion callback is invoked? > > Bear in mind it might be tricky to make completion callbacks in a C++ wrapper in > a thread-safe way. We currently don't have threading or synchronization > primitives available to the C++ layer. We might need to add them anyway, but it > feels like overkill for this if there are other ways.
Sorry for late response because of a Japanese holiday. I revised C API and add two kinds of C++ API samples. C++ API doesn't contain any comments yet. But I'd like to get some comments on them. The first one uses virtual function. I guess this is usual case with Pepper API. The second one defines delegate class. I think it bothers developers to design classes which inherit WebSocket_Dev for each connections. Using delegate class make it possible that developers collect send and receive handling for multiple WebSocket objects into one class implementation. Of course, developers can define the class which provide their own delegate class in the first case. In parallel, I try to make another change which adds a simple Hello API to understand how Pepper API works. For now, I investigate an issue that I can not get my defining browser interface from my unit tests. I'd be happy if anyone could give advice on that. http://codereview.chromium.org/8414054/ http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:10: M16 = 0.1 Thanks. I pinged him with change http://codereview.chromium.org/8460004/. http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:103: int32_t SetOpenCallback([in] PP_Resource web_socket, Thank you for advices. I understand that my previous Set*Callback APIs must be implemented by PPP interface. But, I'd like to just cancel these APIs now. About 'async on background', Dave's surmise is right. I meant the chain ReceiveMessage calls. By the way, I didn't understand which thread invoke each callback for now. To my understanding, calling a pepper API (except for core API?) from non-main thread in NaCl cause crash. So every valid API call must be done in main thread. The main thread in NaCl seems to work in event driven model like JavaScript. So, I guess the invoking callback task will be queued into main thread's event pool, then main thread picks up and executes the callback function in main thread context. Is this understanding right? http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:308: uint64_t GetBufferedAmount([in] PP_Resource web_socket); OK, I'll try to implement it without IPC. I think I must increase the number of local buffer at SendMessage(), and decrease it by the host pushing notification. Because I must assure that an immediate GetBufferAmount() call after SendMessage() returns the value including sending data size just now. WebSocket users must take care that the bufferedAmount keep smaller value than the limit value which depends browser implementation. I should not return a mis-underestimated value.
Now I resolved the following issue myself. I'll continue to develop it to understand Pepper API and finish WebSocket API implementation as soon as possible after this IDL review is done. > In parallel, I try to make another change which adds a simple Hello API to > understand how Pepper API works. For now, I investigate an issue that I can not > get my defining browser interface from my unit tests. > I'd be happy if anyone could give advice on that. > http://codereview.chromium.org/8414054/
http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/10001/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:103: int32_t SetOpenCallback([in] PP_Resource web_socket, On 2011/11/04 08:45:55, toyoshim wrote: > Thank you for advices. > I understand that my previous Set*Callback APIs must be implemented by PPP > interface. But, I'd like to just cancel these APIs now. > > About 'async on background', Dave's surmise is right. I meant the chain > ReceiveMessage calls. > > By the way, I didn't understand which thread invoke each callback for now. > > To my understanding, calling a pepper API (except for core API?) from non-main > thread in NaCl cause crash. This is true for now, but I'm (slowly) working on fixing that. All the APIs need to be designed so that they can be called from background threads (even if it's not supported right away). > So every valid API call must be done in main thread. > The main thread in NaCl seems to work in event driven model like JavaScript. So, > I guess the invoking callback task will be queued into main thread's event pool, > then main thread picks up and executes the callback function in main thread > context. > Is this understanding right? Yes, I was a little confused. CompletionCallbacks will always be invoked on the main thread. And I suppose that I'm going to have to support the creation of CompletionCallback off of the main thread as part of the other threading work anyway, so I think what you're describing is feasible. It occurs to me that there might be some unfortunate performance implications of doing it this way. By having the API require chaining the calls, we're adding an RPC round-trip for each frame, since the plugin has to ask for each frame by calling ReceiveMessage. It might be better if the server can just keep pushing them to a PPP interface. Or am I misunderstanding something? http://codereview.chromium.org/8395037/diff/24001/ppapi/cpp/dev/websocket_dev.h File ppapi/cpp/dev/websocket_dev.h (right): http://codereview.chromium.org/8395037/diff/24001/ppapi/cpp/dev/websocket_dev... ppapi/cpp/dev/websocket_dev.h:64: Var GetURL(); The delegate pattern does seem nice here. If there's a downside, I think it's that it is not at all tied in to the reference counting. The plugin won't know whether the Delegate is still valid. I guess it will assume the Delegate survives as long as the WebSocket_Dev object. But there's not an easy way for the delegate to get deleted at the right time, unless you inherit from WebSocket_Dev anyway. What do you think?
Update C++ API. Is this ready for the final IDL review? http://codereview.chromium.org/8395037/diff/24001/ppapi/cpp/dev/websocket_dev.h File ppapi/cpp/dev/websocket_dev.h (right): http://codereview.chromium.org/8395037/diff/24001/ppapi/cpp/dev/websocket_dev... ppapi/cpp/dev/websocket_dev.h:64: Var GetURL(); OK, I see. Actually, it seems to be difficult that we define how the delegate's lifetime must be handled as you mentioned. I use the first one which use virtual functions.
It looks like we're ready for a final round and hopefully LGTM. Darin, Dave, do you want to do the honors? On Mon, Nov 7, 2011 at 12:35 AM, <toyoshim@chromium.org> wrote: > Update C++ API. > Is this ready for the final IDL review? > > > > http://codereview.chromium.**org/8395037/diff/24001/ppapi/** > cpp/dev/websocket_dev.h<http://codereview.chromium.org/8395037/diff/24001/ppapi/cpp/dev/websocket_dev.h> > File ppapi/cpp/dev/websocket_dev.h (right): > > http://codereview.chromium.**org/8395037/diff/24001/ppapi/** > cpp/dev/websocket_dev.h#**newcode64<http://codereview.chromium.org/8395037/diff/24001/ppapi/cpp/dev/websocket_dev.h#newcode64> > ppapi/cpp/dev/websocket_dev.h:**64: Var GetURL(); > OK, I see. > Actually, it seems to be difficult that we define how the delegate's > lifetime must be handled as you mentioned. > I use the first one which use virtual functions. > > http://codereview.chromium.**org/8395037/<http://codereview.chromium.org/8395... > -- Christian Stefansen Product Manager Google If you received this communication by mistake, please don't forward it to anyone else (it may contain confidential or privileged information), please erase all copies of it, including all attachments, and please let the sender know it went to the wrong person. Thanks.
http://codereview.chromium.org/8395037/diff/28002/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/28002/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:179: [in] PP_CompletionCallback callback); I mentioned before that I was a little worried about the performance of doing the ReceiveMessage this way. Because you have to ask for the next message (over IPC) after you've received a frame, you're guaranteeing that you have to wait for an IPC round-trip between frames. With a PPP interface, you could always push to the plugin without forcing that overhead between frames. I guess the positive side to the API as-written is that the browser gets a better idea of whether the plugin is keeping up. I was hoping Darin would comment, since he has a lot of experience with networking stuff :-/ I guess there's nothing in the API preventing the proxy from pushing data and having it ready right away when ReceiveMessage is called, so maybe my concern isn't really valid. http://codereview.chromium.org/8395037/diff/28002/ppapi/ppapi_cpp.gypi File ppapi/ppapi_cpp.gypi (right): http://codereview.chromium.org/8395037/diff/28002/ppapi/ppapi_cpp.gypi#newcode80 ppapi/ppapi_cpp.gypi:80: 'c/dev/ppb_websocket_dev.h', You haven't added (nor implemented) the C++ version yet. Is that intentional?
http://codereview.chromium.org/8395037/diff/28002/ppapi/api/dev/ppb_websocket... File ppapi/api/dev/ppb_websocket_dev.idl (right): http://codereview.chromium.org/8395037/diff/28002/ppapi/api/dev/ppb_websocket... ppapi/api/dev/ppb_websocket_dev.idl:179: [in] PP_CompletionCallback callback); Receiving data could be automatically transmitted via proxy/ipc to the pepper's in-process buffer using the WebSocketChannelClient interface and so on. So I think IPC round-trip could not happens usually. If data is available, it returns the buffered data immediately without IPC, otherwise the callback is invoked by half round-trip ipc and the next method call can return the received data immediately. If data is not available yet, the callback tell that http://codereview.chromium.org/8395037/diff/28002/ppapi/ppapi_cpp.gypi File ppapi/ppapi_cpp.gypi (right): http://codereview.chromium.org/8395037/diff/28002/ppapi/ppapi_cpp.gypi#newcode80 ppapi/ppapi_cpp.gypi:80: 'c/dev/ppb_websocket_dev.h', Yes. I experimentally add C++ interface, but it doesn't include implementation and actually its interface is not used yet. I'll add it later with implementation in another change.
lgtm Okay, makes sense, thanks for explaining and referencing the design doc in that other thread.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/8395037/28002
Change committed as 109592 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
