We should also add integration tests for this feature, as I don't believe we have any.  Otherwise, big changes like this could end up breaking it without anyone noticing.

blundell: are you the right owner for this?

I might well be. However, first we should add the integration tests before making any changes, and I'm likely not best-placed to write those tests. Matt, when you write "this feature" in c#1 above, can you be more specific as to the exact functionality that you think we need integration tests for here?

My feeling is that each thing hooked up to ChromeResourceDispatcherHost should be tested.  There are 3 signin lines there, I think?  Which would imply at least having tests that fail if any one of those three hooks were removed.  May be worth testing in more detail, but I think that's what we should have at a minimum.

Created  crbug.com/791538  to explicitly track the creation of integration tests. Matt, how have you folks been tackling the fanout of the Network Service work to other feature teams? My hunch is that the best path would be to reach out to the signin team TL/PM (msarda@/ewald@) to get this work on the radar of the signin team OKR-wise etc.

[+dougt, +jam]  I'm not sure - I was just filing bugs for some of the more major tasks I was aware of that haven't previously been on the network service team's radar.

Some notes about signin integration with the network service:

dice_browsertest and mirror tests from ChromeResourceDispatcherHostDelegateBrowserTest are failing because the signin calls in ChromeResourceDispatchHostDelegate are missing in the network service. The first step for fixing these tests would be to restore these missing calls.

In addition to the hooks in ChromeResourceDispatchHostDelegate, the signin code also relies on cookie-change notifications, in particular here:
https://cs.chromium.org/chromium/src/components/signin/core/browser/gaia_cookie_manager_service.cc?rcl=cdb7a0288b34c6116aa911a8ae02cbd85fa285b1&l=345
I don't know if this feature currently exists in the newtork service, but if it doesn't this is going to be a blocker too.

We can't restore those missing calls - RDH will not exist in the network service world, and we don't want to add two IPCs to every hop of a network request to support similar functionality.  Cookie notifications will continue to exist.  I assume this isn't something we can safely move into the renderer for subresource requests, so we'll need to look at making things work differently.  Adding headers based on URL wildcard matches sounds less than ideal, though I'm not sure there's another way to do it, short of just keeping users logged in to google at all times via independent network requests, which sounds like it would raise privacy and battery life issues.

Cookie notifications will be set by a new CookieManager interface, but converting code to us it should be fairly simple (Though will need to set up monitoring in the extensions code, not the network code, and may want to do it on the UI thread instead of the IO thread, and will need to start monitoring again on network service crash, but that's not hard to do).  The new interface is hooked up already on trunk, and should work even when the network service is disabled.

I've started a design doc for this project here:

https://docs.google.com/document/d/1-Byv8VviYS8iIrwBp_viSxnsJlxRF8DK_wDPXOn2EKo/edit

No real content yet though, just TODOs.

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

commit 7a815364eb7f8ff9c5ea9f4e26554316b753c31f
Author: Reilly Grant <reillyg@chromium.org>
Date: Thu Jun 21 18:47:21 2018

Add wrappers around signin access to net::URLRequest

This patch is preparation for allowing signin to work with the Network
Service as in that path there will be no direct access to the
net::URLRequest object. These wrapper classes expose the minimum
interface needed by the signin module to modify headers.

Bug:  789670 
Change-Id: I06bc2d99dba2de8359ab2f93d9a220f6c6e3b502
Reviewed-on: https://chromium-review.googlesource.com/1098377
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569339}
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/chrome/browser/signin/chrome_signin_helper.cc
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/chrome/browser/signin/chrome_signin_helper.h
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/chrome/browser/signin/chrome_signin_helper_unittest.cc
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/chrome/browser/signin/chromeos_mirror_account_consistency_browsertest.cc
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/components/signin/core/browser/signin_header_helper.cc
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/components/signin/core/browser/signin_header_helper.h
[modify] https://crrev.com/7a815364eb7f8ff9c5ea9f4e26554316b753c31f/components/signin/core/browser/signin_header_helper_unittest.cc

 Issue 848174  has been merged into this issue.

Issue 852191 has been merged into this issue.

 Issue 791538  has been merged into this issue.

I have an in-progress patch to add a URLLoaderThrottle that adds/removes Mirror and DICE headers: https://chromium-review.googlesource.com/c/chromium/src/+/1139089

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

commit 3787780199f8ef7bfa5e977aab8180766b10ddad
Author: Reilly Grant <reillyg@google.com>
Date: Sat Aug 18 00:58:35 2018

Implement signin header modifications with a URLLoaderThrottle

This change replaces the header modification logic implemented in
ChromeResourceDispatchHostDelegate with a URLLoaderThrottle that is
configured for all navigation requests.

To support modifying headers (in addition to removing them) before
following a redirect a |modified_request_headers| parameter has been
added to URLLoaderThrottle::WillRedirectRequest().

