Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1978183003:
Refactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs. (Closed)
|
Created:
4 years, 7 months ago by terelius-chromium Modified:
4 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs.
Rationale: The WebRtcLoggingHandlerHost class is currently quite large (over 800 lines) and does many different things including looking up system information, writing the text log, receiving and sending IPC messages and passing finished logs to the LogUploader. In addition to the original text log, the WebRtcLoggingHandlerHost also creates and manages the rtp dump through a WebRtcRtpDumpHandler object, and we would like to add a WebRtcEventLogHandler object.
Proposal: We propose to split out the parts that are specific to the text log into a new WebRtcTextLogHandler class. The WebRtcTextLogHandler will own the actual text buffer, keep track of the logging state and fetch the system information that's written at the start of the log. The new WebRtcLoggingHandlerHost will have ownership of the WebRtcTextLogHandler, WebRtcRtpDumpHandler and WebRtcEventLogHandler objects, receive and send IPC messages, propagate API calls to the right log, and pass the finished logs to the LogUploader.
Committed: https://crrev.com/70957b9b7168c2009099ece92fd0f8928a1b46b8
Cr-Commit-Position: refs/heads/master@{#421239}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Misc. reviewer comments #
Total comments: 3
Patch Set 3 : Fix race when renderer is closing while the log is in STARTING or STOPPING state #
Total comments: 25
Patch Set 4 : Comments from magjed #
Total comments: 13
Patch Set 5 : Comments from magjed 2. #
Total comments: 4
Patch Set 6 : Nits from tommi #Patch Set 7 : Rebase #Patch Set 8 : Fix windows compile error #
Messages
Total messages: 44 (22 generated)
terelius@chromium.org changed reviewers: + grunell@chromium.org
I'd like to discuss the WebRtcLoggingHandlerHost::OnChannelClosing method which seems a bit error prone (both old and new code, but possibly worse in new code). Apart from that, I think the code looks reasonable. Please take a look.
Before going into detail, I think the responsibilities need to be clarified (at least in code). Also, please outline the changes made and why in the description. https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.cc:74: if (text_log_handler_->GetState() != WebRtcTextLogHandler::CLOSED) { How about only if (text_log_handler_->StartLogging(callback)) Send(new WebRtcLoggingMsg_StartLogging()); and let the text log handler deal with the conditions and running callbacks? https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.cc:119: if (text_log_handler_->GetState() != WebRtcTextLogHandler::STOPPED) { The division of responsibilities is not clear. We fire a callback here, but also in the text log handler in FireGenericDoneCallback(). Also, checking the states and making decisions is done here. They seem to be quite coupled and I think they need to be more independent. The uploading deals with both text and rtp logs, right? Still, the text log state is determining what to do. Just reading this function, it looks like it only handles the text log. https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.h:123: friend class WebRtcTextLogHandler; This should be avoided unless one has a very good reason. https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_text_log_handler.h (right): https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_text_log_handler.h:59: class WebRtcTextLogHandler Great to move this out!
Description was changed from ========== Refactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs. BUG= ========== to ========== Refactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs. Rationale: The WebRtcLoggingHandlerHost class is currently quite large (over 800 lines) and does many different things including looking up system information, writing the text log, receiving and sending IPC messages and passing finished logs to the LogUploader. In addition to the original text log, the WebRtcLoggingHandlerHost also creates and manages the rtp dump through a WebRtcRtpDumpHandler object, and we would like to add a WebRtcEventLogHandler object. Proposal: We propose to split out the parts that are specific to the text log into a new WebRtcTextLogHandler class. The WebRtcTextLogHandler will own the actual text buffer, keep track of the logging state and fetch the system information that's written at the start of the log. The new WebRtcLoggingHandlerHost will have ownership of the WebRtcTextLogHandler, WebRtcRtpDumpHandler and WebRtcEventLogHandler objects, receive and send IPC messages, propagate API calls to the right log, and pass the finished logs to the LogUploader. ==========
https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.cc:74: if (text_log_handler_->GetState() != WebRtcTextLogHandler::CLOSED) { On 2016/05/17 07:29:17, Henrik Grunell wrote: > How about only > > if (text_log_handler_->StartLogging(callback)) > Send(new WebRtcLoggingMsg_StartLogging()); > > and let the text log handler deal with the conditions and running callbacks? Done. I have to pass in a pointer to the LogUploader to make that work though. https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.cc:119: if (text_log_handler_->GetState() != WebRtcTextLogHandler::STOPPED) { On 2016/05/17 07:29:17, Henrik Grunell wrote: > The division of responsibilities is not clear. We fire a callback here, but also > in the text log handler in FireGenericDoneCallback(). Also, checking the states > and making decisions is done here. They seem to be quite coupled and I think > they need to be more independent. > > The uploading deals with both text and rtp logs, right? Still, the text log > state is determining what to do. Just reading this function, it looks like it > only handles the text log. I agree that they should preferably be more independent. However, since I am moving quite a lot of code, I'd also like to avoid too many behavioral changes in the same CL. The way I envision it, the LoggingHandlerHost will act as a common controller for all logs. It will send and receive IPC, forward messages to the right log and provide an interface to start, stop and upload logs. The TextLogHandler (and future EventLogHandler) will do the actual logging and keep track of its own state, but be unaware of each other and the LogUploader. Since the LoggingHandlerHost will control the other logs, it probably needs to read (but not change) the logging state. Yes, this function will upload both the text log and rtp dump (if available). How would you suggest that I clarify this? In a comment, or do you have some structural code change in mind? https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.h:123: friend class WebRtcTextLogHandler; On 2016/05/17 07:29:17, Henrik Grunell wrote: > This should be avoided unless one has a very good reason. Done. This was a temporary hack while code as being moved between the classes, and not intended for production code. Thanks for catching it! https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_text_log_handler.h (right): https://codereview.chromium.org/1978183003/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_text_log_handler.h:59: class WebRtcTextLogHandler On 2016/05/17 07:29:17, Henrik Grunell wrote: > Great to move this out! Acknowledged.
Just a comment on the OnChannelClosing() that we discussed offline. https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:288: // TODO(terelius): I think we could also be in the STOPPING state here. It sure seems like we should handle starting and stopping states as well. For the stopping state: I think what we wait for is to let the render side send any buffered log messages to us, so ideally we would allow that to happen. But since we're shutting down all bets are off anyway and we can't rely on that, so we need to just stop and upload what we have. For starting state: We may want to upload even if only meta data and "initial info" is in there, but we could also just don't do that since it's anyway a matter of racing what comes first. So, don't care about starting, but comment on it. And for stopping, just trigger an upload and handle and comment in OnLoggingStoppedInRenderer(). You may want an ugly flag raised here to check there if shutting down is the case if we're not in stopping state.
https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:288: // TODO(terelius): I think we could also be in the STOPPING state here. On 2016/07/04 16:51:36, Henrik Grunell wrote: > It sure seems like we should handle starting and stopping states as well. > > For the stopping state: > I think what we wait for is to let the render side send any buffered log > messages to us, so ideally we would allow that to happen. But since we're > shutting down all bets are off anyway and we can't rely on that, so we need to > just stop and upload what we have. > > For starting state: > We may want to upload even if only meta data and "initial info" is in there, but > we could also just don't do that since it's anyway a matter of racing what comes > first. > > So, don't care about starting, but comment on it. And for stopping, just trigger > an upload and handle and comment in OnLoggingStoppedInRenderer(). You may want > an ugly flag raised here to check there if shutting down is the case if we're > not in stopping state. Ok, but that doesn't solve the issue with checking the state transitions. I'd have to remove DCHECKing the expected state when we enter the each function, to accommodate the OnChannelClosing function. What do you think about adding a DYING state with no transitions out of that state? In the destructor, we could check that we are the state is either CLOSED or DYING, and similar in the other functions.
https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:288: // TODO(terelius): I think we could also be in the STOPPING state here. On 2016/07/05 09:36:54, terelius-chromium wrote: > On 2016/07/04 16:51:36, Henrik Grunell wrote: > > It sure seems like we should handle starting and stopping states as well. > > > > For the stopping state: > > I think what we wait for is to let the render side send any buffered log > > messages to us, so ideally we would allow that to happen. But since we're > > shutting down all bets are off anyway and we can't rely on that, so we need to > > just stop and upload what we have. > > > > For starting state: > > We may want to upload even if only meta data and "initial info" is in there, > but > > we could also just don't do that since it's anyway a matter of racing what > comes > > first. > > > > So, don't care about starting, but comment on it. And for stopping, just > trigger > > an upload and handle and comment in OnLoggingStoppedInRenderer(). You may want > > an ugly flag raised here to check there if shutting down is the case if we're > > not in stopping state. > > Ok, but that doesn't solve the issue with checking the state transitions. I'd > have to remove DCHECKing the expected state when we enter the each function, to > accommodate the OnChannelClosing function. Yes, there's no other way really than to DCHECK for any expected states. > > What do you think about adding a DYING state with no transitions out of that > state? In the destructor, we could check that we are the state is either CLOSED > or DYING, and similar in the other functions. That's fine too, it would make DCHECKing clearer. Maybe call it CHANNEL_CLOSING?
On 2016/07/05 10:56:15, Henrik Grunell wrote: > https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... > File chrome/browser/media/webrtc_logging_handler_host.cc (right): > > https://codereview.chromium.org/1978183003/diff/20001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:288: // TODO(terelius): I > think we could also be in the STOPPING state here. > On 2016/07/05 09:36:54, terelius-chromium wrote: > > On 2016/07/04 16:51:36, Henrik Grunell wrote: > > > It sure seems like we should handle starting and stopping states as well. > > > > > > For the stopping state: > > > I think what we wait for is to let the render side send any buffered log > > > messages to us, so ideally we would allow that to happen. But since we're > > > shutting down all bets are off anyway and we can't rely on that, so we need > to > > > just stop and upload what we have. > > > > > > For starting state: > > > We may want to upload even if only meta data and "initial info" is in there, > > but > > > we could also just don't do that since it's anyway a matter of racing what > > comes > > > first. > > > > > > So, don't care about starting, but comment on it. And for stopping, just > > trigger > > > an upload and handle and comment in OnLoggingStoppedInRenderer(). You may > want > > > an ugly flag raised here to check there if shutting down is the case if > we're > > > not in stopping state. > > > > Ok, but that doesn't solve the issue with checking the state transitions. I'd > > have to remove DCHECKing the expected state when we enter the each function, > to > > accommodate the OnChannelClosing function. > > Yes, there's no other way really than to DCHECK for any expected states. > > > > > What do you think about adding a DYING state with no transitions out of that > > state? In the destructor, we could check that we are the state is either > CLOSED > > or DYING, and similar in the other functions. > > That's fine too, it would make DCHECKing clearer. Maybe call it CHANNEL_CLOSING? Done. Please take another look.
terelius@chromium.org changed reviewers: + guidou@chromium.org, tommi@chromium.org
terelius@chromium.org changed reviewers: - guidou@chromium.org
Tommi, could you review this CL?
tommi@chromium.org changed reviewers: + magjed@chromium.org - tommi@chromium.org
Hej Bjorn - I'm sorry but I won't have time to review today and I'm off next week. I've asked magjed@ to take a look. He'll be around next week. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:288: if (text_log_handler_->GetState() == WebRtcTextLogHandler::STARTING || Call GetState() only once?
First pass. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:288: if (text_log_handler_->GetState() == WebRtcTextLogHandler::STARTING || On 2016/07/15 10:11:36, tommi-chrömium wrote: > Call GetState() only once? How about a switch? switch (text_log_handler_->GetState()) { case WebRtcTextLogHandler::STARTING: case WebRtcTextLogHandler::STARTED: case WebRtcTextLogHandler::STOPPING: case WebRtcTextLogHandler::STOPPED: ... break; case WebRtcTextLogHandler::CLOSED: case WebRtcTextLogHandler::CHANNEL_CLOSING: ... break; default: NOT_REACHED(); } https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:47: void FormatMetaDataAsLogMessage(const MetaDataMap& meta_data, Return the string instead of sending in a pointer. It will use move semantic, so no copy will be made. Actually in this case, RVO will remove the temporary string completely. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:137: DCHECK(!log_buffer_.get()); std::unique_pointer has an operator bool(), so you can remove '.get()'. Same goes for the other places where you check if a std::unique_pointer owns an object. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:160: (*meta_data_.get())[it.first] = it.second; std::map has a copy assignment operator that is cleaner and more efficient than this. Just remove the loop and do '*meta_data_ = *meta_data'. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.h (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:66: // RendererDying(): ANY -> DYING. Is DYING == CHANNEL_CLOSING? https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:73: CHANNEL_CLOSING, // Renderer is closing. TODO TODO what? https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:76: public: Remove this extra 'public:'. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:93: // Called automatically after StartLogging(). Do not call directly. Why not make it private if it shouldn't be called directly? https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:102: // renderer. Do not call directly. Must be called on the IO thread. What does "Do not call directly." mean? It is called directly from WebRtcLoggingHandlerHost. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:144: int render_process_id_; Make const.
The CQ bit was checked by terelius@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:288: if (text_log_handler_->GetState() == WebRtcTextLogHandler::STARTING || On 2016/07/19 14:53:16, magjed_chromium wrote: > On 2016/07/15 10:11:36, tommi-chrömium wrote: > > Call GetState() only once? > > How about a switch? > switch (text_log_handler_->GetState()) { > case WebRtcTextLogHandler::STARTING: > case WebRtcTextLogHandler::STARTED: > case WebRtcTextLogHandler::STOPPING: > case WebRtcTextLogHandler::STOPPED: > ... > break; > > case WebRtcTextLogHandler::CLOSED: > case WebRtcTextLogHandler::CHANNEL_CLOSING: > ... > break; > > default: > NOT_REACHED(); > } Done. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:288: if (text_log_handler_->GetState() == WebRtcTextLogHandler::STARTING || On 2016/07/15 10:11:36, tommi-chrömium wrote: > Call GetState() only once? Done. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:47: void FormatMetaDataAsLogMessage(const MetaDataMap& meta_data, On 2016/07/19 14:53:16, magjed_chromium wrote: > Return the string instead of sending in a pointer. It will use move semantic, so > no copy will be made. Actually in this case, RVO will remove the temporary > string completely. Good point. Please note that most of the code isn't new though; it merely moved from webrtc_logging_handler_host.cc. This also applies to the other comments below. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:137: DCHECK(!log_buffer_.get()); On 2016/07/19 14:53:16, magjed_chromium wrote: > std::unique_pointer has an operator bool(), so you can remove '.get()'. Same > goes for the other places where you check if a std::unique_pointer owns an > object. Done. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:160: (*meta_data_.get())[it.first] = it.second; On 2016/07/19 14:53:16, magjed_chromium wrote: > std::map has a copy assignment operator that is cleaner and more efficient than > this. Just remove the loop and do '*meta_data_ = *meta_data'. That's not quite the same thing. This adds more data to the map while keeping the existing keys-value pairs.At least, I'm assuming this is the intention. I believe there is another flaw in the function though. If meta data is set in STARTED state, but no meta data has been set before, we would deference a null pointer on line 160. I've rewritten the function to avoid this problem and reduce the nesting depth of the if statements. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.h (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:66: // RendererDying(): ANY -> DYING. On 2016/07/19 14:53:17, magjed_chromium wrote: > Is DYING == CHANNEL_CLOSING? Yes. Forgot to update the comment. Fixed now. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:73: CHANNEL_CLOSING, // Renderer is closing. TODO On 2016/07/19 14:53:16, magjed_chromium wrote: > TODO what? Improve comment. Done. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:76: public: On 2016/07/19 14:53:17, magjed_chromium wrote: > Remove this extra 'public:'. Done. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:93: // Called automatically after StartLogging(). Do not call directly. On 2016/07/19 14:53:17, magjed_chromium wrote: > Why not make it private if it shouldn't be called directly? Done, but the symmetry with StopDone is lost. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:102: // renderer. Do not call directly. Must be called on the IO thread. On 2016/07/19 14:53:17, magjed_chromium wrote: > What does "Do not call directly." mean? It is called directly from > WebRtcLoggingHandlerHost. Updated comment. Wdyt? https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:144: int render_process_id_; On 2016/07/19 14:53:17, magjed_chromium wrote: > Make const. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:47: void FormatMetaDataAsLogMessage(const MetaDataMap& meta_data, On 2016/07/21 16:25:14, terelius-chromium wrote: > On 2016/07/19 14:53:16, magjed_chromium wrote: > > Return the string instead of sending in a pointer. It will use move semantic, > so > > no copy will be made. Actually in this case, RVO will remove the temporary > > string completely. > > Good point. Please note that most of the code isn't new though; it merely moved > from webrtc_logging_handler_host.cc. This also applies to the other comments > below. I understand. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:160: (*meta_data_.get())[it.first] = it.second; On 2016/07/21 16:25:14, terelius-chromium wrote: > On 2016/07/19 14:53:16, magjed_chromium wrote: > > std::map has a copy assignment operator that is cleaner and more efficient > than > > this. Just remove the loop and do '*meta_data_ = *meta_data'. > > That's not quite the same thing. This adds more data to the map while keeping > the existing keys-value pairs.At least, I'm assuming this is the intention. > > I believe there is another flaw in the function though. If meta data is set in > STARTED state, but no meta data has been set before, we would deference a null > pointer on line 160. I've rewritten the function to avoid this problem and > reduce the nesting depth of the if statements. I see. Maybe (*meta_data_)[it.first] = std::move(it.second) would be a little bit faster, but you would need to remove the const from the it ref, not sure if that's allowed. https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.h (right): https://codereview.chromium.org/1978183003/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.h:102: // renderer. Do not call directly. Must be called on the IO thread. On 2016/07/21 16:25:14, terelius-chromium wrote: > On 2016/07/19 14:53:17, magjed_chromium wrote: > > What does "Do not call directly." mean? It is called directly from > > WebRtcLoggingHandlerHost. > > Updated comment. Wdyt? Now I understand, thanks. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:339: for (size_t i = 0; i < messages.size(); ++i) { A range-based for loop would be a little bit cleaner. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:49: for (MetaDataMap::const_iterator it = meta_data.begin(); Use a range based for loop instead. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:54: message.resize(message.size() - 1); Use message.pop_back() instead. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:154: FormatMetaDataAsLogMessage(*meta_data.get()); Replace *x.get() with just *x, here and in all other places. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:297: log_buffer_.reset(); std::move is enough, you don't need to reset as well. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:481: for (net::NetworkInterfaceList::const_iterator it = network_list.begin(); nit: use range based for loop instead.
https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:339: for (size_t i = 0; i < messages.size(); ++i) { On 2016/07/22 09:30:31, magjed_chromium wrote: > A range-based for loop would be a little bit cleaner. Done. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:49: for (MetaDataMap::const_iterator it = meta_data.begin(); On 2016/07/22 09:30:31, magjed_chromium wrote: > Use a range based for loop instead. Good point. Done. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:54: message.resize(message.size() - 1); On 2016/07/22 09:30:31, magjed_chromium wrote: > Use message.pop_back() instead. I don't get pop_back to compile. Is our compiler setup C++11 compliant? There seems to be an index-out-of-range issue if the function is called with an empty map. Not sure whether that can happen, but I added a check. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:154: FormatMetaDataAsLogMessage(*meta_data.get()); On 2016/07/22 09:30:31, magjed_chromium wrote: > Replace *x.get() with just *x, here and in all other places. Done. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:297: log_buffer_.reset(); On 2016/07/22 09:30:31, magjed_chromium wrote: > std::move is enough, you don't need to reset as well. True, but I prefer to be explicit here since it is important that the log_buffer_ does not point to anything, rather that pointing to a dummy object for example. It also makes the similarity between ReleaseLog and DiscardLog more apparent. I will change if you insist. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:481: for (net::NetworkInterfaceList::const_iterator it = network_list.begin(); On 2016/07/22 09:30:31, magjed_chromium wrote: > nit: use range based for loop instead. Done.
magjed@chromium.org changed reviewers: + tommi@chromium.org
lgtm. Adding tommi@ back as reviewer in case he wants to take a look. https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:54: message.resize(message.size() - 1); On 2016/07/26 09:51:41, terelius-chromium wrote: > On 2016/07/22 09:30:31, magjed_chromium wrote: > > Use message.pop_back() instead. > > I don't get pop_back to compile. Is our compiler setup C++11 compliant? > > There seems to be an index-out-of-range issue if the function is called with an > empty map. Not sure whether that can happen, but I added a check. We use other C++11 stuff at least. Skip pop_back then if it's causing trouble.
Tommi, could you take a look as owner in chrome/browser/media/ ?
lgtm - sorry about the delay. Somehow I didn't get the review request (I think) https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:53: if (message.size() > 0) nit: if (!message.empty()) https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:54: message.erase(message.size() - 1, 1); I wonder if |message.pop_back()| would accomplish the same thing...
https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... File chrome/browser/media/webrtc_text_log_handler.cc (right): https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:53: if (message.size() > 0) On 2016/08/09 16:10:57, tommi-chrömium wrote: > nit: if (!message.empty()) Done. https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... chrome/browser/media/webrtc_text_log_handler.cc:54: message.erase(message.size() - 1, 1); On 2016/08/09 16:10:57, tommi-chrömium wrote: > I wonder if |message.pop_back()| would accomplish the same thing... It should do the same thing, but I get th following error when I try to use pop_back: no member named 'pop_back' in 'std::basic_string<char, std::char_traits<char>, std::allocator<char> >
On 2016/08/16 12:01:26, terelius-chromium wrote: > https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... > File chrome/browser/media/webrtc_text_log_handler.cc (right): > > https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... > chrome/browser/media/webrtc_text_log_handler.cc:53: if (message.size() > 0) > On 2016/08/09 16:10:57, tommi-chrömium wrote: > > nit: if (!message.empty()) > > Done. > > https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... > chrome/browser/media/webrtc_text_log_handler.cc:54: message.erase(message.size() > - 1, 1); > On 2016/08/09 16:10:57, tommi-chrömium wrote: > > I wonder if |message.pop_back()| would accomplish the same thing... > > It should do the same thing, but I get th following error when I try to use > pop_back: no member named 'pop_back' in 'std::basic_string<char, > std::char_traits<char>, std::allocator<char> > just checking - will this land or is there something more to wait on?
On 2016/08/29 13:45:16, tommi (chrömium) wrote: > On 2016/08/16 12:01:26, terelius-chromium wrote: > > > https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... > > File chrome/browser/media/webrtc_text_log_handler.cc (right): > > > > > https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... > > chrome/browser/media/webrtc_text_log_handler.cc:53: if (message.size() > 0) > > On 2016/08/09 16:10:57, tommi-chrömium wrote: > > > nit: if (!message.empty()) > > > > Done. > > > > > https://codereview.chromium.org/1978183003/diff/80001/chrome/browser/media/we... > > chrome/browser/media/webrtc_text_log_handler.cc:54: > message.erase(message.size() > > - 1, 1); > > On 2016/08/09 16:10:57, tommi-chrömium wrote: > > > I wonder if |message.pop_back()| would accomplish the same thing... > > > > It should do the same thing, but I get th following error when I try to use > > pop_back: no member named 'pop_back' in 'std::basic_string<char, > > std::char_traits<char>, std::allocator<char> > > > just checking - will this land or is there something more to wait on? I wanted to do some more manual testing to verify that the refactoring didn't inadvertently change the behavior.
The CQ bit was checked by terelius@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by terelius@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by terelius@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, magjed@chromium.org Link to the patchset: https://codereview.chromium.org/1978183003/#ps140001 (title: "Fix windows compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs. Rationale: The WebRtcLoggingHandlerHost class is currently quite large (over 800 lines) and does many different things including looking up system information, writing the text log, receiving and sending IPC messages and passing finished logs to the LogUploader. In addition to the original text log, the WebRtcLoggingHandlerHost also creates and manages the rtp dump through a WebRtcRtpDumpHandler object, and we would like to add a WebRtcEventLogHandler object. Proposal: We propose to split out the parts that are specific to the text log into a new WebRtcTextLogHandler class. The WebRtcTextLogHandler will own the actual text buffer, keep track of the logging state and fetch the system information that's written at the start of the log. The new WebRtcLoggingHandlerHost will have ownership of the WebRtcTextLogHandler, WebRtcRtpDumpHandler and WebRtcEventLogHandler objects, receive and send IPC messages, propagate API calls to the right log, and pass the finished logs to the LogUploader. ========== to ========== Refactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs. Rationale: The WebRtcLoggingHandlerHost class is currently quite large (over 800 lines) and does many different things including looking up system information, writing the text log, receiving and sending IPC messages and passing finished logs to the LogUploader. In addition to the original text log, the WebRtcLoggingHandlerHost also creates and manages the rtp dump through a WebRtcRtpDumpHandler object, and we would like to add a WebRtcEventLogHandler object. Proposal: We propose to split out the parts that are specific to the text log into a new WebRtcTextLogHandler class. The WebRtcTextLogHandler will own the actual text buffer, keep track of the logging state and fetch the system information that's written at the start of the log. The new WebRtcLoggingHandlerHost will have ownership of the WebRtcTextLogHandler, WebRtcRtpDumpHandler and WebRtcEventLogHandler objects, receive and send IPC messages, propagate API calls to the right log, and pass the finished logs to the LogUploader. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Refactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs. Rationale: The WebRtcLoggingHandlerHost class is currently quite large (over 800 lines) and does many different things including looking up system information, writing the text log, receiving and sending IPC messages and passing finished logs to the LogUploader. In addition to the original text log, the WebRtcLoggingHandlerHost also creates and manages the rtp dump through a WebRtcRtpDumpHandler object, and we would like to add a WebRtcEventLogHandler object. Proposal: We propose to split out the parts that are specific to the text log into a new WebRtcTextLogHandler class. The WebRtcTextLogHandler will own the actual text buffer, keep track of the logging state and fetch the system information that's written at the start of the log. The new WebRtcLoggingHandlerHost will have ownership of the WebRtcTextLogHandler, WebRtcRtpDumpHandler and WebRtcEventLogHandler objects, receive and send IPC messages, propagate API calls to the right log, and pass the finished logs to the LogUploader. ========== to ========== Refactor WebRtcLoggingHandlerHost in preparation of automatic upload of WebRtcEventLogs. Rationale: The WebRtcLoggingHandlerHost class is currently quite large (over 800 lines) and does many different things including looking up system information, writing the text log, receiving and sending IPC messages and passing finished logs to the LogUploader. In addition to the original text log, the WebRtcLoggingHandlerHost also creates and manages the rtp dump through a WebRtcRtpDumpHandler object, and we would like to add a WebRtcEventLogHandler object. Proposal: We propose to split out the parts that are specific to the text log into a new WebRtcTextLogHandler class. The WebRtcTextLogHandler will own the actual text buffer, keep track of the logging state and fetch the system information that's written at the start of the log. The new WebRtcLoggingHandlerHost will have ownership of the WebRtcTextLogHandler, WebRtcRtpDumpHandler and WebRtcEventLogHandler objects, receive and send IPC messages, propagate API calls to the right log, and pass the finished logs to the LogUploader. Committed: https://crrev.com/70957b9b7168c2009099ece92fd0f8928a1b46b8 Cr-Commit-Position: refs/heads/master@{#421239} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/70957b9b7168c2009099ece92fd0f8928a1b46b8 Cr-Commit-Position: refs/heads/master@{#421239} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
