1550605 - network connection status api

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Shane Caraveo (:mixedpuppy)]

Consider exposing some network connection events.

https://searchfox.org/mozilla-central/source/netwerk/base/nsINetworkLinkService.idl

we could have an api that has:
browser.network.getStatus() (up/down/etc)
browser.network.getLinkType() (wifi/usb/etc)
browser.network.onConnectionChanged

The onConnectionChanged would send an object essentially matching the values in nsINetworkLinkService whenever either "network:link-status-changed" or "network:link-type-changed" notifications are sent.

P3 for more info from dragana

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1550605 add networkStatus api

Comment 2

[Shane Caraveo (:mixedpuppy)]

Due to the simplicity of this api and that soft freeze has not happened yet, I wrote up the patch quickly. But IIUC :dragana has some concerns that the notifications could happen too much.

Comment 3

[Andrew Swan [:aswan]]

This should probably be dup'ed to bug 1514589. I didn't want to do that with the open patch here.
For reference, this could use some explanation about the use case, I could swear we had some discussion about exposing this earlier and decided not to do it, but I can't seem to find it.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrew Swan [:aswan] from comment #3)

This should probably be dup'ed to bug 1514589.

Maybe, maybe not. That seems to be an external request for a public api, for now at least, I was intending this to be a privileged api to avoid public promises around it.

Comment 6

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b99d9fcd915da1eef22feb6ed8ec0161f64e956

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mixedpuppy, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Shane Caraveo (:mixedpuppy)]

This will wait for additional platform work targeting Fx70.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Since the network id landed, we'll try this for 69, better sooner than later for secure proxy.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: networkStatus.xpi

Simple extension dumps status to console when network status updates.

Comment 11

[Shane Caraveo (:mixedpuppy)]

While this will have a test, it uses a mock network service. The attached xpi will dump the status changes to the console. QE could use this to validate that they can see real network changes on the different platforms. e.g. turn wifi off then on. I see:

{"id":"bweB5kWjxX8iirGKroX5tWq38vg=","status":"down","type":"unknown"}
{"status":"down","type":"unknown"}
{"status":"up","type":"unknown"}

and on reload

{"id":"bweB5kWjxX8iirGKroX5tWq38vg=","status":"up","type":"unknown"}

There may be some timing issue about getting the network id via the onConnectionChanged handler.

Comment 12

[Shane Caraveo (:mixedpuppy)]

another QE note: this is a privileged api, the attached extension can only be tested by loading via about:debugging on nightly or dev-ed.

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a21663b3ba302341178aae3d37e6ac6319b383c7

Comment 14

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=754fb318a64c0938cb2499ba762cfb3525ab6c1e

Comment 15

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9536047abc90ecb0925c0d85a417713ed6006a64

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/599521424e2a
add networkStatus api r=rpl,mayhemer

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/599521424e2a

Comment 18

[Nhi Nguyen (:nhi)]

Can this be uplifted to 69, in case we need it DoH?

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Nhi Nguyen (:nhi) from comment #18)

Can this be uplifted to 69, in case we need it DoH?

It's a larger chunk of code than I'd like to uplift, but I also don't feel it's a terribly risky uplift either. I'll put in a request, you may want to touch base with relman or whomever you'll need to convince if necessary.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9063919 [details]
Bug 1550605 add networkStatus api

Beta/Release Uplift Approval Request

• User impact if declined: requested by :nhi for secure-proxy.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Attached extension will dump the network id to console, see comment 11
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This is a larger patch than I like uplifting, but there's still lots of time before next uplift, and I don't feel like anything in it is particularly scary. The dependency which contains the lower level platform support, bug 1561005, landed in time for 69.
• String changes made/needed: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

It would be good for QA to look at this first before we commit to uplifting. I'm not crazy about uplifting new APIs to Beta halfway through the cycle.

Comment 23

[Shane Caraveo (:mixedpuppy)]

of note: values in this api are simply reflected from what platform provides us. The test I outlined for QE is providing them with a network id, but no network type. For myself, I currently get neither id or type. The API itself is working fine.

Comment 24

[Victor Carciu]

Verified as fixed in FF70 with the results described by Shane in comment #23.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9063919 [details]
Bug 1550605 add networkStatus api

Self-contained WebExtension API with test coverage. Approved for 69.0b10.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5db1324ed737