Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8956008:
WebSocket Pepper API: C++ utility class implementation. (Closed)
|
Created:
9 years ago by Takashi Toyoshima Modified:
8 years, 9 months ago CC:
chromium-reviews, piman+watch_chromium.org, ihf+watch_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWebSocket Pepper API: an utility class implementation.
This class privide JS binding like API to Pepper C++ developers.
BUG=87310
TEST=ui_tests --gtest_filter='PPAPI*Test.Websocket_Utility*'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115093
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124238
Patch Set 1 #Patch Set 2 : remove C++ bindings related change #
Total comments: 20
Patch Set 3 : fix reviewed points #
Total comments: 1
Patch Set 4 : rebase #Patch Set 5 : fix flakiness on mac #
Total comments: 2
Patch Set 6 : add comments on server behaviors #
Total comments: 2
Patch Set 7 : fix nit #Patch Set 8 : rebase #Patch Set 9 : consumes optional callbacks #Patch Set 10 : fix completion callback leaks? #
Total comments: 8
Patch Set 11 : rebase #
Total comments: 1
Patch Set 12 : remove helper namespace #Patch Set 13 : remove meaningless condition check #Patch Set 14 : apply patch set 13 to trunk in order to resume #Patch Set 15 : add unit tests #
Total comments: 14
Patch Set 16 : rebase #Patch Set 17 : rebase (out of dev) #Patch Set 18 : revise on comments at Patch Set 15 #
Total comments: 10
Patch Set 19 : reflect review comments #
Total comments: 2
Patch Set 20 : rebase #Patch Set 21 : fix unit test's virtual function names #Patch Set 22 : ready for landing #
Messages
Total messages: 32 (0 generated)
Hi, guys. As we discussed in C++ bindings CL, I prepare helper class for WebSocket. This change contains debatable matters. For now, I define pp::helper namespace for my helper class implementation, and place them into ppapi/cpp/helper/dev/. Do you have any good idea for that?
Who did you want to review the test details? I didn't look at that. The helper directory & namespace seem OK for now. Next year I plan on organizing a clear system for the different stuff we have, so I think we can live with anything reasonable now. http://codereview.chromium.org/8956008/diff/4001/ppapi/cpp/helper/dev/websock... File ppapi/cpp/helper/dev/websocket_api_dev.cc (right): http://codereview.chromium.org/8956008/diff/4001/ppapi/cpp/helper/dev/websock... ppapi/cpp/helper/dev/websocket_api_dev.cc:31: uint32_t protocol_count) { Style nit: in this case, indent this line to be under the "const" (same in similar cases below).
> Who did you want to review the test details? I didn't look at that. These tests are almost the same as the tests for old C++ binding. So, David looked over them once, I think. If we need another reviewer who is familier with WebSocket protocol stack and JS binding, I'll add yutak@.
http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.cc File ppapi/tests/test_websocket.cc (right): http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:52: EventType type, bool was_clean, uint16_t close_code, const pp::Var& var) style nit: We don't normally do the parameters this way. I think we usually prefer 1-per-line in a declaration, when they don't fit on 1 line together. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:63: class TestWebSocketAPI : public pp::helper::WebSocketAPI_Dev { I don't know how I feel about the "helper" namespace. Maybe we could just stick these classes in pp with the others? http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:76: events_.push_back(WebSocketEvent(WebSocketEvent::EVENT_OPEN, style nit: I would put the WebSocketEvent creation on the next line. Ditto for the ones below. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:541: // C++ bindings is simple straightforward, then just verifies interfaces work nit: rephrase, maybe as: The C++ bindings are straightforward. This just verifies that the bindings work fine as an interface bridge. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:558: connect_callback); style nit: I'd break the line elsewhere, maybe after the "=" or ws.Connect(. The way it is, it's not obvious at a glance where connect_callback is passed. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:640: ASSERT_EQ(WebSocketEvent::EVENT_CLOSE, events[1].event_type); Can you add a comment to explain why you expect it to get an error and close immediately (assuming that's the correct behavior)? I see you're passing "good_protocols", which makes me think you would expect it to succeed. Is it because the server doesn't expect them? http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:673: Do you need to clean up by closing the socket? http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:700: } Ditto, do you need to clean up by really closing the socket? http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:739: ASSERT_EQ(WebSocketEvent::EVENT_ERROR, events[0].event_type); I think it's going to be possible when you implement out-of-process that the Connect could succeed, so you might want to allow for an EVENT_OPEN event as the first event to avoid a flaky test.
Thank you for review. I updated my change. http://codereview.chromium.org/8956008/diff/4001/ppapi/cpp/helper/dev/websock... File ppapi/cpp/helper/dev/websocket_api_dev.cc (right): http://codereview.chromium.org/8956008/diff/4001/ppapi/cpp/helper/dev/websock... ppapi/cpp/helper/dev/websocket_api_dev.cc:31: uint32_t protocol_count) { On 2011/12/15 16:54:44, brettw wrote: > Style nit: in this case, indent this line to be under the "const" (same in > similar cases below). Done. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.cc File ppapi/tests/test_websocket.cc (right): http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:52: EventType type, bool was_clean, uint16_t close_code, const pp::Var& var) On 2011/12/15 17:50:54, dmichael wrote: > style nit: We don't normally do the parameters this way. I think we usually > prefer 1-per-line in a declaration, when they don't fit on 1 line together. Done. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:63: class TestWebSocketAPI : public pp::helper::WebSocketAPI_Dev { Brett agreed with pp::helper namespace for helper class WebSocketAPI_Dev in the former review. So, for now, I keep this namespace as is. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:76: events_.push_back(WebSocketEvent(WebSocketEvent::EVENT_OPEN, On 2011/12/15 17:50:54, dmichael wrote: > style nit: I would put the WebSocketEvent creation on the next line. Ditto for > the ones below. Done. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:541: // C++ bindings is simple straightforward, then just verifies interfaces work Thanks. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:558: connect_callback); Thanks. I break it after ws.Connect( because the character length after "=" is over 80. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:640: ASSERT_EQ(WebSocketEvent::EVENT_CLOSE, events[1].event_type); On 2011/12/15 17:50:54, dmichael wrote: > Can you add a comment to explain why you expect it to get an error and close > immediately (assuming that's the correct behavior)? I see you're passing > "good_protocols", which makes me think you would expect it to succeed. Is it > because the server doesn't expect them? Done. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:673: In this test, I omit Close() because it's not verified function here. Destruction without Close() could be possible. So it will be meaningful as a test for abnormal sequence. The next test verify Close(). After that, TestHelperTextSendReceive() verify total process from Connect() to Close(). http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:700: } Also, I intend to omit it. http://codereview.chromium.org/8956008/diff/4001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:739: ASSERT_EQ(WebSocketEvent::EVENT_ERROR, events[0].event_type); Oh, I see. Thank you for catching this. http://codereview.chromium.org/8956008/diff/8001/ppapi/tests/test_websocket.cc File ppapi/tests/test_websocket.cc (right): http://codereview.chromium.org/8956008/diff/8001/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:744: ASSERT_EQ(WebSocketEvent::EVENT_ERROR, events[0].event_type); Miss to add EVENT_OPEN handling like the next test. I'll add it at the next rebase set.
http://codereview.chromium.org/8956008/diff/8005/ppapi/tests/test_websocket.cc File ppapi/tests/test_websocket.cc (right): http://codereview.chromium.org/8956008/diff/8005/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:795: ASSERT_EQ(WebSocketEvent::EVENT_MESSAGE, events[1].event_type); Why do you receive a message here? Can you document why, if it is correct behavior?
http://codereview.chromium.org/8956008/diff/8005/ppapi/tests/test_websocket.cc File ppapi/tests/test_websocket.cc (right): http://codereview.chromium.org/8956008/diff/8005/ppapi/tests/test_websocket.c... ppapi/tests/test_websocket.cc:795: ASSERT_EQ(WebSocketEvent::EVENT_MESSAGE, events[1].event_type); OK, I added comments on server side handler behavior. http://codereview.chromium.org/8956008/diff/16001/ppapi/tests/test_websocket.cc File ppapi/tests/test_websocket.cc (right): http://codereview.chromium.org/8956008/diff/16001/ppapi/tests/test_websocket.... ppapi/tests/test_websocket.cc:26: // pywebsocket server itself is launched in ppapi_ui_test.cc. Also, I added information on server side handlers. That could be useful for other developers.
lgtm http://codereview.chromium.org/8956008/diff/16001/ppapi/tests/test_websocket.cc File ppapi/tests/test_websocket.cc (right): http://codereview.chromium.org/8956008/diff/16001/ppapi/tests/test_websocket.... ppapi/tests/test_websocket.cc:24: // These server is provided by pywebsocket server side handlers in nit: "These server is"->"These servers are"?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/8956008/19001
Presubmit check for 8956008-19001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Warnings **
The old callback system is deprecated. If possible, use base::Bind and
base::Callback instead.
ppapi/cpp/helper/dev/websocket_api_dev.cc:43
Presubmit checks took 6.0s to calculate.
This warning is not correct. PRESUBMIT.py mistakenly detects the same name function in pepper world as a old callback system. I'll submit this change by dcommit.
Reverts because of memory test failures. Added tests seem to have a memory leak.
I see what's the problem. CallbackData created by NewOptionalCallback never deleted if the function completed synchronously. I could not think of simple way to resolve this issue essentially, and suspect that other usages out of WebSocket might have the same issue. In my case, the simple way to fix it is to use NewCallback, I think.
On 2011/12/20 14:42:01, toyoshim wrote: > I see what's the problem. > CallbackData created by NewOptionalCallback never deleted if the function > completed synchronously. Are you sure? If so, I think that's a bug. It's true, however, that if the callback is never run, memory will leak. This would happen for NewCallback as well. Maybe you need to make sure that if you have any callbacks waiting to be invoked when the WebSocketImpl is destroyed that you invoke them with PP_ERROR_ABORTED (or something). > I could not think of simple way to resolve this issue essentially, and suspect > that > other usages out of WebSocket might have the same issue. > > In my case, the simple way to fix it is to use NewCallback, I think.
Optional callbacks are tricky which is why the default is an "required"
callback. The normal way of using the callback factory with optional callbacks
is:
CompletionCallback cb = factory_.NewOptionalCallback(...);
int32_t result = foo.Connect(cb);
if (result != PP_OK_COMPLETIONPENDING)
cb.Run(result);
This is a bit cumbersome. The only place where you really want this is in
network requests where data may be coming quickly and you can avoid going
through the message loop for every read buffer.
In your case, it's also weird that your API either returns connect
synchronously, *OR* calls DidConnect. I'd make this consistent, since some
errors will be returned synchronously, and some asynchronously, and we don't
really guarantee what happens.
If you use my pattern above, you'll be calling DidConnect reentrantly which is
probably not what the caller expected. So I would just convert this to a
required callback and make Connect return void.
You may want to think if or how the caller can get at the error code from the
connect call.
On Tue, Dec 20, 2011 at 4:13 PM, <brettw@chromium.org> wrote: > Optional callbacks are tricky which is why the default is an "required" > callback. The normal way of using the callback factory with optional > callbacks > is: > CompletionCallback cb = factory_.NewOptionalCallback(.**..); > int32_t result = foo.Connect(cb); > if (result != PP_OK_COMPLETIONPENDING) > cb.Run(result); > > This is a bit cumbersome. The only place where you really want this is in > network requests where data may be coming quickly and you can avoid going > through the message loop for every read buffer. > For this reason, I think it's still worthwhile to use an Optional callback in Receive for ReceiveMessage. That way, if there are more frames to receive, you can pull them out quickly without the loop overhead. The client won't be called re-entrantly in this case, since it's only invoked by other callbacks (DidConnect or DidReceive). But yeah, it probably does make sense to make the rest not "Optional". > > In your case, it's also weird that your API either returns connect > synchronously, *OR* calls DidConnect. I'd make this consistent, since some > errors will be returned synchronously, and some asynchronously, and we > don't > really guarantee what happens. > > If you use my pattern above, you'll be calling DidConnect reentrantly > which is > probably not what the caller expected. So I would just convert this to a > required callback and make Connect return void. > > You may want to think if or how the caller can get at the error code from > the > connect call. > > http://codereview.chromium.**org/8956008/<http://codereview.chromium.org/8956... >
Thank you guys. As you guys said, ReceiveMessage must use optional callback for performance optimization. In the cases of Connect and Close, still I'd like to use optional callback. Because I'd like to map exceptions in JavaScript to synchronous error. In JS bindings, the caller know successful connection establishment by OnConnect, or failures by OnError and OnClose. This class is a helper class which looks like JS bindings. So I think it could be reasonable.
It's not possible to map JS exceptions to synchronous errors. The plugin and renderer are running out of sync with each other, so any return from the page should be async. We currently have some things which are synchronous. We're trying to remove these because there is a very large performance penalty for them (say the renderer is doing something complicated or garbage collecting, everything will hang until that's complete).
The errors corresponding to JS exceptions are verified in webkit/plugins/ppapi/ppb_websocket_impl.cc, and will be moved to shared_impl/. So, I think I can return these errors without any IPC, then make them synchronous.
Valgrind report leaks of CompletionCallback still exist. I guess internal ReceiveMessage()'s callback cause leaks. I'll add callback aborting in dtor in ppb_websocket_impl.
Sorry this review is taking so long. Since this is the first such "helper" class there isn't a good example to go on and unfortunately it will take some time to settle. Thanks for putting up with the feedback! http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... File ppapi/cpp/helper/dev/websocket_api_dev.cc (right): http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.cc:18: namespace helper { There was a thread on the Pepper mailing list and we decided to just use the namespace "pp" (no "helper" needed) for this type of classes. http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.cc:33: callback_factory_.NewOptionalCallback(&Implement::DidConnect); Sorry if I wasn't clear in my email, I think we want a required callback here so you don't run the callback directly. This is to avoid reentering the user of the object. Imagine you're writing calling code that calls Connect or Close. You may not necessarily expect that your OnOpen call will be called from within that callstack. Depending on how you wrote your code, you may not be set up to handle a OnOpen call. This is why we have "required" callbacks rather than just making the underlying Pepper implementation always call the completion callback as soon as anything fails. Required callbacks go back to the message loop so your handlers don't get called re-entrantly. With optional callbacks, you have to handle the "synchronous" case explicitly. This is the annoying part of them, but it also means that the caller calls into their code explicitly which makes this type of error very difficult to make. http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.cc:90: api_->OnError(); Maybe we can sent the Pepper error value to OnError? Otherwise it will get lost when everything is async. http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... File ppapi/cpp/helper/dev/websocket_api_dev.h (right): http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.h:108: virtual void OnOpen() = 0; My expectation for this would be that the plugin code would "have" a WebSocket that would then call you back via some delegate API. Mixing everything together would force the calling code to make a wrapper object that derives from WebSocket, which seems a bit weird to me. If we do this, it may be a good idea to rename the functions with "WebSocket" in them and pass the calling object (since you could have more than one socket) so that the caller would look like this: class MyInstance : public pp::Instance, public pp::WebSocket::Delegate { void StartSocket() { // Pass in the WebSocket::Delegate to the connection function for it to call us back. my_socket.Connect(this, "http://foo.com", ...); } virtual void OnWebSocketOpen(WebSocket* socket); virtual void OnWebSocketMessage(WebSocket* socket, const Var& message); virtual void OnWebSocketClose(WebSocket* socket); pp::WebSocket my_socket; }; Sometimes mapping these concepts into C++ can be challenging because there are no closures. Please let me know what you think about this.
Thank you for your kind review. Long review is no problem to me, because I understand this helper class approach is still debatable and unstable, and I enjoy exploring new pepper world. I'm implementing proxy version in parallel because deeper understanding on proxy could be helpful to work on pepper and think of asynchronous APIs. Thank you. http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... File ppapi/cpp/helper/dev/websocket_api_dev.cc (right): http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.cc:18: namespace helper { OK. I fix it. http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.cc:33: callback_factory_.NewOptionalCallback(&Implement::DidConnect); I understand that you afraid of calling Connect or Close recursively from On* virtual methods. Right? As I added comments below, callback.Run is just for consuming objects allocated by factory class. Actually, I passed PP_ERROR_ABORTED in order to do nothing in actual callback DidConnect and DidClose. Synchronous invoking callbacks is totally hidden from caller, and On* virtual methods are never called in these cases. Then caller has no chance to reenter the object. But, I think also you might feel returning error synchronously is not so suitable for pepper. Behaviors on OnOpen, OnError, and OnClose are defined strictly by The WebSocket API specification. So, I think changing their behaviors on pepper bindings is not better way. If you think I had better to return all errors asynchronously, how about adding other virtual functions like OnOpenException, OnSendException, and OnCloseException. It will not break original API's semantics and fit the pepper style. http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.cc:90: api_->OnError(); To tell the truth, only PP_ERROR_ABORTED and PP_OK are passed to the Close's callback. This is because Close() doesn't have any error originally and incoming close events caused by the peer server or underlying network troubles have code and reason arguments in OnClose() handler. Yes, now I notice that the condition 'result == PP_OK' is meaningless here... http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... File ppapi/cpp/helper/dev/websocket_api_dev.h (right): http://codereview.chromium.org/8956008/diff/26007/ppapi/cpp/helper/dev/websoc... ppapi/cpp/helper/dev/websocket_api_dev.h:108: virtual void OnOpen() = 0; I had the same delegate idea originally and proposed in IDL discussion. http://codereview.chromium.org/8395037#msg16 At that time, of course we have no idea on helper classes, and David thought a delegate approach without reference counting could be a little dangerous in viewpoint of object life cycle management and I agreed that. http://codereview.chromium.org/8395037#msg18 Now, I feel I'd like to revive the delegate approach. Should I add interfaces like AttachEventHandler(Delegate*) and DetachEventHandler(Delegate*) to avoid invoking deleted delegates? I think chromium implement disconnect() interface to detach delegate from object usually. I guess callback factory will take care now, then detach interface is not mandatory. Is my understanding right? In your example, my_socket must be deleted at least MyInstance's destructor. Then factory could know that my_socket's callback shouldn't be invoked after that by its destruction. So, delegates also doesn't be invoked. Queued events will be dispatched after destruction of MyInstance, but CallbackData detects its disappearance by back pointer then dismiss the event. http://codereview.chromium.org/8956008/diff/36005/webkit/plugins/ppapi/ppb_we... File webkit/plugins/ppapi/ppb_websocket_impl.cc (right): http://codereview.chromium.org/8956008/diff/36005/webkit/plugins/ppapi/ppb_we... webkit/plugins/ppapi/ppb_websocket_impl.cc:90: buffered_amount_after_close_(0) { Replace initializers for callbacks by my ongoing change.
BTW, what do you think about changes in ppb_websocket_impl.cc. Outstanding callbacks must be aborted on the object destruction? Or must not be invoked after starting to call its destructor? I feel invoking callbacks in destructor is not so polite, but never called callbacks which created via factory cause object memory leaks. If aborting them is better, I'll split aborting change to another one for quick landing. http://codereview.chromium.org/9024005
Hi, I'd like to resume my helper class review. I apply the last patch set to trunk and fix design in order to fit to the latest utility design. If you have an idea about implementation priority, please let me know. For example, I must implement out of proxy ahead of helper class, etc. Thank you.
http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.cc (right): http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:26: ~Implement() {} style nit: virtual http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:34: if (result != PP_OK_COMPLETIONPENDING) { PP_OK is not possible here, right? If this class depends on that fact, it should maybe be documented at the C level. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:37: callback.Run(PP_ERROR_ABORTED); Do all your errors that are reported immediately have PP_ERROR_ABORTED for the code? If not, you're throwing away information that might be useful to the caller, right? Should you pass them the real error code? http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:46: if (result != PP_OK_COMPLETIONPENDING) { ditto about documenting that PP_OK won't happen. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.h (right): http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:23: WebSocketAPI(Instance* instance); explicit http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:132: virtual void OnOpen() = 0; In PPAPI, we use "Did" or "HandleBlah" instead of "OnBlah". So probably: DidOpen HandleMessage (hmm, same name as in the PPB_Messaging interface. Maybe that's a good thing; they're basically the same thing from different sources) HandleError DidClose http://codereview.chromium.org/8956008/diff/41001/webkit/plugins/ppapi/ppb_we... File webkit/plugins/ppapi/ppb_websocket_impl.cc (right): http://codereview.chromium.org/8956008/diff/41001/webkit/plugins/ppapi/ppb_we... webkit/plugins/ppapi/ppb_websocket_impl.cc:432: websocket_->setBinaryType(binary_type_); This looks like leftovers from another CL?
Thanks. We had out of dev changes recently, so first I upload rebase version as patch set 17. I'm sorry if you feel inconvenience on that. I reflect your review comments in patch set 18. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.cc (right): http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:26: ~Implement() {} On 2012/02/01 23:05:18, dmichael wrote: > style nit: virtual Done. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:34: if (result != PP_OK_COMPLETIONPENDING) { Yes. In successful cases it always returns asynchronous PP_OK_COMPLETIONPENDING. But, this implementation doesn't expect this implicit condition. When Connect() returns PP_OK, callback.Run(PP_ERROR_ABORTED) is invoked, but callback just ignore this invocation and do nothing, then returns PP_OK at following "return result" next of this if block. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:37: callback.Run(PP_ERROR_ABORTED); Caller catches the error code as a return value of Connect() for synchronous case, and does as an argument of DidClose() for asynchronous case. This callback.Run invoke WebSocketAPI::Implement::DidConnect() and nothing will be done there. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:46: if (result != PP_OK_COMPLETIONPENDING) { Ditto. I revise these comments to describe more clearly. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.h (right): http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:23: WebSocketAPI(Instance* instance); Oops, sorry I missed again. http://codereview.chromium.org/8956008/diff/41001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:132: virtual void OnOpen() = 0; OK. I also reorder these methods. Maybe DidOpen, DidClose, HandleMessage, and HandleError looks better order. http://codereview.chromium.org/8956008/diff/41001/webkit/plugins/ppapi/ppb_we... File webkit/plugins/ppapi/ppb_websocket_impl.cc (right): http://codereview.chromium.org/8956008/diff/41001/webkit/plugins/ppapi/ppb_we... webkit/plugins/ppapi/ppb_websocket_impl.cc:432: websocket_->setBinaryType(binary_type_); Yes. This difference was removed in the next patch set 16.
http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.cc (right): http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:35: // In synchronous cases, sonsumes callback here and invokes callback nit: sonsumes->consumes http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:38: // any deligate virtual method. deligate->delegate http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:49: // In synchronous cases, sonsumes callback here and invokes callback sonsumes->consumes http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:52: // any deligate virtual method. deligate->delegate http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.h (right): http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:105: /// DidOpen() is invoked when the connection is established by Connect(). Sorry for being wishy-washy about the names. Since this is meant to be inherited from, maybe they should be more specific to avoid clashes or confusion in derived classes? In particular, I like "HandleMessage" because it is consistent, but it is exactly the same name and signature as the one in pp::Instance for receiving messages from JavaScript. What do you think of: SocketDidOpen SocketDidClose HandleSocketMessage HandleSocketError ?
http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.cc (right): http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:35: // In synchronous cases, sonsumes callback here and invokes callback On 2012/02/14 17:59:57, dmichael wrote: > nit: sonsumes->consumes Done. http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:38: // any deligate virtual method. On 2012/02/14 17:59:57, dmichael wrote: > deligate->delegate Done. http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:49: // In synchronous cases, sonsumes callback here and invokes callback On 2012/02/14 17:59:57, dmichael wrote: > sonsumes->consumes Done. http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.cc:52: // any deligate virtual method. On 2012/02/14 17:59:57, dmichael wrote: > deligate->delegate Done. http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.h (right): http://codereview.chromium.org/8956008/diff/55002/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:105: /// DidOpen() is invoked when the connection is established by Connect(). I agree with the namespace issue on inheritance. I prefer WebSocket than just Socket as a signature word because Socket is a little generic and sometime confusing for WebSocket. 'WebSocket' is a little long word in many situation, but 'Ws' is too short and not so clear. And 'Socket' is confusing.
One more name nitpick... o/w LGTM http://codereview.chromium.org/8956008/diff/66001/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.h (right): http://codereview.chromium.org/8956008/diff/66001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:107: virtual void DidWebSocketOpen() = 0; DidWebSocketOpen is a bit ambiguous. It could be interpreted as a method that answers the question: "Did the WebSocket open?" (which is not what this method does). I think: WebSocketDidOpen and WebSocketDidClose are clearer.
http://codereview.chromium.org/8956008/diff/66001/ppapi/utility/websocket/web... File ppapi/utility/websocket/websocket_api.h (right): http://codereview.chromium.org/8956008/diff/66001/ppapi/utility/websocket/web... ppapi/utility/websocket/websocket_api.h:107: virtual void DidWebSocketOpen() = 0; On 2012/02/24 19:49:36, dmichael wrote: > DidWebSocketOpen is a bit ambiguous. It could be interpreted as a method that > answers the question: > "Did the WebSocket open?" (which is not what this method does). > > I think: > WebSocketDidOpen > and > WebSocketDidClose > are clearer. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/8956008/75001
Change committed as 124238 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
