Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1855193002:
Move the call to enable the WebRTC event log from PeerConnectionFactory to PeerConnection. (Closed)
|
Created:
4 years, 8 months ago by Ivo-OOO until feb 6 Modified:
4 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, Stefan, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable the WebRTC event log on PeerConnection instead of PeerConnectionFactory.
This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality.
BUG=chromium:600661, chromium:613499, webrtc:4741
Committed: https://crrev.com/cf0887d3df989061ca653339e7affa8e49a3cfe6
Cr-Commit-Position: refs/heads/master@{#404183}
Patch Set 1 : Added limit to number of log files and the size of the log files. #
Total comments: 12
Patch Set 2 : Addressed comments by dcheng. #
Total comments: 2
Patch Set 3 : Merge with existing WebRtcEventLogHandler. #
Total comments: 34
Patch Set 4 : Introduced WebRTCCallbackInterface. #
Total comments: 36
Patch Set 5 : Removed content/public/ interface, used RenderProcessHost instead. #
Total comments: 7
Patch Set 6 : Moved event log related bookkeeping/IPC from chrome/ to content/, into new class called WebRTCEvent… #
Total comments: 28
Patch Set 7 : Addressed review comments by ncarter and grunell. #
Total comments: 27
Patch Set 8 : Addressed review comments. #
Total comments: 6
Patch Set 9 : Review comments from Tommi. #
Total comments: 2
Patch Set 10 : Small bugfix and Tommi's comment. #
Total comments: 41
Patch Set 11 : Addressed comments from Henrik and enabled logging for newly created render processes. #
Total comments: 14
Patch Set 12 : Added unittest and addressed other comments. #
Total comments: 13
Patch Set 13 : Added unittest for adding more PeerConnections than the maximum limit. #
Total comments: 4
Patch Set 14 : Addressed comments by Henrik. #Patch Set 15 : Rebase. #Patch Set 16 : Small fix to the gypi file for webrtc_eventlog_host_unittest.cc. #Patch Set 17 : Fix for path extensions on windows. #Patch Set 18 : Added CONTENT_EXPORT to WebRTCEventLogHost. #Messages
Total messages: 67 (27 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Move the call to enable the WebRTC event log from PeerConnectionFactory to PeerConnection. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. BUG=chromium:570672 ========== to ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. BUG=chromium:570672 ==========
ivoc@chromium.org changed reviewers: + grunell@chromium.org, nasko@chromium.org, perkj@chromium.org
ivoc@chromium.org changed reviewers: + jochen@chromium.org - nasko@chromium.org
ivoc@chromium.org changed reviewers: + dcheng@chromium.org
Hi guys, this CL is to prepare Chrome for the upcoming change in the API for enabling/disabling WebRTC event logs, which will move from PeerConnectionFactory to PeerConnection. The related WebRTC CL can be found here: https://codereview.webrtc.org/1748403002/ This will result in a seperate logfile being opened for each PeerConnection that is active (up to the hardcoded limit). Note that logging will only start when users click on the checkbox on the chrome://webrtc-internals page, this feature is for debugging purposes, users only need to use this if they want to help WebRTC developers track down an issue. Please have a look, thanks!
Initial thoughts from looking over the CL. https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:24: #define IntToStringType base::IntToString Ugh. https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:50: IPC::PlatformFileForTransit CreateFileForProcess(base::FilePath file_path, const base::FilePath& https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:64: int lid, No abbreviations here. https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:73: rph->Send(new PeerConnectionTracker_StartEventLog(lid, file_for_transit, Does it make sense to send an invalid fd here? https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.h (right): https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.h:199: int nr_event_log_files_; int number_event_log_files_; or int event_log_files_count_; Please don't abbreviate number to nr =) https://codereview.chromium.org/1855193002/diff/20001/content/renderer/media/... File content/renderer/media/peer_connection_tracker.cc (right): https://codereview.chromium.org/1855193002/diff/20001/content/renderer/media/... content/renderer/media/peer_connection_tracker.cc:389: for (PeerConnectionIdMap::iterator it = peer_connection_id_map_.begin(); Consider using for (auto& kv : map_) syntax here and below.
Thanks for the quick review! See my replies below. https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:24: #define IntToStringType base::IntToString On 2016/04/04 21:44:57, dcheng wrote: > Ugh. I copied this from render_process_host_impl.cc and I don't really know why it's needed :-) I guess windows filenames are stored in UTF16? Do you think it's okay to just use base::IntToString on all platforms? https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:50: IPC::PlatformFileForTransit CreateFileForProcess(base::FilePath file_path, On 2016/04/04 21:44:57, dcheng wrote: > const base::FilePath& Done. https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:64: int lid, On 2016/04/04 21:44:57, dcheng wrote: > No abbreviations here. Done. https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:73: rph->Send(new PeerConnectionTracker_StartEventLog(lid, file_for_transit, On 2016/04/04 21:44:57, dcheng wrote: > Does it make sense to send an invalid fd here? Good point, I guess that doesn't make sense. I will add a check. https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.h (right): https://codereview.chromium.org/1855193002/diff/20001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.h:199: int nr_event_log_files_; On 2016/04/04 21:44:57, dcheng wrote: > int number_event_log_files_; > > or > > int event_log_files_count_; > > Please don't abbreviate number to nr =) okay, done :) https://codereview.chromium.org/1855193002/diff/20001/content/renderer/media/... File content/renderer/media/peer_connection_tracker.cc (right): https://codereview.chromium.org/1855193002/diff/20001/content/renderer/media/... content/renderer/media/peer_connection_tracker.cc:389: for (PeerConnectionIdMap::iterator it = peer_connection_id_map_.begin(); On 2016/04/04 21:44:57, dcheng wrote: > Consider using for (auto& kv : map_) syntax here and below. Thanks, looks better.
Description was changed from ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. BUG=chromium:570672 ========== to ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. BUG=chromium:570672,chromium:600661,webrtc:4741 ==========
One important comment you need to address first. https://codereview.chromium.org/1855193002/diff/40001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/40001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:49: // Creates a file used for handing over to the renderer. Webrtc internals is not the right place to handle this - it's purpose is to handle data. You should sync up with terelius@ and his CL https://codereview.chromium.org/1650133002/ which introduces WebrtcEventLogHandler.
https://codereview.chromium.org/1855193002/diff/40001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/40001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:49: // Creates a file used for handing over to the renderer. On 2016/04/07 09:22:33, Henrik Grunell wrote: > Webrtc internals is not the right place to handle this - it's purpose is to > handle data. You should sync up with terelius@ and his CL > https://codereview.chromium.org/1650133002/ which introduces > WebrtcEventLogHandler. Thanks for pointing that out, I was not aware of this CL. It does make sense to unify this functionality in that class (although I think it has to be extended a bit). I will start working on that, so please don't review this CL until that is complete.
On 2016/04/07 12:10:50, Ivo wrote: > https://codereview.chromium.org/1855193002/diff/40001/content/browser/media/w... > File content/browser/media/webrtc/webrtc_internals.cc (right): > > https://codereview.chromium.org/1855193002/diff/40001/content/browser/media/w... > content/browser/media/webrtc/webrtc_internals.cc:49: // Creates a file used for > handing over to the renderer. > On 2016/04/07 09:22:33, Henrik Grunell wrote: > > Webrtc internals is not the right place to handle this - it's purpose is to > > handle data. You should sync up with terelius@ and his CL > > https://codereview.chromium.org/1650133002/ which introduces > > WebrtcEventLogHandler. > > Thanks for pointing that out, I was not aware of this CL. It does make sense to > unify this functionality in that class (although I think it has to be extended a > bit). I will start working on that, so please don't review this CL until that is > complete. I've updated this CL and moved most of the implementation from WebRtc internals into the recently introduced WebrtcEventLogHandler. So please have another look at this CL, I would like to get this in before the M52 cut if possible.
https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.cc:92: typedef void (WebRtcEventLogHandler::*StopEventLogFn)( Using. Formatting is also a bit off (* is generally by the typename, with no space) https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_event_log_handler.h (right): https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:74: void OnPeerConnectionAdded(int render_id, int local_id); Nit: Call this |process_id| to be consistent with terminology elsewhere (and make it less ambiguous what the ID is for, since we have RenderFrame and RenderView as well). Same comment below. Also, what is "local_id"? I'm guessing it's an ID corresponding to the connection? Maybe connection_id? https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.h (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.h:111: typedef base::Callback<void()> event_log_stop_func; Nit: Prefer using to typedef. Also TypeNamesAreWrittenLikeThis. https://codereview.chromium.org/1855193002/diff/60001/content/common/media/pe... File content/common/media/peer_connection_tracker_messages.h (right): https://codereview.chromium.org/1855193002/diff/60001/content/common/media/pe... content/common/media/peer_connection_tracker_messages.h:48: int64_t /* max_file_size_bytes */) Why not unsigned?
First round. https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_event_log_handler.h (right): https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:54: // Starts an RTC event log for each peerconnection on the specified |host|. It's not clear to me how this function relates to the above. Does the above one also start for each peer connection? https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:112: std::set<int> active_peer_connection_lid_; Write out what "l" stands for. (local?) https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:115: int number_log_files_; So, this is the max number of files allowed? If so, use a constant in the implementation file instead of a member variable. https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (left): https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:39: Keep empty line. https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:358: int rid = i.GetCurrentValue()->GetID(); I'd prefer to have some other component keep track of the event log handler. Though it's not much code here, so it's not too bad. Also, when the event log can be enabled through an extension API, it seems more suitable to have it somewhere else. Is the plan to keep it here when that is added or change? https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.h (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.h:212: event_log_handlers; Trailing _. https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/DEPS (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/DEPS:2: "+chrome/browser/media", This is not allowed. content/ cannot depend on chrome/. https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/peer_connection_tracker_host.cc:7: #include "chrome/browser/media/webrtc_event_log_handler.h" Not allowed. Chrome has to implement a public content interface function instead, see content_browser_client.h. Or better, Chrome can register a callback. https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (left): https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:146: void EnableEventLogRecordings(const base::FilePath& file) override; Very nice to get rid of these! :) https://codereview.chromium.org/1855193002/diff/60001/content/browser/resourc... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/resourc... content/browser/resources/media/dump_creator.js:70: ' multiple log files to be created. When enabling, a filename for the' + If you select a filename and multiple files are created, how are they named? Explain that here. https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_tracker.h (right): https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_tracker.h:191: void OnStartEventLog(int local_id, What is "local_id"? Can you use a more informative name? https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:1454: IPC::PlatformFileForTransitToPlatformFile(file), max_file_size_bytes); DCHECK the file and size here or somewhere in the path?
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
I introduced a new class called WebRTCCallbackInterface in content/public/browser to resolve the dependency issues in this CL. The code in chrome/ uses it to register callback functions and the code in content/ uses it to call the registered callback functions. Please have another look and thanks for the feedback. https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.cc:92: typedef void (WebRtcEventLogHandler::*StopEventLogFn)( On 2016/05/10 06:48:17, dcheng wrote: > Using. Formatting is also a bit off (* is generally by the typename, with no > space) Done. https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_event_log_handler.h (right): https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:54: // Starts an RTC event log for each peerconnection on the specified |host|. On 2016/05/10 08:44:56, Henrik Grunell wrote: > It's not clear to me how this function relates to the above. Does the above one > also start for each peer connection? Yes it does, I will add a comment to clarify that. https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:74: void OnPeerConnectionAdded(int render_id, int local_id); On 2016/05/10 06:48:17, dcheng wrote: > Nit: Call this |process_id| to be consistent with terminology elsewhere (and > make it less ambiguous what the ID is for, since we have RenderFrame and > RenderView as well). Same comment below. > > Also, what is "local_id"? I'm guessing it's an ID corresponding to the > connection? Maybe connection_id? Good idea, done. I changed the argument to the constructor to process_id as well. https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:112: std::set<int> active_peer_connection_lid_; On 2016/05/10 08:44:56, Henrik Grunell wrote: > Write out what "l" stands for. (local?) Done. https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:115: int number_log_files_; On 2016/05/10 08:44:56, Henrik Grunell wrote: > So, this is the max number of files allowed? If so, use a constant in the > implementation file instead of a member variable. This is the not the maximum, but the number of files that can still be created before the maximum is reached. It starts at Max, and each time a log file is started this number is decreased by one. I will clarify the description. https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (left): https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:39: On 2016/05/10 08:44:56, Henrik Grunell wrote: > Keep empty line. Done. https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.cc:358: int rid = i.GetCurrentValue()->GetID(); On 2016/05/10 08:44:56, Henrik Grunell wrote: > I'd prefer to have some other component keep track of the event log handler. > Though it's not much code here, so it's not too bad. > > Also, when the event log can be enabled through an extension API, it seems more > suitable to have it somewhere else. Is the plan to keep it here when that is > added or change? I have moved this into a new class: WebRTCCallbackInterface. https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... File content/browser/media/webrtc/webrtc_internals.h (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.h:111: typedef base::Callback<void()> event_log_stop_func; On 2016/05/10 06:48:17, dcheng wrote: > Nit: Prefer using to typedef. Also TypeNamesAreWrittenLikeThis. Done. https://codereview.chromium.org/1855193002/diff/60001/content/browser/media/w... content/browser/media/webrtc/webrtc_internals.h:212: event_log_handlers; On 2016/05/10 08:44:56, Henrik Grunell wrote: > Trailing _. Done. https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/DEPS (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/DEPS:2: "+chrome/browser/media", On 2016/05/10 08:44:56, Henrik Grunell wrote: > This is not allowed. content/ cannot depend on chrome/. Good point, I've rewritten this to avoid this dependency. https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/peer_connection_tracker_host.cc:7: #include "chrome/browser/media/webrtc_event_log_handler.h" On 2016/05/10 08:44:56, Henrik Grunell wrote: > Not allowed. > > Chrome has to implement a public content interface function instead, see > content_browser_client.h. > > Or better, Chrome can register a callback. Oops, good point. I will add callbacks to do this. https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (left): https://codereview.chromium.org/1855193002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:146: void EnableEventLogRecordings(const base::FilePath& file) override; On 2016/05/10 08:44:56, Henrik Grunell wrote: > Very nice to get rid of these! :) Agreed :) https://codereview.chromium.org/1855193002/diff/60001/content/browser/resourc... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/60001/content/browser/resourc... content/browser/resources/media/dump_creator.js:70: ' multiple log files to be created. When enabling, a filename for the' + On 2016/05/10 08:44:56, Henrik Grunell wrote: > If you select a filename and multiple files are created, how are they named? > Explain that here. Good idea, done. https://codereview.chromium.org/1855193002/diff/60001/content/common/media/pe... File content/common/media/peer_connection_tracker_messages.h (right): https://codereview.chromium.org/1855193002/diff/60001/content/common/media/pe... content/common/media/peer_connection_tracker_messages.h:48: int64_t /* max_file_size_bytes */) On 2016/05/10 06:48:17, dcheng wrote: > Why not unsigned? This value represents a file size on disk. Although we currently don't have plans to use really large log files, they could conceivably become larger than 2GB at some point in the future (if we log more data than we currently do for a very long call). Also, on the WebRTC API, this value will be passed as an int64_t, so I think it makes sense to use the same type internally as well. https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_tracker.h (right): https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_tracker.h:191: void OnStartEventLog(int local_id, On 2016/05/10 08:44:56, Henrik Grunell wrote: > What is "local_id"? Can you use a more informative name? This is the ID for the peerconnection within the render process, in many other places it's called local_id or lid (for example in the SendPeerConenctionUpdate function below), but you're right that it's a bit confusing. I'll change it to peer_connection_id. https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1855193002/diff/60001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:1454: IPC::PlatformFileForTransitToPlatformFile(file), max_file_size_bytes); On 2016/05/10 08:44:56, Henrik Grunell wrote: > DCHECK the file and size here or somewhere in the path? I added a DCHECK for the file, but the size can actually be 0 or negative to indicate that no file size limit should be used.
https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:43: .AddExtension(base::Int64ToString(rtc_event_log_id)); Uint64ToString, since the input type is unsigned. https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:73: const int64_t max_filesize_bytes = 30000000; Out of curiosity... why do we need to send this over IPC? Couldn't it just be hardcoded in the renderer? https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:91: content::RenderProcessHost* host, const base::FilePath& file_path); I think it's probably shorter (and clearer) to just write: content::WebRTCCallbackInterface::GetInstance()->RegisterEventLogHandler( process_id, base::Bind(&WebRtcEventLogHandler::StartWebRtcEventLogging, this, host), base::Bind(&WebRtcEventLogHandler::StopWebRtcEventLogging, this, host))); https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:126: is_rtc_event_logging_in_progress_ = true; This is triggered by a renderer IPC, right? What if a renderer sends this IPC multiple times? Seems like it might let the renderer exceed the number_log_files_ limit. https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... File content/browser/media/webrtc/webrtc_callback_interface_impl.h (right): https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... content/browser/media/webrtc/webrtc_callback_interface_impl.h:37: std::map<int, std::pair<EventLogStartFunc, EventLogStopFunc>> pair is pretty inscrutable (first and second are very opaque and give no indication of the member's actual purpose). Just make a simple struct to wrap this. The typenames for these members will be a lot shorter too. https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... File content/public/browser/webrtc_callback_interface.h (right): https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:25: EventLogStartFunc start_logging_callback, Callbacks should be passed by const reference. Same for line 35.
Seems like Jochen is ooo, you may want to pick another //content reviewer. https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_event_log_handler.h (right): https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:54: // Starts an RTC event log for each peerconnection on the specified |host|. On 2016/05/12 13:23:23, Ivo wrote: > On 2016/05/10 08:44:56, Henrik Grunell wrote: > > It's not clear to me how this function relates to the above. Does the above > one > > also start for each peer connection? > > Yes it does, I will add a comment to clarify that. I also don't understand why one can specify either delay+callbacks or base file path, but not all. Could you explain that in the comments? https://codereview.chromium.org/1855193002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_event_log_handler.h:115: int number_log_files_; On 2016/05/12 13:23:23, Ivo wrote: > On 2016/05/10 08:44:56, Henrik Grunell wrote: > > So, this is the max number of files allowed? If so, use a constant in the > > implementation file instead of a member variable. > > This is the not the maximum, but the number of files that can still be created > before the maximum is reached. It starts at Max, and each time a log file is > started this number is decreased by one. I will clarify the description. I think it's clearer to have this variable count the number of open files (instead of "remaining"), and compare to the max allowed specified as a constant in the impl file (which it is now I think). https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:73: const int64_t max_filesize_bytes = 30000000; On 2016/05/13 06:50:11, dcheng wrote: > Out of curiosity... why do we need to send this over IPC? Couldn't it just be > hardcoded in the renderer? Agree. https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:129: number_log_files_ = 3; Are these number based on something particular? https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... File content/browser/media/webrtc/webrtc_callback_interface_impl.h (right): https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... content/browser/media/webrtc/webrtc_callback_interface_impl.h:15: class WebRTCCallbackInterfaceImpl : public WebRTCCallbackInterface { The class name doesn't really say much. Could you come up with something more descriptive? https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... content/browser/media/webrtc/webrtc_callback_interface_impl.h:15: class WebRTCCallbackInterfaceImpl : public WebRTCCallbackInterface { Add comment and describe the class briefly. https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... content/browser/media/webrtc/webrtc_callback_interface_impl.h:37: std::map<int, std::pair<EventLogStartFunc, EventLogStopFunc>> Hmm, I thought there would be one callback only. Could you outline in the CL description what is changed and why, and how things relate to each other? https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:58: '<p class=audio-recordings-info>A diagnostic packet and event' + Nit: The class name "audio-recordings-info" isn't accurate longer as it has expanded to event log. Would you mind changing it to something more suitable? https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:71: ' recording can be selected. The entered filename is used as a' + Nit: "entered" -> "selected". https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:73: ' <p><div><base filename>.<render process ID>' + Nit: No need for <div></div> since there's only one in the <p></p>. https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:75: '<p class=audio-recordings-info>Event logs from different tabs will' + This is not always true; different tabs can share render process in some cases. You could just skip that part or rephrase. https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:77: ' peerconnections within the same tab will have a different recording' + Nit: PeerConnections https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:77: ' peerconnections within the same tab will have a different recording' + "...will have different recording IDs." https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... File content/public/browser/webrtc_callback_interface.h (right): https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Remove "(c)". Same in other new files. https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:5: #ifndef WEBRTC_INTERFACE_H Use full path. "CONTENT_PUBLIC_..._WEBRTC_CALLBACK_INTERFACE_H" https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:14: class CONTENT_EXPORT WebRTCCallbackInterface { Which of the functions are called from //chrome? If it's just setting callbacks, it should be possible to add those functions to another existing class in content public. Add a //content reviewer asap to review if this is OK and to get guidance on how the public folder should be used. The class needs a comment describing it, if kept.
ivoc@chromium.org changed reviewers: + nick@chromium.org - jochen@chromium.org
Patchset #5 (id:140001) has been deleted
Description was changed from ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. BUG=chromium:570672,chromium:600661,webrtc:4741 ========== to ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:570672,chromium:600661,webrtc:4741 ==========
I replaced jochen with ncarter as reviewer for content/ due to jochen being OOO. In this update I removed the content/public API in favor of adding functions to RenderProcessHost (ncarter: would appreciate your feedback on this, see discussion below). I changed the callback handler to be a member to RenderProcessHostImpl instead of a singleton, which simplifies the callback registering/handling code. Please have another look, and thanks for the feedback so far! https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:43: .AddExtension(base::Int64ToString(rtc_event_log_id)); On 2016/05/13 06:50:11, dcheng wrote: > Uint64ToString, since the input type is unsigned. Done. https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:73: const int64_t max_filesize_bytes = 30000000; On 2016/05/13 09:05:31, Henrik Grunell wrote: > On 2016/05/13 06:50:11, dcheng wrote: > > Out of curiosity... why do we need to send this over IPC? Couldn't it just be > > hardcoded in the renderer? > > Agree. Ok, I will move it there, good point. https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:91: content::RenderProcessHost* host, const base::FilePath& file_path); On 2016/05/13 06:50:11, dcheng wrote: > I think it's probably shorter (and clearer) to just write: > > content::WebRTCCallbackInterface::GetInstance()->RegisterEventLogHandler( > process_id, > base::Bind(&WebRtcEventLogHandler::StartWebRtcEventLogging, this, host), > base::Bind(&WebRtcEventLogHandler::StopWebRtcEventLogging, this, host))); > You're right that it's both shorter and clearer, however it doesn't work in this case. I think it's because there are two functions with the same name, and the compiler cannot figure out which one to use. Changing the name of the start/stop functions would probably fix this, but I think function names are more important. https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:126: is_rtc_event_logging_in_progress_ = true; On 2016/05/13 06:50:11, dcheng wrote: > This is triggered by a renderer IPC, right? What if a renderer sends this IPC > multiple times? Seems like it might let the renderer exceed the > number_log_files_ limit. This is not triggered by renderer IPC (it is called from WebRTCInternals via WebRTCCallbackInterface in the browser process), but you're right that calling multiple times could cause problems, so I'll add some code to prevent that, thanks. https://codereview.chromium.org/1855193002/diff/120001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:129: number_log_files_ = 3; On 2016/05/13 09:05:31, Henrik Grunell wrote: > Are these number based on something particular? Not really, just on what seems like a reasonably upper limit for most situations and on the intuition that desktops/laptops are faster than mobile phones. https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... File content/browser/media/webrtc/webrtc_callback_interface_impl.h (right): https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... content/browser/media/webrtc/webrtc_callback_interface_impl.h:15: class WebRTCCallbackInterfaceImpl : public WebRTCCallbackInterface { On 2016/05/13 09:05:31, Henrik Grunell wrote: > The class name doesn't really say much. Could you come up with something more > descriptive? I changed it to WebRTCEventLogCallbackHandler. https://codereview.chromium.org/1855193002/diff/120001/content/browser/media/... content/browser/media/webrtc/webrtc_callback_interface_impl.h:37: std::map<int, std::pair<EventLogStartFunc, EventLogStopFunc>> On 2016/05/13 06:50:11, dcheng wrote: > pair is pretty inscrutable (first and second are very opaque and give no > indication of the member's actual purpose). Just make a simple struct to wrap > this. The typenames for these members will be a lot shorter too. @dcheng: I have rewritten this code to avoid pair. @grunell: The reason for the multiple callbacks is that this class was a singleton, which means that there is one pair of callbacks per render process. This has now changed, I made this class a member of RenderProcessHostImpl, so there is a one to one relation. I will update the description to make it more clear. https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:58: '<p class=audio-recordings-info>A diagnostic packet and event' + On 2016/05/13 09:05:31, Henrik Grunell wrote: > Nit: The class name "audio-recordings-info" isn't accurate longer as it has > expanded to event log. Would you mind changing it to something more suitable? Done. https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:71: ' recording can be selected. The entered filename is used as a' + On 2016/05/13 09:05:31, Henrik Grunell wrote: > Nit: "entered" -> "selected". Done. https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:73: ' <p><div><base filename>.<render process ID>' + On 2016/05/13 09:05:31, Henrik Grunell wrote: > Nit: No need for <div></div> since there's only one in the <p></p>. Done. https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:75: '<p class=audio-recordings-info>Event logs from different tabs will' + On 2016/05/13 09:05:31, Henrik Grunell wrote: > This is not always true; different tabs can share render process in some cases. > You could just skip that part or rephrase. I did not know that. I think removing this part would be better otherwise it gets a bit confusing. https://codereview.chromium.org/1855193002/diff/120001/content/browser/resour... content/browser/resources/media/dump_creator.js:77: ' peerconnections within the same tab will have a different recording' + On 2016/05/13 09:05:31, Henrik Grunell wrote: > Nit: PeerConnections This text is removed now. https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... File content/public/browser/webrtc_callback_interface.h (right): https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/13 09:05:31, Henrik Grunell wrote: > Remove "(c)". Same in other new files. Done. https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:5: #ifndef WEBRTC_INTERFACE_H On 2016/05/13 09:05:31, Henrik Grunell wrote: > Use full path. "CONTENT_PUBLIC_..._WEBRTC_CALLBACK_INTERFACE_H" Done. https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:14: class CONTENT_EXPORT WebRTCCallbackInterface { On 2016/05/13 09:05:31, Henrik Grunell wrote: > Which of the functions are called from //chrome? If it's just setting callbacks, > it should be possible to add those functions to another existing class in > content public. Add a //content reviewer asap to review if this is OK and to get > guidance on how the public folder should be used. > > The class needs a comment describing it, if kept. I removed it, and moved the functions to RenderProcessHost. Would indeed be good to get input from a //content reviewer. https://codereview.chromium.org/1855193002/diff/120001/content/public/browser... content/public/browser/webrtc_callback_interface.h:25: EventLogStartFunc start_logging_callback, On 2016/05/13 06:50:12, dcheng wrote: > Callbacks should be passed by const reference. Same for line 35. Done.
https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/DEPS File chrome/browser/media/DEPS (right): https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/D... chrome/browser/media/DEPS:2: "+content/common/media", This is not an acceptable DEPS rule. Outside of the content/ directory itself, only content/public can be included. https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:18: #include "content/common/media/peer_connection_tracker_messages.h" As general a rule, IPC messages are not part of the content public API. This means that if an IPC is sent by content/renderer, it should be handled by content/browser, and not by e.g. chrome/ -- and vice versa. Perhaps, if you need to send the IPC message from the chrome/ layer, they should be declared in chrome/common/media/webrtc_logging_messages.h , instead of content/? https://codereview.chromium.org/1855193002/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1855193002/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:144: #if defined(ENABLE_WEBRTC) These WebRTC calls don't really make sense in RenderProcessHost. In fact, bug 570672 which you link to in the description, is about exactly that. It seems like you're actually adding complexity to the interface here, not removing it. What's the road map for getting rid of this? Is amistry's CL (linked from the bug) completely abandoned? Does making this change help move that forward, or would it make more sense to fix 570672 first, and proceed with this functionality change afterwards?
https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:18: #include "content/common/media/peer_connection_tracker_messages.h" On 2016/05/18 21:49:56, ncarter wrote: > As general a rule, IPC messages are not part of the content public API. This > means that if an IPC is sent by content/renderer, it should be handled by > content/browser, and not by e.g. chrome/ -- and vice versa. > > Perhaps, if you need to send the IPC message from the chrome/ layer, they should > be declared in chrome/common/media/webrtc_logging_messages.h , instead of > content/? Ah, good point. The IPC should be done over content. If done in Chrome we need to add another point of interface between content and the embedder in the renderer, which is unfortunate.
Description was changed from ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:570672,chromium:600661,webrtc:4741 ========== to ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:570672,chromium:600661,chromium:613499,webrtc:4741 ==========
ivoc@chromium.org changed reviewers: + sergeyu@chromium.org
Description was changed from ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:570672,chromium:600661,chromium:613499,webrtc:4741 ========== to ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:600661,chromium:613499,webrtc:4741 ==========
Thanks for the feedback, I made another attempt at fixing the dependency issues in this CL. I added a new class member (WebRTCEventLogHost) to RenderProcessHostImpl, to keep track of the event log related bookkeeping and send the IPC messages to activate the logging. This means that the IPC messages are now being sent from inside /content. The callback registering mechanism is no longer needed in this revision, which simplifies the code quite a bit. Added sergeyu@ as reviewer for chrome/browser/media. Please have another look! https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/DEPS File chrome/browser/media/DEPS (right): https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/D... chrome/browser/media/DEPS:2: "+content/common/media", On 2016/05/18 21:49:56, ncarter wrote: > This is not an acceptable DEPS rule. > > Outside of the content/ directory itself, only content/public can be included. Thanks for pointing that out, I'm not that familiar with Chromium and the dependency restrictions. I worked around this by moving the IPC sending code into content/, so this is no longer needed. https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/160001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:18: #include "content/common/media/peer_connection_tracker_messages.h" On 2016/05/19 14:10:41, Henrik Grunell wrote: > On 2016/05/18 21:49:56, ncarter wrote: > > As general a rule, IPC messages are not part of the content public API. This > > means that if an IPC is sent by content/renderer, it should be handled by > > content/browser, and not by e.g. chrome/ -- and vice versa. > > > > Perhaps, if you need to send the IPC message from the chrome/ layer, they > should > > be declared in chrome/common/media/webrtc_logging_messages.h , instead of > > content/? > > Ah, good point. The IPC should be done over content. If done in Chrome we need > to add another point of interface between content and the embedder in the > renderer, which is unfortunate. I moved the IPC sending code into content/, in a new class called WebRTCEventLogHost, which is a member of RenderProcessHostImpl. https://codereview.chromium.org/1855193002/diff/160001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1855193002/diff/160001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:144: #if defined(ENABLE_WEBRTC) On 2016/05/18 21:49:56, ncarter wrote: > These WebRTC calls don't really make sense in RenderProcessHost. In fact, bug > 570672 which you link to in the description, is about exactly that. It seems > like you're actually adding complexity to the interface here, not removing it. > > What's the road map for getting rid of this? Is amistry's CL (linked from the > bug) completely abandoned? Does making this change help move that forward, or > would it make more sense to fix 570672 first, and proceed with this > functionality change afterwards? You're absolutely right that it would be nicer be better if WebRTC related code would be removed from RenderProcessHost. I think that issue should be tackled later on in a seperate CL (I don't know the status of amistry's CL). You're right that this CL does not help with bug 570672, so I will remove it from the description (an earlier revision of the CL did help, which is why I added it). The goal of this CL is to prepare chrome for an upcoming API change in WebRTC, so I don't think we should wait for bug 570672 to be fixed.
I don't think this patch compiles. https://codereview.chromium.org/1855193002/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:113: is_manual_stop, ++current_rtc_event_log_id_, callback, This ++ doesn't make any sense to me. The parameter value here is used to recompute the FilePath at stop time. If you change this here, the Stop operation will see a different FilePath value from the Start operation. Is that really what you intend? https://codereview.chromium.org/1855193002/diff/180001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1855193002/diff/180001/content/public/browser... content/public/browser/render_process_host.h:249: virtual bool StartWebRTCEventLog(const base::FilePath& file_path) = 0; This looks basically like a rename of the old functions (I like the new names and documentation much better). I'd hoist these up to line 221, where EnableEventLogRecordings() used to be. https://codereview.chromium.org/1855193002/diff/180001/content/public/test/mo... File content/public/test/mock_render_process_host.h (right): https://codereview.chromium.org/1855193002/diff/180001/content/public/test/mo... content/public/test/mock_render_process_host.h:90: const WebRtcRtpPacketCallback& packet_callback) override; Doesn't this need an implementation of StartWebRTCEventLog?
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855193002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855193002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
I didn't get all the way through, but a bunch of comments. https://codereview.chromium.org/1855193002/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:31: uint64_t rtc_event_log_id) { Isn't an int enough? https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:17: namespace { Empty line above. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:60: } // namespace Ditto. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:71: void WebRTCEventLogHost::PeerConnectionAdded(int connection_id) { Are these two functions be called on any thread? I.e. add thread check or make them thread safe. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:72: active_peer_connection_local_id_.insert(connection_id); DCHECK that the id isn't already in the set? https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:83: number_active_log_files_++; Nit: ++number_active_log_file_; https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:83: number_active_log_files_++; I don't see this ever being decreased. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:89: active_peer_connection_local_id_.erase(connection_id); DCHECK that it is actually in the set? https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:33: void PeerConnectionAdded(int connection_id); Please comment on these functions too. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:48: int number_active_log_files_; Can active_peer_connection_local_id_.size() be used instead of this variable? https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:53: }; DISALLOW_COPY_AND_ASSIGN()
ivoc@chromium.org changed reviewers: + tommi@chromium.org - sergeyu@chromium.org
@ncarter: About not compiling, this CL is preparing for an upcoming API change in WebRTC which has not landed yet (see https://codereview.webrtc.org/1748403002/ ). I'm waiting with landing the API change in WebRTC until this CL is ready, otherwise it would stop the WebRTC event log functionality from functioning in Chrome. It definitely compiles on my machine with a patched version of WebRTC with the new API. Replaced sergeyu@ with tommi@ as reviewer, since sergeyu@ is OOO. https://codereview.chromium.org/1855193002/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.cc (right): https://codereview.chromium.org/1855193002/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:31: uint64_t rtc_event_log_id) { On 2016/05/24 13:31:59, Henrik Grunell wrote: > Isn't an int enough? It probably is, but since the current code uses a uint64_t here, I don't think changing that should be a part of this CL. https://codereview.chromium.org/1855193002/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.cc:113: is_manual_stop, ++current_rtc_event_log_id_, callback, On 2016/05/20 18:27:20, ncarter wrote: > This ++ doesn't make any sense to me. The parameter value here is used to > recompute the FilePath at stop time. If you change this here, the Stop operation > will see a different FilePath value from the Start operation. Is that really > what you intend? Right, good catch. This ++ is indeed in the wrong place. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:17: namespace { On 2016/05/24 13:31:59, Henrik Grunell wrote: > Empty line above. Done. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:60: } // namespace On 2016/05/24 13:31:59, Henrik Grunell wrote: > Ditto. Done. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:71: void WebRTCEventLogHost::PeerConnectionAdded(int connection_id) { On 2016/05/24 13:31:59, Henrik Grunell wrote: > Are these two functions be called on any thread? I.e. add thread check or make > them thread safe. Good point, added thread checker. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:72: active_peer_connection_local_id_.insert(connection_id); On 2016/05/24 13:31:59, Henrik Grunell wrote: > DCHECK that the id isn't already in the set? std::set contains unique elements, so adding an element that's already in the set simply has no effect. I guess I can still DCHECK it, since it's not supposed to be happening. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:83: number_active_log_files_++; On 2016/05/24 13:31:59, Henrik Grunell wrote: > I don't see this ever being decreased. It is set to zero when the logging is enabled (line 100 in this version). After that it is increased each time a log file is opened until the maximum is reached. At that point the only way to create more logfiles is to call stop and then start. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:89: active_peer_connection_local_id_.erase(connection_id); On 2016/05/24 13:31:59, Henrik Grunell wrote: > DCHECK that it is actually in the set? Done. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:33: void PeerConnectionAdded(int connection_id); On 2016/05/24 13:31:59, Henrik Grunell wrote: > Please comment on these functions too. Done. https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:48: int number_active_log_files_; On 2016/05/24 13:31:59, Henrik Grunell wrote: > Can active_peer_connection_local_id_.size() be used instead of this variable? That's an interesting question. Theoretically it could happen that a single peerconnection gets repeatedly created and then destroyed. In that scenario, active_peer_connection_local_id_.size() would only be zero or one, but many files would be created. I'm not sure if that's something we should try to prevent, what do you think? https://codereview.chromium.org/1855193002/diff/180001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:53: }; On 2016/05/24 13:31:59, Henrik Grunell wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1855193002/diff/180001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/1855193002/diff/180001/content/public/browser... content/public/browser/render_process_host.h:249: virtual bool StartWebRTCEventLog(const base::FilePath& file_path) = 0; On 2016/05/20 18:27:21, ncarter wrote: > This looks basically like a rename of the old functions (I like the new names > and documentation much better). I'd hoist these up to line 221, where > EnableEventLogRecordings() used to be. Good idea, done. https://codereview.chromium.org/1855193002/diff/180001/content/public/test/mo... File content/public/test/mock_render_process_host.h (right): https://codereview.chromium.org/1855193002/diff/180001/content/public/test/mo... content/public/test/mock_render_process_host.h:90: const WebRtcRtpPacketCallback& packet_callback) override; On 2016/05/20 18:27:21, ncarter wrote: > Doesn't this need an implementation of StartWebRTCEventLog? Hmm, that's strange. I would expect that too, but the CL seems to build fine on my machine without it. I will add an implementation anyway.
https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:45: VLOG(1) << "Could not open WebRTC event log file, error=" LOGP(ERROR)? https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:55: if (file_for_transit == IPC::InvalidPlatformFileForTransit()) { nit: skip {} in these cases. it makes the code slightly cleaner with an empty line that separates this block and the rph->Send() line. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:114: number_active_log_files_++; ++foo https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:128: for (int local_id : active_peer_connection_local_id_) { nit: can skip {} https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:50: std::set<int> active_peer_connection_local_id_; Can this be a vector or unordered_set instead? set<> is rather expensive. (vector would be preferable) https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:55: bool is_rtc_event_logging_in_progress_; what does 'in progress' really mean? Would is_rtc_event_logging_enabled_ be an alternative name? https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (left): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:346: DCHECK(select_file_dialog_->HasOneRef()); yikes. shows the value of testing with dcheck enabled. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:359: static_cast<RenderProcessHostImpl*>(i.GetCurrentValue()); this feels hacky. How can you be sure that this isn't e.g. MockRenderProcessHost? https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:494: static_cast<RenderProcessHostImpl*>(i.GetCurrentValue()); same here https://codereview.chromium.org/1855193002/diff/200001/content/renderer/media... File content/renderer/media/peer_connection_tracker.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/renderer/media... content/renderer/media/peer_connection_tracker.cc:390: #if defined(OS_ANDROID) add a comment for why we use different values
lgtm https://codereview.chromium.org/1855193002/diff/180001/content/public/test/mo... File content/public/test/mock_render_process_host.h (right): https://codereview.chromium.org/1855193002/diff/180001/content/public/test/mo... content/public/test/mock_render_process_host.h:90: const WebRtcRtpPacketCallback& packet_callback) override; On 2016/05/25 14:57:00, Ivo wrote: > On 2016/05/20 18:27:21, ncarter wrote: > > Doesn't this need an implementation of StartWebRTCEventLog? > > Hmm, that's strange. I would expect that too, but the CL seems to build fine on > my machine without it. I will add an implementation anyway. Were you building the unittests, or just chrome? https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:359: static_cast<RenderProcessHostImpl*>(i.GetCurrentValue()); On 2016/05/25 15:16:21, tommi-chrömium wrote: > this feels hacky. How can you be sure that this isn't e.g. > MockRenderProcessHost? As tommi suggests, this is an illegal cast. In content, you can usually downcast from interface to Impl, but RPH is a glaring exception to that rule, since the Mock doesn't inherit from Impl. To the extent this downcast ever works, it means there is no unittest coverage for this code. You don't need this cast anyway; since the method you need to call is on the interface. https://codereview.chromium.org/1855193002/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:156: bool StopWebRTCEventLog() override; Move these up, to keep them in order with the interface
ipc lgtm https://codereview.chromium.org/1855193002/diff/200001/content/common/media/p... File content/common/media/peer_connection_tracker_messages.h (right): https://codereview.chromium.org/1855193002/diff/200001/content/common/media/p... content/common/media/peer_connection_tracker_messages.h:46: int /* lid */, Nit: s/lid/<something more descriptive> (for instance, peer_connection_id? Since that's what it's called in the message handler… or local_id since that's what it's called on the sender side. Maybe these two things should be consistent about terminology?) https://codereview.chromium.org/1855193002/diff/200001/content/renderer/media... File content/renderer/media/peer_connection_tracker.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/renderer/media... content/renderer/media/peer_connection_tracker.cc:391: const int64_t max_filesize_bytes = 10000000; Nit: kMaxFilesizeBytes
Thanks again for the excellent comments. In addition to addressing the comments, I made some extra changes: - The maximum number of logfiles is now tracked globally instead of per renderprocess. - The RTC event log should now stop when closing the webrtc-internals page. - For desktop, the maximum number of logfiles is reduced from 10 to 5, and their maximum size is increased from 30MB to 60MB, as this better aligns with our internal use cases. Please take another look. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:45: VLOG(1) << "Could not open WebRTC event log file, error=" On 2016/05/25 15:16:20, tommi-chrömium wrote: > LOGP(ERROR)? Done. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:55: if (file_for_transit == IPC::InvalidPlatformFileForTransit()) { On 2016/05/25 15:16:20, tommi-chrömium wrote: > nit: skip {} in these cases. it makes the code slightly cleaner with an empty > line that separates this block and the rph->Send() line. Done. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:114: number_active_log_files_++; On 2016/05/25 15:16:20, tommi-chrömium wrote: > ++foo Done. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:128: for (int local_id : active_peer_connection_local_id_) { On 2016/05/25 15:16:20, tommi-chrömium wrote: > nit: can skip {} Done. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:50: std::set<int> active_peer_connection_local_id_; On 2016/05/25 15:16:20, tommi-chrömium wrote: > Can this be a vector or unordered_set instead? set<> is rather expensive. > (vector would be preferable) Since this collection is not very frequently accessed (only when a PC is added/removed or an event log is started/stopped), I think unordered_set would be a good balance between performance and keeping the code simple. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:55: bool is_rtc_event_logging_in_progress_; On 2016/05/25 15:16:21, tommi-chrömium wrote: > what does 'in progress' really mean? Would is_rtc_event_logging_enabled_ be an > alternative name? Good point, I decided to remove the "is_" part as well, so the new name is rtc_event_log_enabled_. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (left): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:346: DCHECK(select_file_dialog_->HasOneRef()); On 2016/05/25 15:16:21, tommi-chrömium wrote: > yikes. shows the value of testing with dcheck enabled. Indeed. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:359: static_cast<RenderProcessHostImpl*>(i.GetCurrentValue()); On 2016/05/25 21:28:58, ncarter wrote: > On 2016/05/25 15:16:21, tommi-chrömium wrote: > > this feels hacky. How can you be sure that this isn't e.g. > > MockRenderProcessHost? > > As tommi suggests, this is an illegal cast. In content, you can usually downcast > from interface to Impl, but RPH is a glaring exception to that rule, since the > Mock doesn't inherit from Impl. To the extent this downcast ever works, it means > there is no unittest coverage for this code. > > You don't need this cast anyway; since the method you need to call is on the > interface. Good points, removed the cast since it's not necessary. https://codereview.chromium.org/1855193002/diff/200001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:494: static_cast<RenderProcessHostImpl*>(i.GetCurrentValue()); On 2016/05/25 15:16:21, tommi-chrömium wrote: > same here Removed here as well. https://codereview.chromium.org/1855193002/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/1855193002/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.h:156: bool StopWebRTCEventLog() override; On 2016/05/25 21:28:58, ncarter wrote: > Move these up, to keep them in order with the interface Done, moved the implementation in the .cc as well. https://codereview.chromium.org/1855193002/diff/200001/content/common/media/p... File content/common/media/peer_connection_tracker_messages.h (right): https://codereview.chromium.org/1855193002/diff/200001/content/common/media/p... content/common/media/peer_connection_tracker_messages.h:46: int /* lid */, On 2016/05/26 22:05:15, dcheng wrote: > Nit: s/lid/<something more descriptive> (for instance, peer_connection_id? Since > that's what it's called in the message handler… or local_id since that's what > it's called on the sender side. Maybe these two things should be consistent > about terminology?) Good point. I will use peer_connection_local_id here, and updated it in the parts of code that this CL touches. https://codereview.chromium.org/1855193002/diff/200001/content/renderer/media... File content/renderer/media/peer_connection_tracker.cc (right): https://codereview.chromium.org/1855193002/diff/200001/content/renderer/media... content/renderer/media/peer_connection_tracker.cc:390: #if defined(OS_ANDROID) On 2016/05/25 15:16:21, tommi-chrömium wrote: > add a comment for why we use different values Done. https://codereview.chromium.org/1855193002/diff/200001/content/renderer/media... content/renderer/media/peer_connection_tracker.cc:391: const int64_t max_filesize_bytes = 10000000; On 2016/05/26 22:05:15, dcheng wrote: > Nit: kMaxFilesizeBytes Done.
https://codereview.chromium.org/1855193002/diff/220001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.h (right): https://codereview.chromium.org/1855193002/diff/220001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.h:11: #include <set> do we still need this? https://codereview.chromium.org/1855193002/diff/220001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/220001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:75: DCHECK(active_peer_connection_local_id_.count(peer_connection_local_id) == 0); It looks like unordered_set is also too complex for what you want. I think you just want std::vector. In these DCHECKs, you can then just use std::find() to make sure an element either exists or does not exist in the vector. The reason I'm pushing back on using set and now unordered_set, btw is that if we don't actually need the code that's behind these types, it adds to the binary size, which increases our footprint. We need to take care of the pennies so that the pounds can take care of themselves. https://codereview.chromium.org/1855193002/diff/220001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/1855193002/diff/220001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:19: event_log_host_(event_log_host) {} if the event_loc_host param must never be nullptr, please add a dcheck for that here so that we can catch it at the source.
https://codereview.chromium.org/1855193002/diff/220001/chrome/browser/media/w... File chrome/browser/media/webrtc_event_log_handler.h (right): https://codereview.chromium.org/1855193002/diff/220001/chrome/browser/media/w... chrome/browser/media/webrtc_event_log_handler.h:11: #include <set> On 2016/05/30 15:37:16, tommi-chrömium wrote: > do we still need this? Good point, this is no longer needed. https://codereview.chromium.org/1855193002/diff/220001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/220001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:75: DCHECK(active_peer_connection_local_id_.count(peer_connection_local_id) == 0); On 2016/05/30 15:37:16, tommi-chrömium wrote: > It looks like unordered_set is also too complex for what you want. I think you > just want std::vector. In these DCHECKs, you can then just use std::find() to > make sure an element either exists or does not exist in the vector. > The reason I'm pushing back on using set and now unordered_set, btw is that if > we don't actually need the code that's behind these types, it adds to the binary > size, which increases our footprint. We need to take care of the pennies so > that the pounds can take care of themselves. Okay, that makes sense, I'll change to vector. https://codereview.chromium.org/1855193002/diff/220001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.cc (right): https://codereview.chromium.org/1855193002/diff/220001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.cc:19: event_log_host_(event_log_host) {} On 2016/05/30 15:37:16, tommi-chrömium wrote: > if the event_loc_host param must never be nullptr, please add a dcheck for that > here so that we can catch it at the source. Good idea, added.
lgtm https://codereview.chromium.org/1855193002/diff/240001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/240001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:103: active_peer_connection_local_id_.erase(std::find( nit: const auto found = std::find(...); DCHECK(found != active_peer_connection_local_id_.end()); active_peer_connection_local_id_.erase(found);
Looks good, mainly nits. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:24: const int kMaxNumberLogFiles = 3; Nit: Given the much smaller storage on Android, should it be even more limited? We talked about the max file size offline, what will it be again? https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:46: file_path, base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND); Should it overwrite instead perhaps? https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:90: base::Bind(&SendEventLogFileToRenderer, host, How is it guaranteed that |host| is valid when SendEventLogFileToRenderer is executed? It seems like it could be a use after free. Since the WebRTCEventLogHost is tied to a render process (and RPH) you could reply to a member function and use WeakPtr. Then you can also get the host ptr in that function. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:92: ++number_active_log_files_; If creating the file fails, this will be incorrectly increased. If you reply to a member function, you can increase there. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:104: active_peer_connection_local_id_.erase(found); This means that the log has stopped for that PC, right? Should the counter be decreased to allow a new PC to be logged? I imagine a test case could be that logging is enabled, and PCs come and go several times but just one is alive at a time. After 5 PCs we wouldn't log more. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:117: BrowserThread::PostTaskAndReplyWithResult( Nit: to make it more readable and decrease redundancy, you could put the common code (here and PeerConnectionAdded()) in its own function. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:14: // This class is used to active and disable WebRTC event logs on each of the Nit: "enable and disable"? https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:15: // peer connections in the render process. To be able to do this, it needs to Nit: "PeerConnections". Also, a bit of repetition in this comment. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:22: // Starts an RTC event log for each peerconnection on the render process. Nit: "PeerConnection". https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:26: // from the Browser UI thread. You should comment that it will start logging for all new PeerConnections added after starting. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:50: std::vector<int> active_peer_connection_local_id_; Nit: ..._ids_? (plural) https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:53: // accessed from the Browser UI thread, it is safe to access this from every Nit: You could comment in the class description that this class lives on the UI thread, and all function must be called on it. Then remove thread comments on functions and variables. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:338: if (enable) { Nit: Better to have two separate functions instead of enable/disable parameter. No need to in this CL, but feel free to be a hero and change that. :) (I understand if you don't.) https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.h:63: WebRTCEventLogHost* const event_log_host_; Is the event log host guaranteed to outlive the PC tracker? https://codereview.chromium.org/1855193002/diff/260001/content/browser/resour... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/resour... content/browser/resources/media/dump_creator.js:78: ' overwritten. </p>'; Nit: fits on above line. https://codereview.chromium.org/1855193002/diff/260001/content/browser/resour... content/browser/resources/media/dump_creator.js:78: ' overwritten. </p>'; Overwritten or appended? https://codereview.chromium.org/1855193002/diff/260001/content/common/media/p... File content/common/media/peer_connection_tracker_messages.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/common/media/p... content/common/media/peer_connection_tracker_messages.h:48: IPC_MESSAGE_CONTROL1(PeerConnectionTracker_StopEventLog, int /* lid */) What's lid? Spell out would be good.
Patchset #11 (id:280001) has been deleted
Patchset #11 (id:300001) has been deleted
While testing I noticed that when event logs were not being recorded for tabs created after ticking the checkbox. I fixed that in this iteration. Please take another look. https://codereview.chromium.org/1855193002/diff/240001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/240001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:103: active_peer_connection_local_id_.erase(std::find( On 2016/05/31 09:11:22, tommi-chrömium wrote: > nit: > > const auto found = std::find(...); > DCHECK(found != active_peer_connection_local_id_.end()); > active_peer_connection_local_id_.erase(found); Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:24: const int kMaxNumberLogFiles = 3; On 2016/06/01 08:31:11, Henrik Grunell wrote: > Nit: Given the much smaller storage on Android, should it be even more limited? > We talked about the max file size offline, what will it be again? On android the max file size limit is set to 10MB per file (see content/renderer/media/peer_connection_tracker.cc), so the maximum amount of storage is 30 MB. Do you think it should be restricted further? https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:46: file_path, base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND); On 2016/06/01 08:31:11, Henrik Grunell wrote: > Should it overwrite instead perhaps? Good point, I guess that makes more sense. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:90: base::Bind(&SendEventLogFileToRenderer, host, On 2016/06/01 08:31:10, Henrik Grunell wrote: > How is it guaranteed that |host| is valid when SendEventLogFileToRenderer is > executed? It seems like it could be a use after free. Since the > WebRTCEventLogHost is tied to a render process (and RPH) you could reply to a > member function and use WeakPtr. Then you can also get the host ptr in that > function. Good catch, I did not consider that. I changed the code to reply to a member function and use a WeakPtr like you suggest. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:92: ++number_active_log_files_; On 2016/06/01 08:31:10, Henrik Grunell wrote: > If creating the file fails, this will be incorrectly increased. If you reply to > a member function, you can increase there. Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:104: active_peer_connection_local_id_.erase(found); On 2016/06/01 08:31:10, Henrik Grunell wrote: > This means that the log has stopped for that PC, right? Should the counter be > decreased to allow a new PC to be logged? I imagine a test case could be that > logging is enabled, and PCs come and go several times but just one is alive at a > time. After 5 PCs we wouldn't log more. I guess that depends on what kind of behavior we want. I made it this way because I liked that we can guarantee that no more than 5 logfiles will be created no matter what happens, and the amount of storage that can be used for logs has a hard cap (even if you forget about clicking the checkbox and leave the tab open for months). Let me know if you think the other behavior makes more sense. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:117: BrowserThread::PostTaskAndReplyWithResult( On 2016/06/01 08:31:10, Henrik Grunell wrote: > Nit: to make it more readable and decrease redundancy, you could put the common > code (here and PeerConnectionAdded()) in its own function. Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:14: // This class is used to active and disable WebRTC event logs on each of the On 2016/06/01 08:31:11, Henrik Grunell wrote: > Nit: "enable and disable"? Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:15: // peer connections in the render process. To be able to do this, it needs to On 2016/06/01 08:31:11, Henrik Grunell wrote: > Nit: "PeerConnections". Also, a bit of repetition in this comment. Good point. I think the second part is not that useful so I removed it. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:22: // Starts an RTC event log for each peerconnection on the render process. On 2016/06/01 08:31:11, Henrik Grunell wrote: > Nit: "PeerConnection". Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:26: // from the Browser UI thread. On 2016/06/01 08:31:11, Henrik Grunell wrote: > You should comment that it will start logging for all new PeerConnections added > after starting. I added a remark a few lines above. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:50: std::vector<int> active_peer_connection_local_id_; On 2016/06/01 08:31:11, Henrik Grunell wrote: > Nit: ..._ids_? (plural) Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:53: // accessed from the Browser UI thread, it is safe to access this from every On 2016/06/01 08:31:11, Henrik Grunell wrote: > Nit: You could comment in the class description that this class lives on the UI > thread, and all function must be called on it. Then remove thread comments on > functions and variables. Good idea. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:338: if (enable) { On 2016/06/01 08:31:11, Henrik Grunell wrote: > Nit: Better to have two separate functions instead of enable/disable parameter. > No need to in this CL, but feel free to be a hero and change that. :) (I > understand if you don't.) I added it to the CL, I can't resist being called a hero when I get a chance ;) https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.h:63: WebRTCEventLogHost* const event_log_host_; On 2016/06/01 08:31:11, Henrik Grunell wrote: > Is the event log host guaranteed to outlive the PC tracker? Both are members of RenderProcessHostImpl, so they should have the same lifetime. https://codereview.chromium.org/1855193002/diff/260001/content/browser/resour... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/resour... content/browser/resources/media/dump_creator.js:78: ' overwritten. </p>'; On 2016/06/01 08:31:11, Henrik Grunell wrote: > Overwritten or appended? Since I changed it to actually overwrite, the text is now correct :-) https://codereview.chromium.org/1855193002/diff/260001/content/common/media/p... File content/common/media/peer_connection_tracker_messages.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/common/media/p... content/common/media/peer_connection_tracker_messages.h:48: IPC_MESSAGE_CONTROL1(PeerConnectionTracker_StopEventLog, int /* lid */) On 2016/06/01 08:31:11, Henrik Grunell wrote: > What's lid? Spell out would be good. Good point, I updated the Start message but forgot this one. Thanks.
Besides the comments, you need to add a unit test for the new class. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:24: const int kMaxNumberLogFiles = 3; On 2016/06/02 14:43:38, Ivo wrote: > On 2016/06/01 08:31:11, Henrik Grunell wrote: > > Nit: Given the much smaller storage on Android, should it be even more > limited? > > We talked about the max file size offline, what will it be again? > > On android the max file size limit is set to 10MB per file (see > content/renderer/media/peer_connection_tracker.cc), so the maximum amount of > storage is 30 MB. Do you think it should be restricted further? Ah OK, that sounds fine. Nit: Could you comment on that there is a file size limit and point to where that is? Although that pointer might be stale, it's better than nothing. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:104: active_peer_connection_local_id_.erase(found); On 2016/06/02 14:43:38, Ivo wrote: > On 2016/06/01 08:31:10, Henrik Grunell wrote: > > This means that the log has stopped for that PC, right? Should the counter be > > decreased to allow a new PC to be logged? I imagine a test case could be that > > logging is enabled, and PCs come and go several times but just one is alive at > a > > time. After 5 PCs we wouldn't log more. > > I guess that depends on what kind of behavior we want. I made it this way > because I liked that we can guarantee that no more than 5 logfiles will be > created no matter what happens, and the amount of storage that can be used for > logs has a hard cap (even if you forget about clicking the checkbox and leave > the tab open for months). > Let me know if you think the other behavior makes more sense. That's a good point. OK, let's keep it this way. Please update the description (on html page) that when the limit is reached, logging must be restarted. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:338: if (enable) { On 2016/06/02 14:43:39, Ivo wrote: > On 2016/06/01 08:31:11, Henrik Grunell wrote: > > Nit: Better to have two separate functions instead of enable/disable > parameter. > > No need to in this CL, but feel free to be a hero and change that. :) (I > > understand if you don't.) > > I added it to the CL, I can't resist being called a hero when I get a chance ;) Awesome! :D https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.h:63: WebRTCEventLogHost* const event_log_host_; On 2016/06/02 14:43:39, Ivo wrote: > On 2016/06/01 08:31:11, Henrik Grunell wrote: > > Is the event log host guaranteed to outlive the PC tracker? > > Both are members of RenderProcessHostImpl, so they should have the same > lifetime. OK, then one will be deleted before the other, so it depends on that order. Please check this, and if you decide to depend on the order comment on that clearly in the RPHI class declaration, i.e. at the member variables. https://codereview.chromium.org/1855193002/diff/260001/content/browser/resour... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/resour... content/browser/resources/media/dump_creator.js:78: ' overwritten. </p>'; On 2016/06/02 14:43:39, Ivo wrote: > On 2016/06/01 08:31:11, Henrik Grunell wrote: > > Overwritten or appended? > > Since I changed it to actually overwrite, the text is now correct :-) Acknowledged. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:91: bool WebRTCEventLogHost::StartWebRTCEventLog(const base::FilePath& file_path) { Why did the thread check go away? You should have it in all functions. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:97: DoStartEventLog(local_id); Nit: Name function "StartEventLogForPeerConnection()"? Increased readability imo. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:115: if (number_active_log_files_ < kMaxNumberLogFiles) { OK, moving the counter increase causes a problem with this condition. It needs to be increased here, and decreased in SendEventLogFileToRenderer if failed. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:137: IPC::PlatformFileForTransitToFile(file_for_transit).Close(); Great that you're closing it here. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:28: // Should be called from the Browser UI thread. Nit: Remove thread comment. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:355: // Tear down the dialog since the user has unchecked the event log checkbox. Nit: Is the DCHECK for one ref not useful anymore? https://codereview.chromium.org/1855193002/diff/320001/content/browser/resour... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/resour... content/browser/resources/media/dump_creator.js:77: ' already exists, it will be overwritten. </p>'; Comment on that there's a limit of number of files, and size of file.
Added a unittest like you suggested, PTAL. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:24: const int kMaxNumberLogFiles = 3; On 2016/06/03 07:40:52, Henrik Grunell wrote: > On 2016/06/02 14:43:38, Ivo wrote: > > On 2016/06/01 08:31:11, Henrik Grunell wrote: > > > Nit: Given the much smaller storage on Android, should it be even more > > limited? > > > We talked about the max file size offline, what will it be again? > > > > On android the max file size limit is set to 10MB per file (see > > content/renderer/media/peer_connection_tracker.cc), so the maximum amount of > > storage is 30 MB. Do you think it should be restricted further? > > Ah OK, that sounds fine. > > Nit: Could you comment on that there is a file size limit and point to where > that is? Although that pointer might be stale, it's better than nothing. Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:104: active_peer_connection_local_id_.erase(found); On 2016/06/03 07:40:52, Henrik Grunell wrote: > On 2016/06/02 14:43:38, Ivo wrote: > > On 2016/06/01 08:31:10, Henrik Grunell wrote: > > > This means that the log has stopped for that PC, right? Should the counter > be > > > decreased to allow a new PC to be logged? I imagine a test case could be > that > > > logging is enabled, and PCs come and go several times but just one is alive > at > > a > > > time. After 5 PCs we wouldn't log more. > > > > I guess that depends on what kind of behavior we want. I made it this way > > because I liked that we can guarantee that no more than 5 logfiles will be > > created no matter what happens, and the amount of storage that can be used for > > logs has a hard cap (even if you forget about clicking the checkbox and leave > > the tab open for months). > > Let me know if you think the other behavior makes more sense. > > That's a good point. OK, let's keep it this way. Please update the description > (on html page) that when the limit is reached, logging must be restarted. Done. https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... File content/browser/renderer_host/media/peer_connection_tracker_host.h (right): https://codereview.chromium.org/1855193002/diff/260001/content/browser/render... content/browser/renderer_host/media/peer_connection_tracker_host.h:63: WebRTCEventLogHost* const event_log_host_; On 2016/06/03 07:40:52, Henrik Grunell wrote: > On 2016/06/02 14:43:39, Ivo wrote: > > On 2016/06/01 08:31:11, Henrik Grunell wrote: > > > Is the event log host guaranteed to outlive the PC tracker? > > > > Both are members of RenderProcessHostImpl, so they should have the same > > lifetime. > > OK, then one will be deleted before the other, so it depends on that order. > Please check this, and if you decide to depend on the order comment on that > clearly in the RPHI class declaration, i.e. at the member variables. Actually, I don't think that the order matters in this case. Since WebRTCEventLogHost is only ever accessed from the Browser::UI thread, I don't think it's possible that any calls to WebRTCEventLogHost happen during the tearing down of the RenderProcessHost, which also happens on the Browser::UI thread. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:91: bool WebRTCEventLogHost::StartWebRTCEventLog(const base::FilePath& file_path) { On 2016/06/03 07:40:52, Henrik Grunell wrote: > Why did the thread check go away? You should have it in all functions. Done. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:97: DoStartEventLog(local_id); On 2016/06/03 07:40:52, Henrik Grunell wrote: > Nit: Name function "StartEventLogForPeerConnection()"? Increased readability > imo. Done. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:115: if (number_active_log_files_ < kMaxNumberLogFiles) { On 2016/06/03 07:40:52, Henrik Grunell wrote: > OK, moving the counter increase causes a problem with this condition. It needs > to be increased here, and decreased in SendEventLogFileToRenderer if failed. Good catch, thanks. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.h (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.h:28: // Should be called from the Browser UI thread. On 2016/06/03 07:40:53, Henrik Grunell wrote: > Nit: Remove thread comment. Done. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:355: // Tear down the dialog since the user has unchecked the event log checkbox. On 2016/06/03 07:40:53, Henrik Grunell wrote: > Nit: Is the DCHECK for one ref not useful anymore? The DCHECK is problematic because the file dialog is shared with the AEC dump checkbox, it can be set to nullptr in the DisableAudioDebugRecordings() function. https://codereview.chromium.org/1855193002/diff/320001/content/browser/resour... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/resour... content/browser/resources/media/dump_creator.js:77: ' already exists, it will be overwritten. </p>'; On 2016/06/03 07:40:53, Henrik Grunell wrote: > Comment on that there's a limit of number of files, and size of file. Done.
Just some comments on the new test. https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... File content/browser/media/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/1855193002/diff/320001/content/browser/media/... content/browser/media/webrtc/webrtc_internals.cc:355: // Tear down the dialog since the user has unchecked the event log checkbox. On 2016/06/03 16:07:02, Ivo wrote: > On 2016/06/03 07:40:53, Henrik Grunell wrote: > > Nit: Is the DCHECK for one ref not useful anymore? > > The DCHECK is problematic because the file dialog is shared with the AEC dump > checkbox, it can be set to nullptr in the DisableAudioDebugRecordings() > function. Acknowledged. https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc (right): https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:52: ASSERT_TRUE(CreateTemporaryFile(&base_file_)); base:: ? (Does this compile?) https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:64: void validateStartIPCMessageAndCloseFile(const IPC::Message* msg, ValidateStart... (capital first letter). Same below. https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:66: ASSERT_TRUE(msg); Does this compile? Doesn't ASSERT_XYZ() return true/false? https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:201: } Could you add a test case for going over the limit and ensure no log is enabled for further PeerConnections? https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:203: } // namespace The tests are typically not put in an anonymous namespace. Sometimes the test class is. In this case, only GetExpectedEventLogFileName should have to be in an anonymous namespace.
https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc (right): https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:52: ASSERT_TRUE(CreateTemporaryFile(&base_file_)); On 2016/06/08 10:08:50, Henrik Grunell wrote: > base:: ? (Does this compile?) It does compile, but I will remove it https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:64: void validateStartIPCMessageAndCloseFile(const IPC::Message* msg, On 2016/06/08 10:08:50, Henrik Grunell wrote: > ValidateStart... (capital first letter). > > Same below. Done. https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:66: ASSERT_TRUE(msg); On 2016/06/08 10:08:50, Henrik Grunell wrote: > Does this compile? Doesn't ASSERT_XYZ() return true/false? It compiles and runs. In this case, if the ASSERT fails it will return from this function, which I don't see as a problem. I put it here because the rest of the function doesn't make sense if msg is a nullptr. https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:201: } On 2016/06/08 10:08:50, Henrik Grunell wrote: > Could you add a test case for going over the limit and ensure no log is enabled > for further PeerConnections? Done. https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:203: } // namespace On 2016/06/08 10:08:50, Henrik Grunell wrote: > The tests are typically not put in an anonymous namespace. Sometimes the test > class is. In this case, only GetExpectedEventLogFileName should have to be in an > anonymous namespace. Done.
lgtm with two minor comments fixed. Thanks for your patience! https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc (right): https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:52: ASSERT_TRUE(CreateTemporaryFile(&base_file_)); On 2016/06/08 14:52:29, Ivo wrote: > On 2016/06/08 10:08:50, Henrik Grunell wrote: > > base:: ? (Does this compile?) > > It does compile, but I will remove it I think you should keep it. There's no using statement in this file, I guess it's in any of the included files which this code should not depend on. https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:66: ASSERT_TRUE(msg); On 2016/06/08 14:52:29, Ivo wrote: > On 2016/06/08 10:08:50, Henrik Grunell wrote: > > Does this compile? Doesn't ASSERT_XYZ() return true/false? > > It compiles and runs. In this case, if the ASSERT fails it will return from this > function, which I don't see as a problem. > I put it here because the rest of the function doesn't make sense if msg is a > nullptr. OK. I just recalled incorrectly then. (Thought it returned a bool.) https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:113: if (host) { Just a note that if a render process goes away, we won't decrease the counter. I think that's fine, but good to keep in mind. https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc (right): https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:195: for (int i = 0; i < kNumberOfPeerConnections; ++i) { Nit: No {}.
Patchset #14 (id:380001) has been deleted
Thanks for all your comments, the code improved a lot! https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc (right): https://codereview.chromium.org/1855193002/diff/340001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:52: ASSERT_TRUE(CreateTemporaryFile(&base_file_)); On 2016/06/09 10:28:22, Henrik Grunell wrote: > On 2016/06/08 14:52:29, Ivo wrote: > > On 2016/06/08 10:08:50, Henrik Grunell wrote: > > > base:: ? (Does this compile?) > > > > It does compile, but I will remove it > > I think you should keep it. There's no using statement in this file, I guess > it's in any of the included files which this code should not depend on. Oh, I misunderstood, I thought you were suggesting I should not use base::, I will add it back :-) https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host.cc (right): https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host.cc:113: if (host) { On 2016/06/09 10:28:22, Henrik Grunell wrote: > Just a note that if a render process goes away, we won't decrease the counter. I > think that's fine, but good to keep in mind. Acknowledged. https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... File content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc (right): https://codereview.chromium.org/1855193002/diff/360001/content/browser/media/... content/browser/media/webrtc/webrtc_eventlog_host_unittest.cc:195: for (int i = 0; i < kNumberOfPeerConnections; ++i) { On 2016/06/09 10:28:22, Henrik Grunell wrote: > Nit: No {}. Done.
The CQ bit was checked by ivoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, dcheng@chromium.org, tommi@chromium.org, grunell@chromium.org Link to the patchset: https://codereview.chromium.org/1855193002/#ps420001 (title: "Rebase.")
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
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ivoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, nick@chromium.org, tommi@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1855193002/#ps480001 (title: "Added CONTENT_EXPORT to WebRTCEventLogHost.")
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 ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:600661,chromium:613499,webrtc:4741 ========== to ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:600661,chromium:613499,webrtc:4741 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:600661,chromium:613499,webrtc:4741 ========== to ========== Enable the WebRTC event log on PeerConnection instead of PeerConnectionFactory. This allows simultaneous recording of event logs of multiple PeerConnections, each in a seperate file. The number of files is limited, as well as the size of each file. A callback handler is added to RenderProcessHost to allow code in chrome/ to register callbacks to implement the WebRTC event log functionality. BUG=chromium:600661,chromium:613499,webrtc:4741 Committed: https://crrev.com/cf0887d3df989061ca653339e7affa8e49a3cfe6 Cr-Commit-Position: refs/heads/master@{#404183} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/cf0887d3df989061ca653339e7affa8e49a3cfe6 Cr-Commit-Position: refs/heads/master@{#404183} |
