New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 754709 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Proj-Servicification

Blocked on:
issue 754695

Blocking:
issue 598073



Sign in to add a comment

Add NetworkChangeNotifier functionalities to network service

Project Member Reported by xunji...@chromium.org, Aug 11 2017

Issue description

Add an interface to allow consumers to subscribe/unsubscribe to network change events, and convert content/'s and chrome/'s net::NetworkChangeNotifier (NCN) consumers to using it. 

There are some cleanup needed in  Issue 754695  to make NCN observer interface cleaner.
 
Blockedon: 754695

Comment 2 by mmenke@chromium.org, Aug 11 2017

One cause of weirdness here is that the NCN is a singleton.  Ideally, that would mean you can just create an NCN subclass that acts as a proxy for the NCN in the network service, but NetworkService is also being run in process currently, to help with migration (And on Android, the network service will always run in process).  Anyhow, sounds like you're not planning on going down that path, anyways, just thought I'd point out that option ends up being weirder than one might think.

Comment 3 by jam@chromium.org, Aug 16 2017

Blocking: 598073
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 19 2017

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

commit f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053
Author: Helen Li <xunjieli@chromium.org>
Date: Thu Oct 19 17:26:07 2017

[network service] Add a NetworkChangeManager interface

This CL adds an interface to allow consumers to subscribe/unsubscribe
to network change events.

- mojom::NetworkChangeManager
  Listens to net::NetworkChangeNotifier and propagate notifications to
  mojom::NetworkChangeManagerClient.
  NetworkService will have a pointer to the impl of this interface.

- mojom::NetworkChangeManagerClient
  The implementation of this interface(content::NetworkConnectionTracker)
  receives notifications from mojom::NetworkChangeManager, and then sends
  those down to its observers.

Design doc:
https://docs.google.com/document/d/1kBp_vTIH-1Jx4M9DN7mnqRRyUmeHe3BZ1NqTbsJ4_S8/edit

Bug:  754709 
Change-Id: Ia98dc41da9bb618fd00adfd6412d70d839cd13bb
Reviewed-on: https://chromium-review.googlesource.com/644352
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510117}
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/BUILD.gn
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/DEPS
[add] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/network_change_manager_impl.cc
[add] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/network_change_manager_impl.h
[add] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/network_change_manager_impl_unittest.cc
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/network_service_impl.cc
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/network_service_impl.h
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/network/network_service_unittest.cc
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/public/common/BUILD.gn
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/public/common/DEPS
[add] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/public/common/network_change_manager.mojom
[add] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/public/common/network_connection_tracker.cc
[add] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/public/common/network_connection_tracker.h
[add] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/public/common/network_connection_tracker_unittest.cc
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/public/common/network_service.mojom
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/content/test/BUILD.gn
[modify] https://crrev.com/f18af04dd5cd0f6e4f4607b56eea0fec8c0ca053/net/BUILD.gn

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 20 2017

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

commit c014ab4412ec876e6d471cb8c447e5e8a2f40529
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri Oct 20 04:53:17 2017

Disable NetworkChangeNotifier creation in the network service temporarily to fix the failing tests.

See https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.fyi%2FMojo_Linux%2F6423%2F%2B%2Frecipes%2Fsteps%2Fnetwork_service_content_browsertests%2F0%2Flogs%2FResourceFetcherTests.ResourceFetcherDownload%2F0 as an example

The issue is that when the NetworkServiceImpl is run in the browser, i.e. in --single-process mode as ResourceFetcherTests.ResourceFetcherDownload exposes, there's already a NetworkChangeNotifier global constructed by the browser main loop.

Bug:  754709 
Change-Id: Id5a0fabcaa58f28f61fc49d186994422a23cc120
Reviewed-on: https://chromium-review.googlesource.com/729562
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510340}
[modify] https://crrev.com/c014ab4412ec876e6d471cb8c447e5e8a2f40529/content/network/network_service_impl.cc
[modify] https://crrev.com/c014ab4412ec876e6d471cb8c447e5e8a2f40529/content/network/network_service_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 23 2017

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

commit d19d0f0fed7546f9edddd3d338ff9d842d327e03
Author: Helen Li <xunjieli@chromium.org>
Date: Mon Oct 23 13:49:57 2017

Skip creating NetworkChangeNotifier if NetworkService is running in process

net::NetworkChangeNotifier's constructor checks to see that it is
the only instance in the process. If NetworkService is running in
process (which happens for some browser tests), an
NetworkChangeNotifier would have been created. If so, skip creating
a second net::NetworkChangeNotifier in NetworkService.

