Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1530863002:
Allow audio debug recordings through a private API extension. (Closed)
|
Created:
5 years ago by Guido Urdaneta Modified:
5 years ago CC:
arv+watch_chromium.org, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag.
TBR=grunell@chromium.org
BUG=568169
Committed: https://crrev.com/19d3df7ad0956e8ac2f366c67957603ca06ff1cf
Cr-Commit-Position: refs/heads/master@{#366258}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Callback updates tommi's comments #
Total comments: 25
Patch Set 3 : tommi and grunell's comments #
Total comments: 2
Patch Set 4 : tommi's comment #
Total comments: 11
Patch Set 5 : mek's comments #Patch Set 6 : rebase #Messages
Total messages: 38 (16 generated)
Description was changed from ========== Add diagnostic audio recordings through a private API extension. BUG=568169 ========== to ========== Allow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag. BUG=568169 ==========
guidou@chromium.org changed reviewers: + grunell@chromium.org, tommi@chromium.org
Hi, PTAL.
https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:52: bool is_dump_in_progress = false; document what thread these variables are used on. also, for non-const ones add g_ prefix. Actually, can those be member variables of the AecDump function class? https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:53: char kDumpFilePrefix[] = "aecdump."; const https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:64: current_aec_dump_id++; ++foo; https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:82: // Prevent a old posted StopAecDump() call to stop a newer dump. nit: Prevent an old posted StopAecDump() call from stopping a newer dump. https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:90: DCHECK_LE(dump_id, current_aec_dump_id); I don't think this DCHECK will ever hit due to the previous if() https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:420: bool WebrtcLoggingPrivateStartAecDumpFunction::RunAsync() { do we need some sort of cleanup code here? E.g. delete any old AEC recordings to free up space? Otherwise things are just going to build up. We could e.g. keep logs around for a week or so. https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:422: switches::kEnableAecDumps)) { can we call the constant kAllowAecDumpsFromExtension ? Technically AEC dumps will be enabled via webrtc-internals, so I'd like to make it clear for what functionality this switch is. https://codereview.chromium.org/1530863002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1530863002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:376: const char kEnableAecDumps[] = "enable-aec-dumps"; nit: allow-aec-dumps-from-extension https://codereview.chromium.org/1530863002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1530863002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/webrtc_logging_private.idl:126: // is zero, recording will continue until stopAecDump() is explicitly called. >80 https://codereview.chromium.org/1530863002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/webrtc_logging_private.idl:130: //static void startAecDump(optional long seconds, [Default=Undefined] optional GenericDoneCallback callback); remove?
Patchset #2 (id:20001) has been deleted
PTAL https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:52: bool is_dump_in_progress = false; On 2015/12/17 11:03:26, tommi wrote: > document what thread these variables are used on. > also, for non-const ones add g_ prefix. Actually, can those be member variables > of the AecDump function class? Done. https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:53: char kDumpFilePrefix[] = "aecdump."; On 2015/12/17 11:03:26, tommi wrote: > const Done. https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:64: current_aec_dump_id++; On 2015/12/17 11:03:26, tommi wrote: > ++foo; Done. https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:82: // Prevent a old posted StopAecDump() call to stop a newer dump. On 2015/12/17 11:03:26, tommi wrote: > nit: Prevent an old posted StopAecDump() call from stopping a newer dump. Done. https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:90: DCHECK_LE(dump_id, current_aec_dump_id); On 2015/12/17 11:03:26, tommi wrote: > I don't think this DCHECK will ever hit due to the previous if() Moved to the beginning of the function. https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:420: bool WebrtcLoggingPrivateStartAecDumpFunction::RunAsync() { On 2015/12/17 11:03:26, tommi wrote: > do we need some sort of cleanup code here? E.g. delete any old AEC recordings > to free up space? Otherwise things are just going to build up. We could e.g. > keep logs around for a week or so. The functions are changed now so that the callback is always invoked after Stop is invoked (either manually or automatically due to timeout). At this point the user can do the cleanup. Implementing a cleanup policy ourselves would require changes to the lower layers, no? https://codereview.chromium.org/1530863002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:422: switches::kEnableAecDumps)) { On 2015/12/17 11:03:26, tommi wrote: > can we call the constant kAllowAecDumpsFromExtension ? > Technically AEC dumps will be enabled via webrtc-internals, so I'd like to make > it clear for what functionality this switch is. Done. https://codereview.chromium.org/1530863002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1530863002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:376: const char kEnableAecDumps[] = "enable-aec-dumps"; On 2015/12/17 11:03:26, tommi wrote: > nit: allow-aec-dumps-from-extension Done. https://codereview.chromium.org/1530863002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1530863002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/webrtc_logging_private.idl:126: // is zero, recording will continue until stopAecDump() is explicitly called. On 2015/12/17 11:03:26, tommi wrote: > >80 Done. https://codereview.chromium.org/1530863002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/webrtc_logging_private.idl:130: //static void startAecDump(optional long seconds, [Default=Undefined] optional GenericDoneCallback callback); On 2015/12/17 11:03:26, tommi wrote: > remove? Done.
https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:52: uint64 current_aec_dump_id = 0; uint64_t https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:52: uint64 current_aec_dump_id = 0; Use g_ prefix for these two. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:75: GetAecDumpPrefixPath(profile, current_aec_dump_id); current_aec_dump_id++ and remove increment above. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:75: GetAecDumpPrefixPath(profile, current_aec_dump_id); I assume the reason for using a incrementing counter in the file path is so that files won't be overwritten if the user don't move the file before enabled again? Maybe clarify that in a comment. (This is in contrast with enabling with webrtc-internals when the path is selected by the user.) https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:102: callback.Run(prefix_path, false); Should we really run the callback if the dump was stopped manually before the timer hit? It seems more natural to not. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:466: BrowserThread::PostDelayedTask( Maybe use a Timer? It wraps a delayed task and can be stopped easily. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:8: #include <string> Needed? https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:82: void FireSuccessCallback(bool is_manual_stop, Nit: arguments in same order as in the idl. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/resource... File chrome/browser/resources/hangout_services/thunk.js (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:170: } else if (method == 'logging.startAecDump') { Also update the version number in the manifest file. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:176: chrome.webrtcLoggingPrivate.stopRtpDump(doSendResponse); Not RtpDump. :) https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... chrome/common/extensions/api/webrtc_logging_private.idl:32: // didStop=false and isManualStop=false means startAecDump() was called Could these combinations be an enum(-ish variable) instead? https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... chrome/common/extensions/api/webrtc_logging_private.idl:40: dictionary AecDumpInfo { Rename "AecDump" to "AudioDebugRecordings" everywhere. AEC dump is only one part that's being enabled. https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... chrome/common/extensions/api/webrtc_logging_private.idl:135: // Starts an AEC Dump for all renderer processes with open peer connections. The more general description is that there's one file per APM, which happens to be one per render process right now. This will likely change in the future. See the description in webrtc internals. (Maybe point to that here to avoid duplication.)
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
PTAL https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:52: uint64 current_aec_dump_id = 0; On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Use g_ prefix for these two. Not needed anymore https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:52: uint64 current_aec_dump_id = 0; On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > uint64_t Done, but moved to webrtc logging handler https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:75: GetAecDumpPrefixPath(profile, current_aec_dump_id); On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > current_aec_dump_id++ and remove increment above. Done (with preincrement) in new version. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:102: callback.Run(prefix_path, false); On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Should we really run the callback if the dump was stopped manually before the > timer hit? It seems more natural to not. Since it is the caller who will remove/process files, this callback can be interpreted as a "cancel" operation, so it's good to have it. It is not called anymore when no dump is in process. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:466: BrowserThread::PostDelayedTask( On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Maybe use a Timer? It wraps a delayed task and can be stopped easily. Using PostDelayedTask looked more straightforward to me in this case. Now moved to webrtc_logging_handler_host https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:8: #include <string> On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Needed? According to lint, we should have it. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:82: void FireSuccessCallback(bool is_manual_stop, On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Nit: arguments in same order as in the idl. Done. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/resource... File chrome/browser/resources/hangout_services/thunk.js (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:170: } else if (method == 'logging.startAecDump') { On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Also update the version number in the manifest file. Done. https://codereview.chromium.org/1530863002/diff/40001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:176: chrome.webrtcLoggingPrivate.stopRtpDump(doSendResponse); On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Not RtpDump. :) Done. https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... chrome/common/extensions/api/webrtc_logging_private.idl:32: // didStop=false and isManualStop=false means startAecDump() was called On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Could these combinations be an enum(-ish variable) instead? I tried it, but the problem is that the handler would need to know about this enum, which did not look right. However, I simplified the meanings of the boolean fields. https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... chrome/common/extensions/api/webrtc_logging_private.idl:40: dictionary AecDumpInfo { On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > Rename "AecDump" to "AudioDebugRecordings" everywhere. AEC dump is only one part > that's being enabled. Done. https://codereview.chromium.org/1530863002/diff/40001/chrome/common/extension... chrome/common/extensions/api/webrtc_logging_private.idl:135: // Starts an AEC Dump for all renderer processes with open peer connections. On 2015/12/18 12:34:21, Henrik Grunell OOO until Jan 7 wrote: > The more general description is that there's one file per APM, which happens to > be one per render process right now. This will likely change in the future. See > the description in webrtc internals. (Maybe point to that here to avoid > duplication.) Given that we are now recording for a single process, I decided to simplify the documentation instead.
guidou@chromium.org changed reviewers: + isherman@chromium.org, mek@chromium.org
mek@chromium.org: Please review changes in chrome/common isherman@chromium.org: Please review changes in extensions/browser/ and tools/metrics/histograms
lg but qq - how is the command line switch propagated to the extension process? https://codereview.chromium.org/1530863002/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1530863002/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc_logging_handler_host.cc:815: base::FilePath log_directory) { pass by const&?
Patchset #4 (id:70014) has been deleted
https://codereview.chromium.org/1530863002/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1530863002/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc_logging_handler_host.cc:815: base::FilePath log_directory) { On 2015/12/18 19:54:04, tommi ooo until 7th wrote: > pass by const&? Done.
https://codereview.chromium.org/1530863002/diff/100001/chrome/browser/media/w... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/1530863002/diff/100001/chrome/browser/media/w... chrome/browser/media/webrtc_logging_handler_host.cc:815: base::FilePath log_directory) { On 2015/12/18 19:54:04, tommi ooo until 7th wrote: > pass by const&? Done.
On 2015/12/18 19:54:04, tommi ooo until 7th wrote: > lg but qq - how is the command line switch propagated to the extension process? > I don't know these mechanisms in detail, but my understanding is that the private API functions exchange messages with the extension, but the private API runs on the browser and thus has access to the command line flag. The extension will be able to "see" the javascript function, but the functionality will not be available because of the flag checks in the implementation. Perhaps mek can clarify.
On 2015/12/18 20:40:01, Guido Urdaneta wrote: > On 2015/12/18 19:54:04, tommi ooo until 7th wrote: > > lg but qq - how is the command line switch propagated to the extension > process? > > > > I don't know these mechanisms in detail, but my understanding is that the > private API functions exchange messages with the extension, but the private API > runs on the browser and thus has access to the command line flag. > The extension will be able to "see" the javascript function, but the > functionality will not be available because of the flag checks in the > implementation. > Perhaps mek can clarify. Thanks, that works for me. lgtm.
histograms.xml lgtm
https://codereview.chromium.org/1530863002/diff/130001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1530863002/diff/130001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:389: FireErrorCallback("seconds must be greater than or equal to 0"); It seems really weird to "asynchronously" return (by returning true in the next line), but really call SendResponse before returning from RunAsync. But it seems like the rest of the code does the same, so I assume it works correctly. (If this would be using UIThreadExtensionFunction instead of the deprecated AsyncExtensionFunction it definitely would not be okay though, but I guess that doesn't matter) https://codereview.chromium.org/1530863002/diff/130001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/1530863002/diff/130001/chrome/common/chrome_s... chrome/common/chrome_switches.h:111: extern const char kEnableAudioDebugRecordingsFromExtension[]; for alphabetical sorting this should be one line down https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:31: // This contains information about the result of an audio debug recordings. "an recordings" is not a thing. Either make it singular or get rid of the "an". https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:32: dictionary AudioDebugRecordingsInfo { AudioDebugRecordings sounds really weird to me. Is there a reason it is plural Recordings? https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:136: // |callback| is not invoked. If |callback| is only sometimes invoked, how do you make sure extension internals don't indefinitely hold on to a reference to this callback?
https://codereview.chromium.org/1530863002/diff/130001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1530863002/diff/130001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:389: FireErrorCallback("seconds must be greater than or equal to 0"); On 2015/12/18 23:40:10, Marijn Kruisselbrink wrote: > It seems really weird to "asynchronously" return (by returning true in the next > line), but really call SendResponse before returning from RunAsync. But it seems > like the rest of the code does the same, so I assume it works correctly. > (If this would be using UIThreadExtensionFunction instead of the deprecated > AsyncExtensionFunction it definitely would not be okay though, but I guess that > doesn't matter) I'll leave it as it is for consistency with the rest of the code. However, how should it be done if I were using the more modern UIThreadExtensionFunction? https://codereview.chromium.org/1530863002/diff/130001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/1530863002/diff/130001/chrome/common/chrome_s... chrome/common/chrome_switches.h:111: extern const char kEnableAudioDebugRecordingsFromExtension[]; On 2015/12/18 23:40:10, Marijn Kruisselbrink wrote: > for alphabetical sorting this should be one line down Done. https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_logging_private.idl (right): https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:31: // This contains information about the result of an audio debug recordings. On 2015/12/18 23:40:10, Marijn Kruisselbrink wrote: > "an recordings" is not a thing. Either make it singular or get rid of the "an". Done. https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:32: dictionary AudioDebugRecordingsInfo { On 2015/12/18 23:40:10, Marijn Kruisselbrink wrote: > AudioDebugRecordings sounds really weird to me. Is there a reason it is plural > Recordings? The underlying mechanism this is based on uses plural (RenderProcessHost::EnableAudioDebugRecordings()). The reason it's plural is that it creates separate recording files for each active WebRTC peer connection, and, in addition to that, there are multiple recordings for each peer connection (e.g., audio before and after applying echo cancellation). https://codereview.chromium.org/1530863002/diff/130001/chrome/common/extensio... chrome/common/extensions/api/webrtc_logging_private.idl:136: // |callback| is not invoked. On 2015/12/18 23:40:10, Marijn Kruisselbrink wrote: > If |callback| is only sometimes invoked, how do you make sure extension > internals don't indefinitely hold on to a reference to this callback? Fixed the comment. I had fixed this in the code, but forgot to update the comment.
lgtm https://codereview.chromium.org/1530863002/diff/130001/chrome/browser/extensi... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/1530863002/diff/130001/chrome/browser/extensi... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:389: FireErrorCallback("seconds must be greater than or equal to 0"); On 2015/12/19 at 00:32:42, Guido Urdaneta wrote: > On 2015/12/18 23:40:10, Marijn Kruisselbrink wrote: > > It seems really weird to "asynchronously" return (by returning true in the next > > line), but really call SendResponse before returning from RunAsync. But it seems > > like the rest of the code does the same, so I assume it works correctly. > > (If this would be using UIThreadExtensionFunction instead of the deprecated > > AsyncExtensionFunction it definitely would not be okay though, but I guess that > > doesn't matter) > > I'll leave it as it is for consistency with the rest of the code. > However, how should it be done if I were using the more modern UIThreadExtensionFunction? In UIThreadExtensionFunction::Run you'd return RespondNow(Error(...)) to synchronously fail, or return RespondLater() and after returning from Run call Respond() with the actual response.
The CQ bit was checked by guidou@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/1530863002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530863002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by guidou@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/1530863002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530863002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Allow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag. BUG=568169 ========== to ========== Allow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag. TBR=grunell@chromium.org BUG=568169 ==========
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, tommi@chromium.org, mek@chromium.org Link to the patchset: https://codereview.chromium.org/1530863002/#ps170001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1530863002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1530863002/170001
Message was sent while issue was closed.
Description was changed from ========== Allow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag. TBR=grunell@chromium.org BUG=568169 ========== to ========== Allow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag. TBR=grunell@chromium.org BUG=568169 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Allow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag. TBR=grunell@chromium.org BUG=568169 ========== to ========== Allow diagnostic audio recordings (AEC dumps) through a private API extension protected behind a flag. TBR=grunell@chromium.org BUG=568169 Committed: https://crrev.com/19d3df7ad0956e8ac2f366c67957603ca06ff1cf Cr-Commit-Position: refs/heads/master@{#366258} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/19d3df7ad0956e8ac2f366c67957603ca06ff1cf Cr-Commit-Position: refs/heads/master@{#366258} |
