Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816523002:
Update text about diagnostic audio recordings on chrome://webrtc-internals. (Closed)
|
Created:
3 years, 8 months ago by Henrik Grunell Modified:
3 years, 6 months ago Reviewers:
tommi (sloooow) - chröme CC:
chromium-reviews, jam, darin-cc_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate text about diagnostic audio recordings on chrome://webrtc-internals.
* Include info about newly added output recording.
* Some clarifications in rest of text.
BUG=531883
Review-Url: https://codereview.chromium.org/2816523002
Cr-Commit-Position: refs/heads/master@{#475489}
Committed: https://chromium.googlesource.com/chromium/src/+/ee98b8df54c1574586fdabce27bc9ab72aea2b77
Patch Set 1 #
Total comments: 30
Patch Set 2 : Code review. #
Total comments: 4
Patch Set 3 : Code review. #Messages
Total messages: 14 (6 generated)
Description was changed from ========== Update text about diagnostic audio recordings on chrome://webrtc-internals. BUG=531883 ========== to ========== Update text about diagnostic audio recordings on chrome://webrtc-internals. * Include info about newly added output recording. * Some clarifications in rest of text. BUG=531883 ==========
grunell@chromium.org changed reviewers: + tommi@chromium.org
Tommi ping for review.
https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:38: ' contains the audio played out to the speaker and recorded from' + nit: could replace the last 'and' with a comma. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:40: ' will enable the recording for ongoing WebRTC calls, all ongoing' + "recording of ongoing" https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:41: ' playout streams (also non-WebRTC), all ongoing recording streams' + "(including non-WebRTC)" It's not clear to me what 'recording streams' means here, since that seems to indicate that we're recording a recording? Does this relate to a particular API? https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:42: ' (also non-WebRTC) and for future WebRTC calls and streams. When the' + nit: you could replace "calls and" with "audio" -> "for future WebRTC audio streams" https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:43: ' box is unchecked or this page is closed, all ongoing recordings' + 'this page is closed' Does that stop an ongoing recording? I wasn't aware of that, so just double checking if that's true. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:44: ' will be stopped and this recording functionality will be disabled' + nit: "the recording functionality disabled" https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:45: ' for future WebRTC calls and streams. Recordings in multiple tabs' + I'd skip "fore future WebRTC calls and streams" since it actually makes things a little less clear in my opinion. The first part of the sentence talks about 'ongoing' whereas the last part talks about 'future'. I think that the middle part where it says 'recording functionality will be disabled', already covers the 'future' part. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:45: ' for future WebRTC calls and streams. Recordings in multiple tabs' + "Recordings in multiple tabs" doesn't sound right. Suggest "Recording audio from multiple tabs is supported" instead. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:46: ' are supported as well as multiple recordings in the same tab. When' + "from the same tab" https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:47: ' enabling, you select a base filename to which suffixes will be' + remove "you " Also suggest: "to which the following suffixes will be added:" https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:55: '<p class=audio-diagnostic-dumps-info>If recordings are disabled and' + 'recordings are disabled' also doesn't sound right. The recordings themselves cannot be disabled, the feature can though. Hmm.. it's subtle. I think that it might be a good idea to have a native english speaker read this over and rephrase. As we're making small changes here and there, I think the overall flow of the text, gets altered and possibly less clear. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:56: ' then enabled using the same base filename, the microphone recording' + re-enabled https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:57: ' files ("source_input") will be overwritten, and the AEC' + instead of "the microphone recording files", I'd just say "the audio files". I'd also put the parenthesis before 'audio files'. I'd also suggest putting a period after 'overwritten' and start the next sentence with "The AEC dump file..." https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:58: ' dump file will be appended to and may become invalid. It is' + this isn't really clear. "and the AEC dump file will be appended to and may become invalid." I think the subject may have gotten lost. (appended to what? the previous files? are the new files not fresh in the sense that appending is incorrect?) https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:59: ' recommended to choose a new base filename each time or move the' + Perhaps it might be best to simply say something like. "It is recommended to choose a new base filename each time the feature is enabled [or turned on] to avoid ending up with partially overwritten or unusable audio files." I feel the whole thing is otherwise hard to parse.
Ptal. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:38: ' contains the audio played out to the speaker and recorded from' + On 2017/05/03 09:12:12, tommi - chröme wrote: > nit: could replace the last 'and' with a comma. Hmm, that seems awkward too. Changed the "saved to" statement to its own sentence instead, have a look. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:40: ' will enable the recording for ongoing WebRTC calls, all ongoing' + On 2017/05/03 09:12:12, tommi - chröme wrote: > "recording of ongoing" Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:41: ' playout streams (also non-WebRTC), all ongoing recording streams' + On 2017/05/03 09:12:11, tommi - chröme wrote: > "(including non-WebRTC)" Done. > > It's not clear to me what 'recording streams' means here, since that seems to > indicate that we're recording a recording? Does this relate to a particular > API? Good point. "record[ing]" refers to mic input here. Changed to "output" and "input". Changed to only use "record" and "recording" for the recording to file throughout the text. Using "input" or "capture" for notifying mic side. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:42: ' (also non-WebRTC) and for future WebRTC calls and streams. When the' + On 2017/05/03 09:12:12, tommi - chröme wrote: > nit: you could replace "calls and" with "audio" -> "for future WebRTC audio > streams" WebRTC call would refer to the AEC dump, which is WebRTC specific. This is getting confusing. It's better to just write input and output streams; the AEC dump data is part of the streams pipeline. Changed, wdyt? https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:43: ' box is unchecked or this page is closed, all ongoing recordings' + On 2017/05/03 09:12:11, tommi - chröme wrote: > 'this page is closed' Does that stop an ongoing recording? I wasn't aware of > that, so just double checking if that's true. Yes, it's (still) true. I verified just now. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:44: ' will be stopped and this recording functionality will be disabled' + On 2017/05/03 09:12:12, tommi - chröme wrote: > nit: "the recording functionality disabled" Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:45: ' for future WebRTC calls and streams. Recordings in multiple tabs' + On 2017/05/03 09:12:11, tommi - chröme wrote: > I'd skip "fore future WebRTC calls and streams" since it actually makes things a > little less clear in my opinion. The first part of the sentence talks about > 'ongoing' whereas the last part talks about 'future'. I think that the middle > part where it says 'recording functionality will be disabled', already covers > the 'future' part. Sounds good. Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:45: ' for future WebRTC calls and streams. Recordings in multiple tabs' + On 2017/05/03 09:12:12, tommi - chröme wrote: > "Recordings in multiple tabs" doesn't sound right. Suggest "Recording audio > from multiple tabs is supported" instead. Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:46: ' are supported as well as multiple recordings in the same tab. When' + On 2017/05/03 09:12:11, tommi - chröme wrote: > "from the same tab" Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:47: ' enabling, you select a base filename to which suffixes will be' + On 2017/05/03 09:12:11, tommi - chröme wrote: > remove "you " > Also suggest: "to which the following suffixes will be added:" Good suggestions. Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:55: '<p class=audio-diagnostic-dumps-info>If recordings are disabled and' + On 2017/05/03 09:12:11, tommi - chröme wrote: > 'recordings are disabled' also doesn't sound right. The recordings themselves > cannot be disabled, the feature can though. Hmm.. it's subtle. I think that it > might be a good idea to have a native english speaker read this over and > rephrase. As we're making small changes here and there, I think the overall flow > of the text, gets altered and possibly less clear. Good point. Also agree it would be good having a native speaker go over this. Now I updated based on what you pointed out. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:56: ' then enabled using the same base filename, the microphone recording' + On 2017/05/03 09:12:12, tommi - chröme wrote: > re-enabled Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:57: ' files ("source_input") will be overwritten, and the AEC' + On 2017/05/03 09:12:12, tommi - chröme wrote: > instead of "the microphone recording files", I'd just say "the audio files". > I'd also put the parenthesis before 'audio files'. > I'd also suggest putting a period after 'overwritten' and start the next > sentence with "The AEC dump file..." Done. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:58: ' dump file will be appended to and may become invalid. It is' + On 2017/05/03 09:12:12, tommi - chröme wrote: > this isn't really clear. "and the AEC dump file will be appended to and may > become invalid." > I think the subject may have gotten lost. (appended to what? the previous files? > are the new files not fresh in the sense that appending is incorrect?) Appending to the previous file. Changed wording. https://codereview.chromium.org/2816523002/diff/1/content/browser/resources/m... content/browser/resources/media/dump_creator.js:59: ' recommended to choose a new base filename each time or move the' + On 2017/05/03 09:12:11, tommi - chröme wrote: > Perhaps it might be best to simply say something like. "It is recommended to > choose a new base filename each time the feature is enabled [or turned on] to > avoid ending up with partially overwritten or unusable audio files." I feel the > whole thing is otherwise hard to parse. Yes, I think that's a good idea. Done.
lgtm https://codereview.chromium.org/2816523002/diff/20001/content/browser/resourc... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/2816523002/diff/20001/content/browser/resourc... content/browser/resources/media/dump_creator.js:39: ' from the microphone (input). The data is saved to the local disk.' + nit: "The data is saved locally." https://codereview.chromium.org/2816523002/diff/20001/content/browser/resourc... content/browser/resources/media/dump_creator.js:45: ' supported as well as multiple recordings from the same tab. When' + nit: Would be nice to add a line break or end the paragraph (</p>) and start a new one, before "When"
https://codereview.chromium.org/2816523002/diff/20001/content/browser/resourc... File content/browser/resources/media/dump_creator.js (right): https://codereview.chromium.org/2816523002/diff/20001/content/browser/resourc... content/browser/resources/media/dump_creator.js:39: ' from the microphone (input). The data is saved to the local disk.' + On 2017/05/17 07:34:20, tommi (sloooow) - chröme wrote: > nit: "The data is saved locally." Done. https://codereview.chromium.org/2816523002/diff/20001/content/browser/resourc... content/browser/resources/media/dump_creator.js:45: ' supported as well as multiple recordings from the same tab. When' + On 2017/05/17 07:34:20, tommi (sloooow) - chröme wrote: > nit: Would be nice to add a line break or end the paragraph (</p>) and start a > new one, before "When" SG, done.
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2816523002/#ps40001 (title: "Code review.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496140802064600,
"parent_rev": "36d3ee32727627fb3bde2990eb43de8c85879183", "commit_rev":
"ee98b8df54c1574586fdabce27bc9ab72aea2b77"}
Message was sent while issue was closed.
Description was changed from ========== Update text about diagnostic audio recordings on chrome://webrtc-internals. * Include info about newly added output recording. * Some clarifications in rest of text. BUG=531883 ========== to ========== Update text about diagnostic audio recordings on chrome://webrtc-internals. * Include info about newly added output recording. * Some clarifications in rest of text. BUG=531883 Review-Url: https://codereview.chromium.org/2816523002 Cr-Commit-Position: refs/heads/master@{#475489} Committed: https://chromium.googlesource.com/chromium/src/+/ee98b8df54c1574586fdabce27bc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ee98b8df54c1574586fdabce27bc... |