This CL disables the tests for OS_FUCHSIA which doesn't yet have a
platform NetworkChangeNotifier implementation.

Bug:  776599 ,  754709 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ia6fa294be7a6d1adb2119aa6d8743fa780216350
Reviewed-on: https://chromium-review.googlesource.com/731244
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510781}
[modify] https://crrev.com/d19d0f0fed7546f9edddd3d338ff9d842d327e03/content/network/network_service_impl.cc
[modify] https://crrev.com/d19d0f0fed7546f9edddd3d338ff9d842d327e03/content/network/network_service_unittest.cc
[modify] https://crrev.com/d19d0f0fed7546f9edddd3d338ff9d842d327e03/net/base/network_change_notifier.cc
[modify] https://crrev.com/d19d0f0fed7546f9edddd3d338ff9d842d327e03/net/base/network_change_notifier.h

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 6 2017

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

commit 72c87ae48a786e813c036eb64e3179c568a6db84
Author: Helen Li <xunjieli@chromium.org>
Date: Mon Nov 06 18:31:56 2017

Instantiate an NetworkConnectionTracker in BrowserProcessImpl

This CL
- instantiates an NetworkConnectionTracker in BrowserProcessImpl.
- adds a browser test to test NetworkConnectionTracker with and without
  NetworkService.

Bug:  754709 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I94f8140e37c38e05c4bd2c16825dc1974ede08b2
Reviewed-on: https://chromium-review.googlesource.com/734082
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514189}
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/browser/browser_process.h
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/browser/browser_process_impl.h
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/browser/io_thread.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/browser/io_thread.h
[add] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/browser/net/network_connection_tracker_browsertest.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/test/BUILD.gn
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/test/base/testing_browser_process.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/chrome/test/base/testing_browser_process.h
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/network/network_service_impl.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/network/network_service_impl.h
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/public/common/network_connection_tracker.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/public/common/network_connection_tracker_unittest.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/public/common/network_service_test.mojom
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/public/common/simple_url_loader_unittest.cc
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/public/network/network_service.h
[modify] https://crrev.com/72c87ae48a786e813c036eb64e3179c568a6db84/content/public/test/network_service_test_helper.cc

Comment 8 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 10 2017

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

commit 7b22daa692a05ef9466771f46d47b0bb9104f032
Author: Helen Li <xunjieli@chromium.org>
Date: Fri Nov 10 21:28:07 2017

Create NetworkChangeNotifier from NetworkChangeNotifierFactory on Android

On Android, we assume that embedders will initialize Android specific objects
using net::NetworkChangeNotifier::SetFactory() on UI thread.

This CL changes network_service_impl.cc skip creating NCN on Android.

Bug:  754709 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I065fe8c46e28f958a2a298e6a99d7e8d9cb0b39e
Reviewed-on: https://chromium-review.googlesource.com/755923
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515684}
[modify] https://crrev.com/7b22daa692a05ef9466771f46d47b0bb9104f032/content/network/network_service_impl.cc
[modify] https://crrev.com/7b22daa692a05ef9466771f46d47b0bb9104f032/content/network/network_service_impl.h
[modify] https://crrev.com/7b22daa692a05ef9466771f46d47b0bb9104f032/content/network/network_service_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 13 2017

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

commit 4e6d1c857c3a474e4c6533e07bfdff6e432ce13a
Author: Helen Li <xunjieli@chromium.org>
Date: Mon Nov 13 16:58:51 2017

Make ChromeSigninClient use content::NetworkConnectionTracker

This CL makes ChromeSigninClient use content::NetworkConnectionTracker
instead of net::NetworkChangeNotifier.

This CL additionally modifies NetworkConnectionTracker's
constructor so we can mock it in TestingBrowserProcess and tests.

Bug:  754709 
Change-Id: Ic05662830839403d5b327793f8faf7284a0da0bd
Reviewed-on: https://chromium-review.googlesource.com/757583
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515965}
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/chrome/browser/signin/chrome_signin_client.h
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/chrome/browser/signin/chrome_signin_client_unittest.cc
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/chrome/test/base/testing_browser_process.cc
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/chrome/test/base/testing_browser_process.h
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/content/public/common/network_connection_tracker.cc
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/content/public/common/network_connection_tracker.h
[modify] https://crrev.com/4e6d1c857c3a474e4c6533e07bfdff6e432ce13a/content/public/common/network_connection_tracker_unittest.cc

Cc: -rdsmith@chromium.org
Status: Fixed (was: Assigned)
Closing this bug now and will file bugs to convert consumers over to content/public/common/network_connection_tracker.h.

Sign in to add a comment