Bug:  789670 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ibd58b0367bbe2a519dfb1cf85a4e5e6ec7eeeffa
Reviewed-on: https://chromium-review.googlesource.com/1139089
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584270}
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/browser/BUILD.gn
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/browser/chrome_content_browser_client.cc
[add] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/browser/signin/chrome_signin_url_loader_throttle.cc
[add] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/browser/signin/chrome_signin_url_loader_throttle.h
[add] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/browser/signin/chrome_signin_url_loader_throttle_delegate_impl.cc
[add] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/browser/signin/chrome_signin_url_loader_throttle_delegate_impl.h
[add] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/browser/signin/chrome_signin_url_loader_throttle_unittest.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/common/google_url_loader_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/common/google_url_loader_throttle.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/common/prerender_url_loader_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/common/prerender_url_loader_throttle.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/chrome/test/BUILD.gn
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/components/safe_browsing/browser/base_parallel_resource_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/components/safe_browsing/browser/browser_url_loader_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/components/safe_browsing/browser/browser_url_loader_throttle.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/components/safe_browsing/renderer/renderer_url_loader_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/components/safe_browsing/renderer/renderer_url_loader_throttle.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/components/subresource_filter/content/common/ad_delay_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/components/subresource_filter/content/common/ad_delay_throttle.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/content/browser/web_package/signed_exchange_cert_fetcher_unittest.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/content/common/throttling_url_loader.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/content/common/throttling_url_loader.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/content/common/throttling_url_loader_unittest.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/content/public/common/url_loader_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/content/public/common/url_loader_throttle.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/extensions/renderer/extension_url_loader_throttle.cc
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/extensions/renderer/extension_url_loader_throttle.h
[modify] https://crrev.com/3787780199f8ef7bfa5e977aab8180766b10ddad/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

 Issue 876032  has been merged into this issue.

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

commit 24bf833af478c16567b4bc3396f777801cd91c95
Author: Reilly Grant <reillyg@chromium.org>
Date: Mon Aug 27 21:55:01 2018

Implement signin header modifications for sub-resource requests

This change is a follow-up to r584270 and adds support for modifying
headers sent in sub-resource requests made to the GAIA signon realm.
This is done by inserting a proxy between the render process and network
service when the frame's origin matches the GAIA signon realm.

Bug:  789670 
Change-Id: I762a54dd6c8fec57c97f522fc6ef7de2179f3722
Reviewed-on: https://chromium-review.googlesource.com/1187063
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586431}
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/base/containers/unique_ptr_adapters.h
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/BUILD.gn
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/chrome_content_browser_client.h
[add] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_proxying_url_loader_factory.cc
[add] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_proxying_url_loader_factory.h
[add] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_proxying_url_loader_factory_manager.cc
[add] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_proxying_url_loader_factory_manager.h
[add] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_proxying_url_loader_factory_unittest.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_url_loader_throttle.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_url_loader_throttle.h
[delete] https://crrev.com/3e4ed553c59041d3b0bfb61bf8a0152e91a046bc/chrome/browser/signin/chrome_signin_url_loader_throttle_delegate_impl.h
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/chrome_signin_url_loader_throttle_unittest.cc
[add] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/header_modification_delegate.h
[rename] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/header_modification_delegate_impl.cc
[add] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/browser/signin/header_modification_delegate_impl.h
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/chrome/test/BUILD.gn
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/content/public/browser/content_browser_client.h
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/extensions/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/24bf833af478c16567b4bc3396f777801cd91c95/extensions/shell/browser/shell_content_browser_client.h

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

commit 76bf7ae244896650827051344dcf3637439cfb76
Author: Reilly Grant <reillyg@chromium.org>
Date: Mon Aug 27 22:09:58 2018

Implement signin header modifications for sub-resource requests

This change is a follow-up to r584270 and adds support for modifying
headers sent in sub-resource requests made to the GAIA signon realm.
This is done by inserting a proxy between the render process and network
service when the frame's origin matches the GAIA signon realm.

Bug:  789670 
Change-Id: I762a54dd6c8fec57c97f522fc6ef7de2179f3722
Reviewed-on: https://chromium-review.googlesource.com/1187063
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#586431}(cherry picked from commit 24bf833af478c16567b4bc3396f777801cd91c95)
Reviewed-on: https://chromium-review.googlesource.com/1192204
Cr-Commit-Position: refs/branch-heads/3534@{#5}
Cr-Branched-From: 5c1309f3276665c501e62f79a3bfb9244b3dca26-refs/heads/master@{#586175}
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/base/containers/unique_ptr_adapters.h
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/BUILD.gn
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/chrome_content_browser_client.h
[add] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_proxying_url_loader_factory.cc
[add] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_proxying_url_loader_factory.h
[add] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_proxying_url_loader_factory_manager.cc
[add] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_proxying_url_loader_factory_manager.h
[add] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_proxying_url_loader_factory_unittest.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_url_loader_throttle.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_url_loader_throttle.h
[delete] https://crrev.com/329d286bf09a8a53d2966cc7a3538543c15d3729/chrome/browser/signin/chrome_signin_url_loader_throttle_delegate_impl.h
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/chrome_signin_url_loader_throttle_unittest.cc
[add] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/header_modification_delegate.h
[rename] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/header_modification_delegate_impl.cc
[add] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/browser/signin/header_modification_delegate_impl.h
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/chrome/test/BUILD.gn
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/content/public/browser/content_browser_client.h
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/extensions/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/76bf7ae244896650827051344dcf3637439cfb76/extensions/shell/browser/shell_content_browser_client.h

Update:

Rechecked the above issue on Windows(7,10)OS using latest Chrome Canary build #70.0.3535.0 and observed that the issue is fixed.Kindly refer the attached screencast for the same.

Thank you.