New issue
Advanced search Search tips

Issue 840378 link

Starred by 6 users
Status: Fixed
Owner:
morlovich@chromium.org
Closed: Jul 18
Cc:
skare@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocked on:
issue 808498
Blocking:
issue 598073
issue 773295



Sign in to add a comment

Port omnibox to network service

Project Member Reported by xunji...@chromium.org, May 7 2018 
components/omnibox/browser/ is a dependency of //chrome.
We need to migrate it to network service.

Currently, there are ~4 places where it uses URLFetchers to issue network requests. 
https://cs.chromium.org/search/?q=TrafficAnnotation+file:%5Esrc/components/omnibox/browser/+package:%5Echromium$&type=cs

These four call places need to be converted to using network service's URLLoader/URLLoaderFactory.

Omnibox also depends on other //net classes e.g. HostResolver and NetworkChangeNotifier indirectly through chrome/browser/intranet_redirect_detector.h. intranet_redirect_detector.h can be converted afterwards.

Comment 1 by xunji...@chromium.org, May 7 2018

Components: UI>Browser>Omnibox Internals>Services>Network

Comment 2 by xunji...@chromium.org, May 7 2018 

jam@ noted that intranet_redirect_detector.h has already been converted to using SimpleURLLoader in https://chromium-review.googlesource.com/c/chromium/src/+/986675. 

components/omnibox/browser probably can use that as an example to convert their URLFetchers to SimpleURLLoader

Comment 3 by xunji...@chromium.org, May 7 2018 

There is another omnibox's net::URLFetcher usage at chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc.

Comment 4 by morlovich@chromium.org, May 7 2018 

I think I could take it, unless someone else wants to? It looks like something w/o a massive dependency chain for a change.

Though, one think I am not certain off: would the proper equivalent for profile_->GetRequestContext() for fetching purposes be content::BrowserContext::GetDefaultStoragePartition(context)->GetURLLoaderFactoryForBrowserProcess(profile_) ?

Comment 6 by xunji...@chromium.org, May 7 2018

Blockedon: 808498
Adding dependent  Issue 808498

Comment 7 by xunji...@chromium.org, May 7 2018

Owner: morlovich@chromium.org
Status: Assigned (was: Available)
Maks, I'm putting this in your queue. Thank you!

I was told that we take out the data accounting and leave a todo like it was done in https://chromium-review.googlesource.com/c/chromium/src/+/1044348/3/chrome/browser/supervised_user/child_accounts/family_info_fetcher.cc#211

Comment 8 by mmenke@chromium.org, May 7 2018 

Maks:  You're correct that content::BrowserContext::GetDefaultStoragePartition(context)->GetURLLoaderFactoryForBrowserProcess(profile_) replaces profile_->GetRequestContext().  I'm trying to make the ProfileIOData replacement less tightly bound to the profile, which I'm hoping will make startup order less of a problem in the future.

Comment 9 by xunji...@chromium.org, May 8 2018

Blocking: 773295

Comment 10 by morlovich@chromium.org, May 8 2018

Status: Started (was: Assigned)

Comment 11 by morlovich@chromium.org, May 11 2018

Cc: skare@chromium.org
FYI given the design doc you just mailed out.

Comment 12 by dxie@chromium.org, May 14 2018

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

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

Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows

Comment 14 by morlovich@chromium.org, May 21 2018

Labels: Proj-Servicification-network-url

Comment 15 by morlovich@chromium.org, May 21 2018 

 Issue 844921  has been merged into this issue.

Comment 16 by morlovich@chromium.org, May 21 2018 

 Issue 844946  has been merged into this issue.

Comment 17 by morlovich@chromium.org, May 21 2018 

 Issue 844945  has been merged into this issue.

Comment 18 by morlovich@chromium.org, May 21 2018 

 Issue 844944  has been merged into this issue.

Comment 19 by morlovich@chromium.org, May 21 2018

Labels: Proj-Servicification
Project Member

Comment 20 by bugdroid1@chromium.org, Jun 13 2018 

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

commit 1b20851a6266529c473b1669e30a2457fbd6e78c
Author: Maks Orlovich <morlovich@chromium.org>
Date: Wed Jun 13 21:08:17 2018

S13n: Port components/omnibox to SimpleURLLoader.

URLFetcher will stop working with advent of Network Service, and
SimpleURLLoader is the replacement API for most clients.

Bug:  840378 
Change-Id: Ie5686493b7f57387c5bbf0086b92f80bfb216c05
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/1050465
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566986}
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/chrome/browser/autocomplete/chrome_autocomplete_provider_client.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/chrome/browser/autocomplete/contextual_suggestions_service_factory.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/chrome/browser/autocomplete/search_provider_unittest.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/BUILD.gn
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/DEPS
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/autocomplete_provider_client.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/base_search_provider.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/base_search_provider.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/contextual_suggestions_service.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/contextual_suggestions_service.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/mock_autocomplete_provider_client.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/mock_autocomplete_provider_client.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/search_provider.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/search_provider.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/search_suggestion_parser.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/search_suggestion_parser.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/zero_suggest_provider.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/zero_suggest_provider.h
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/components/omnibox/browser/zero_suggest_provider_unittest.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.cc
[modify] https://crrev.com/1b20851a6266529c473b1669e30a2457fbd6e78c/ios/chrome/browser/autocomplete/autocomplete_provider_client_impl.h
Project Member

Comment 21 by bugdroid1@chromium.org, Jul 18 

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

commit d823592858bb25d894822d14c1db6a8e2ed06d2c
Author: Maks Orlovich <morlovich@chromium.org>
Date: Wed Jul 18 01:46:04 2018

S13n: Port ChromeOmniboxNavigationObserver to SimpleURLLoader

URLFetcher will stop working with advent of Network Service, and
SimpleURLLoader is the replacement API for most clients.


Bug:  840378 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ie318335f2d3a21cac1042862fd6a49e62cc931a4
Reviewed-on: https://chromium-review.googlesource.com/1124771
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575900}
[modify] https://crrev.com/d823592858bb25d894822d14c1db6a8e2ed06d2c/chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc
[modify] https://crrev.com/d823592858bb25d894822d14c1db6a8e2ed06d2c/chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.h
[modify] https://crrev.com/d823592858bb25d894822d14c1db6a8e2ed06d2c/chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer_unittest.cc
[modify] https://crrev.com/d823592858bb25d894822d14c1db6a8e2ed06d2c/services/network/test/test_url_loader_factory.cc
[modify] https://crrev.com/d823592858bb25d894822d14c1db6a8e2ed06d2c/services/network/test/test_url_loader_factory.h
[modify] https://crrev.com/d823592858bb25d894822d14c1db6a8e2ed06d2c/services/network/test/test_url_loader_factory_unittest.cc

Comment 22 by morlovich@chromium.org, Jul 18

Status: Fixed (was: Started)
►

Sign in to add a comment