Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1037483003:
[PresentationAPI] Implementing send() from WebPresentationClient to the PresentationService. (Closed)
|
Created:
5 years, 9 months ago by USE s.singapati at gmail.com Modified:
5 years, 7 months ago Reviewers:
Avi (use Gerrit), mlamouri (slow - plz ping), mark a. foltz, haibinlu, nasko, imcheng (use chromium acct), whywhat, imcheng CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, haibinlu, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[PresentationAPI] Implements send() API for String and ArrayBuffer/View from WebPresentationClient to the PresentationServiceDelegate. Send requests are queued in PresentationDispatcher and handled one at a time.
Depends on the Blink CL: https://codereview.chromium.org/1002293005
BUG=459008
Committed: https://crrev.com/834d10a11de9909062d6817583f1a769d6465a3c
Cr-Commit-Position: refs/heads/master@{#329154}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added queuing mechanism for postMessage requests. (WIP) #
Total comments: 19
Patch Set 3 : send msg callback from mojo, invalidating pending requests, other review fixes. #Patch Set 4 : Use simple queue<> instead of hash_map of vectors & send one message at a time. #Patch Set 5 : Rebased. #
Total comments: 17
Patch Set 6 : callback handling for send requests, other fixes. #
Total comments: 12
Patch Set 7 : Extended SendStringMessage() to delegate, Added unit test & other fixes. #
Total comments: 7
Patch Set 8 : Updated callback handling, message invalidation and and sending generic message struct. #
Total comments: 8
Patch Set 9 : message throttling and invalidation changes and renamed struct. #Patch Set 10 : Rebase and minor fixing. #Patch Set 11 : Extended unit tests and nullable fields in mojo SessionMessage struct. #Patch Set 12 : moved presentation_message.h/.cc to content/public/browser/ #
Total comments: 40
Patch Set 13 : Review fixes. #Patch Set 14 : passing scoped_ptr to delegate, updated UT. #Patch Set 15 : Rebase #Patch Set 16 : merge. #
Total comments: 20
Patch Set 17 : presentation_constants.h/cc and other fixes. #Patch Set 18 : Use Swap to avoid copying data. #Patch Set 19 : #Patch Set 20 : Fixed trybots errors. #
Total comments: 10
Patch Set 21 : Fix nits, trybots errors. #
Total comments: 4
Patch Set 22 : No (c) in new files Copyright message. #Messages
Total messages: 90 (24 generated)
s.singapati@samsung.com changed reviewers: + avayvod@chromium.org, mfoltz@chromium.org
PTaL.
https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... content/common/presentation/presentation_service.mojom:74: PostMessage(string presentation_url, string presentation_id, string message); nit: fit into 80 lines Mojo guidelines suggest we get a void result from the service to know when it's safe to send the next message over the Mojo channel (e.g. if the service gets stuck sending the message, the Mojo channel is not overloaded with messages submitted via postMessage()). This also means we could potentially send an array of messages to avoid calling the method many times if there's a queue: PostMessage(string presentation_url, string presentation_id, array[string] messages) => (); implementation wise it means some queueing of messages mapped by url/id pair is needed in the PresentationDispatcher
On 2015/03/26 00:24:14, whywhat wrote: > https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... > File content/common/presentation/presentation_service.mojom (right): > > https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... > content/common/presentation/presentation_service.mojom:74: PostMessage(string > presentation_url, string presentation_id, string message); > nit: fit into 80 lines > > Mojo guidelines suggest we get a void result from the service to know when it's > safe to send the next message over the Mojo channel (e.g. if the service gets > stuck sending the message, the Mojo channel is not overloaded with messages > submitted via postMessage()). This also means we could potentially send an array > of messages to avoid calling the method many times if there's a queue: > > PostMessage(string presentation_url, string presentation_id, array[string] > messages) => (); > > implementation wise it means some queueing of messages mapped by url/id pair is > needed in the PresentationDispatcher Yes, this needs queuing mechanism. Also, I think we can come back to this patch bit later, after implementing Blink side implementation with new postMessage api approach.
On 2015/03/26 21:52:01, s.singapati wrote: > On 2015/03/26 00:24:14, whywhat wrote: > > > https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... > > File content/common/presentation/presentation_service.mojom (right): > > > > > https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... > > content/common/presentation/presentation_service.mojom:74: PostMessage(string > > presentation_url, string presentation_id, string message); > > nit: fit into 80 lines > > > > Mojo guidelines suggest we get a void result from the service to know when > it's > > safe to send the next message over the Mojo channel (e.g. if the service gets > > stuck sending the message, the Mojo channel is not overloaded with messages > > submitted via postMessage()). This also means we could potentially send an > array > > of messages to avoid calling the method many times if there's a queue: > > > > PostMessage(string presentation_url, string presentation_id, array[string] > > messages) => (); > > > > implementation wise it means some queueing of messages mapped by url/id pair > is > > needed in the PresentationDispatcher > > Yes, this needs queuing mechanism. Also, I think we can come back to this patch > bit later, after implementing Blink side implementation with new postMessage api > approach. Hey, do you have an ETA when this would be ready to review and submit? Thanks, Anton.
On 2015/04/02 10:34:03, whywhat wrote: > On 2015/03/26 21:52:01, s.singapati wrote: > > On 2015/03/26 00:24:14, whywhat wrote: > > > > > > https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... > > > File content/common/presentation/presentation_service.mojom (right): > > > > > > > > > https://codereview.chromium.org/1037483003/diff/1/content/common/presentation... > > > content/common/presentation/presentation_service.mojom:74: > PostMessage(string > > > presentation_url, string presentation_id, string message); > > > nit: fit into 80 lines > > > > > > Mojo guidelines suggest we get a void result from the service to know when > > it's > > > safe to send the next message over the Mojo channel (e.g. if the service > gets > > > stuck sending the message, the Mojo channel is not overloaded with messages > > > submitted via postMessage()). This also means we could potentially send an > > array > > > of messages to avoid calling the method many times if there's a queue: > > > > > > PostMessage(string presentation_url, string presentation_id, array[string] > > > messages) => (); > > > > > > implementation wise it means some queueing of messages mapped by url/id pair > > is > > > needed in the PresentationDispatcher > > > > Yes, this needs queuing mechanism. Also, I think we can come back to this > patch > > bit later, after implementing Blink side implementation with new postMessage > api > > approach. > > Hey, > > do you have an ETA when this would be ready to review and submit? > > Thanks, > Anton. Hi, I have uploaded work in progress patch but I would like to get your feedback already to speed this up. some issues: -This patch is based on initial DOMString handling patch from blink side. But, our queuing mechanism should take arraybuffer & blob data also into account, to maintain the correct order of message delivery for individual sessions. so, this mojo method needs to be updated to handle other data also. Probably we need to send array<struct ...> PostMessages( string presentation_url, string presentation_id, array<string> string_messages) - Sending array of strings/(objects) over mojo service makes it slightly more work to maintain hashmap and vector inside. But this will make less mojo calls. (Other option is to just have normal queue and send individual request, instead of array) - Also i think, if we go with hashmap of vectors, key should also contain "presentation url" to be more strict. (currently key has only presentation id). - Another issue is when to send next array of messages (for another session) over mojo channel. HandlePostMessageRequests() can be called appropriately. We need to check this. Any ideas on this? I think, We can estimate based on review of current patch. Thank you.
https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:166: void PostMessages( For all types that can be sent/received, we need to design this implementation based on the constraints of Mojo and the desire to not allow a rogue client to spam the browser. For example: - What if the data to be written exceeds the MessagePipe's incoming buffer? - Will there be any advantage to chunking up a large write into smaller writes? - We should acknowledge that the last message was sent onto its destination before requesting the next message from the client (flow control) For now we could limit the size of individual messages to something small and reasonable (64k) while we work through the other issues. Anton/Derek, WDYT? https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:169: mojo::Array<mojo::String> string_messages) override; Mojo guarantees FIFO order for messages sent through a MessagePipe. https://docs.google.com/a/chromium.org/document/d/1kvWHSHe-r7ty6wl3vhXXcSbyus... I don't batching them is necessary to guarantee order. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:123: void PresentationDispatcher::postMessage( I don't think we should be queuing on the browser side. Mojo should queue requests on the sender (renderer) side if necessary.
mfoltz@chromium.org changed reviewers: + imcheng@chromium.org
https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:166: void PostMessages( On 2015/04/02 20:46:26, mark a. foltz wrote: > For all types that can be sent/received, we need to design this implementation > based on the constraints of Mojo and the desire to not allow a rogue client to > spam the browser. > > For example: > - What if the data to be written exceeds the MessagePipe's incoming buffer? > - Will there be any advantage to chunking up a large write into smaller writes? > - We should acknowledge that the last message was sent onto its destination > before requesting the next message from the client (flow control) > > For now we could limit the size of individual messages to something small and > reasonable (64k) while we work through the other issues. > > Anton/Derek, WDYT? We must limit the size of the messages. 64k sounds fine. https://codereview.chromium.org/1037483003/diff/20001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/20001/content/common/presenta... content/common/presentation/presentation_service.mojom:24: enum MessageType { Doesn't look like these are used. Remove them for now? https://codereview.chromium.org/1037483003/diff/20001/content/common/presenta... content/common/presentation/presentation_service.mojom:88: array<string> string_messages); => (); This means PresentationDispatcher will supply a no-arg callback to be invoked by PresentationServiceImpl to acknowledge that it has received the message and ready for the next one. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:177: void PresentationDispatcher::DoPostMessages( I think we need a mechanism to invalidate pending messages, e.g. if the frame navigated to a different page. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:46: // Need to handle ArrayBuffer/View and Blob data also Please add a TODO here. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:119: typedef std::vector<linked_ptr<PostMessageRequest>> PostMessageVector; using PostMessageVector = std::vector<linked_ptr<PostMessageRequest>>; Same for PostMessageRequestMap. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:123: PostMessageRequestMap post_message_requests_map_; If we are sending 1 message per mojo call (I think that's what mark is suggesting), then we can consider using a queue<PostMessageRequest> as it's simpler?
https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:166: void PostMessages( On 2015/04/02 20:46:26, mark a. foltz wrote: > For all types that can be sent/received, we need to design this implementation > based on the constraints of Mojo and the desire to not allow a rogue client to > spam the browser. > > For example: > - What if the data to be written exceeds the MessagePipe's incoming buffer? > - Will there be any advantage to chunking up a large write into smaller writes? > - We should acknowledge that the last message was sent onto its destination > before requesting the next message from the client (flow control) > > For now we could limit the size of individual messages to something small and > reasonable (64k) while we work through the other issues. > > Anton/Derek, WDYT? Acknowledged. Do we want to stop sending messages exceeding the limit for now? This can be done on Blink side also. Or break them into smaller somehow and send as chunks? this might be tricky and needs more handling. https://codereview.chromium.org/1037483003/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:169: mojo::Array<mojo::String> string_messages) override; On 2015/04/02 20:46:26, mark a. foltz wrote: > Mojo guarantees FIFO order for messages sent through a MessagePipe. > > https://docs.google.com/a/chromium.org/document/d/1kvWHSHe-r7ty6wl3vhXXcSbyus... > > I don't batching them is necessary to guarantee order. I've created a patch with new queuing approach to discuss both approaches and decide: https://codereview.chromium.org/1061363002/. (I will merge queue changes in new patch into current patch based review) In this patch simple queue is used for message requests (std::queue<linked_ptr<MessageRequest>>) and sends one message at time, instead of sending array of messages. Also, current hash_map of vector implementation has below problems: - When first message (one message in vector) is sent to presentation service and waiting for callback, Then if some new message requests come from session and inserted into the vector. When callback is received, the first element in the hash_map will be deleted, thus newly inserted messages into vector will be lost. - When a session sends different messages in order (for eg., string, array buffer, blob, string...), it is difficult send as array of messages. https://codereview.chromium.org/1037483003/diff/20001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/20001/content/common/presenta... content/common/presentation/presentation_service.mojom:24: enum MessageType { On 2015/04/02 23:57:24, imcheng1 wrote: > Doesn't look like these are used. Remove them for now? Done. https://codereview.chromium.org/1037483003/diff/20001/content/common/presenta... content/common/presentation/presentation_service.mojom:88: array<string> string_messages); On 2015/04/02 23:57:24, imcheng1 wrote: > => (); > > This means PresentationDispatcher will supply a no-arg callback to be invoked by > PresentationServiceImpl to acknowledge that it has received the message and > ready for the next one. Done. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:123: void PresentationDispatcher::postMessage( On 2015/04/02 20:46:26, mark a. foltz wrote: > I don't think we should be queuing on the browser side. Mojo should queue > requests on the sender (renderer) side if necessary. Acknowledged. Commented already about current hash_map of vector vs simple queue of messages. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:177: void PresentationDispatcher::DoPostMessages( On 2015/04/02 23:57:24, imcheng1 wrote: > I think we need a mechanism to invalidate pending messages, e.g. if the frame > navigated to a different page. Done. Implemented FrameWillClose(). This is the one i could find to be reasonable. Any other ideas please? https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:46: // Need to handle ArrayBuffer/View and Blob data also On 2015/04/02 23:57:24, imcheng1 wrote: > Please add a TODO here. Done. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:119: typedef std::vector<linked_ptr<PostMessageRequest>> PostMessageVector; On 2015/04/02 23:57:24, imcheng1 wrote: > using PostMessageVector = std::vector<linked_ptr<PostMessageRequest>>; > > Same for PostMessageRequestMap. Done. https://codereview.chromium.org/1037483003/diff/20001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:123: PostMessageRequestMap post_message_requests_map_; On 2015/04/02 23:57:24, imcheng1 wrote: > If we are sending 1 message per mojo call (I think that's what mark is > suggesting), then we can consider using a queue<PostMessageRequest> as it's > simpler? Acknowledged. Simple queuing approach in new patch: https://codereview.chromium.org/1061363002/. (Need to merge that with current patch based review). Also with this approach all message types can be sent individually in order to presentation_service_impl, for eg., send(struct MessageRequest) or send(.., .., Messagetype, BinaryType) overloads.
Please ignore other CL (https://codereview.chromium.org/1061363002/). I have merged the changes into current CL. Patchset#3: Uses hash_map of vectors. Patchset#4: Modified to use simple queue. Mainly one thing to handle is to limit the size of individual messages to reasonable size (eg., 64k). Is it enough to stop sending the message from blink (PresentationSession::send()) when dataLength / string size is more than limit?
This looks pretty good, although I would like to see unit tests for the new behaviors. @imcheng, can you check the usage of Mojo callbacks here? https://codereview.chromium.org/1037483003/diff/80001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/80001/content/common/presenta... content/common/presentation/presentation_service.mojom:77: // Called when send is called by the frame. The result callback is to get Called when send(String) is ... https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:156: // TODO(): Handle ArrayBuffer data. Include login. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:170: base::Unretained(this))); The pattern for other callbacks in this file is to transfer ownership to the Mojo service so they are destroyed when it is. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:175: message_request_queue_.pop(); I don't quite follow the logic of the queue. It seems like if you pop before the callback is invoked, you won't know whether to send the next message immediately or queue it on send(). What about: - Peeking at the head of the queue here and sending - Dequeuing in the callback - send() enqueues and calls DoSendStringMessage if the queue was previously empty The invariant is that the head of the queue is always being sent. The code you have may be equivalent, just a bit harder for me to follow? https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:186: message_request_queue_.pop(); If there is a pending Mojo callback it needs to be invoked, otherwise Mojo will disconnect the service from the frame. Maybe not a big deal since the frame is closing anyway?
https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:174: DCHECK(!message_request_queue_.empty()); Is the following scenario possible? 1. presentation_service_->SendStringMessage is called 2. FrameWillClose() was called (or navigation occurred; see below comment), which cleared message_request_queue_ 3. The callback from (1) is invoked, and blows up here and/or pops the wrong request from the queue https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:184: void PresentationDispatcher::RemoveAllMessageRequests() { Do we also need to invalidate all pending send message requests if a frame navigated away? https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:272: void PresentationDispatcher::OnSendMessageCallback() { This function looks redundant -- looks like you can base::Bind HandleSendMessageRequests above instead and get rid of this function. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:43: // TODO(): Handle ArrayBuffer/View and Blob data. Add username to TODO.
I will work on adding unit tests for PresentationDispatcher. https://codereview.chromium.org/1037483003/diff/80001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/80001/content/common/presenta... content/common/presentation/presentation_service.mojom:77: // Called when send is called by the frame. The result callback is to get On 2015/04/08 23:43:45, mark a. foltz wrote: > Called when send(String) is ... Done. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:156: // TODO(): Handle ArrayBuffer data. On 2015/04/08 23:43:45, mark a. foltz wrote: > Include login. Done. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:170: base::Unretained(this))); On 2015/04/08 23:43:45, mark a. foltz wrote: > The pattern for other callbacks in this file is to transfer ownership to the > Mojo service so they are destroyed when it is. Done. Now callback is stored in PresentationServiceImpl::send_message_cb_ptr. callback is run and deleted appropriately. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:175: message_request_queue_.pop(); On 2015/04/08 23:43:45, mark a. foltz wrote: > I don't quite follow the logic of the queue. It seems like if you pop before > the callback is invoked, you won't know whether to send the next message > immediately or queue it on send(). > > What about: > - Peeking at the head of the queue here and sending > - Dequeuing in the callback > - send() enqueues and calls DoSendStringMessage if the queue was previously > empty > > The invariant is that the head of the queue is always being sent. > > The code you have may be equivalent, just a bit harder for me to follow? Now, flow goes like this: 1. send() enques requests and 2. if queue was empty previously, then call DoSendStringMessage() and wait for callback. 3. In callback HandleSendMessageRequests(), pop the request and continue DoSendStringMessage() if queue is not empty. (Special case is to check for empty queue before pop to handle FrameWillClose()). https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:184: void PresentationDispatcher::RemoveAllMessageRequests() { On 2015/04/09 00:29:57, imcheng1 wrote: > Do we also need to invalidate all pending send message requests if a frame > navigated away? Not sure. But, Isn't this enough to invalidate requests in FrameWillClose()? as it comments "This is the last opportunity to send messages to the host (e.g., for clean-up, shutdown, etc.)" https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:186: message_request_queue_.pop(); On 2015/04/08 23:43:45, mark a. foltz wrote: > If there is a pending Mojo callback it needs to be invoked, otherwise Mojo will > disconnect the service from the frame. Maybe not a big deal since the frame is > closing anyway? Done. in PresentationServiceImpl, callback send_message_cb_ptr_ is invoked and deleted in this case. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:272: void PresentationDispatcher::OnSendMessageCallback() { On 2015/04/09 00:29:57, imcheng1 wrote: > This function looks redundant -- looks like you can base::Bind > HandleSendMessageRequests above instead and get rid of this function. Done. https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1037483003/diff/80001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.h:43: // TODO(): Handle ArrayBuffer/View and Blob data. On 2015/04/09 00:29:57, imcheng1 wrote: > Add username to TODO. Done.
Please add a test case for SendStringMessage in presentation_service_impl_unittest.cc. I also recommend unit testing the queuing behavior by adding a presentation_dispatcher_unittest.cc. https://codereview.chromium.org/1037483003/diff/100001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/100001/content/common/present... content/common/presentation/presentation_service.mojom:77: // Called when send(String) is called by the frame. The result callback is to get Please wrap at 80 columns. https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:173: DCHECK(!message_request_queue_.empty()); It seems like the realistic situation described below would cause this DCHECK to fire. Instead what about: if (message_request_queue_.empty()) return; https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:9: #include "base/containers/hash_tables.h" This #include doesn't seem necessary.
We did consider having the following signature for the Mojo service method: SendStringMessage(String url, String id, array[MessageRequest] messages) => (); E.g. sending all the queued messages in one call and let the PresentationService dequeue the messages. I guess we'd have to pass some generic enough object instead of string to support ArrayBuffer/View and Blob ordering but is there any other reason why we would not do this? I'm thinking about some input heavy scenario (e.g. an action game, page generating events or user rewinding a video/slides) when the queue could potentially grow faster then the service sending messages over. https://codereview.chromium.org/1037483003/diff/100001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:392: send_message_cb_ptr_->Run(); This might result in the PresentationDispatcher sending the next message. Either we need to run the callback with an error or just reset it (if that doesn't break Mojo connection). https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:146: DoSendStringMessage(presentationUrl.utf8(), use {} for multiline if branches. https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:189: while (!message_request_queue_.empty()) I think you just can do message_request_queue_.swap(MessageRequestQueue()); I also don't think this deserves its own method yet.
Need to add unit tests for queuing behavior in PresentationDispatcher. But before that need to address the comments from Anton about sending array<MesageRequet> from PresentationDispatcher to PresentationServiceImpl. https://codereview.chromium.org/1037483003/diff/100001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:392: send_message_cb_ptr_->Run(); On 2015/04/13 13:31:21, whywhat wrote: > This might result in the PresentationDispatcher sending the next message. Either > we need to run the callback with an error or just reset it (if that doesn't > break Mojo connection). Done. callback is Reset for now. May be Mojo disconnection from the frame is not a issue since the frame is closing anyway? (Or it needs a callback like OnSendMessageCallback(bool success) in PresentationDispatcher.) https://codereview.chromium.org/1037483003/diff/100001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/100001/content/common/present... content/common/presentation/presentation_service.mojom:77: // Called when send(String) is called by the frame. The result callback is to get On 2015/04/10 19:03:18, mark a. foltz wrote: > Please wrap at 80 columns. Done. https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:146: DoSendStringMessage(presentationUrl.utf8(), On 2015/04/13 13:31:21, whywhat wrote: > use {} for multiline if branches. Done. https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:173: DCHECK(!message_request_queue_.empty()); On 2015/04/10 19:03:18, mark a. foltz wrote: > It seems like the realistic situation described below would cause this DCHECK to > fire. > > Instead what about: > > if (message_request_queue_.empty()) > return; > > > Done. https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:189: while (!message_request_queue_.empty()) On 2015/04/13 13:31:21, whywhat wrote: > I think you just can do message_request_queue_.swap(MessageRequestQueue()); > I also don't think this deserves its own method yet. Done. Had to use std::swap(). message_request_queue_.swap() did not work somehow. https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.h (right): https://codereview.chromium.org/1037483003/diff/100001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.h:9: #include "base/containers/hash_tables.h" On 2015/04/10 19:03:18, mark a. foltz wrote: > This #include doesn't seem necessary. Done.
On 2015/04/13 13:31:21, whywhat wrote:
> We did consider having the following signature for the Mojo service method:
>
> SendStringMessage(String url, String id, array[MessageRequest] messages) =>
();
>
> E.g. sending all the queued messages in one call and let the
PresentationService
> dequeue the messages. I guess we'd have to pass some generic enough object
> instead of string to support ArrayBuffer/View and Blob ordering but is there
any
> other reason why we would not do this?
>
> I'm thinking about some input heavy scenario (e.g. an action game, page
> generating events or user rewinding a video/slides) when the queue could
> potentially grow faster then the service sending messages over.
>
There were some review comments about having a simple queue, sending one message
per mojo call. I think that time there was implementation only for string and
sending only array of strings does not preserve the order. Also hash_map
implementation had some issues.
@mark a.foltz / imcheng: Any comments please?
Now if we just go with normal queue of MessageRequests, and by combining String
& ArrayBuffer implementation, we can try to have generic message dispatching.
Could you Please comment on my ideas and questions below?
- For eg. we can have like this.
struct MessageRequest {
const std::string presentation_url;
const std::string presentation_id;
MessageType type; // TEXT, ARRAYBUFFER, BLOB
const std::string message;
Vector<uint8_t> arraybuffer_data;
}
- And mojo call:
SendMessage(
string presentation_url,
string presentation_id,
array<MessageRequest> message_array) => ();
Message flow:
1. PresentationDispatcher:
- PresentationDispatcher enqueues MessageRequest as done curently (std::queue)
(order of messages can be for different sessions and for different message
types)
- sends array<MessageRequest> to PresentationServiceImpl. (for eg., 'n'
messages)
- waits for call back from PresentationServiceImpl.
- Meanwhile more MessageRequests could be added to the queue.
- Delete first 'n' messages from queue after receiving callback from
PresentationServiceImpl.
- sends next array<>
2. How does PresentationServiceImpl send these messages to delegate?
- It needs to send one message at time (by checking MessageType and something
like: delegate->sendString()/sendBinaryData()). Then PresentationServiceImpl has
to maintain it's OWN Queue.
- Should PresentationServiceImpl run callback to PresentationDispatcher
immediately after receiving array<> (as main purpose of sending array<> here is
to minimize mojo calls as I understand)
OR should it wait until all requests are sent and processed by delegate?
- Currently mojo callback is non-argument. There won't be indication whether
message has been sent successfully or not for some reason. SPEC does not say
anything about this.?
https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:313: send_message_cb_ptr_.reset(new SendMessageMojoCallback(callback)); Given that incoming SendStringMessage requests are already throttled by the mojo flow control, can we simplify this by invoking the callback after calling the delegate? The other problem with this approach is that resetting a non-null ptr will drop the previous callback, resulting in a DCHECK. https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:408: send_message_cb_ptr_.reset(); It cannot simply be reset since it will DCHECK per the mojo team. Also, note that Reset() is also called as a result of navigation not just frame deletion. https://codereview.chromium.org/1037483003/diff/120001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/120001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:207: void PresentationDispatcher::FrameWillClose() { How about overriding DidCommitProvisionalLoad to invalidate messages on non in-page navigation as well? https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren... I think it would make sense to drop send() requests after navigating away from the page. If we do that, please correct the queueing logic above.
https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:313: send_message_cb_ptr_.reset(new SendMessageMojoCallback(callback)); On 2015/04/14 21:29:13, imcheng1 wrote: > Given that incoming SendStringMessage requests are already throttled by the mojo > flow control, can we simplify this by invoking the callback after calling the > delegate? > > The other problem with this approach is that resetting a non-null ptr will drop > the previous callback, resulting in a DCHECK. Acknowledged. Yes, I also had almost same question here: https://codereview.chromium.org/1037483003/#msg19 - Should PresentationServiceImpl run callback to PresentationDispatcher immediately after receiving message / array<> (as main purpose of sending array<> here is to minimize mojo calls as I understand) OR should it wait until all requests are sent and processed by delegate? Any ideas from other please. I will update the patch based on the comments. https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:408: send_message_cb_ptr_.reset(); On 2015/04/14 21:29:13, imcheng1 wrote: > It cannot simply be reset since it will DCHECK per the mojo team. Also, note > that Reset() is also called as a result of navigation not just frame deletion. Acknowledged. - Then it must run the callback for eg. with an error and then reset it. (eg., callback OnSendMessageCallback(bool success) in PresentationDispatcher). I will modify current non-arg callback. - DidNavigateAnyFrame() does not call Reset() if it is in_page_navigation. https://codereview.chromium.org/1037483003/diff/120001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/120001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:207: void PresentationDispatcher::FrameWillClose() { On 2015/04/14 21:29:13, imcheng1 wrote: > How about overriding DidCommitProvisionalLoad to invalidate messages on non > in-page navigation as well? > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren... > > I think it would make sense to drop send() requests after navigating away from > the page. If we do that, please correct the queueing logic above. I have tested that FrameWillClose() is always invoked prior to the DidCommitProvisionalLoad(), when page is refreshed or navigated away by clicking back/forward buttons. well, but to be safe side may be we can override this. DidCommitProvisionalLoad(bool is_new_navigation,bool is_same_page_navigation) { // if this is top-level frame AND Not is_same_page_navigation // empty the queue. // For Back/forward navigation: is_new_navigation: 1, is_same_page_navigation: 0 // For refresh: is_new_navigation: 0, is_same_page_navigation: 0 } your comments please?
https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/120001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:313: send_message_cb_ptr_.reset(new SendMessageMojoCallback(callback)); On 2015/04/15 16:09:08, s.singapati wrote: > On 2015/04/14 21:29:13, imcheng1 wrote: > > Given that incoming SendStringMessage requests are already throttled by the > mojo > > flow control, can we simplify this by invoking the callback after calling the > > delegate? > > > > The other problem with this approach is that resetting a non-null ptr will > drop > > the previous callback, resulting in a DCHECK. > > Acknowledged. Yes, I also had almost same question here: > https://codereview.chromium.org/1037483003/#msg19 > > - Should PresentationServiceImpl run callback to PresentationDispatcher > immediately after receiving message / array<> (as main purpose of sending > array<> here is to minimize mojo calls as I understand) > OR should it wait until all requests are sent and processed by delegate? > > Any ideas from other please. I will update the patch based on the comments. Also, if callback is invoked immediately after calling the delegate, we can avoid Run & resetting in Reset(). Is this OK ? Could there be any other issues?
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
It seems to me that the throttling on the renderer isn't needed and highly increase complexity. I have sent a message to mojo-dev to see what's their opinion: https://groups.google.com/a/chromium.org/d/msg/mojo-dev/HIlSGRUVOfk/J0qCTB3SJVMJ
Latest patch set tries to address some previous review comments and discussion in mojo-dev channel. - PresentationDispatcher queues MessageRequests and sends as MessageRequest to mojo service instead of individual string / uint*. - PresentationServiceImpl pushes the MessageRequest to delegate (something TODO/Fix) and invokes the callback immediately after calling the delegate. (This avoids saving the callback and run & resetting it later, to avoid possibilities of dropping the callback and resulting in DCHECKs). In this case Delegate has to handle the requests and possible queuing again. Also comments in mojo-dev channel suggests this: "This throttling makes sense given that the service just pushes the SendStringMessage parameters onto another queue (delegate_->SendStringMessage)" - Invalidating send requests when page refreshed / navigated away scenarios. Implemented both RenderFrameObserver::FrameWillClose() & DidCommitProvisionalLoad(). But DidCommitProvisionalLoad() looks redundant for me. I have noticed FrameWillClose() is invoked always prior to the DidCommitProvisionalLoad(). TODOs: - Fix invalid include of "content/common/presentation/presentation_message.h" in content/public/browser/presentation_service_delegate.h - To add Unit test for SendMessage() in presentation_service_impl_unittest.cc - Possibility of sending array<MessageRequest> over mojo. - Unit test for PresentationDispatcher based on finalization of current implementation.
On 2015/04/21 19:31:33, s.singapati wrote: > Latest patch set tries to address some previous review comments and discussion > in mojo-dev channel. > > - PresentationDispatcher queues MessageRequests and sends as MessageRequest to > mojo service instead of individual string / uint*. > - PresentationServiceImpl pushes the MessageRequest to delegate (something > TODO/Fix) and invokes the callback immediately after calling the delegate. (This > avoids saving the callback and run & resetting it later, to avoid possibilities > of dropping the callback and resulting in DCHECKs). In this case Delegate has to > handle the requests and possible queuing again. > > Also comments in mojo-dev channel suggests this: "This throttling makes sense > given that the service just pushes the SendStringMessage parameters onto another > queue (delegate_->SendStringMessage)" > > - Invalidating send requests when page refreshed / navigated away scenarios. > Implemented both RenderFrameObserver::FrameWillClose() & > DidCommitProvisionalLoad(). But DidCommitProvisionalLoad() looks redundant for > me. I have noticed FrameWillClose() is invoked always prior to the > DidCommitProvisionalLoad(). > > TODOs: > - Fix invalid include of "content/common/presentation/presentation_message.h" in > content/public/browser/presentation_service_delegate.h > - To add Unit test for SendMessage() in presentation_service_impl_unittest.cc > - Possibility of sending array<MessageRequest> over mojo. > - Unit test for PresentationDispatcher based on finalization of current > implementation. It seems like the queueing added on the renderer side is not strictly necessary, as the MessagePipe will already queue messages on the renderer side. The advantage of keeping it in is to batch requests going across the Mojo interface (assuming we update the interface to support arrays) and to drain the queue when the page navigates, so it seems worth keeping in. My concern is that there is no real throttling happening yet, because each incoming request is immediately handed off to the delegate without a chance for the delegate to throttle the rate of incoming requests. Based on Darin's comment in the mojo-dev thread: "The MessagePipe is already a queue. If you could arrange for the service impl to not read messages from the pipe faster than it can fully process them, then the client would not need to worry about throttling messages at all. It would be nicer if the service impl could protect itself somehow instead." It seems like we should: (1) wrap the Mojo callback to another callback passed to the PresentationServiceImpl delegate (2) have the delegate fire the callback when the downstream consumer (i.e. media router) is ready to accept another message (3) have that callback fire the Mojo callback (4) also have that callback trigger sending the next request from the renderer queue. Then we will have end-to-end flow control. Make sense?
On 2015/04/22 18:00:44, mark a. foltz wrote: > On 2015/04/21 19:31:33, s.singapati wrote: > > Latest patch set tries to address some previous review comments and discussion > > in mojo-dev channel. > > > > - PresentationDispatcher queues MessageRequests and sends as MessageRequest to > > mojo service instead of individual string / uint*. > > - PresentationServiceImpl pushes the MessageRequest to delegate (something > > TODO/Fix) and invokes the callback immediately after calling the delegate. > (This > > avoids saving the callback and run & resetting it later, to avoid > possibilities > > of dropping the callback and resulting in DCHECKs). In this case Delegate has > to > > handle the requests and possible queuing again. > > > > Also comments in mojo-dev channel suggests this: "This throttling makes sense > > given that the service just pushes the SendStringMessage parameters onto > another > > queue (delegate_->SendStringMessage)" > > > > - Invalidating send requests when page refreshed / navigated away scenarios. > > Implemented both RenderFrameObserver::FrameWillClose() & > > DidCommitProvisionalLoad(). But DidCommitProvisionalLoad() looks redundant for > > me. I have noticed FrameWillClose() is invoked always prior to the > > DidCommitProvisionalLoad(). > > > > TODOs: > > - Fix invalid include of "content/common/presentation/presentation_message.h" > in > > content/public/browser/presentation_service_delegate.h > > - To add Unit test for SendMessage() in presentation_service_impl_unittest.cc > > - Possibility of sending array<MessageRequest> over mojo. > > - Unit test for PresentationDispatcher based on finalization of current > > implementation. > > It seems like the queueing added on the renderer side is not strictly necessary, > as the MessagePipe will already queue messages on the renderer side. The > advantage of keeping it in is to batch requests going across the Mojo interface > (assuming we update the interface to support arrays) and to drain the queue when > the page navigates, so it seems worth keeping in. > > My concern is that there is no real throttling happening yet, because each > incoming request is immediately handed off to the delegate without a chance for > the delegate to throttle the rate of incoming requests. > > Based on Darin's comment in the mojo-dev thread: > > "The MessagePipe is already a queue. If you could arrange for the service impl > to not read messages from the pipe faster than it can fully process them, then > the client would not need to worry about throttling messages at all. It would be > nicer if the service impl could protect itself somehow instead." > > It seems like we should: > > (1) wrap the Mojo callback to another callback passed to the > PresentationServiceImpl delegate > (2) have the delegate fire the callback when the downstream consumer (i.e. media > router) is ready to accept another message > (3) have that callback fire the Mojo callback > (4) also have that callback trigger sending the next request from the renderer > queue. > > Then we will have end-to-end flow control. > > Make sense? PatchSet#7 had almost the same functionality. - Callback is passed to the delegate - After callback is invoked by delegate (ofc, no functionality yet), PresentationServiceImpl invokes mojo callback. - Then PresentationDispatcher sends next message request from the queue.
I see, Derek/Mounir, do you see any problems with that approach? I had one unsent comment as well. https://codereview.chromium.org/1037483003/diff/140001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:158: const std::vector<uint8> vector(data, data + length); Are you sure this is efficient (as good as or better than std::copy())? The std::vector ctor seems to want to loop over the input range, vs. using memcpy internally. Also, do we want to copy |data| here or can we transfer ownership to the queue?
That approach SGTM. I have a couple other comments concerning the TODOs. > - Invalidating send requests when page refreshed / navigated away scenarios. > Implemented both RenderFrameObserver::FrameWillClose() & > DidCommitProvisionalLoad(). But DidCommitProvisionalLoad() looks redundant for > me. I have noticed FrameWillClose() is invoked always prior to the > DidCommitProvisionalLoad(). > If FrameWillClose() is always invoked for both frame close and non in-page navigations, then it may be safe to just have DidCommitProvisionalLoad(). > Also, if callback is invoked immediately after calling the delegate, we can avoid Run & resetting in Reset(). Is this OK ? Could there be any other issues? So this doesn't sound like a good idea after all. Since we want to have throttling based on how fast the delegate can handle the messages. So PS#7 was correct (but still need to make sure not to drop callbacks)
https://codereview.chromium.org/1037483003/diff/140001/content/common/present... File content/common/presentation/presentation_message.h (right): https://codereview.chromium.org/1037483003/diff/140001/content/common/present... content/common/presentation/presentation_message.h:5: #ifndef CONTENT_COMMON_PRESENTATION_PRESENTATION_MESSAGE_H_ This file and the .cc file should go under content/public/browser. That way PresentationServiceDelegate will be able to legally include this file. https://codereview.chromium.org/1037483003/diff/140001/content/common/present... content/common/presentation/presentation_message.h:22: struct CONTENT_EXPORT MessageRequest { Please be more specific in the class name. Perhaps PresentationMessageRequest? Also, more documentation would be good.
https://codereview.chromium.org/1037483003/diff/140001/content/common/present... File content/common/presentation/presentation_message.h (right): https://codereview.chromium.org/1037483003/diff/140001/content/common/present... content/common/presentation/presentation_message.h:5: #ifndef CONTENT_COMMON_PRESENTATION_PRESENTATION_MESSAGE_H_ On 2015/04/22 20:12:02, imcheng1 wrote: > This file and the .cc file should go under content/public/browser. That way > PresentationServiceDelegate will be able to legally include this file. presentation_message.h needs to be included both in the PresentationDispatcher and PresentationServiceDelegate. If this go under content/public/browser, including from presentation_dispatcher.h fails. Moving presentation_message.h/.cc into "content/public/common/" works for PresentationDispatcher & PresentationServiceDelegate. BUT there is Illegal include problem in including "content/common/presentation/presentation_service.mojom.h" in the presentation_message.h. would it need to move presentation_service.mojom appropriately?
On 2015/04/23 12:39:23, s.singapati wrote: > https://codereview.chromium.org/1037483003/diff/140001/content/common/present... > File content/common/presentation/presentation_message.h (right): > > https://codereview.chromium.org/1037483003/diff/140001/content/common/present... > content/common/presentation/presentation_message.h:5: #ifndef > CONTENT_COMMON_PRESENTATION_PRESENTATION_MESSAGE_H_ > On 2015/04/22 20:12:02, imcheng1 wrote: > > This file and the .cc file should go under content/public/browser. That way > > PresentationServiceDelegate will be able to legally include this file. > > presentation_message.h needs to be included both in the PresentationDispatcher > and PresentationServiceDelegate. If this go under content/public/browser, > including from presentation_dispatcher.h fails. > > Moving presentation_message.h/.cc into "content/public/common/" works for > PresentationDispatcher & PresentationServiceDelegate. BUT there is Illegal > include problem in including > "content/common/presentation/presentation_service.mojom.h" in the > presentation_message.h. > > would it need to move presentation_service.mojom appropriately? Would it be possible to only depend on presentation::MessageRequest in PresentationDispatcher? It looks like what you are doing is converting incoming data into a content::MessageRequest, and then convert it to a presentation::MessageRequest right when it is about to be sent to PresentationServiceImpl. Looks like you should be able to have a queue of presentation::MessageRequest instead.
> Would it be possible to only depend on presentation::MessageRequest in > PresentationDispatcher? It looks like what you are doing is converting incoming > data into a content::MessageRequest, and then convert it to a > presentation::MessageRequest right when it is about to be sent to > PresentationServiceImpl. Looks like you should be able to have a queue of > presentation::MessageRequest instead. Yes, I will update the patch on Monday. Also, I am trying to construct content::MessageRequest directly form presentation::MessageRequest when sending to delegate. But it is invalid to include "presentation_service.mojom.h" in "content/public/browser/presentation_message.h". I will avoid this.
haibinlu@chromium.org changed reviewers: + haibinlu@chromium.org
https://codereview.chromium.org/1037483003/diff/140001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/140001/content/common/present... content/common/presentation/presentation_service.mojom:34: struct MessageRequest { can this be renamed to SessionMessage so that on-message can use it too?
Still updating the patch. Need cleanup and renaming the MessageRequest struct better. Had some problems in queuing mojo::MessageRequest as mojo::StructPtr, because of move only behavior of StructPtr. Currently having the end to end implementation using queue of content::MessageRequest in PresentationDispatcher, as this was easier than handling mojo::StructPtr. I can update patch tomorrow for review. https://codereview.chromium.org/1037483003/diff/140001/content/common/present... File content/common/presentation/presentation_message.h (right): https://codereview.chromium.org/1037483003/diff/140001/content/common/present... content/common/presentation/presentation_message.h:22: struct CONTENT_EXPORT MessageRequest { On 2015/04/22 20:12:02, imcheng1 wrote: > Please be more specific in the class name. Perhaps PresentationMessageRequest? > Also, more documentation would be good. In mojom file: struct "SessionMessage" would be better naming i think. so this would be accessed presentation::SessionMessage always. Also this will be used in onmessage implementation. Based on that "PresentationSessionMessage" probably the choice in content? Or any proposal for naming finalization? https://codereview.chromium.org/1037483003/diff/140001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/140001/content/common/present... content/common/presentation/presentation_service.mojom:34: struct MessageRequest { On 2015/04/24 22:13:25, haibinlu wrote: > can this be renamed to SessionMessage so that on-message can use it too? Yes, planning to rename this. SessionMessage would be better naming in mojom.h And may be struct "PresentationSessionMessage" in content module? https://codereview.chromium.org/1037483003/diff/140001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/140001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:158: const std::vector<uint8> vector(data, data + length); On 2015/04/22 19:34:23, mark a. foltz wrote: > Are you sure this is efficient (as good as or better than std::copy())? > > The std::vector ctor seems to want to loop over the input range, vs. using > memcpy internally. well, std::copy might be faster, since the vector implementations probably use std::copy internally. or may be compilers already doing such optimizations. Not sure. Only thing is it needs two step construction. std::vector<uint8> vector(length); std::copy(data, data + length, vector.begin()); OR std::vector<uint8> vector std::copy(data, data + length, back_inserter(vector)); > Also, do we want to copy |data| here or can we transfer ownership to the queue? Just to have the data in vector, i have made it like this. Also to have the MessageRequest struct usage common in browser and renderer side. I think functionality wise there is no problem.
Patchset #9 (id:160001) has been deleted
Patchset #10 (id:200001) has been deleted
Patchset #10 (id:220001) has been deleted
Is there anything we can do to move this patch forward?
On 2015/04/30 00:14:45, mark a. foltz wrote: > Is there anything we can do to move this patch forward? Hi, I have updated the patchset#9 according to our discussion. - Callback is passed to the delegate - After callback is invoked by delegate, PresentationServiceImpl invokes mojo callback. - PresentationDispatcher sends next message after receiving callback. - To handle message invalidation during page navigation scenarios, one arg mojo callback is used. (Service Impl to Dispatcher) SendMessageMojoCallback = mojo::Callback<void(bool)>; - And Non arg callback is used between ServiceImpl to Delegate, as there is no need of one arg callback as of now. - Rebased & resolved merge conflicts - Also extended unit tests. Now it has end to end functionality according to our review discussions last week. Could you please review latest patchset? Then I will try to finalize this.
lgtm with nits https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:322: callback.Run(true); nit: did you want to Run(false) here? https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:336: static_cast<PresentationMessageType>(message_request->type), nit: it'd be safer to have a static function that converts one type to another https://codereview.chromium.org/1037483003/diff/280001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/280001/content/common/present... content/common/presentation/presentation_service.mojom:91: // to get notified that message has been received and ready for next one. nits: s/message/the message s/and/and the service is/ I think you need to specify what does the value of the |success| mean too. https://codereview.chromium.org/1037483003/diff/280001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1037483003/diff/280001/content/content_browse... content/content_browser.gypi:191: 'public/browser/presentation_message.cc', do you need to update the corresponding .gn file? https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:139: presentation::SessionMessage* session_message = We probably should limit the size of the message (e.g. to 64k) and return an error for now (we could split bigger messages into chunks later). https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:161: DCHECK(data); Ditto
imcheng@google.com changed reviewers: + imcheng@google.com
Looking much better. Thank you for your patience. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:327: DCHECK(!send_message_cb_ptr_); instead of DCHECKing and/or dropping the old callback, should the new callback be invoked with false in this scenario? https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:336: static_cast<PresentationMessageType>(message_request->type), To add to what Anton said, I think it would be good to have a helper function that converts a presentation::SessionMessagePtr to a PresentationMessageRequest. There should be some browser-side validation on the message as well, such as size check and message/data matches the type. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:344: DCHECK(send_message_cb_ptr_); Hmm, Reset() could have occurred between SendMessage() and OnSendMessageCallback(). So this DCHECK isn't safe. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:226: EXPECT_EQ(success, true); EXPECT_TRUE(success) https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:705: PresentationMessageRequest* test_message = nullptr; do we have a memory leak here since the test_message is set with a new'ed value? https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:747: PresentationMessageRequest* test_message = nullptr; ditto on memory leak https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... File content/public/browser/presentation_message.h (right): https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_message.h:34: const std::string message; Could you add some comments about this field being used for TEXT type only and similarly for |data|? https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:42: using SendMessageCallback = base::Callback<void()>; using SendMessageCallback = base::Closure; https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:138: // and delegate is responsible for deallocating the message_request. Please also document |send_message_cb|. https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:142: PresentationMessageRequest* message_request, Can this be a scoped_ptr so it's less error prone to memory leaks? For mocking it's a bit tricky since gmock doesn't seem to support mocking of move-only parameters. One way around it is that in the mock delegate class, add a mock function that takes a PresentationMessageRequest* in the instead (e.g., SendMessageRawPtr), and then provide a concrete implementation of SendMessage that forwards the parameters (but |message_request| as a raw pointer) to SendMessageRawPtr().
Thanks for review. I will update the patch on Monday. Today is holiday here. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:322: callback.Run(true); On 2015/04/30 15:50:58, whywhat wrote: > nit: did you want to Run(false) here? Yes. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:705: PresentationMessageRequest* test_message = nullptr; On 2015/04/30 20:11:29, imcheng wrote: > do we have a memory leak here since the test_message is set with a new'ed value? yes, delegate is responsible for deleting the PresentationMessageRequest*. test_message needs to be deleted here in testcase. https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:142: PresentationMessageRequest* message_request, On 2015/04/30 20:11:30, imcheng wrote: > Can this be a scoped_ptr so it's less error prone to memory leaks? For mocking > it's a bit tricky since gmock doesn't seem to support mocking of move-only > parameters. One way around it is that in the mock delegate class, add a mock > function that takes a PresentationMessageRequest* in the instead (e.g., > SendMessageRawPtr), and then provide a concrete implementation of SendMessage > that forwards the parameters (but |message_request| as a raw pointer) to > SendMessageRawPtr(). Ok, I will check this one. https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:139: presentation::SessionMessage* session_message = On 2015/04/30 15:50:58, whywhat wrote: > We probably should limit the size of the message (e.g. to 64k) and return an > error for now (we could split bigger messages into chunks later). Is it ok to DCHECK the size or just return if size exceeds? or do you mean, we need api change for "returning an error"?
https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:139: presentation::SessionMessage* session_message = On 2015/05/01 10:30:25, s.singapati wrote: > On 2015/04/30 15:50:58, whywhat wrote: > > We probably should limit the size of the message (e.g. to 64k) and return an > > error for now (we could split bigger messages into chunks later). > > Is it ok to DCHECK the size or just return if size exceeds? > or do you mean, we need api change for "returning an error"? DCHECK surely won't work since a website is not restricted to 64k in anyway. Let's return and leave a TODO and a crbug.com to throw a DOMException later on.
Working on sending scoped_ptr<PresentationMessageRequest> to delegate & unit test. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:322: callback.Run(true); On 2015/04/30 15:50:58, whywhat wrote: > nit: did you want to Run(false) here? Done. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:327: DCHECK(!send_message_cb_ptr_); On 2015/04/30 20:11:29, imcheng wrote: > instead of DCHECKing and/or dropping the old callback, should the new callback > be invoked with false in this scenario? Done. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:336: static_cast<PresentationMessageType>(message_request->type), On 2015/04/30 15:50:58, whywhat wrote: > nit: it'd be safer to have a static function that converts one type to another Done for PresentationMessageType conversion. Need to add helper function for converting SessionMessagePtr to PresentationMessageRequest. This needs to consider passing scoped_ptr<PresentationMessageRequest>. I am working on this. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:344: DCHECK(send_message_cb_ptr_); On 2015/04/30 20:11:29, imcheng wrote: > Hmm, Reset() could have occurred between SendMessage() and > OnSendMessageCallback(). So this DCHECK isn't safe. Done. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:226: EXPECT_EQ(success, true); On 2015/04/30 20:11:29, imcheng wrote: > EXPECT_TRUE(success) Done. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:705: PresentationMessageRequest* test_message = nullptr; On 2015/05/01 10:30:25, s.singapati wrote: > On 2015/04/30 20:11:29, imcheng wrote: > > do we have a memory leak here since the test_message is set with a new'ed > value? > > yes, delegate is responsible for deleting the PresentationMessageRequest*. > test_message needs to be deleted here in testcase. Done. deleting test_message for now. Working on passing scoped_ptr<>. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:747: PresentationMessageRequest* test_message = nullptr; On 2015/04/30 20:11:29, imcheng wrote: > ditto on memory leak Done. deleting test_message for now. Working on passing scoped_ptr<>. https://codereview.chromium.org/1037483003/diff/280001/content/common/present... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1037483003/diff/280001/content/common/present... content/common/presentation/presentation_service.mojom:91: // to get notified that message has been received and ready for next one. On 2015/04/30 15:50:58, whywhat wrote: > nits: > s/message/the message > s/and/and the service is/ > I think you need to specify what does the value of the |success| mean too. Done. https://codereview.chromium.org/1037483003/diff/280001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1037483003/diff/280001/content/content_browse... content/content_browser.gypi:191: 'public/browser/presentation_message.cc', On 2015/04/30 15:50:58, whywhat wrote: > do you need to update the corresponding .gn file? I think it is not needed. everything corresponding to .gypi & .gn should be in place already. or Am i missing something here? https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... File content/public/browser/presentation_message.h (right): https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_message.h:34: const std::string message; On 2015/04/30 20:11:30, imcheng wrote: > Could you add some comments about this field being used for TEXT type only and > similarly for |data|? Done. https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:42: using SendMessageCallback = base::Callback<void()>; On 2015/04/30 20:11:30, imcheng wrote: > using SendMessageCallback = base::Closure; Done. https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:138: // and delegate is responsible for deallocating the message_request. On 2015/04/30 20:11:30, imcheng wrote: > Please also document |send_message_cb|. Done. https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:142: PresentationMessageRequest* message_request, On 2015/04/30 20:11:30, imcheng wrote: > Can this be a scoped_ptr so it's less error prone to memory leaks? For mocking > it's a bit tricky since gmock doesn't seem to support mocking of move-only > parameters. One way around it is that in the mock delegate class, add a mock > function that takes a PresentationMessageRequest* in the instead (e.g., > SendMessageRawPtr), and then provide a concrete implementation of SendMessage > that forwards the parameters (but |message_request| as a raw pointer) to > SendMessageRawPtr(). sending scoped_ptr to delegate is fine. But mocking in unittest seems to be tricky as you mentioned. I am still working on this. https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:139: presentation::SessionMessage* session_message = On 2015/05/01 14:09:08, whywhat wrote: > On 2015/05/01 10:30:25, s.singapati wrote: > > On 2015/04/30 15:50:58, whywhat wrote: > > > We probably should limit the size of the message (e.g. to 64k) and return an > > > error for now (we could split bigger messages into chunks later). > > > > Is it ok to DCHECK the size or just return if size exceeds? > > or do you mean, we need api change for "returning an error"? > > DCHECK surely won't work since a website is not restricted to 64k in anyway. > Let's return and leave a TODO and a http://crbug.com to throw a DOMException later on. Done. https://codereview.chromium.org/1037483003/diff/280001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:161: DCHECK(data); On 2015/04/30 15:50:58, whywhat wrote: > Ditto Done.
PTaL. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:336: static_cast<PresentationMessageType>(message_request->type), On 2015/04/30 20:11:29, imcheng wrote: > To add to what Anton said, I think it would be good to have a helper function > that converts a presentation::SessionMessagePtr to a PresentationMessageRequest. > There should be some browser-side validation on the message as well, such as > size check and message/data matches the type. Done. https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:705: PresentationMessageRequest* test_message = nullptr; On 2015/05/04 16:40:28, s.singapati wrote: > On 2015/05/01 10:30:25, s.singapati wrote: > > On 2015/04/30 20:11:29, imcheng wrote: > > > do we have a memory leak here since the test_message is set with a new'ed > > value? > > > > yes, delegate is responsible for deleting the PresentationMessageRequest*. > > test_message needs to be deleted here in testcase. > > Done. deleting test_message for now. Working on passing scoped_ptr<>. Done. passing scoped_ptr<> to delegate and MOCK SendMessageRawPtr(). https://codereview.chromium.org/1037483003/diff/280001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:747: PresentationMessageRequest* test_message = nullptr; On 2015/05/04 16:40:28, s.singapati wrote: > On 2015/04/30 20:11:29, imcheng wrote: > > ditto on memory leak > > Done. deleting test_message for now. Working on passing scoped_ptr<>. Done. https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1037483003/diff/280001/content/public/browser... content/public/browser/presentation_service_delegate.h:142: PresentationMessageRequest* message_request, On 2015/04/30 20:11:30, imcheng wrote: > Can this be a scoped_ptr so it's less error prone to memory leaks? For mocking > it's a bit tricky since gmock doesn't seem to support mocking of move-only > parameters. One way around it is that in the mock delegate class, add a mock > function that takes a PresentationMessageRequest* in the instead (e.g., > SendMessageRawPtr), and then provide a concrete implementation of SendMessage > that forwards the parameters (but |message_request| as a raw pointer) to > SendMessageRawPtr(). Done.
lgtm, but you will need to merge with https://codereview.chromium.org/1118103002/. Please see if you could use swap() to avoid copying messages.
lgtm modulo remaining comments, +1 to imcheng@'s swap() comment. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:51: DCHECK_LE(input->message.size(), Don't we want to validate this in all builds and return a null PSM? https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:57: scoped_ptr<std::string>(new std::string(input->message))); Instead of allocating a new string, pass an empty scoped_ptr and have output take ownership of the input contents as is done above. output->message.Swap(input.message.get()) https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:69: new std::vector<uint8_t>(input->data))); Same comment as above. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:345: // There could be one send message request at a time. Did you mean to say, There can be only one send message request at a time https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:346: scoped_ptr<SendMessageMojoCallback> send_message_cb_ptr_; Can you put this before |on_session_messages_callback_| to group the messaging related callbacks together? https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:815: std::string presentation_url("http://fooUrl"); Nit: Please use a proper URL with a TLD. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:855: std::string presentation_url("http://fooUrl"); Ditto https://codereview.chromium.org/1037483003/diff/360001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/360001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:22: static const size_t kMaxPresentationSessionMessageSize = 64 * 1024; // 64 KB. This should be declared in one place! Create presentation_constants.{h,cc} files if need be.
lgtm https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:346: scoped_ptr<SendMessageMojoCallback> send_message_cb_ptr_; send_message_callback_ to be consistent?
PTAL. One question. How do we differentiate ArrayBuffer & Blob in content::PresentationSessionMessage? https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:51: DCHECK_LE(input->message.size(), On 2015/05/07 01:34:30, mark a. foltz wrote: > Don't we want to validate this in all builds and return a null PSM? Done. Now it returns null PresentationSessionMessage to delegate. Added unit test. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:57: scoped_ptr<std::string>(new std::string(input->message))); On 2015/05/07 01:34:30, mark a. foltz wrote: > Instead of allocating a new string, pass an empty scoped_ptr and have output > take ownership of the input contents as is done above. > > output->message.Swap(input.message.get()) swap() can not take input->message.get() which is "const std::string&". https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:69: new std::vector<uint8_t>(input->data))); On 2015/05/07 01:34:30, mark a. foltz wrote: > Same comment as above. swap() can not take input->data.storage() which is "const std::vector<>". https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:345: // There could be one send message request at a time. On 2015/05/07 01:34:30, mark a. foltz wrote: > Did you mean to say, > > There can be only one send message request at a time Done. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:346: scoped_ptr<SendMessageMojoCallback> send_message_cb_ptr_; On 2015/05/07 01:34:30, mark a. foltz wrote: > Can you put this before |on_session_messages_callback_| to group the messaging > related callbacks together? Done. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.h:346: scoped_ptr<SendMessageMojoCallback> send_message_cb_ptr_; On 2015/05/07 02:28:20, haibinlu wrote: > send_message_callback_ to be consistent? Done. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:815: std::string presentation_url("http://fooUrl"); On 2015/05/07 01:34:30, mark a. foltz wrote: > Nit: Please use a proper URL with a TLD. Done. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl_unittest.cc:855: std::string presentation_url("http://fooUrl"); On 2015/05/07 01:34:30, mark a. foltz wrote: > Ditto Done. https://codereview.chromium.org/1037483003/diff/360001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/360001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:22: static const size_t kMaxPresentationSessionMessageSize = 64 * 1024; // 64 KB. On 2015/05/07 01:34:30, mark a. foltz wrote: > This should be declared in one place! > Create presentation_constants.{h,cc} files if need be. Done.
Re: your question - we don't yet. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:57: scoped_ptr<std::string>(new std::string(input->message))); On 2015/05/07 14:08:51, s.singapati wrote: > On 2015/05/07 01:34:30, mark a. foltz wrote: > > Instead of allocating a new string, pass an empty scoped_ptr and have output > > take ownership of the input contents as is done above. > > > > output->message.Swap(input.message.get()) > > swap() can not take input->message.get() which is "const std::string&". Swap is reflective so you can use input->message.Swap(output->message.get()); If possible (e.g. dependency is allowed) I would pass mojo::String to createStringMessage so this swapping happens in the function, not afterwards. https://codereview.chromium.org/1037483003/diff/360001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:69: new std::vector<uint8_t>(input->data))); On 2015/05/07 14:08:51, s.singapati wrote: > On 2015/05/07 01:34:30, mark a. foltz wrote: > > Same comment as above. > > swap() can not take input->data.storage() which is "const std::vector<>". Ditto.
PTAL. Addressed all comments. I'm sorry for delays but I will do better. Thanks. > > On 2015/05/07 01:34:30, mark a. foltz wrote: > > > Instead of allocating a new string, pass an empty scoped_ptr and have output > > > take ownership of the input contents as is done above. > > > > > > output->message.Swap(input.message.get()) > > > > swap() can not take input->message.get() which is "const std::string&". > > Swap is reflective so you can use input->message.Swap(output->message.get()); > If possible (e.g. dependency is allowed) I would pass mojo::String to > createStringMessage so this swapping happens in the function, not afterwards. input->message.Swap(output->message.get()) works. > > swap() can not take input->data.storage() which is "const std::vector<>". > > Ditto. Done.
lgtm
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, imcheng@google.com, haibinlu@chromium.org Link to the patchset: https://codereview.chromium.org/1037483003/#ps400001 (title: "Use Swap to avoid copying data.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037483003/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, haibinlu@chromium.org, imcheng@google.com, avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/1037483003/#ps420001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037483003/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
s.singapati@samsung.com changed reviewers: + nasko@chromium.org
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, haibinlu@chromium.org, imcheng@google.com, avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/1037483003/#ps440001 (title: "Fixed trybots errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037483003/440001
s.singapati@samsung.com changed reviewers: + avi@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Just adding an OWNER to the review list won't let the CQ pass. Let's wait for Avi's review.
Hi Avi, Could you please take a look? Thanks.
lgtm Fix the style nits, typos, etc. https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:51: // Return null PresentationSessionMessage if size excceded. typo: exceeded https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:58: scoped_ptr<std::string>(new std::string)); indent 4 also, make_scoped_ptr https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:65: // Return null PresentationSessionMessage if size excceded. ditto https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:72: scoped_ptr<std::vector<uint8_t>>(new std::vector<uint8_t>)); indent 4 also, make_scoped_ptr https://codereview.chromium.org/1037483003/diff/440001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/440001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:210: session_message.data.storage()); wrap at 4
https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:51: // Return null PresentationSessionMessage if size excceded. On 2015/05/11 14:34:41, Avi wrote: > typo: exceeded Done. https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:58: scoped_ptr<std::string>(new std::string)); On 2015/05/11 14:34:41, Avi wrote: > indent 4 > > also, make_scoped_ptr Done. https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:65: // Return null PresentationSessionMessage if size excceded. On 2015/05/11 14:34:41, Avi wrote: > ditto Done. https://codereview.chromium.org/1037483003/diff/440001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:72: scoped_ptr<std::vector<uint8_t>>(new std::vector<uint8_t>)); On 2015/05/11 14:34:41, Avi wrote: > indent 4 > > also, make_scoped_ptr Done. https://codereview.chromium.org/1037483003/diff/440001/content/renderer/prese... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1037483003/diff/440001/content/renderer/prese... content/renderer/presentation/presentation_dispatcher.cc:210: session_message.data.storage()); On 2015/05/11 14:34:41, Avi wrote: > wrap at 4 Done.
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, mfoltz@chromium.org, haibinlu@chromium.org, imcheng@google.com, avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/1037483003/#ps460001 (title: "Fix nits, trybots errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037483003/460001
Drive-by comments on newly added files. https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... File content/public/common/presentation_constants.cc (right): https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... content/public/common/presentation_constants.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. No "(c)" in new files. https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... File content/public/common/presentation_constants.h (right): https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... content/public/common/presentation_constants.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. No "(c)" in new files.
The CQ bit was unchecked by nasko@chromium.org
https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... File content/public/common/presentation_constants.cc (right): https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... content/public/common/presentation_constants.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/05/11 15:18:55, nasko wrote: > No "(c)" in new files. Done. https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... File content/public/common/presentation_constants.h (right): https://codereview.chromium.org/1037483003/diff/460001/content/public/common/... content/public/common/presentation_constants.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/05/11 15:18:55, nasko wrote: > No "(c)" in new files. Done.
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, mfoltz@chromium.org, haibinlu@chromium.org, imcheng@google.com, avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/1037483003/#ps480001 (title: "No (c) in new files Copyright message.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1037483003/480001
Message was sent while issue was closed.
Committed patchset #22 (id:480001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/834d10a11de9909062d6817583f1a769d6465a3c Cr-Commit-Position: refs/heads/master@{#329154} |
