New issue
Advanced search Search tips

Issue 810555 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug
Proj-Servicification

Blocked on:
issue 826869

Blocking:
issue 598073



Sign in to add a comment

Convert speech recognition code to use SimpleURLLoader

Project Member Reported by mmenke@chromium.org, Feb 8 2018

Issue description

It's currently using the legacy URLFetcher API, and needs to be updated to use the SimpleURLLoader API to work with the network service.

I also plan to reword the chunked upload API to make the consumer do more of the work, and avoid unbounded buffers in the network process (Since speech recognition is the only consumer of the API, this doesn't require any extra code code, either).

The main issue, other than the chunked upload, is that the speech recognizer code lives on the IO thread, so we'll need to get it a URLLoaderFactory that can be used there, or a URLLoaderFactoryGetter of some sort.  Need to figure out what is / is not worth the effort of doing here here, as Mojo pipes aren't free.
 

Comment 1 by mmenke@chromium.org, Mar 30 2018

Blockedon: 826869
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5ce7dd19171acf855438692d735ff692c7ebc47

commit b5ce7dd19171acf855438692d735ff692c7ebc47
Author: Matt Menke <mmenke@chromium.org>
Date: Thu Apr 05 04:51:08 2018

Make SpeechRecognitionBrowserTest.OneShotRecognition use the test server

Using the EmbeddedTestServer will make the same test pass before/after
the underlying code is switched to use the network service. The new
test is also more integrationy, using real network requests, which
seems particularly useful here, as this is the only code that sends
chunked uploads in Chrome. The one downside is that it requires a
test-only method to set the server URL.

Bug:  810555 
Change-Id: I46b9321f83027cbc1c3a002ce1b35b693a6560aa
Reviewed-on: https://chromium-review.googlesource.com/987526
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548328}
[modify] https://crrev.com/b5ce7dd19171acf855438692d735ff692c7ebc47/content/browser/speech/speech_recognition_browsertest.cc
[modify] https://crrev.com/b5ce7dd19171acf855438692d735ff692c7ebc47/content/browser/speech/speech_recognition_engine.cc
[modify] https://crrev.com/b5ce7dd19171acf855438692d735ff692c7ebc47/content/browser/speech/speech_recognition_engine.h
[modify] https://crrev.com/b5ce7dd19171acf855438692d735ff692c7ebc47/content/test/BUILD.gn
[delete] https://crrev.com/0f0ca6ab9a3d88d72f8a0b13d5cf8f236f846cfa/content/test/mock_google_streaming_server.cc
[delete] https://crrev.com/0f0ca6ab9a3d88d72f8a0b13d5cf8f236f846cfa/content/test/mock_google_streaming_server.h
[modify] https://crrev.com/b5ce7dd19171acf855438692d735ff692c7ebc47/net/test/embedded_test_server/controllable_http_response.cc
[modify] https://crrev.com/b5ce7dd19171acf855438692d735ff692c7ebc47/net/test/embedded_test_server/controllable_http_response.h

Components: Internals>Services>Network
Came across this while code-searching for HttpUserAgentSettings usage. I guess this will also address the TODO below?

https://cs.chromium.org/chromium/src/content/browser/speech/speech_recognition_engine.cc?rcl=c02169b8c9cf360d462c615d480ec3a4d5fe4a44&l=697

    // TODO(pauljensen): SpeechRecognitionEngine should be constructed with
    // a reference to the HttpUserAgentSettings rather than accessing the
    // accept language through the URLRequestContext.
    if (request_context->http_user_agent_settings()) {
      std::string accepted_language_list =
          request_context->http_user_agent_settings()->GetAcceptLanguage();
No, it won't - I'm just converting over the use of URLFetcher.  Plan to punt on that line, for the moment.
Project Member

Comment 5 by bugdroid1@chromium.org, May 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1bb2294d3b65dd885183de52a2984f7b6181d16

commit d1bb2294d3b65dd885183de52a2984f7b6181d16
Author: Matt Menke <mmenke@chromium.org>
Date: Sat May 12 00:40:28 2018

ChunkedDataPipeUploadDataStream: Fix case where size is received early.

ChunkedDataPipeUploadDataStream was incorrectly failing requests when
the size was received before the stream was initialized.

Bug:  810555 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: If5e163644be5a4d27e8e7a3f4d2539e4b445ded1
Reviewed-on: https://chromium-review.googlesource.com/1055752
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558071}
[modify] https://crrev.com/d1bb2294d3b65dd885183de52a2984f7b6181d16/services/network/chunked_data_pipe_upload_data_stream.cc
[modify] https://crrev.com/d1bb2294d3b65dd885183de52a2984f7b6181d16/services/network/chunked_data_pipe_upload_data_stream.h
[modify] https://crrev.com/d1bb2294d3b65dd885183de52a2984f7b6181d16/services/network/chunked_data_pipe_upload_data_stream_unittest.cc

Comment 6 by mmenke@chromium.org, May 17 2018

Labels: -Pri-3 Proj-Servicification-Canary Pri-1

Comment 7 by dxie@chromium.org, May 18 2018

Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Cc: mmenke@chromium.org morlovich@chromium.org
 Issue 844922  has been merged into this issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f

commit 7b2266ee0c88b02dcef6c9dca650524fd1b6f20f
Author: Matt Menke <mmenke@chromium.org>
Date: Thu Jun 07 19:32:09 2018

Convert Speech Recognition code to use SimplerURLLoader.

The SimplerURLLoader API is compatible with the out-of-process network
service.

This is the only code that uses chunked uploads, and the new API for
that requires the consumer handle all the complexities of that, which
makes this CL much more complicated than most URLFetcher to
SimplerURLLoader conversions.

BUG:   810555 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.linux:linux_vr
Change-Id: I4cc1f5f8710d08830062e95296ae5d814a6097e8
Reviewed-on: https://chromium-review.googlesource.com/1050825
Reviewed-by: Biao She <bshe@chromium.org>
Reviewed-by: Tommi <tommi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Doug Turner <dougt@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565378}
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/android/vr/vr_shell.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/chromeos/accessibility/dictation_chromeos.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/speech/speech_recognizer.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/speech/speech_recognizer.h
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/speech/speech_recognizer_browsertest.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/vr/BUILD.gn
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/vr/speech_recognizer.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/vr/speech_recognizer.h
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/chrome/browser/vr/speech_recognizer_unittest.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/chunked_byte_buffer.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/chunked_byte_buffer.h
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/speech_recognition_dispatcher_host.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/speech_recognition_dispatcher_host.h
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/speech_recognition_engine.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/speech_recognition_engine.h
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/speech_recognition_engine_unittest.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/speech_recognition_manager_impl.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/browser/speech/speech_recognizer_impl_unittest.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/content/public/browser/speech_recognition_session_config.h
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/services/network/test/test_url_loader_factory.cc
[modify] https://crrev.com/7b2266ee0c88b02dcef6c9dca650524fd1b6f20f/services/network/test/test_url_loader_factory.h

Status: Fixed (was: Started)

Sign in to add a comment