Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
188007:
Web Socket support: Chromium IPC (Closed)
|
Created:
11 years, 3 months ago by ukai Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Visibility:
Public. |
DescriptionWeb Socket support: Chromium IPC
Add IPC messages used in SocketStreamHandle for Web Sockets.
BUG=12497
TEST=none
Patch Set 1 #
Total comments: 18
Patch Set 2 : SendData is no more sync message #
Total comments: 4
Patch Set 3 : addressed michaeln comment #
Total comments: 18
Patch Set 4 : Addressed yuzo's comment #Patch Set 5 : fix ViewMsg_SocketStrean_DataSent: CONTROL1->CONTROL2' #
Total comments: 15
Patch Set 6 : Fix ViewHostMsg_SocketStream_Connect async #Patch Set 7 : use WebKit::WebData instead of std::vector<char> #
Total comments: 2
Patch Set 8 : revert previous change #Patch Set 9 : s/DataSent/SentData/ #Messages
Total messages: 24 (0 generated)
These IPC messages are used for Web Sockets support. For more detailed design of Web Sockets implementation in chromium, see See http://docs.google.com/View?docID=0AW9YPSbGR0ECZGZtN2dmdmdfMWRtOTdxeGdt. Will follow CLs for - chrome/common/net/socket_stream.h for common constants - net/socket_stream - browser/renderer_host/socket_stream_* - renderer/socket_stream_* - command line switch of --enable-web-sockets - webkit/glue & webkit/api If it's better to merge larger CLs, I'll do it. Thanks.
http://codereview.chromium.org/188007/diff/1/2 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/1/2#newcode704 Line 704: // These messages from the browser to the Socket Stream on a renderer. These are http://codereview.chromium.org/188007/diff/1/2#newcode705 Line 705: // A |socket id| is assigned by ViewHostMsg_SocketStream_Connect. Let's use underscore even for commented-out arguments. http://codereview.chromium.org/188007/diff/1/2#newcode709 Line 709: int /* socket id */) indentation? http://codereview.chromium.org/188007/diff/1/2#newcode711 Line 711: // |Data| is received on the Socket Stream. |data|? Looking at other comments in this file where a parameter appears on the start of the sentence, they're not capitalized. http://codereview.chromium.org/188007/diff/1/2#newcode723 Line 723: int /* socket id */) indentation? http://codereview.chromium.org/188007/diff/1/2#newcode1725 Line 1725: // These messages from the SocketStreamHandle to the browser. These are http://codereview.chromium.org/188007/diff/1/2#newcode1737 Line 1737: // If the browser accpets the request, then set |accepted| to be true and accepts http://codereview.chromium.org/188007/diff/1/2#newcode1751 Line 1751: int /* socket id */) indentation?
http://codereview.chromium.org/188007/diff/1/2 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/1/2#newcode1742 Line 1742: IPC_SYNC_MESSAGE_CONTROL2_1(ViewHostMsg_SocketStream_SendData, Does this really need to be a sync message? Generally we try to avoid sync messages unless absolutely necessary.
http://codereview.chromium.org/188007/diff/1/2 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/1/2#newcode704 Line 704: // These messages from the browser to the Socket Stream on a renderer. On 2009/09/03 11:04:13, tyoshino wrote: > These are Done. http://codereview.chromium.org/188007/diff/1/2#newcode705 Line 705: // A |socket id| is assigned by ViewHostMsg_SocketStream_Connect. On 2009/09/03 11:04:13, tyoshino wrote: > Let's use underscore even for commented-out arguments. Done. http://codereview.chromium.org/188007/diff/1/2#newcode709 Line 709: int /* socket id */) On 2009/09/03 11:04:13, tyoshino wrote: > indentation? Done. http://codereview.chromium.org/188007/diff/1/2#newcode711 Line 711: // |Data| is received on the Socket Stream. On 2009/09/03 11:04:13, tyoshino wrote: > |data|? Looking at other comments in this file where a parameter appears on the > start of the sentence, they're not capitalized. Done. http://codereview.chromium.org/188007/diff/1/2#newcode723 Line 723: int /* socket id */) On 2009/09/03 11:04:13, tyoshino wrote: > indentation? Done. http://codereview.chromium.org/188007/diff/1/2#newcode1725 Line 1725: // These messages from the SocketStreamHandle to the browser. On 2009/09/03 11:04:13, tyoshino wrote: > These are Done. http://codereview.chromium.org/188007/diff/1/2#newcode1737 Line 1737: // If the browser accpets the request, then set |accepted| to be true and On 2009/09/03 11:04:13, tyoshino wrote: > accepts Done. http://codereview.chromium.org/188007/diff/1/2#newcode1742 Line 1742: IPC_SYNC_MESSAGE_CONTROL2_1(ViewHostMsg_SocketStream_SendData, On 2009/09/03 17:38:03, John Abd-El-Malek wrote: > Does this really need to be a sync message? Generally we try to avoid sync > messages unless absolutely necessary. Ok, I think it would be easier to use sync message, but it is not absolutely neccessary and can avoid it. http://codereview.chromium.org/188007/diff/1/2#newcode1751 Line 1751: int /* socket id */) On 2009/09/03 11:04:13, tyoshino wrote: > indentation? Done.
http://codereview.chromium.org/188007/diff/4002/4003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/4002/4003#newcode718 Line 718: IPC_MESSAGE_CONTROL1(ViewMsg_SocketStream_CanSend, Consider calling this 'DataSent' since its delivered in response to a 'SendData' msg from the client. http://codereview.chromium.org/188007/diff/4002/4003#newcode1741 Line 1741: // to send, the connection will be closed. Ouch... what impact will send/sent, sendmore/sentmore handshaking have on network utilization when a script is written to issue a series of socket.send(smallstring) calls? Maybe the 'Connected' msg could contain a 'max_pending_send_allowed' parameter. And the 'DataSent' reply message could contain an 'amount_sent' parameter. The client could keep track of how much it has pending (how much it has requested to be sent) at anytime, and shouldn't go over the max allowed amount. As the server pushes stuff out, it reports that back to the client which decrements its pending count. If the client ever tries to exceed the max allowed amount, the connection will be closed. In this view, there would not necessarily be a 1 to 1 relationship between 'SendData' requests and 'DataSent' replies.
Thanks for review! http://codereview.chromium.org/188007/diff/4002/4003 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/4002/4003#newcode718 Line 718: IPC_MESSAGE_CONTROL1(ViewMsg_SocketStream_CanSend, On 2009/09/05 18:19:51, michaeln wrote: > Consider calling this 'DataSent' since its delivered in response to a 'SendData' > msg from the client. Done. http://codereview.chromium.org/188007/diff/4002/4003#newcode1741 Line 1741: // to send, the connection will be closed. On 2009/09/05 18:19:51, michaeln wrote: > Ouch... what impact will send/sent, sendmore/sentmore handshaking have on > network utilization when a script is written to issue a series of > socket.send(smallstring) calls? > > Maybe the 'Connected' msg could contain a 'max_pending_send_allowed' parameter. > And the 'DataSent' reply message could contain an 'amount_sent' parameter. > > The client could keep track of how much it has pending (how much it has > requested to be sent) at anytime, and shouldn't go over the max allowed amount. > As the server pushes stuff out, it reports that back to the client which > decrements its pending count. If the client ever tries to exceed the max allowed > amount, the connection will be closed. > > In this view, there would not necessarily be a 1 to 1 relationship between > 'SendData' requests and 'DataSent' replies. Thanks for suggestion! It sounds better.
ping.
Sorry, I've missed this somehow. http://codereview.chromium.org/188007/diff/5001/5002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/5001/5002#newcode708 Line 708: // The Socket Stream is connected. The SocketStreamHandle should track of how s/track/keep track/ ? http://codereview.chromium.org/188007/diff/5001/5002#newcode712 Line 712: int /* socket_id */, Is int large enough? or do we need long? http://codereview.chromium.org/188007/diff/5001/5002#newcode724 Line 724: int /* amount_sent */) How about adding int /* free_buffer_size */ or so? This way, the SocketStreamHandle need not keep track of the size of pending messages. I'd say if the pending messages are managed by the browser, the size of free buffer (i.e., how many more bytes can be pending) should also be managed by it. It looks somewhat unnatural to me that SocketStreamHandle keeps track of it. http://codereview.chromium.org/188007/diff/5001/5002#newcode1734 Line 1734: // start connecting asynchronously. Who starts connecting? The browser? Then: "The browser assigns a unique |socket_id| for the socket stream and starts connecting asynchronously" is easier for me to read. http://codereview.chromium.org/188007/diff/5001/5002#newcode1744 Line 1744: // The number of bytes can be tracked by size of |data| it was sent and s/number of bytes/number of pending bytes/ ? s/it was sent/being sent/ ? http://codereview.chromium.org/188007/diff/5001/5002#newcode1745 Line 1745: // |amount_sent| parameter of ViewMsg_SocketStream_DataSent. Can more than one ViewMsg_SocketStream_DataSent be sent for a single ViewHostMsg_SocketStream_SendData? http://codereview.chromium.org/188007/diff/5001/5002#newcode1754 Line 1754: // stream is completely closed. Can a socket stream be partially closed?? What are the intermediate state(s) between "open" and "completely closed"?
http://codereview.chromium.org/188007/diff/5001/5002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/5001/5002#newcode708 Line 708: // The Socket Stream is connected. The SocketStreamHandle should track of how On 2009/09/10 03:56:06, Yuzo wrote: > s/track/keep track/ ? Done. http://codereview.chromium.org/188007/diff/5001/5002#newcode712 Line 712: int /* socket_id */, On 2009/09/10 03:56:06, Yuzo wrote: > Is int large enough? or do we need long? I use IDMap to map socket_id to handle, and IDMap uses int32 for key id. http://codereview.chromium.org/188007/diff/5001/5002#newcode724 Line 724: int /* amount_sent */) On 2009/09/10 03:56:06, Yuzo wrote: > How about adding > int /* free_buffer_size */ > or so? > > This way, the SocketStreamHandle need not keep track of > the size of pending messages. > > I'd say if the pending messages are managed by the browser, > the size of free buffer (i.e., how many more bytes can be pending) > should also be managed by it. It looks somewhat unnatural to me that > SocketStreamHandle keeps track of it. Hmm, I don't think so. SocketStreamHandle may send data several times after one ViewMsg_SocketStream_DataSent received, so it must keep track of the size of pending message (and available size to be sent) whether or not with such free_buffer_size parameter in DataSent. http://codereview.chromium.org/188007/diff/5001/5002#newcode1734 Line 1734: // start connecting asynchronously. On 2009/09/10 03:56:06, Yuzo wrote: > Who starts connecting? The browser? > > Then: > "The browser assigns a unique |socket_id| for the socket stream and starts > connecting asynchronously" > is easier for me to read. > > Done. http://codereview.chromium.org/188007/diff/5001/5002#newcode1744 Line 1744: // The number of bytes can be tracked by size of |data| it was sent and On 2009/09/10 03:56:06, Yuzo wrote: > s/number of bytes/number of pending bytes/ ? > > s/it was sent/being sent/ ? Done. http://codereview.chromium.org/188007/diff/5001/5002#newcode1745 Line 1745: // |amount_sent| parameter of ViewMsg_SocketStream_DataSent. On 2009/09/10 03:56:06, Yuzo wrote: > Can more than one ViewMsg_SocketStream_DataSent be sent for a single > ViewHostMsg_SocketStream_SendData? It would be possible. http://codereview.chromium.org/188007/diff/5001/5002#newcode1754 Line 1754: // stream is completely closed. On 2009/09/10 03:56:06, Yuzo wrote: > Can a socket stream be partially closed?? > What are the intermediate state(s) between "open" and "completely closed"? requested to close from renderer, but raw socket is still open, because it is not sync operation.
http://codereview.chromium.org/188007/diff/5001/5002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/5001/5002#newcode724 Line 724: int /* amount_sent */) On 2009/09/10 03:56:06, Yuzo wrote: > How about adding > int /* free_buffer_size */ I think the arrangement ukai has is good. By the time the client receives the 'free_buffer_size', that data is stale. It is reflective of what the free buffer size was when the 'ack' was issued. Since that time, the client may have 'sent' more stuff. An attempt to send 'free_buffer_size' amount of data upon receipt of the 'ack' could very well overflow the max buffer size the server allows per stream.
ping.
LGTM http://codereview.chromium.org/188007/diff/10001/10002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/10001/10002#newcode723 Line 723: // |amount_sent| of data requested by ViewHostMsg_SocketStream_SendData has s/|amount_sent|/|amount_sent| bytes/ ? http://codereview.chromium.org/188007/diff/10001/10002#newcode1766 Line 1766: // The browser assigns a unique |socket_id| for the socket stream and Use capital "Socket Stream" for consistency http://codereview.chromium.org/188007/diff/10001/10002#newcode1767 Line 1767: // start connecting asynchronously. starts http://codereview.chromium.org/188007/diff/10001/10002#newcode1778 Line 1778: // and |amount_sent| parameter of ViewMsg_SocketStream_DataSent. Is the constraint described by this formula (Accumulated total of |data| (so to say seq in TCP)) - (Accumulated total of |amount_sent| (so to say ack in TCP)) <= |max_pending_send_allowed|? (i.e. in other words, inflight data <= window, you mean?) If correct, could you add this formula for clarification? http://codereview.chromium.org/188007/diff/10001/10002#newcode1786 Line 1786: // The browser will send ViewMsg_SocketStream_Closed back when the socket Use capital "Socket Stream" for consistency?
LGTM http://codereview.chromium.org/188007/diff/5001/5002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/5001/5002#newcode712 Line 712: int /* socket_id */, I see. On 2009/09/10 08:19:29, ukai wrote: > On 2009/09/10 03:56:06, Yuzo wrote: > > Is int large enough? or do we need long? > > I use IDMap to map socket_id to handle, and IDMap uses int32 for key id. http://codereview.chromium.org/188007/diff/5001/5002#newcode724 Line 724: int /* amount_sent */) Ah, I see. Thanks. On 2009/09/10 08:19:29, ukai wrote: > On 2009/09/10 03:56:06, Yuzo wrote: > > How about adding > > int /* free_buffer_size */ > > or so? > > > > This way, the SocketStreamHandle need not keep track of > > the size of pending messages. > > > > I'd say if the pending messages are managed by the browser, > > the size of free buffer (i.e., how many more bytes can be pending) > > should also be managed by it. It looks somewhat unnatural to me that > > SocketStreamHandle keeps track of it. > > Hmm, I don't think so. > SocketStreamHandle may send data several times after one > ViewMsg_SocketStream_DataSent received, so it must keep track of the size of > pending message (and available size to be sent) whether or not with such > free_buffer_size parameter in DataSent. http://codereview.chromium.org/188007/diff/5001/5002#newcode1754 Line 1754: // stream is completely closed. I see. On 2009/09/10 08:19:29, ukai wrote: > On 2009/09/10 03:56:06, Yuzo wrote: > > Can a socket stream be partially closed?? > > What are the intermediate state(s) between "open" and "completely closed"? > > requested to close from renderer, but raw socket is still open, because it is > not sync operation. >
Sorry for the unacceptable delays. I will review this today!-Darin On Thu, Sep 17, 2009 at 1:52 AM, <ukai@chromium.org> wrote: > ping. > > > http://codereview.chromium.org/188007 >
http://codereview.chromium.org/188007/diff/10001/10002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/10001/10002#newcode1770 Line 1770: IPC_SYNC_MESSAGE_CONTROL1_1(ViewHostMsg_SocketStream_Connect, per jam's feedback, i thought the plan was to make this async. synchronous IPCs should only be used when absolutely necessary. they add latency to the renderer and are bad for performance. in this case, i think each renderer should be able to manage its own socket IDs. on the browser side, we can combine the process ID of the renderer with the supplied socket ID. that will create a unique ID across the entire browser. we do something very similar for normal resource requests. i suggest following that approach here. http://codereview.chromium.org/188007/diff/10001/10002#newcode1783 Line 1783: std::vector<char> /* data */) i wonder how many buffer copies we are going to have here. it is hard for me to know what to suggest here since i can't see all of the code. however, looking over the webcore code, it seems to me that it might be wise to change SocketStreamHandleBase::m_buffer to be a SharedBuffer. that way you can pass it through the WebKit API using WebData without paying any penalty to copy data. finally, we might want to just use raw IPC::Message methods to write data into the ViewHostMsg_SocketStream_SendData object. that way you can copy from the SharedBuffer (WebData object) directly into the IPC::Message, which should leave you with only one temporary copy of the data. you might want to do something similar for ViewMsg_SocketStream_ReceivedData. it may also be premature optimization to do this work :)
http://codereview.chromium.org/188007/diff/10001/10002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/10001/10002#newcode1783 Line 1783: std::vector<char> /* data */) > however, looking over the webcore code, it seems to me that it might be wise to > change SocketStreamHandleBase::m_buffer to be a SharedBuffer. that way you can > pass it through the WebKit API using WebData without paying any penalty to copy > data. > > finally, we might want to just use raw IPC::Message methods to write data into > the ViewHostMsg_SocketStream_SendData object. that way you can copy from the > SharedBuffer (WebData object) directly into the IPC::Message, which should leave > you with only one temporary copy of the data. Hmmm... I should handle 'master entry' resource data accordingly for the appcache gunk.
Thanks for review! http://codereview.chromium.org/188007/diff/10001/10002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/10001/10002#newcode723 Line 723: // |amount_sent| of data requested by ViewHostMsg_SocketStream_SendData has On 2009/09/17 09:25:55, tyoshino wrote: > s/|amount_sent|/|amount_sent| bytes/ ? Done. http://codereview.chromium.org/188007/diff/10001/10002#newcode1766 Line 1766: // The browser assigns a unique |socket_id| for the socket stream and On 2009/09/17 09:25:55, tyoshino wrote: > Use capital "Socket Stream" for consistency > Done. http://codereview.chromium.org/188007/diff/10001/10002#newcode1767 Line 1767: // start connecting asynchronously. On 2009/09/17 09:25:55, tyoshino wrote: > starts Done. http://codereview.chromium.org/188007/diff/10001/10002#newcode1770 Line 1770: IPC_SYNC_MESSAGE_CONTROL1_1(ViewHostMsg_SocketStream_Connect, On 2009/09/17 21:45:33, darin wrote: > per jam's feedback, i thought the plan was to make this async. synchronous IPCs > should only be used when absolutely necessary. they add latency to the renderer > and are bad for performance. > > in this case, i think each renderer should be able to manage its own socket IDs. > on the browser side, we can combine the process ID of the renderer with the > supplied socket ID. that will create a unique ID across the entire browser. > > we do something very similar for normal resource requests. i suggest following > that approach here. Thanks for suggestion. Fixed. http://codereview.chromium.org/188007/diff/10001/10002#newcode1778 Line 1778: // and |amount_sent| parameter of ViewMsg_SocketStream_DataSent. On 2009/09/17 09:25:55, tyoshino wrote: > Is the constraint described by this formula (Accumulated total of |data| (so to > say seq in TCP)) - (Accumulated total of |amount_sent| (so to say ack in TCP)) > <= |max_pending_send_allowed|? (i.e. in other words, inflight data <= window, > you mean?) > If correct, could you add this formula for clarification? Done. http://codereview.chromium.org/188007/diff/10001/10002#newcode1783 Line 1783: std::vector<char> /* data */) Thanks for suggestion. I'll be OOO next week by Japanese national holidays, so I'll consider it after I come back. Please look over the code at http://codereview.chromium.org/202058, if you have time. On 2009/09/17 21:45:33, darin wrote: > i wonder how many buffer copies we are going to have here. it is hard for me to > know what to suggest here since i can't see all of the code. > > however, looking over the webcore code, it seems to me that it might be wise to > change SocketStreamHandleBase::m_buffer to be a SharedBuffer. that way you can > pass it through the WebKit API using WebData without paying any penalty to copy > data. > > finally, we might want to just use raw IPC::Message methods to write data into > the ViewHostMsg_SocketStream_SendData object. that way you can copy from the > SharedBuffer (WebData object) directly into the IPC::Message, which should leave > you with only one temporary copy of the data. > > you might want to do something similar for ViewMsg_SocketStream_ReceivedData. > > it may also be premature optimization to do this work :) http://codereview.chromium.org/188007/diff/10001/10002#newcode1786 Line 1786: // The browser will send ViewMsg_SocketStream_Closed back when the socket On 2009/09/17 09:25:55, tyoshino wrote: > Use capital "Socket Stream" for consistency? > Done.
On 2009/09/18 12:04:20, ukai wrote: > Thanks for review! > > http://codereview.chromium.org/188007/diff/10001/10002 > File chrome/common/render_messages_internal.h (right): > > http://codereview.chromium.org/188007/diff/10001/10002#newcode723 > Line 723: // |amount_sent| of data requested by > ViewHostMsg_SocketStream_SendData has > On 2009/09/17 09:25:55, tyoshino wrote: > > s/|amount_sent|/|amount_sent| bytes/ ? > > Done. > > http://codereview.chromium.org/188007/diff/10001/10002#newcode1766 > Line 1766: // The browser assigns a unique |socket_id| for the socket stream and > On 2009/09/17 09:25:55, tyoshino wrote: > > Use capital "Socket Stream" for consistency > > > > Done. > > http://codereview.chromium.org/188007/diff/10001/10002#newcode1767 > Line 1767: // start connecting asynchronously. > On 2009/09/17 09:25:55, tyoshino wrote: > > starts > > Done. > > http://codereview.chromium.org/188007/diff/10001/10002#newcode1770 > Line 1770: IPC_SYNC_MESSAGE_CONTROL1_1(ViewHostMsg_SocketStream_Connect, > On 2009/09/17 21:45:33, darin wrote: > > per jam's feedback, i thought the plan was to make this async. synchronous > IPCs > > should only be used when absolutely necessary. they add latency to the > renderer > > and are bad for performance. > > > > in this case, i think each renderer should be able to manage its own socket > IDs. > > on the browser side, we can combine the process ID of the renderer with the > > supplied socket ID. that will create a unique ID across the entire browser. > > > > we do something very similar for normal resource requests. i suggest > following > > that approach here. > > Thanks for suggestion. > Fixed. > > http://codereview.chromium.org/188007/diff/10001/10002#newcode1778 > Line 1778: // and |amount_sent| parameter of ViewMsg_SocketStream_DataSent. > On 2009/09/17 09:25:55, tyoshino wrote: > > Is the constraint described by this formula (Accumulated total of |data| (so > to > > say seq in TCP)) - (Accumulated total of |amount_sent| (so to say ack in TCP)) > > <= |max_pending_send_allowed|? (i.e. in other words, inflight data <= window, > > you mean?) > > If correct, could you add this formula for clarification? > > Done. > > http://codereview.chromium.org/188007/diff/10001/10002#newcode1783 > Line 1783: std::vector<char> /* data */) > Thanks for suggestion. I'll be OOO next week by Japanese national holidays, so > I'll consider it after I come back. > Please look over the code at http://codereview.chromium.org/202058, if you have > time. > > > On 2009/09/17 21:45:33, darin wrote: > > i wonder how many buffer copies we are going to have here. it is hard for me > to > > know what to suggest here since i can't see all of the code. > > > > however, looking over the webcore code, it seems to me that it might be wise > to > > change SocketStreamHandleBase::m_buffer to be a SharedBuffer. that way you > can > > pass it through the WebKit API using WebData without paying any penalty to > copy > > data. I'm not sure changing SocketStreamHandleBase::m_buffer to be a SharedBuffer is a good idea. If we can always send whole data in m_buffer at once, it should do. But, in SocketStreamHandle, we'll send partial data in m_buffer to platform code... > > > > finally, we might want to just use raw IPC::Message methods to write data into > > the ViewHostMsg_SocketStream_SendData object. that way you can copy from the > > SharedBuffer (WebData object) directly into the IPC::Message, which should > leave > > you with only one temporary copy of the data. > > > > you might want to do something similar for ViewMsg_SocketStream_ReceivedData. I changed to use WebKit::WebData in these two IPC messages. Could you take a look again, please? Thanks. > > > > it may also be premature optimization to do this work :) > > http://codereview.chromium.org/188007/diff/10001/10002#newcode1786 > Line 1786: // The browser will send ViewMsg_SocketStream_Closed back when the > socket > On 2009/09/17 09:25:55, tyoshino wrote: > > Use capital "Socket Stream" for consistency? > > > > Done.
http://codereview.chromium.org/188007/diff/19001/20002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/19001/20002#newcode778 Line 778: WebKit::WebData /* data */) sorry, i did not mean to suggest that WebData should be passed here. see the comment in webkit_param_traits.h about not serializing non-POD WebKit API types over IPC. the problem is that many WebKit API types such as WebData are not safe to use on multiple threads b/c the internal ref counting is not thread safe. given how we intercept IPCs on the IO thread in the browser, i am concerned about subtle multi-threading bugs creeping in. (i think it is probably fine to leave this as you originally had it for now. we can worry about optimizing data exchange later.)
Thanks for review. http://codereview.chromium.org/188007/diff/19001/20002 File chrome/common/render_messages_internal.h (right): http://codereview.chromium.org/188007/diff/19001/20002#newcode778 Line 778: WebKit::WebData /* data */) On 2009/09/30 08:03:12, darin wrote: > sorry, i did not mean to suggest that WebData should be passed here. see the > comment in webkit_param_traits.h about not serializing non-POD WebKit API types > over IPC. the problem is that many WebKit API types such as WebData are not > safe to use on multiple threads b/c the internal ref counting is not thread > safe. given how we intercept IPCs on the IO thread in the browser, i am > concerned about subtle multi-threading bugs creeping in. > > (i think it is probably fine to leave this as you originally had it for now. we > can worry about optimizing data exchange later.) I see. revert the change.
ping On 2009/09/30 08:53:30, ukai wrote: > Thanks for review. > > http://codereview.chromium.org/188007/diff/19001/20002 > File chrome/common/render_messages_internal.h (right): > > http://codereview.chromium.org/188007/diff/19001/20002#newcode778 > Line 778: WebKit::WebData /* data */) > On 2009/09/30 08:03:12, darin wrote: > > sorry, i did not mean to suggest that WebData should be passed here. see the > > comment in webkit_param_traits.h about not serializing non-POD WebKit API > types > > over IPC. the problem is that many WebKit API types such as WebData are not > > safe to use on multiple threads b/c the internal ref counting is not thread > > safe. given how we intercept IPCs on the IO thread in the browser, i am > > concerned about subtle multi-threading bugs creeping in. > > > > (i think it is probably fine to leave this as you originally had it for now. > we > > can worry about optimizing data exchange later.) > > I see. revert the change.
Given... > (i think it is probably fine to leave this as you originally had it for now. we > can worry about optimizing data exchange later.) ... about marshalling std::vector<char>, this looks reasonable to me.
Still LGTM... and SentData is better :)
On 2009/10/09 06:33:56, michaeln wrote: > Still LGTM... and SentData is better :) Thanks for review! Since no comment long time, landed this CL. |
