Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801663002:
Network traffic annotation added to speech_recognition_engine. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, battre, msramek Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to speech_recognition_engine.
Network traffic annotation is added to network requests of
content/browser/speech/speech_recognition_engine.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2801663002
Cr-Commit-Position: refs/heads/master@{#472054}
Committed: https://chromium.googlesource.com/chromium/src/+/38db9afb06dea37010459a2c0cfa99e7cdadf47c
Patch Set 1 #
Total comments: 20
Patch Set 2 : Annotations updated. #
Total comments: 7
Patch Set 3 : Comment addressed. #Patch Set 4 : Annotation updated. #
Total comments: 2
Patch Set 5 : Annotation Updated. #
Total comments: 6
Patch Set 6 : Comments addressed. #
Total comments: 2
Patch Set 7 : Comment addressed. #Messages
Total messages: 39 (11 generated)
rhalavati@chromium.org changed reviewers: + tommi@chromium.org
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified speech_recognition_engine and added 2 annotation templates to it. Please review them and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
On 2017/04/05 11:03:05, Ramin Halavati wrote: > We are annotating all network requests in Chromium with a new > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit > the requests and configure Chrome in a way that meets their security policies > and expectations. Furthermore, it allows us to generate better debugging data in > chrome://net-internals and measure bandwidth consumption on a per-request-type > basis. > > I've modified speech_recognition_engine and added 2 annotation templates to it. > Please review them and suggest the required answers for the empty parts (marked > with '...'). Please note that the claims should be thorough and covering all > cases. In case you believe that annotation should be passed to the modified > function instead of originating from it, please tell me to change the CL > accordingly. > > Please take a look at the protobuf scheme in: > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > for the definition of the annotations. > > You can find a sample annotation in: > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > Entriprise policy options are here: > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > And more description on enterprise policy settings is here: > http://dev.chromium.org/administrators/policy-list-3 > > Please tell me if you need any clarifications from my side. If you want to learn > more, see: > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. Hi, A gentle reminder on this.
lgtm
On 2017/04/18 13:24:13, tommi - chröme wrote: > lgtm Thanks, but please suggest values for all the missing items. Please refer to the first message.
rhalavati@chromium.org changed reviewers: + grunell@chromium.org
+Henrik.
https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:341: net::NetworkTrafficAnnotationTag downstream_traffic_annotation = Oh, I wrote the upstream text here. Please change the upstream annotation below to these texts. I'll provide downstream texts after that. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:342: net::DefineNetworkTrafficAnnotation("...", R"( "speech_recognition" https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:344: sender: "..." "Speech Recognition" https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:345: description: "..." "Chromium provides translation from speech audio recorded with a microphone to text, by using the Google speech recognition web service. Audio is sent to Google's servers and text is returned." https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:346: trigger: "..." "The user chooses to start the recognition by clicking the microphone icon in the Google search field." https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:346: trigger: "..." This needs to be expanded and clarified. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:347: data: "..." "Audio recorded with a microphone." https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:347: data: "..." How much data is recorded? https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:348: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:351: cookies_allowed: false I don't know, but guess false. Tommi? https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:352: setting: "..." "The user must allow the browser to access the microphone in a popup notification. This is set per site (hostname pattern). In the content settings, microphone access can be turned off for all sites and site specific settings can be changed." https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:359: policy_exception_justification: "..." No policy is implemented.
Thank you Henirk. I copied the provided text for both annotations to make next suggestions easier. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:341: net::NetworkTrafficAnnotationTag downstream_traffic_annotation = On 2017/05/09 08:27:11, Henrik Grunell wrote: > Oh, I wrote the upstream text here. Please change the upstream annotation below > to these texts. I'll provide downstream texts after that. Done, I will copy it here to make it easier for next suggestions. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:342: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/09 08:27:11, Henrik Grunell wrote: > "speech_recognition" Done. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:344: sender: "..." On 2017/05/09 08:27:11, Henrik Grunell wrote: > "Speech Recognition" Done. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:345: description: "..." On 2017/05/09 08:27:11, Henrik Grunell wrote: > "Chromium provides translation from speech audio recorded with a microphone to > text, by using the Google speech recognition web service. Audio is sent to > Google's servers and text is returned." Done. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:348: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/05/09 08:27:11, Henrik Grunell wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:351: cookies_allowed: false On 2017/05/09 08:27:11, Henrik Grunell wrote: > I don't know, but guess false. > > Tommi? They are disabled in line 365. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:352: setting: "..." On 2017/05/09 08:27:12, Henrik Grunell wrote: > "The user must allow the browser to access the microphone in a popup > notification. This is set per site (hostname pattern). In the content settings, > microphone access can be turned off for all sites and site specific settings can > be changed." Done. https://codereview.chromium.org/2801663002/diff/1/content/browser/speech/spee... content/browser/speech/speech_recognition_engine.cc:359: policy_exception_justification: "..." On 2017/05/09 08:27:11, Henrik Grunell wrote: > No policy is implemented. Done.
https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:352: data: "Audio recorded with the microphone." This only gets text data, afaik it only sends a request for the data providing a unique random id for this particular speech recognition request. I'm not sure what should be written here. https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:428: "service. Audio is sent to Google's servers and text is returned." Change to "Audio is sent to Google's servers (upstream) and text is returned (downstream)." Here and above in downstream annotation. I think it could make sense to have a general description, or should the it be strictly for this request?
Comments addressed, please review. https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:352: data: "Audio recorded with the microphone." On 2017/05/09 09:03:30, Henrik Grunell wrote: > This only gets text data, afaik it only sends a request for the data providing a > unique random id for this particular speech recognition request. I'm not sure > what should be written here. You mean this request just sends the id to initialize the speech recognition request, and the next one sends the actual data? Could you please briefly explain the steps? https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:428: "service. Audio is sent to Google's servers and text is returned." On 2017/05/09 09:03:30, Henrik Grunell wrote: > Change to "Audio is sent to Google's servers (upstream) and text is returned > (downstream)." > > Here and above in downstream annotation. > > I think it could make sense to have a general description, or should the it be > strictly for this request? We are working on a next version of annotation functions, which will allow defining a general annotation and adding details to it for different cases, but it is not finalized yet. We can merge these two after that has landed.
https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:352: data: "Audio recorded with the microphone." On 2017/05/09 09:24:18, Ramin Halavati wrote: > On 2017/05/09 09:03:30, Henrik Grunell wrote: > > This only gets text data, afaik it only sends a request for the data providing > a > > unique random id for this particular speech recognition request. I'm not sure > > what should be written here. > > You mean this request just sends the id to initialize the speech recognition > request, and the next one sends the actual data? > Could you please briefly explain the steps? A unique id is generated for a speech recognition request. This network request (downstream) sends an id for getting the text response. Then the next request (upstream) sends the audio data along with the id. When the server has finished processing the audio data and produced a text response it replies to first request. https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:428: "service. Audio is sent to Google's servers and text is returned." On 2017/05/09 09:24:18, Ramin Halavati wrote: > On 2017/05/09 09:03:30, Henrik Grunell wrote: > > Change to "Audio is sent to Google's servers (upstream) and text is returned > > (downstream)." > > > > Here and above in downstream annotation. > > > > I think it could make sense to have a general description, or should the it be > > strictly for this request? > > We are working on a next version of annotation functions, which will allow > defining a general annotation and adding details to it for different cases, but > it is not finalized yet. > We can merge these two after that has landed. Acknowledged.
Thank you very much Henrik, annotation updated, please review. https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/20001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:352: data: "Audio recorded with the microphone." On 2017/05/11 07:14:08, Henrik Grunell wrote: > On 2017/05/09 09:24:18, Ramin Halavati wrote: > > On 2017/05/09 09:03:30, Henrik Grunell wrote: > > > This only gets text data, afaik it only sends a request for the data > providing > > a > > > unique random id for this particular speech recognition request. I'm not > sure > > > what should be written here. > > > > You mean this request just sends the id to initialize the speech recognition > > request, and the next one sends the actual data? > > Could you please briefly explain the steps? > > A unique id is generated for a speech recognition request. This network request > (downstream) sends an id for getting the text response. Then the next request > (upstream) sends the audio data along with the id. When the server has finished > processing the audio data and produced a text response it replies to first > request. Done.
Tommi: ptal. Looking good now. Since I'm not that familiar with speech recognition, Tommi should review this now and make sure it's correct and includes everything. https://codereview.chromium.org/2801663002/diff/60001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/60001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:438: data: "Audio recorded with the microphone." Perhaps add that the id is sent here as well?
Thank you Henrik, comment addressed. Tommi, Please review. https://codereview.chromium.org/2801663002/diff/60001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/60001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:438: data: "Audio recorded with the microphone." On 2017/05/11 08:22:35, Henrik Grunell wrote: > Perhaps add that the id is sent here as well? Done.
lgtm
Thank you very much Tomas. Martin, Any comments?
lgtm
msramek@chromium.org changed reviewers: + msramek@chromium.org
https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:364: "popup notification. This is set per site (hostname pattern). In " s/popup notification/permission prompt/ The words "popup" and "notification" are both names of different content settings, so to avoid confusion. https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:365: "the content settings, microphone access can be turned off for all " nit: s/content settings/content settings menu/ to be clearer https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:367: policy_exception_justification: "Not implemented." The microphone content setting described in "setting" is controlled by the AudioCaptureAllowed and AudioCaptureAllowedURLs policies.
Thank you Martin, comments addressed. Tomas, Henrik, Do you agree with policy update? https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:364: "popup notification. This is set per site (hostname pattern). In " On 2017/05/15 13:04:28, msramek wrote: > s/popup notification/permission prompt/ > > The words "popup" and "notification" are both names of different content > settings, so to avoid confusion. Done. https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:365: "the content settings, microphone access can be turned off for all " On 2017/05/15 13:04:28, msramek wrote: > nit: s/content settings/content settings menu/ to be clearer Done. https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... content/browser/speech/speech_recognition_engine.cc:367: policy_exception_justification: "Not implemented." On 2017/05/15 13:04:28, msramek wrote: > The microphone content setting described in "setting" is controlled by the > AudioCaptureAllowed and AudioCaptureAllowedURLs policies. Done.
Yes On Mon, May 15, 2017 at 3:48 PM <rhalavati@chromium.org> wrote: > Thank you Martin, comments addressed. > > Tomas, Henrik, > Do you agree with policy update? > > > > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > File content/browser/speech/speech_recognition_engine.cc (right): > > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > content/browser/speech/speech_recognition_engine.cc:364: "popup > notification. This is set per site (hostname pattern). In " > On 2017/05/15 13:04:28, msramek wrote: > > s/popup notification/permission prompt/ > > > > The words "popup" and "notification" are both names of different > content > > settings, so to avoid confusion. > > Done. > > > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > content/browser/speech/speech_recognition_engine.cc:365: "the content > settings, microphone access can be turned off for all " > On 2017/05/15 13:04:28, msramek wrote: > > nit: s/content settings/content settings menu/ to be clearer > > Done. > > > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > content/browser/speech/speech_recognition_engine.cc:367: > policy_exception_justification: "Not implemented." > On 2017/05/15 13:04:28, msramek wrote: > > The microphone content setting described in "setting" is controlled by > the > > AudioCaptureAllowed and AudioCaptureAllowedURLs policies. > > Done. > > https://codereview.chromium.org/2801663002/ > -- 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 2017/05/15 13:48:43, Ramin Halavati wrote: > Thank you Martin, comments addressed. > > Tomas, Henrik, > Do you agree with policy update? Yes. Technically it's a policy for a feature the speech recognition depends on, but it's vital and a future dependency change should be very rare. I'm fine with that. > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > File content/browser/speech/speech_recognition_engine.cc (right): > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > content/browser/speech/speech_recognition_engine.cc:364: "popup notification. > This is set per site (hostname pattern). In " > On 2017/05/15 13:04:28, msramek wrote: > > s/popup notification/permission prompt/ > > > > The words "popup" and "notification" are both names of different content > > settings, so to avoid confusion. > > Done. > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > content/browser/speech/speech_recognition_engine.cc:365: "the content settings, > microphone access can be turned off for all " > On 2017/05/15 13:04:28, msramek wrote: > > nit: s/content settings/content settings menu/ to be clearer > > Done. > > https://codereview.chromium.org/2801663002/diff/80001/content/browser/speech/... > content/browser/speech/speech_recognition_engine.cc:367: > policy_exception_justification: "Not implemented." > On 2017/05/15 13:04:28, msramek wrote: > > The microphone content setting described in "setting" is controlled by the > > AudioCaptureAllowed and AudioCaptureAllowedURLs policies. > > Done.
Thanks for the update! LGTM % one last comment (since the second paragraph should match the first one). https://codereview.chromium.org/2801663002/diff/100001/content/browser/speech... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/100001/content/browser/speech... content/browser/speech/speech_recognition_engine.cc:458: "popup notification. This is set per site (hostname pattern). In " Please update the two changes (permission prompt, content settings menu) here as well.
Thank you, comment addressed, landing. https://codereview.chromium.org/2801663002/diff/100001/content/browser/speech... File content/browser/speech/speech_recognition_engine.cc (right): https://codereview.chromium.org/2801663002/diff/100001/content/browser/speech... content/browser/speech/speech_recognition_engine.cc:458: "popup notification. This is set per site (hostname pattern). In " On 2017/05/15 16:16:30, msramek wrote: > Please update the two changes (permission prompt, content settings menu) here as > well. Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, tommi@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2801663002/#ps120001 (title: "Comment addressed.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rhalavati@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rhalavati@chromium.org
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": 120001, "attempt_start_ts": 1494922971603980,
"parent_rev": "765e161bf83b6625bdd01796594b4fcc63c91762", "commit_rev":
"38db9afb06dea37010459a2c0cfa99e7cdadf47c"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to speech_recognition_engine. Network traffic annotation is added to network requests of content/browser/speech/speech_recognition_engine.cc BUG=656607 ========== to ========== Network traffic annotation added to speech_recognition_engine. Network traffic annotation is added to network requests of content/browser/speech/speech_recognition_engine.cc BUG=656607 Review-Url: https://codereview.chromium.org/2801663002 Cr-Commit-Position: refs/heads/master@{#472054} Committed: https://chromium.googlesource.com/chromium/src/+/38db9afb06dea37010459a2c0cfa... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/38db9afb06dea37010459a2c0cfa... |
