Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1118103002:
Implements ListenForSessionMessages in PresentationServiceImpl; uses Swap to avoid copying large da… (Closed)
DescriptionImplements ListenForSessionMessages in PresentationServiceImpl; uses Swap to avoid copying large data fields. Because media router merely passes messages back and forth, it is better to move data field between mojo message struct and mr message struct.
BUG=
Committed: https://crrev.com/f7b39b78d4e052cd57fe9a277e578c4078c7ce5f
Cr-Commit-Position: refs/heads/master@{#328417}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added unit test #
Total comments: 17
Patch Set 3 : Addresses Anton's comments. #
Total comments: 27
Patch Set 4 : Addresses Mark's comments #
Total comments: 16
Patch Set 5 : Addresses Derek's comments #Patch Set 6 : #
Total comments: 8
Patch Set 7 : #Patch Set 8 : rebase #
Total comments: 1
Patch Set 9 : Avoids uniform initialization when creating std:vector #Patch Set 10 : #Messages
Total messages: 37 (13 generated)
imcheng@google.com changed reviewers: + imcheng@google.com
https://codereview.chromium.org/1118103002/diff/1/content/browser/presentatio... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/1/content/browser/presentatio... content/browser/presentation/presentation_service_impl.cc:348: weak_factory_.GetWeakPtr(), callback)); I am not sure about binding the mojo callback to OnSessionMessages. There is no guarantee when OnSessionMessages will be invoked and by that time the onmessage handler could already be gone. (or page navigated, etc.) One possible approach is to store callback as a scoped_ptr<> in PresentationServiceImpl and then invoke it when OnSessionMessages comes back. That way we can also invoke the callback and then reset the ptr on Reset() (which is called on navigation and frame deletion) https://codereview.chromium.org/1118103002/diff/1/content/browser/presentatio... content/browser/presentation/presentation_service_impl.cc:353: scoped_ptr<ScopedVector<PresentationSessionMessage>> messages) { should probably add a DCHECK on messages
haibinlu@chromium.org changed reviewers: + avayvod@chromium.org, mfoltz@chromium.org
https://codereview.chromium.org/1118103002/diff/1/content/browser/presentatio... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/1/content/browser/presentatio... content/browser/presentation/presentation_service_impl.cc:348: weak_factory_.GetWeakPtr(), callback)); On 2015/04/30 21:52:01, imcheng wrote: > I am not sure about binding the mojo callback to OnSessionMessages. There is no > guarantee when OnSessionMessages will be invoked and by that time the onmessage > handler could already be gone. (or page navigated, etc.) > > One possible approach is to store callback as a scoped_ptr<> in > PresentationServiceImpl and then invoke it when OnSessionMessages comes back. > That way we can also invoke the callback and then reset the ptr on Reset() > (which is called on navigation and frame deletion) Done.
lgtm with nits https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:366: mojoMessages[i] = nits: this needs to be in curly braces {} https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:378: if (input->message->size() == 0) { nit: could be if (input->message->empty()) { https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:271: void OnSessionMessages( Please, document that for optimization purposes, this method will empty the messages passed to it. On the related note, do we know if copying the messages would be so bad? How often to pages send messages and how big the payload typically is? https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:274: static presentation::SessionMessagePtr ToMojoSessionMessage( move to the .cc file and the unnamed namespace? doesn't have to be a class member. https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:328: scoped_ptr<SessionMessagesCallback> on_session_messages_; nit: postfix the name with callback_. without knowing the type this looks like a vector of messages, not a callback. https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:92: void(int render_process_id, nit: use normal indent, e.g. 4 spaces. https://codereview.chromium.org/1118103002/diff/20001/content/content_browser... File content/content_browser.gypi (right): https://codereview.chromium.org/1118103002/diff/20001/content/content_browser... content/content_browser.gypi:437: 'browser/background_sync/background_sync_network_observer.h', Accidental change? https://codereview.chromium.org/1118103002/diff/20001/content/public/browser/... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1118103002/diff/20001/content/public/browser/... content/public/browser/presentation_service_delegate.h:8: #include <vector> Doesn't seem like you need this include here. https://codereview.chromium.org/1118103002/diff/20001/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1118103002/diff/20001/content/public/browser/... content/public/browser/presentation_session_message.h:17: // If this is a text message, data.size is 0; otherwise, message.size is 0. Why not one of the message/data is nullptr? Why not a union with an enum for the type? At least a method like isBinary() would be better than exposing this implementation detail to the caller.
PTAL https://codereview.chromium.org/1118103002/diff/1/content/browser/presentatio... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/1/content/browser/presentatio... content/browser/presentation/presentation_service_impl.cc:353: scoped_ptr<ScopedVector<PresentationSessionMessage>> messages) { On 2015/04/30 21:52:01, imcheng wrote: > should probably add a DCHECK on messages Done. https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:366: mojoMessages[i] = On 2015/05/01 15:26:12, whywhat wrote: > nits: this needs to be in curly braces {} Done. https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:378: if (input->message->size() == 0) { On 2015/05/01 15:26:12, whywhat wrote: > nit: could be if (input->message->empty()) { Done. https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl.h (right): https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:271: void OnSessionMessages( On 2015/05/01 15:26:12, whywhat wrote: > Please, document that for optimization purposes, this method will empty the > messages passed to it. > On the related note, do we know if copying the messages would be so bad? How > often to pages send messages and how big the payload typically is? Done. Message size is up to 64KB. With get-next-batch-message pattern, messages will be sent as fast as blink+MR can handle. With copying, each message will be copied at least 4 times along the way, one per each end of the two mojo pipes when converting to/from mojo message struct type. https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:274: static presentation::SessionMessagePtr ToMojoSessionMessage( On 2015/05/01 15:26:12, whywhat wrote: > move to the .cc file and the unnamed namespace? doesn't have to be a class > member. Done. https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl.h:328: scoped_ptr<SessionMessagesCallback> on_session_messages_; On 2015/05/01 15:26:12, whywhat wrote: > nit: postfix the name with callback_. without knowing the type this looks like a > vector of messages, not a callback. Done. https://codereview.chromium.org/1118103002/diff/20001/content/content_browser... File content/content_browser.gypi (right): https://codereview.chromium.org/1118103002/diff/20001/content/content_browser... content/content_browser.gypi:437: 'browser/background_sync/background_sync_network_observer.h', On 2015/05/01 15:26:12, whywhat wrote: > Accidental change? "git cl format" auto removed the trailing spaces. https://codereview.chromium.org/1118103002/diff/20001/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1118103002/diff/20001/content/public/browser/... content/public/browser/presentation_session_message.h:17: // If this is a text message, data.size is 0; otherwise, message.size is 0. On 2015/05/01 15:26:12, whywhat wrote: > Why not one of the message/data is nullptr? Why not a union with an enum for the > type? > At least a method like isBinary() would be better than exposing this > implementation detail to the caller. Done.
https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:22: presentation::SessionMessagePtr ToMojoSessionMessage( Add // documentation for this function. Note that the return value takes ownership of the content of |input|. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:23: content::PresentationSessionMessage* input) { const content::PresentationSessionMessage& for |input| https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:368: if (on_session_messages_callback_.get()) { When could this happen? Does this mean that the caller is invoking ListenForSessionMessages() a second time without waiting for the callback to complete? This seems like a bug - I would DCHECK() and LOG(WARNING) if this happens. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:381: scoped_ptr<ScopedVector<PresentationSessionMessage>> messages) { I've usually seen std::vector<linked_ptr<Foo>> used to ensure that all Foo's get deleted when the vector (and other containers that have a Foo) are deleted. It seems like the messages vector "owns" the PresentationSessionMessage so this is probably okay. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:382: DCHECK(messages.get()); What if messages->size() == 0? https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:384: // Reseted What was reset? The callback? https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:651: } Would be a good idea to test: - Reset() runs the session messages cb - Invoking Listen() twice runs and resets the callback - Empty binary + string messages are okay https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_service_delegate.h:140: // Gtts the next batch of messages of all presentation session in the frame. Typo in Gtts https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_service_delegate.h:140: // Gtts the next batch of messages of all presentation session in the frame. s/of/from/ s/session/sessions/ https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_service_delegate.h:142: // |message_cb|: Invoked with a list of messages. Can this list be empty? Clarify. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... File content/public/browser/presentation_session_message.cc (right): https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.cc:30: return data != nullptr && !data->empty(); Empty messages are allowed by the current spec. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.h:17: // If this is a text message, data.size is 0; otherwise, message.size is 0. data/message are nullptr, not a zero-length object. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.h:19: explicit PresentationSessionMessage(const std::string& presentation_url, explicit is only required for 1-arg ctors. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.h:22: explicit PresentationSessionMessage(const std::string& presentation_url, Prefer making these ctors private and exposing static factory methods: PresentationSessionMessage::CreateStringMessage() PresentationSessionMessage::CreateBinaryMessage()
PTAL https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1118103002/diff/20001/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:92: void(int render_process_id, On 2015/05/01 15:26:12, whywhat wrote: > nit: use normal indent, e.g. 4 spaces. Done. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:22: presentation::SessionMessagePtr ToMojoSessionMessage( On 2015/05/01 19:42:07, mark a. foltz wrote: > Add // documentation for this function. > Note that the return value takes ownership of the content of |input|. Done. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:23: content::PresentationSessionMessage* input) { On 2015/05/01 19:42:07, mark a. foltz wrote: > const content::PresentationSessionMessage& for |input| cosnt does not work with Swap https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:368: if (on_session_messages_callback_.get()) { On 2015/05/01 19:42:07, mark a. foltz wrote: > When could this happen? Does this mean that the caller is invoking > ListenForSessionMessages() a second time without waiting for the callback to > complete? > > This seems like a bug - I would DCHECK() and LOG(WARNING) if this happens. Done. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:381: scoped_ptr<ScopedVector<PresentationSessionMessage>> messages) { On 2015/05/01 19:42:07, mark a. foltz wrote: > I've usually seen std::vector<linked_ptr<Foo>> used to ensure that all Foo's get > deleted when the vector (and other containers that have a Foo) are deleted. > > It seems like the messages vector "owns" the PresentationSessionMessage so this > is probably okay. > > Acknowledged. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:382: DCHECK(messages.get()); On 2015/05/01 19:42:07, mark a. foltz wrote: > What if messages->size() == 0? Done. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:384: // Reseted On 2015/05/01 19:42:07, mark a. foltz wrote: > What was reset? The callback? Clarified the comments. https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1118103002/diff/40001/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:651: } On 2015/05/01 19:42:07, mark a. foltz wrote: > Would be a good idea to test: > - Reset() runs the session messages cb > - Invoking Listen() twice runs and resets the callback > - Empty binary + string messages are okay > Done. Now DCHECK multiple listen, thus this case is not tested. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... File content/public/browser/presentation_service_delegate.h (right): https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_service_delegate.h:140: // Gtts the next batch of messages of all presentation session in the frame. On 2015/05/01 19:42:07, mark a. foltz wrote: > Typo in Gtts Done. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_service_delegate.h:142: // |message_cb|: Invoked with a list of messages. On 2015/05/01 19:42:07, mark a. foltz wrote: > Can this list be empty? Clarify. Done. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... File content/public/browser/presentation_session_message.cc (right): https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.cc:30: return data != nullptr && !data->empty(); On 2015/05/01 19:42:07, mark a. foltz wrote: > Empty messages are allowed by the current spec. Done. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.h:17: // If this is a text message, data.size is 0; otherwise, message.size is 0. On 2015/05/01 19:42:07, mark a. foltz wrote: > data/message are nullptr, not a zero-length object. Done. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.h:19: explicit PresentationSessionMessage(const std::string& presentation_url, On 2015/05/01 19:42:07, mark a. foltz wrote: > explicit is only required for 1-arg ctors. Acknowledged. https://codereview.chromium.org/1118103002/diff/40001/content/public/browser/... content/public/browser/presentation_session_message.h:22: explicit PresentationSessionMessage(const std::string& presentation_url, On 2015/05/01 19:42:07, mark a. foltz wrote: > Prefer making these ctors private and exposing static factory methods: > > PresentationSessionMessage::CreateStringMessage() > PresentationSessionMessage::CreateBinaryMessage() Done.
https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:369: DCHECK(!on_session_messages_callback_.get()); I think we should just CHECK here. If renderer is misbehaving, it's better to crash early. https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:380: DCHECK(messages.get() && messages->size() > 0); nit: check !messages->empty() instead of size() > 0. https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:391: on_session_messages_callback_->Run(mojoMessages.Pass()); Reset on_session_messages_callback_ after Run(). https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:446: mojo::Array<presentation::SessionMessagePtr>()); It looks like you would also need to change PresentationDispatcher to handle null in the callback. https://codereview.chromium.org/1118103002/diff/60001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1118103002/diff/60001/content/common/presenta... content/common/presentation/presentation_service.mojom:102: => (array<SessionMessage>? messages); Document what it means to invoke the callback with null. https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... content/public/browser/presentation_session_message.h:24: static scoped_ptr<PresentationSessionMessage> CreateStringMessage( I think you also need to add CONTENT_EXPORT to static functions.
https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... File content/public/browser/presentation_session_message.cc (right): https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... content/public/browser/presentation_session_message.cc:29: scoped_ptr<PresentationSessionMessage> Add comment: // static https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... content/public/browser/presentation_session_message.cc:38: scoped_ptr<PresentationSessionMessage> ditto
PTAL https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:369: DCHECK(!on_session_messages_callback_.get()); On 2015/05/04 19:53:55, imcheng wrote: > I think we should just CHECK here. If renderer is misbehaving, it's better to > crash early. Acknowledged. https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:380: DCHECK(messages.get() && messages->size() > 0); On 2015/05/04 19:53:55, imcheng wrote: > nit: check !messages->empty() instead of size() > 0. Done. https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:391: on_session_messages_callback_->Run(mojoMessages.Pass()); On 2015/05/04 19:53:55, imcheng wrote: > Reset on_session_messages_callback_ after Run(). Done. https://codereview.chromium.org/1118103002/diff/60001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:446: mojo::Array<presentation::SessionMessagePtr>()); On 2015/05/04 19:53:55, imcheng wrote: > It looks like you would also need to change PresentationDispatcher to handle > null in the callback. Done. https://codereview.chromium.org/1118103002/diff/60001/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1118103002/diff/60001/content/common/presenta... content/common/presentation/presentation_service.mojom:102: => (array<SessionMessage>? messages); On 2015/05/04 19:53:55, imcheng wrote: > Document what it means to invoke the callback with null. Done. https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... File content/public/browser/presentation_session_message.cc (right): https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... content/public/browser/presentation_session_message.cc:29: scoped_ptr<PresentationSessionMessage> On 2015/05/04 21:21:58, imcheng wrote: > Add comment: > > // static Done. https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... content/public/browser/presentation_session_message.cc:38: scoped_ptr<PresentationSessionMessage> On 2015/05/04 21:21:58, imcheng wrote: > ditto Done. https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1118103002/diff/60001/content/public/browser/... content/public/browser/presentation_session_message.h:24: static scoped_ptr<PresentationSessionMessage> CreateStringMessage( On 2015/05/04 19:53:55, imcheng wrote: > I think you also need to add CONTENT_EXPORT to static functions. Done.
Did you upload the most recent patchset? I don't see the changes mentioned in your latest comments.
the latest change was uploaded.
lgtm
haibinlu@chromium.org changed reviewers: + avi@chromium.org
mfoltz@chromium.org changed reviewers: - avi@chromium.org
lgtm % a few remaining comments. Can you please look at https://codereview.chromium.org/1037483003/ before landing and see if we can use one version of presentation_session_message for both send and receive? https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:22: // The return value takes ownership of the actual message of |input|. s/the actual message/the contents/ https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:26: output->presentation_url = input->presentation_url; Use std::swap() here, so that we avoid copying and avoid returning a partially-filled |input|. https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:27: output->presentation_id = input->presentation_id; Ditto https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:365: callback.Run(mojo::Array<presentation::SessionMessagePtr>()); Does |callback| also need to be reset() here?
Yes, https://codereview.chromium.org/1037483003/ and this CL can use the same presentation_session_message. However, the current presentation_session_message of CL/1037483003 uses copying for its content (up to 64K). The class in this CL uses move. https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:22: // The return value takes ownership of the actual message of |input|. On 2015/05/05 00:22:13, mark a. foltz wrote: > s/the actual message/the contents/ Done. https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:26: output->presentation_url = input->presentation_url; On 2015/05/05 00:22:13, mark a. foltz wrote: > Use std::swap() here, so that we avoid copying and avoid returning a > partially-filled |input|. Done. https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:27: output->presentation_id = input->presentation_id; On 2015/05/05 00:22:13, mark a. foltz wrote: > Ditto Done. https://codereview.chromium.org/1118103002/diff/100001/content/browser/presen... content/browser/presentation/presentation_service_impl.cc:365: callback.Run(mojo::Array<presentation::SessionMessagePtr>()); On 2015/05/05 00:22:13, mark a. foltz wrote: > Does |callback| also need to be reset() here? no. callback is a const.
haibinlu@chromium.org changed reviewers: + avi@chromium.org
Hi, Avi, Could you take a look at files under content/public/browser/? Thanks!
lgtm
The CQ bit was checked by haibinlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, imcheng@google.com, mfoltz@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1118103002/#ps20002 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118103002/20002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
https://codereview.chromium.org/1118103002/diff/20002/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1118103002/diff/20002/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:658: std::vector<uint8_t> binary_data{'\1', '\2', '\3'}; Oops, uniform initialization is banned in Chromium until we have library support on all platforms: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw
The CQ bit was checked by haibinlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@google.com, avayvod@chromium.org, avi@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1118103002/#ps150001 (title: "Avoids uniform initialization when creating std:vector")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118103002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by haibinlu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@google.com, avayvod@chromium.org, avi@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1118103002/#ps170001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1118103002/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f7b39b78d4e052cd57fe9a277e578c4078c7ce5f Cr-Commit-Position: refs/heads/master@{#328417} |
