Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1650133002:
Start and stop RTC event logs from private extension API. (Closed)
|
Created:
4 years, 10 months ago by terelius-chromium Modified:
4 years, 8 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, posciak+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, Ivo-OOO until feb 6 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded functions to start and stop RTC event logs from a private extension API. The API is also protected by a command line flag.
Renamed AudioDebugRecordingsCallback to TimeLimitedRecordingCallback and similar for AudioDebugRecordingsErrorCallback and AudioDebugRecordingsInfo.
BUG=582469
Committed: https://crrev.com/fa159cc7961317c4a1d743d2386e19e656fef78c
Cr-Commit-Position: refs/heads/master@{#386993}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Create a new handler for the internal WebRTC logs instead of using WebRTCLoggingHandlerHost. #
Total comments: 11
Patch Set 3 : Drop MessageFilter, add keys for use in GetUserData() #
Total comments: 2
Patch Set 4 : Split the logging handler into one for audio recordings and one for event logs #
Total comments: 43
Patch Set 5 : Comments from grunell and guido #Patch Set 6 : Unused includes #
Total comments: 8
Patch Set 7 : Test that the event log actually produces some content #
Total comments: 9
Patch Set 8 : More comments #Patch Set 9 : Fix bug in current time-limited audio debug recordings. #
Total comments: 30
Patch Set 10 : Some comments from tommi #Messages
Total messages: 44 (8 generated)
Description was changed from ========== Added functions to start and stop RTC event logs from hangouts API. Renamed AudioDebugRecordingsCallback to TimeLimitedRecordingCallback and similar for AudioDebugRecordingsErrorCallback and AudioDebugRecordingsInfo. BUG= ========== to ========== Added functions to start and stop RTC event logs from a private extension API. The API is also protected by a command line flag. Renamed AudioDebugRecordingsCallback to TimeLimitedRecordingCallback and similar for AudioDebugRecordingsErrorCallback and AudioDebugRecordingsInfo. BUG=582469 ==========
terelius@chromium.org changed reviewers: + grunell@chromium.org, guidou@chromium.org
Please take a look before I submit this CL to OWNERS.
Started with high level review. Main concern is that WebRtcLoggingHandlerHost does too much. https://codereview.chromium.org/1650133002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_apitest.cc:466: // TODO(terelius): These tests are missing verification of the actual RTC At a minimum, I think you should check that the file was created. https://codereview.chromium.org/1650133002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/1650133002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.h:178: // Starts an RTC event log. The call writes the most recent events to a This class was originally only for the WebRTC native logging. I don't think audio debug recordings should have been added here, since it's a separate functionalities. Same with RTC event log. This class is getting much too large and has multiple responsibilities. Please put it in a separate file. https://codereview.chromium.org/1650133002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1650133002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/webrtc_logging_private.idl:33: dictionary TimeLimitedRecordingInfo { The recordings don't have to be time limited (|seconds| == 0), so the naming is a bit confusing. Maybe just skip "TimeLimited"?
On 2016/02/01 13:58:13, Henrik Grunell wrote: > Started with high level review. Main concern is that WebRtcLoggingHandlerHost > does too much. > I think there are practical advantages to having these functions in WebRtcLoggingHandlerHost. We avoid all the boilerplate of a new extension for just a couple of wrapper functions, and we make it easier for the hangouts people, as they have to load only this extension. With debug recordings we started with a separate extension, but after seeing that the boilerplate was more than the actual functionality we wanted to introduce, it was decided to do it here. Adding tommi@ for his thoughts.
guidou@chromium.org changed reviewers: + tommi@chromium.org
+tommi
On 2016/02/02 13:23:05, Guido Urdaneta wrote: > On 2016/02/01 13:58:13, Henrik Grunell wrote: > > Started with high level review. Main concern is that WebRtcLoggingHandlerHost > > does too much. > > > > I think there are practical advantages to having these functions in > WebRtcLoggingHandlerHost. > We avoid all the boilerplate of a new extension for just a couple of wrapper > functions, and we make it easier for the hangouts people, as they have to load > only this extension. > With debug recordings we started with a separate extension, but after seeing > that the boilerplate was more than the actual functionality we wanted to > introduce, it was decided to do it here. > Adding tommi@ for his thoughts. Yeah, I think the new functions should go into the existing extension API. I meant the WebRtcLoggingHandlerHost class which is separate from that.
Right, so no change in the Javascript layer, but we instantiate the
new class in chrome_content_browser_client.cc
WebRtcInternalLogHandlerHost* webrtc_internal_log_handler_host =
new WebRtcInternalLogHandlerHost(profile);
Then in webrtc_logging_private_api.cc, in the function
WebrtcLoggingPrivateStartAudioDebugRecordingsFunction::RunAsync()
we call
webrtc_internal_log_handler_host->StartAudioDebugRecordings(...)
instead of
webrtc_logging_handler_host->StartAudioDebugRecordings(...)
and similarly for the other functions.
Does this sound correct?
On Tue, Feb 2, 2016 at 2:32 PM, <grunell@chromium.org> wrote:
>
> On 2016/02/02 13:23:05, Guido Urdaneta wrote:
>> On 2016/02/01 13:58:13, Henrik Grunell wrote:
>> > Started with high level review. Main concern is that
> WebRtcLoggingHandlerHost
>> > does too much.
>> >
>>
>> I think there are practical advantages to having these functions in
>> WebRtcLoggingHandlerHost.
>> We avoid all the boilerplate of a new extension for just a couple of
>> wrapper
>> functions, and we make it easier for the hangouts people, as they have to
>> load
>> only this extension.
>> With debug recordings we started with a separate extension, but after
>> seeing
>> that the boilerplate was more than the actual functionality we wanted to
>> introduce, it was decided to do it here.
>> Adding tommi@ for his thoughts.
>
> Yeah, I think the new functions should go into the existing extension API. I
> meant the WebRtcLoggingHandlerHost class which is separate from that.
>
> https://codereview.chromium.org/1650133002/
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/02 13:44:35, chromium-reviews wrote: > Right, so no change in the Javascript layer, but we instantiate the > new class in chrome_content_browser_client.cc > WebRtcInternalLogHandlerHost* webrtc_internal_log_handler_host = > new WebRtcInternalLogHandlerHost(profile); > > Then in webrtc_logging_private_api.cc, in the function > WebrtcLoggingPrivateStartAudioDebugRecordingsFunction::RunAsync() > we call > webrtc_internal_log_handler_host->StartAudioDebugRecordings(...) > instead of > webrtc_logging_handler_host->StartAudioDebugRecordings(...) > and similarly for the other functions. > > Does this sound correct? Yes, that sounds good. Though I don't really like the naming of the new class. I know it comes from webrtc-internals, but e.g. debug audio recordings isn't really WebRTC specific. Well, let me think about it a bit. > > On Tue, Feb 2, 2016 at 2:32 PM, <mailto:grunell@chromium.org> wrote: > > > > On 2016/02/02 13:23:05, Guido Urdaneta wrote: > >> On 2016/02/01 13:58:13, Henrik Grunell wrote: > >> > Started with high level review. Main concern is that > > WebRtcLoggingHandlerHost > >> > does too much. > >> > > >> > >> I think there are practical advantages to having these functions in > >> WebRtcLoggingHandlerHost. > >> We avoid all the boilerplate of a new extension for just a couple of > >> wrapper > >> functions, and we make it easier for the hangouts people, as they have to > >> load > >> only this extension. > >> With debug recordings we started with a separate extension, but after > >> seeing > >> that the boilerplate was more than the actual functionality we wanted to > >> introduce, it was decided to do it here. > >> Adding tommi@ for his thoughts. > > > > Yeah, I think the new functions should go into the existing extension API. I > > meant the WebRtcLoggingHandlerHost class which is separate from that. > > > > https://codereview.chromium.org/1650133002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. >
https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:936: host->SetUserData(host, I think this will override the previous call above. (Same key |host|.)
https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:936: host->SetUserData(host, On 2016/02/03 08:37:32, Henrik Grunell wrote: > I think this will override the previous call above. (Same key |host|.) Just a follow-up after our discussion. Do you have any suggestion on how I should handle this? What should I use as key, or should some other object own the handler?
Tommi, would you mind taking over looking at this? The ownership and naming of the new class needs to be sorted out, it should not be a filter. Then code details review. https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:936: host->SetUserData(host, On 2016/02/05 09:27:37, terelius2 wrote: > On 2016/02/03 08:37:32, Henrik Grunell wrote: > > I think this will override the previous call above. (Same key |host|.) > > Just a follow-up after our discussion. Do you have any suggestion on how I > should handle this? What should I use as key, or should some other object own > the handler? Yeah, the new class should not be a filter since it's not used for IPC. Thus it should not be added as filter to the host, and ownership must be explicit somewhere. (I think the host has references to filters.)
looks like there will be some big changes coming. can you ping back when current comments have been addressed? https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:932: profile /*, g_browser_process->webrtc_log_uploader()*/); Something that's intended to be cleaned up? (next lines too) https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:446: switches::kEnableRtcEventLoggingFromExtension)) { Is there information about when this switch will be used and why we don't want to allow access to this function by default? https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:498: base::Bind(&WebrtcLoggingPrivateStopRtcEventLoggingFunction::FireCallback, nit: use the same way of wrapping as you do two lines below.
Yes, there are several things to be cleaned up. However, I don't think it is meaningful to do this until the overall design has been settled. 1) Should we create a new class for managing the aec dump and the combined bwe/acm event log? The alternatives are (a) create a new class, (b) reuse some existing class, or (c) call functions in RenderProcessHost directly from WebrtcLoggingPrivateStartRtcEventLoggingFunction::RunAsync() without any intermediate class. I'd prefer option (c), but that raises the question of where we should store state, e.g. LOGGING/NOT_LOGGING. There might be other issues, but AFAIK it would work. The questions below pertain to option (a) which is what grunell@ suggested. 2) Should the new WebRtcInternalLogHandlerHost class inherit from something? If so, from what? 3) Who should own the reference to the WebRtcInternalLogHandlerHost, and how should the ownership work? 4) What should the WebRtcInternalLogHandlerHost be named?
Is there some doc you can point us to so that we can help answer those questions? It would also be good to understand the command line switch. On Fri, Feb 5, 2016 at 1:32 PM <terelius@chromium.org> wrote: > > Yes, there are several things to be cleaned up. However, I don't think it is > meaningful to do this until the overall design has been settled. > > 1) Should we create a new class for managing the aec dump and the combined > bwe/acm event log? The alternatives are (a) create a new class, (b) reuse some > existing class, or (c) call functions in RenderProcessHost directly from > WebrtcLoggingPrivateStartRtcEventLoggingFunction::RunAsync() without any > intermediate class. > > I'd prefer option (c), but that raises the question of where we should store > state, e.g. LOGGING/NOT_LOGGING. There might be other issues, but AFAIK it would > work. The questions below pertain to option (a) which is what grunell@ > suggested. > > 2) Should the new WebRtcInternalLogHandlerHost class inherit from something? If > so, from what? > > 3) Who should own the reference to the WebRtcInternalLogHandlerHost, and how > should the ownership work? > > 4) What should the WebRtcInternalLogHandlerHost be named? > https://codereview.chromium.org/1650133002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
There is a short design doc, but it won't help with these questions. https://docs.google.com/document/d/1Md8FITMEOFow8pVbz7ZWjoda3uD5FcO4TXP_JMy37Vw/ The idea was to propagate start and stop messages from Javascript to the RenderProcessHost in the same way as it is done for the AEC dump. grunell@ pointed out this makes the WebRtcLoggignHandlerHost class rather large and said that he would prefer to move both the event log and the AEC dump features out of that class. I split the class into the old WebRtcLoggingHandlerHost and new class handling the AEC dump and the upcoming event log. This does not work, however, because of the way SetUserData is used to reference the WebRtcLoggingHandlerHost from RenderProcessHost. The SetUserData call uses the host address as a key, so to add two data objects we would have to add use a different key. "host+1" would do the trick, but it feels like an ugly hack. grunell@ also pointed out that the new handler doesn't have to be a MessageFilter, so we could use some other method to attach it to the RenderProcess. The real question is how. The command line switch is there mostly because that's how it was done for AEC dumps. I suppose it acts as an extra layer of privacy protection; you're less likely to accidentally start logging your calls if you need to pass a command line switch to the browser. I;m not sure what the rationale was though. On 2016/02/05 13:45:22, tommi-chromium wrote: > Is there some doc you can point us to so that we can help answer those > questions? > It would also be good to understand the command line switch. >
https://codereview.chromium.org/1650133002/diff/1/chrome/browser/media/webrtc... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/1650133002/diff/1/chrome/browser/media/webrtc... chrome/browser/media/webrtc_logging_handler_host.h:178: // Starts an RTC event log. The call writes the most recent events to a On 2016/02/01 13:58:13, Henrik Grunell wrote: > This class was originally only for the WebRTC native logging. I don't think > audio debug recordings should have been added here, since it's a separate > functionalities. Same with RTC event log. This class is getting much too large > and has multiple responsibilities. Please put it in a separate file. Done. https://codereview.chromium.org/1650133002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1650133002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/webrtc_logging_private.idl:33: dictionary TimeLimitedRecordingInfo { On 2016/02/01 13:58:13, Henrik Grunell wrote: > The recordings don't have to be time limited (|seconds| == 0), so the naming is > a bit confusing. Maybe just skip "TimeLimited"? Since the main difference between this callback and e.g. GenericDoneCallback is that the new callback can fire after a certain time limit (in addition to a manual stop), I thought it was an appropriate name. I am open to suggestions. https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:932: profile /*, g_browser_process->webrtc_log_uploader()*/); On 2016/02/05 11:09:00, tommi-chromium wrote: > Something that's intended to be cleaned up? > (next lines too) Yes. Done. https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:936: host->SetUserData(host, On 2016/02/05 10:05:44, Henrik Grunell wrote: > On 2016/02/05 09:27:37, terelius2 wrote: > > On 2016/02/03 08:37:32, Henrik Grunell wrote: > > > I think this will override the previous call above. (Same key |host|.) > > > > Just a follow-up after our discussion. Do you have any suggestion on how I > > should handle this? What should I use as key, or should some other object own > > the handler? > > Yeah, the new class should not be a filter since it's not used for IPC. Thus it > should not be added as filter to the host, and ownership must be explicit > somewhere. (I think the host has references to filters.) Removed the filter. Added static const strings to each of the logging handlers for use as keys. According to the documentation for SetUserData: // This object will TAKE OWNERSHIP of the given data pointer, and will // delete the object if it is changed or the object is destroyed. So apparently there is no need for an explicit ownership. https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:446: switches::kEnableRtcEventLoggingFromExtension)) { On 2016/02/05 11:09:00, tommi-chromium wrote: > Is there information about when this switch will be used and why we don't want > to allow access to this function by default? Extra privacy protection. See previous comments and email discussion. https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:498: base::Bind(&WebrtcLoggingPrivateStopRtcEventLoggingFunction::FireCallback, On 2016/02/05 11:09:00, tommi-chromium wrote: > nit: use the same way of wrapping as you do two lines below. Done.
https://codereview.chromium.org/1650133002/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_internal_log_handler_host.h (right): https://codereview.chromium.org/1650133002/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_internal_log_handler_host.h:23: class WebRtcInternalLogHandlerHost I'd prefer separate classes for the audio recordings and the rtc event log, named say AudioDebugRecordingsHandler and RtcEventLogHandler. Is there a lot of boilerplate code?
https://codereview.chromium.org/1650133002/diff/40001/chrome/browser/media/we... File chrome/browser/media/webrtc_internal_log_handler_host.h (right): https://codereview.chromium.org/1650133002/diff/40001/chrome/browser/media/we... chrome/browser/media/webrtc_internal_log_handler_host.h:23: class WebRtcInternalLogHandlerHost On 2016/02/17 12:33:02, Henrik Grunell wrote: > I'd prefer separate classes for the audio recordings and the rtc event log, > named say AudioDebugRecordingsHandler and RtcEventLogHandler. Is there a lot of > boilerplate code? Done. Please take a look.
Only nits from me. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:404: scoped_refptr<AudioDebugRecordingsHandler> audio_debug_recrdings_handler( typo: recrdings -> recordings https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... File chrome/browser/media/audio_debug_recordings_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:22: // - Starts and stops RTC event logs. This class is only for AudioDebugRecordings. Should the class be called WebRtcAudioDebugRecordingsHandler to emphasize that this is related to WebRTC? https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... File chrome/browser/media/rtc_event_log_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:22: // - Starts and stops RTC event logs. And this one is only for RTC event logs. WebRtcEventLogHandler as class name?
Thanks a lot for refactoring/moving out audio debug recording code! Getting closer... https://codereview.chromium.org/1650133002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1650133002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/webrtc_logging_private.idl:33: dictionary TimeLimitedRecordingInfo { On 2016/02/16 20:09:12, terelius1 wrote: > On 2016/02/01 13:58:13, Henrik Grunell wrote: > > The recordings don't have to be time limited (|seconds| == 0), so the naming > is > > a bit confusing. Maybe just skip "TimeLimited"? > > Since the main difference between this callback and e.g. GenericDoneCallback is > that the new callback can fire after a certain time limit (in addition to a > manual stop), I thought it was an appropriate name. I am open to suggestions. The main difference is that the generic callback don't have any arguments, while the recording callback does, and it contains recording info. Drop "TimeLimited", it's enough to know that it can be time limited in the function call. We also don't make any difference between time limited and unlimited info and callback. https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:936: host->SetUserData(host, On 2016/02/16 20:09:12, terelius1 wrote: > On 2016/02/05 10:05:44, Henrik Grunell wrote: > > On 2016/02/05 09:27:37, terelius2 wrote: > > > On 2016/02/03 08:37:32, Henrik Grunell wrote: > > > > I think this will override the previous call above. (Same key |host|.) > > > > > > Just a follow-up after our discussion. Do you have any suggestion on how I > > > should handle this? What should I use as key, or should some other object > own > > > the handler? > > > > Yeah, the new class should not be a filter since it's not used for IPC. Thus > it > > should not be added as filter to the host, and ownership must be explicit > > somewhere. (I think the host has references to filters.) > > Removed the filter. Added static const strings to each of the logging handlers > for use as keys. > > According to the documentation for SetUserData: > // This object will TAKE OWNERSHIP of the given data pointer, and will > // delete the object if it is changed or the object is destroyed. > So apparently there is no need for an explicit ownership. Acknowledged. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:935: AudioDebugRecordingsHandler::kAudioDebugRecordingsHandlerKey, Looks like different indentation than above. Run 'git cl format' to format the entire CL. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:451: switches::kEnableRtcEventLoggingFromExtension)) { kEnableRtcEventLoggingFromExtension -> kEnableWebRtcEventLoggingFromExtension https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:501: host, RtcEventLogHandler::kRtcEventLogHandlerKey)); Same here. Rtc -> WebRtc https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:76: class WebrtcLoggingPrivateFunctionWithTimeLimitedRecordingCallback Drop "TimeLimited". https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_apitest.cc:468: // and solved at the same time? Please add test for content. It will help catch regressions. Any reason for not doing it now? https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... File chrome/browser/media/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.cc:56: // TODO: Can we check that we dont hold any references to any Javascript Did this todo come from WebRtcLoggingHandlerHost? https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... File chrome/browser/media/audio_debug_recordings_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:13: #include "content/public/browser/browser_message_filter.h" Remove. Seems like other includes could be removed too. Check all the new files. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:22: // - Starts and stops RTC event logs. On 2016/02/23 13:57:34, Guido Urdaneta wrote: > This class is only for AudioDebugRecordings. > Should the class be called WebRtcAudioDebugRecordingsHandler to emphasize that > this is related to WebRTC? It's not strictly WebRTC since it also does recording of audio data independent of WebRTC (mic input). I think the name should be kept as is. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:22: // - Starts and stops RTC event logs. Remove comment about RTC event log. :) Actually, rewrite the comment since it's only one bullet left. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:28: TimeLimitedRecordingErrorCallback; Drop "TimeLimited". https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:59: friend class content::BrowserThread; Not sure why these friends are needed. Can be removed? https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... File chrome/browser/media/rtc_event_log_handler.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.cc:64: DCHECK_CURRENTLY_ON(BrowserThread::UI); Use base::ThreadChecker instead for all UI thread checks. You can find plenty of usage examples. (You may have to detach in the ctor.) https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... File chrome/browser/media/rtc_event_log_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:21: // - Starts and stops AudioDebugRecordings, aka AEC dumps. Remove this line and rewrite without any bullet. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:22: // - Starts and stops RTC event logs. On 2016/02/23 13:57:34, Guido Urdaneta wrote: > And this one is only for RTC event logs. > WebRtcEventLogHandler as class name? Agree with that name suggestion. (Sorry if I previously suggested the one you used now...) https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:28: TimeLimitedRecordingErrorCallback; Drop "TimeLimited". https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:32: // Key used to attach the handler to the RenderProcessHost Nit: End with . https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:60: friend class content::BrowserThread; These three needed? https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.h (left): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:164: void StartAudioDebugRecordings( Thanks for moving this out!
https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:935: AudioDebugRecordingsHandler::kAudioDebugRecordingsHandlerKey, On 2016/02/23 15:29:10, Henrik Grunell wrote: > Looks like different indentation than above. Run 'git cl format' to format the > entire CL. Done, but I don't think 'git cl format' does what you want it to do. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:404: scoped_refptr<AudioDebugRecordingsHandler> audio_debug_recrdings_handler( On 2016/02/23 13:57:34, Guido Urdaneta wrote: > typo: recrdings -> recordings Done. Thanks! https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:451: switches::kEnableRtcEventLoggingFromExtension)) { On 2016/02/23 15:29:10, Henrik Grunell wrote: > kEnableRtcEventLoggingFromExtension -> kEnableWebRtcEventLoggingFromExtension Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:501: host, RtcEventLogHandler::kRtcEventLogHandlerKey)); On 2016/02/23 15:29:10, Henrik Grunell wrote: > Same here. Rtc -> WebRtc Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:76: class WebrtcLoggingPrivateFunctionWithTimeLimitedRecordingCallback On 2016/02/23 15:29:10, Henrik Grunell wrote: > Drop "TimeLimited". Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_apitest.cc:468: // and solved at the same time? On 2016/02/23 15:29:10, Henrik Grunell wrote: > Please add test for content. It will help catch regressions. Any reason for not > doing it now? Still working on this. Let's discuss offline. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... File chrome/browser/media/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.cc:56: // TODO: Can we check that we dont hold any references to any Javascript On 2016/02/23 15:29:10, Henrik Grunell wrote: > Did this todo come from WebRtcLoggingHandlerHost? No, this was based on my interpretation of a comment in https://codereview.chromium.org/1530863002/ The comment was mostly for my own benefit since I don't fully understand how callbacks to Javascript work. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... File chrome/browser/media/audio_debug_recordings_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:13: #include "content/public/browser/browser_message_filter.h" On 2016/02/23 15:29:10, Henrik Grunell wrote: > Remove. Seems like other includes could be removed too. Check all the new files. Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:22: // - Starts and stops RTC event logs. On 2016/02/23 15:29:10, Henrik Grunell wrote: > Remove comment about RTC event log. :) Actually, rewrite the comment since it's > only one bullet left. Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:28: TimeLimitedRecordingErrorCallback; On 2016/02/23 15:29:10, Henrik Grunell wrote: > Drop "TimeLimited". Done. Changed to RecordingErrorCallback and RecordingDoneCallback. (RecordingDoneCallback for consistency with the existing GenericDoneCallback and UploadDoneCallback.) https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:59: friend class content::BrowserThread; On 2016/02/23 15:29:10, Henrik Grunell wrote: > Not sure why these friends are needed. Can be removed? Based on a comment in base/memory/ref_counted.h, I believe friend class base::RefCountedThreadSafe<AudioDebugRecordingsHandler>; is recommended unless you provide a custom Traits class. I removed the others. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... File chrome/browser/media/rtc_event_log_handler.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.cc:64: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2016/02/23 15:29:10, Henrik Grunell wrote: > Use base::ThreadChecker instead for all UI thread checks. You can find plenty of > usage examples. (You may have to detach in the ctor.) Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... File chrome/browser/media/rtc_event_log_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:21: // - Starts and stops AudioDebugRecordings, aka AEC dumps. On 2016/02/23 15:29:10, Henrik Grunell wrote: > Remove this line and rewrite without any bullet. Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:22: // - Starts and stops RTC event logs. On 2016/02/23 13:57:34, Guido Urdaneta wrote: > And this one is only for RTC event logs. > WebRtcEventLogHandler as class name? Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:28: TimeLimitedRecordingErrorCallback; On 2016/02/23 15:29:10, Henrik Grunell wrote: > Drop "TimeLimited". Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:32: // Key used to attach the handler to the RenderProcessHost On 2016/02/23 15:29:10, Henrik Grunell wrote: > Nit: End with . Done. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:60: friend class content::BrowserThread; On 2016/02/23 15:29:10, Henrik Grunell wrote: > These three needed? Based on a comment in base/memory/ref_counted.h, I believe friend class base::RefCountedThreadSafe<WebRtcEventLogHandler>; is recommended unless you provide a custom Traits class. I removed the others. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.h (left): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:164: void StartAudioDebugRecordings( On 2016/02/23 15:29:11, Henrik Grunell wrote: > Thanks for moving this out! Acknowledged.
Finally I got around to review this again. Only minor comments now. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:935: AudioDebugRecordingsHandler::kAudioDebugRecordingsHandlerKey, On 2016/03/02 10:01:09, terelius2 wrote: > On 2016/02/23 15:29:10, Henrik Grunell wrote: > > Looks like different indentation than above. Run 'git cl format' to format the > > entire CL. > > Done, but I don't think 'git cl format' does what you want it to do. I think it usually works well. If it messes up the formatting somewhere, feel free to revert that change. You can keep this as is. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... File chrome/browser/media/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.cc:56: // TODO: Can we check that we dont hold any references to any Javascript On 2016/03/02 10:01:09, terelius2 wrote: > On 2016/02/23 15:29:10, Henrik Grunell wrote: > > Did this todo come from WebRtcLoggingHandlerHost? > > No, this was based on my interpretation of a comment in > https://codereview.chromium.org/1530863002/ > > The comment was mostly for my own benefit since I don't > fully understand how callbacks to Javascript work. OK. All todos should have a reference. Change to TODO(terelius): ... https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... File chrome/browser/media/audio_debug_recordings_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/au... chrome/browser/media/audio_debug_recordings_handler.h:59: friend class content::BrowserThread; On 2016/03/02 10:01:10, terelius2 wrote: > On 2016/02/23 15:29:10, Henrik Grunell wrote: > > Not sure why these friends are needed. Can be removed? > > Based on a comment in base/memory/ref_counted.h, I believe > friend class base::RefCountedThreadSafe<AudioDebugRecordingsHandler>; > is recommended unless you provide a custom Traits class. > I removed the others. Acknowledged. https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... File chrome/browser/media/rtc_event_log_handler.h (right): https://codereview.chromium.org/1650133002/diff/60001/chrome/browser/media/rt... chrome/browser/media/rtc_event_log_handler.h:60: friend class content::BrowserThread; On 2016/03/02 10:01:10, terelius2 wrote: > On 2016/02/23 15:29:10, Henrik Grunell wrote: > > These three needed? > > Based on a comment in base/memory/ref_counted.h, I believe > friend class base::RefCountedThreadSafe<WebRtcEventLogHandler>; > is recommended unless you provide a custom Traits class. > I removed the others. Acknowledged. https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... File chrome/browser/media/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.cc:48: // TODO: Can we check that we dont hold any references to any Javascript Nit: "dont" -> "don't" https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... File chrome/browser/media/audio_debug_recordings_handler.h (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.h:24: // AudioDebugRecordings, aka AEC dumps. Nit: "aka" -> "including". https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/r... File chrome/browser/media/rtc_event_log_handler.cc (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/r... chrome/browser/media/rtc_event_log_handler.cc:48: // TODO: Can we check that we dont hold any references to any Javascript TODO(terelius) https://codereview.chromium.org/1650133002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:152: // startWebRtcEventLogging() logs the most recent events that happened before Over 80 cols?
I've added a test which checks that the event log actually writes to the file during a call. Please take a look.
On 2016/03/18 16:45:58, terelius2 wrote: > I've added a test which checks that the event log actually writes to the file > during a call. Please take a look. fyi - since grunell was the last to reply, I'm assuming you mean only for him to take a look. If it's for someone else, please clarify whom and for what files.
Great with the new test! Comments on it. There's also a previous comments I had that aren't addressed yet. https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:125: int seconds = 0; const? https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:172: // Check that the Start and Stop functions return the same file name. Remove this comment - it's clear from the code what it does. https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:181: while (!base::PathExists(full_file_name)) { This is dangerous. If the file don't exist the test will spin here forever. Why do we have to sleep at all? https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:244: // The log stops on it's own. Check that the file exists and is non-empty. Nit: "The log has stopped automatically. (...)".
https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... File chrome/browser/media/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.cc:48: // TODO: Can we check that we dont hold any references to any Javascript On 2016/03/10 21:33:34, Henrik Grunell wrote: > Nit: "dont" -> "don't" Removed. https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... File chrome/browser/media/audio_debug_recordings_handler.h (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.h:24: // AudioDebugRecordings, aka AEC dumps. On 2016/03/10 21:33:34, Henrik Grunell wrote: > Nit: "aka" -> "including". Done. https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/r... File chrome/browser/media/rtc_event_log_handler.cc (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/browser/media/r... chrome/browser/media/rtc_event_log_handler.cc:48: // TODO: Can we check that we dont hold any references to any Javascript On 2016/03/10 21:33:34, Henrik Grunell wrote: > TODO(terelius) Removed. https://codereview.chromium.org/1650133002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1650133002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:152: // startWebRtcEventLogging() logs the most recent events that happened before On 2016/03/10 21:33:34, Henrik Grunell wrote: > Over 80 cols? Done. https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:125: int seconds = 0; On 2016/03/24 09:36:22, Henrik Grunell wrote: > const? Done. https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:172: // Check that the Start and Stop functions return the same file name. On 2016/03/24 09:36:22, Henrik Grunell wrote: > Remove this comment - it's clear from the code what it does. Done. https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:181: while (!base::PathExists(full_file_name)) { On 2016/03/24 09:36:22, Henrik Grunell wrote: > This is dangerous. If the file don't exist the test will spin here forever. > > Why do we have to sleep at all? As discussed offline, we rely on the test timeout. I added a log message to make it easier to see that the test is waiting. https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:244: // The log stops on it's own. Check that the file exists and is non-empty. On 2016/03/24 09:36:22, Henrik Grunell wrote: > Nit: "The log has stopped automatically. (...)". Done.
lgtm https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:181: while (!base::PathExists(full_file_name)) { On 2016/03/24 16:21:23, terelius2 wrote: > On 2016/03/24 09:36:22, Henrik Grunell wrote: > > This is dangerous. If the file don't exist the test will spin here forever. > > > > Why do we have to sleep at all? > > As discussed offline, we rely on the test timeout. I added a log message to make > it easier to see that the test is waiting. Acknowledged.
terelius@chromium.org changed reviewers: + finnur@chromium.org, mpearson@chromium.org
Guido, do you have any more comments? Tommi, could you review chrome/browser/media/ (and whatever else you feel like) Mark, could you review tools/metrics/ and extensions/browser/ Finnur, could you review chrome/common/extensions
lgtm. Nice work with the test!
Owners LGTM for chrome/common/extensions
histograms lgtm
lg mostly but would like to avoid using Sleep() in the test code. Can we do something about that? https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:42: const int kExpectedConsumerId = 1; nit: static const int kExpectedConsumerId = 1; (kFooName constant naming style is for compile-time constants, not runtime) https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:83: scoped_refptr<T> CreateFunction() { nit: CreateExtensionFunction https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:106: if (OnWinXp()) I don't think XP is supported any longer, so this shouldn't be necessary. https://chrome.googleblog.com/2015/11/updates-to-chrome-platform-support.html https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:179: GetExpectedEventLogFileName(file_name_stop, render_process_id); what if there were two tests running at the same time that call this function. Would they conflict? Is there a way to instead use dynamically chosen temporary files or folder? (there's code in base specifically to handle that) https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:181: LOG(INFO) << "Waiting for logfile to become available..."; I think 'INFO' is discouraged. Instead use VLOG(1) << ... https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:182: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1)); ehm... ok, I see now what this loop is about. Sleep is generally less popular than 'goto', so we should avoid this :) Instead, use an event notification mechanism (e.g. check out use of RunLoop in various tests), incorporating a timeout and an exact notification for when the event has occurred. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:184: EXPECT_TRUE(base::PathExists(full_file_name)); should this be ASSERT_TRUE()? As is, you'll continue to execute the below code and it looks to me like it would surely fail. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:190: base::DeleteFile(full_file_name, false); If you use some of the temporary file utilities in base/, this could be done automatically. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:253: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1)); Use RunLoop instead https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:46: api::webrtc_logging_private::StartWebRtcEventLogging; instead of using 'namespace' for this, can we use 'using'? That seems to be the appropriate thing here and the way this is being used is perhaps a misuse of the namespace keyword but maybe there's more to it. Can you try: using api::webrtc_logging_private::StartWebRtcEventLogging; https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:508: base::Bind(&WebrtcLoggingPrivateStopWebRtcEventLoggingFunction:: nit: looks like this Bind() and the one on the line above are indented in two different ways. Did |git cl format| do this? https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/media/a... File chrome/browser/media/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.cc:75: host, true /* manual stop */, nit: we usually don't use these kinds of comments (and very rarely /* */ style commenting). If you want to keep the comment, consider // Same for other instances of the same. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.cc:138: // Start(10); //Start dump 1. Post Stop() to run after 10 seconds. nit: space after// https://codereview.chromium.org/1650133002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:161: DOMString securityOrigin, indent looks off here and below
https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:42: const int kExpectedConsumerId = 1; On 2016/04/07 12:44:29, tommi-sloooow wrote: > nit: static const int kExpectedConsumerId = 1; > (kFooName constant naming style is for compile-time constants, not runtime) Done. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:83: scoped_refptr<T> CreateFunction() { On 2016/04/07 12:44:29, tommi-sloooow wrote: > nit: CreateExtensionFunction Done. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:106: if (OnWinXp()) On 2016/04/07 12:44:29, tommi-sloooow wrote: > I don't think XP is supported any longer, so this shouldn't be necessary. > https://chrome.googleblog.com/2015/11/updates-to-chrome-platform-support.html Done. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:179: GetExpectedEventLogFileName(file_name_stop, render_process_id); On 2016/04/07 12:44:29, tommi-chrömium wrote: > what if there were two tests running at the same time that call this function. > Would they conflict? Is there a way to instead use dynamically chosen temporary > files or folder? (there's code in base specifically to handle that) The path is based on Profile::GetPath(). This path contains a 6 character identifier that changes every time, at least when I run the test locally. I don't know how this path is generated (there are multiple overrides) but assuming that the 6 characters are chosen randomly from [A-Za-z0-9_], the probability that two simultaneous tests would try to write to the same directory is less than 10^(-10). In addition, the file name also uses the render process id and some other values, so I think it is very unlikely that the file names will conflict. There is currently no way to use choose a special file path from the testing code. The test uses the same code as a real javascript call. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:181: LOG(INFO) << "Waiting for logfile to become available..."; On 2016/04/07 12:44:29, tommi-chrömium wrote: > I think 'INFO' is discouraged. Instead use VLOG(1) << ... Done. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:182: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1)); On 2016/04/07 12:44:29, tommi-chrömium wrote: > ehm... ok, I see now what this loop is about. Sleep is generally less popular > than 'goto', so we should avoid this :) > > Instead, use an event notification mechanism (e.g. check out use of RunLoop in > various tests), incorporating a timeout and an exact notification for when the > event has occurred. I've updated the loop based on our offline discussion. During local tests it has never entered the loop body. Receiving an event would be preferable, but there is currently no way to send events from the WebRTC event log object. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:184: EXPECT_TRUE(base::PathExists(full_file_name)); On 2016/04/07 12:44:29, tommi-chrömium wrote: > should this be ASSERT_TRUE()? As is, you'll continue to execute the below code > and it looks to me like it would surely fail. Done. However, the loop above will not exit until the path exists, so this EXPECT/ASSERT should never fail. I added it mostly to document the intention of the test. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:190: base::DeleteFile(full_file_name, false); On 2016/04/07 12:44:29, tommi-chrömium wrote: > If you use some of the temporary file utilities in base/, this could be done > automatically. Acknowledged. The idea was to exercise the same code path as a real javascript call, so there is currently no way inject a temporary file for testing. Let's discuss. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:253: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1)); On 2016/04/07 12:44:29, tommi-chrömium wrote: > Use RunLoop instead There is currently no easy way to get a callback from the event log. Let's discuss this. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:46: api::webrtc_logging_private::StartWebRtcEventLogging; On 2016/04/07 12:44:29, tommi-chrömium wrote: > instead of using 'namespace' for this, can we use 'using'? That seems to be the > appropriate thing here and the way this is being used is perhaps a misuse of the > namespace keyword but maybe there's more to it. Can you try: > > using api::webrtc_logging_private::StartWebRtcEventLogging; I've tried it. The problem is that StartWebRtcEventLogging is a namespace and we want to access the symbols Params and Result so we can't do 'using a::b::StartWebRtcEventLogging. Moreover, StartWebRtcEventLogging and the other namespaces are generated from the .idl file, so our options to restructure the namespace nesting or symbol names are rather limited. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:508: base::Bind(&WebrtcLoggingPrivateStopWebRtcEventLoggingFunction:: On 2016/04/07 12:44:29, tommi-chrömium wrote: > nit: looks like this Bind() and the one on the line above are indented in two > different ways. Did |git cl format| do this? Yes. I have changed it multiple times, but |git cl format| changes back every time I run it. I've changed to something I think looks a bit better. If you agree about the formatting, I can try to remember not to |git cl format| this CL again. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/media/a... File chrome/browser/media/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.cc:75: host, true /* manual stop */, On 2016/04/07 12:44:29, tommi-chrömium wrote: > nit: we usually don't use these kinds of comments (and very rarely /* */ style > commenting). If you want to keep the comment, consider // > Same for other instances of the same. I added a variable is_manual_stop = true; to make it clear what "true" means in this context. Similarly in the rest of the file and in webrtc_event_log_handler.cc https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/media/a... chrome/browser/media/audio_debug_recordings_handler.cc:138: // Start(10); //Start dump 1. Post Stop() to run after 10 seconds. On 2016/04/07 12:44:29, tommi-chrömium wrote: > nit: space after// Done. https://codereview.chromium.org/1650133002/diff/160001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:161: DOMString securityOrigin, On 2016/04/07 12:44:29, tommi-chrömium wrote: > indent looks off here and below You're right. Done.
lgtm. thanks for updating the loop. Based on offline discussion, we don't expect to hit the Sleep() under normal circumstances but might on very slow machines and I think that's acceptable given the complexity involved with getting a notification instead. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:46: api::webrtc_logging_private::StartWebRtcEventLogging; On 2016/04/13 08:52:05, terelius-chromium wrote: > On 2016/04/07 12:44:29, tommi-chrömium wrote: > > instead of using 'namespace' for this, can we use 'using'? That seems to be > the > > appropriate thing here and the way this is being used is perhaps a misuse of > the > > namespace keyword but maybe there's more to it. Can you try: > > > > using api::webrtc_logging_private::StartWebRtcEventLogging; > > I've tried it. The problem is that StartWebRtcEventLogging is a namespace and we > want to access the symbols Params and Result so we can't do 'using > a::b::StartWebRtcEventLogging. Moreover, StartWebRtcEventLogging and the other > namespaces are generated from the .idl file, so our options to restructure the > namespace nesting or symbol names are rather limited. ah, thanks for that. strange that it's a namespace but oh well. https://codereview.chromium.org/1650133002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:508: base::Bind(&WebrtcLoggingPrivateStopWebRtcEventLoggingFunction:: On 2016/04/13 08:52:05, terelius-chromium wrote: > On 2016/04/07 12:44:29, tommi-chrömium wrote: > > nit: looks like this Bind() and the one on the line above are indented in two > > different ways. Did |git cl format| do this? > > Yes. I have changed it multiple times, but |git cl format| changes back every > time I run it. > > I've changed to something I think looks a bit better. If you agree about the > formatting, I can try to remember not to |git cl format| this CL again. looks good now
The CQ bit was checked by terelius@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, guidou@chromium.org, mpearson@chromium.org, finnur@chromium.org Link to the patchset: https://codereview.chromium.org/1650133002/#ps180001 (title: "Some comments from tommi")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1650133002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1650133002/180001
Message was sent while issue was closed.
Description was changed from ========== Added functions to start and stop RTC event logs from a private extension API. The API is also protected by a command line flag. Renamed AudioDebugRecordingsCallback to TimeLimitedRecordingCallback and similar for AudioDebugRecordingsErrorCallback and AudioDebugRecordingsInfo. BUG=582469 ========== to ========== Added functions to start and stop RTC event logs from a private extension API. The API is also protected by a command line flag. Renamed AudioDebugRecordingsCallback to TimeLimitedRecordingCallback and similar for AudioDebugRecordingsErrorCallback and AudioDebugRecordingsInfo. BUG=582469 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Added functions to start and stop RTC event logs from a private extension API. The API is also protected by a command line flag. Renamed AudioDebugRecordingsCallback to TimeLimitedRecordingCallback and similar for AudioDebugRecordingsErrorCallback and AudioDebugRecordingsInfo. BUG=582469 ========== to ========== Added functions to start and stop RTC event logs from a private extension API. The API is also protected by a command line flag. Renamed AudioDebugRecordingsCallback to TimeLimitedRecordingCallback and similar for AudioDebugRecordingsErrorCallback and AudioDebugRecordingsInfo. BUG=582469 Committed: https://crrev.com/fa159cc7961317c4a1d743d2386e19e656fef78c Cr-Commit-Position: refs/heads/master@{#386993} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/fa159cc7961317c4a1d743d2386e19e656fef78c Cr-Commit-Position: refs/heads/master@{#386993} |
