New issue
Advanced search Search tips

Issue 859134 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 19
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Proj-Servicification

Blocking:
issue 821009



Sign in to add a comment

Migrate privet_traffic_detector.cc to using NetworkConnectionTracker

Project Member Reported by xunji...@chromium.org, Jun 29 2018

Issue description

PrivetTrafficDetector currently uses net::NetworkChangeNotifier to receive network changes. 

With network service, that will need to be converted to using NetworkConnectionTracker's observer APIs.


https://cs.chromium.org/chromium/src/chrome/browser/printing/cloud_print/privet_traffic_detector.cc?rcl=7ec4a3efb227417051ef7c7f4a949ef5ce38e101&l=95
 
Owner: rmcelrath@chromium.org
Status: Started (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 12

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

commit 9b78521765f33993e81dfba771127fe9d5eb4add
Author: Robbie McElrath <rmcelrath@chromium.org>
Date: Thu Jul 12 20:20:54 2018

Migrate PrivetTrafficDetector to use NetworkConnectionTracker.

This migrates PrivetTrafficDetector from using
net::NetworkChangeNotifier to content::NetworkConnectionTracker, which
works with the network service enabled.

Bug:  859134 
Change-Id: I06f2ecbe142bbed9a8772f69ab109b0c0dc32581
Reviewed-on: https://chromium-review.googlesource.com/1123681
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574700}
[modify] https://crrev.com/9b78521765f33993e81dfba771127fe9d5eb4add/chrome/browser/printing/cloud_print/privet_traffic_detector.cc
[modify] https://crrev.com/9b78521765f33993e81dfba771127fe9d5eb4add/chrome/browser/printing/cloud_print/privet_traffic_detector.h

Status: Fixed (was: Started)
Thank you for the amazing work!
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 13

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

commit 2a578d4484584a4b6eff0e66ce1dbf449229e1e5
Author: Robbie McElrath <rmcelrath@chromium.org>
Date: Fri Jul 13 20:03:30 2018

Revert "Migrate PrivetTrafficDetector to use NetworkConnectionTracker."

This reverts commit 9b78521765f33993e81dfba771127fe9d5eb4add.

Reason for revert: The tests for this class weren't actually running. https://crrev.com/c/1136864 breaks because of this CL when re-enabling the tests.

Original change's description:
> Migrate PrivetTrafficDetector to use NetworkConnectionTracker.
> 
> This migrates PrivetTrafficDetector from using
> net::NetworkChangeNotifier to content::NetworkConnectionTracker, which
> works with the network service enabled.
> 
> Bug:  859134 
> Change-Id: I06f2ecbe142bbed9a8772f69ab109b0c0dc32581
> Reviewed-on: https://chromium-review.googlesource.com/1123681
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574700}

TBR=pastarmovj@chromium.org,thestig@chromium.org,rmcelrath@chromium.org

Change-Id: I15aeff8eed59935b9640d73e95a638c9ffb35f71
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  859134 
Reviewed-on: https://chromium-review.googlesource.com/1136398
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575037}
[modify] https://crrev.com/2a578d4484584a4b6eff0e66ce1dbf449229e1e5/chrome/browser/printing/cloud_print/privet_traffic_detector.cc
[modify] https://crrev.com/2a578d4484584a4b6eff0e66ce1dbf449229e1e5/chrome/browser/printing/cloud_print/privet_traffic_detector.h

Status: Started (was: Fixed)
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 19

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

commit 77922074848a3c728117a9eb65a7b09a1d0af334
Author: Robbie McElrath <rmcelrath@chromium.org>
Date: Thu Jul 19 00:37:37 2018

Reland "Migrate PrivetTrafficDetector to use NetworkConnectionTracker."

This is a reland of 9b78521765f33993e81dfba771127fe9d5eb4add with test
fixes. The failing unit tests weren't being run when I first landed
this, but were enabled afterwards and started breaking, so I rolled
back.

Specifically, I PostTask'd (un)registering with the
NetworkConnectionTracker to the right threads, and added a
PrivetTrafficDetector::Stop method to clean up the registration and
call it from PrivetNotificationService.

Original change's description:
> Migrate PrivetTrafficDetector to use NetworkConnectionTracker.
>
> This migrates PrivetTrafficDetector from using
> net::NetworkChangeNotifier to content::NetworkConnectionTracker, which
> works with the network service enabled.
>
> Bug:  859134 
> Change-Id: I06f2ecbe142bbed9a8772f69ab109b0c0dc32581
> Reviewed-on: https://chromium-review.googlesource.com/1123681
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574700}

Bug:  859134 
Change-Id: I19bc5bf0abdbd41cfbd3da1c0881f122adca6c2b
Reviewed-on: https://chromium-review.googlesource.com/1137411
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576278}
[modify] https://crrev.com/77922074848a3c728117a9eb65a7b09a1d0af334/chrome/browser/printing/cloud_print/privet_notifications.cc
[modify] https://crrev.com/77922074848a3c728117a9eb65a7b09a1d0af334/chrome/browser/printing/cloud_print/privet_traffic_detector.cc
[modify] https://crrev.com/77922074848a3c728117a9eb65a7b09a1d0af334/chrome/browser/printing/cloud_print/privet_traffic_detector.h

Status: Fixed (was: Started)

Sign in to add a comment