Daniel Stenberg [:bagder] Assignee
	Description • 5 years ago

When detecting a new network topology, new routing table or new IP address, send a NS_NETWORK_LINK_DATA_CHANGED event on Mac OS X.

(like bug 939318 for Windows and bug 1008091 for FxOS/Linux)

	Daniel Stenberg [:bagder] Assignee
	Comment 1 • 5 years ago

Version 1. This is functionality for the Mac OS X specific backend to detect network changes and send NS_NETWORK_LINK_DATA_CHANGED events. The main frame work for handling those events were already made in bug 939318.

This code uses Mac-specific APIs I'm not very experienced in. Josh: someone suggested you might be able to review this patch. Let me know how you feel about that.

	Josh Aas
	Comment 2 • 5 years ago

Comment on attachment 8509336 [details] [diff] [review]
0001-bug-1079385-Send-NS_NETWORK_LINK_DATA_CHANGED-event-.patch

Review of attachment 8509336 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/system/mac/nsNetworkLinkService.mm
@@ +162,5 @@
> +        ::SCDynamicStoreCreate(NULL,
> +                               CFSTR("AddIPAddressListChangeCallbackSCF"),
> +                               IPConfigChanged, &storeContext);
> +
> +    CFStringRef patterns[2] = {NULL, NULL};

Why create a basic array here and then create a CFArray with it later? Why not just create a CFMutableArray here and use that? Seems like it'd simplify things.

@@ +228,5 @@
>          return NS_ERROR_NOT_AVAILABLE;
>      }
>      ::CFRetain(mCFRunLoop);
>  
> +    CFRunLoopAddSource(mCFRunLoop, mRunLoopSource, kCFRunLoopDefaultMode);

Convention in our code is to prefix calls into Apple C code with '::'. See other code in this file.

	Daniel Stenberg [:bagder] Assignee
	Comment 3 • 5 years ago

(In reply to Josh Aas (Mozilla Corporation) from comment #2)

> Why create a basic array here and then create a CFArray with it later? Why
> not just create a CFMutableArray here and use that? Seems like it'd simplify
> things.

The answer is simple: I'm a newbie in Mac API land and it is a very big maze to get lost in. I found that style of array use in example code on apple.com. I didn't' even know CFMutableArray existed. Let me see if I can convert my code to using that instead...

	Daniel Stenberg [:bagder] Assignee
	Comment 4 • 5 years ago

(In reply to Josh Aas (Mozilla Corporation) from comment #2)

> Why create a basic array here and then create a CFArray with it later? Why
> not just create a CFMutableArray here and use that? Seems like it'd simplify
> things.

Back on this again. Do you think this issue is important to fix? Because it is not immediately obvious to me what exactly you mean by this and my research down this road today proves that I'm not able to quickly convert this code but I will need time to educate myself more on the details first. I'm hence asking if it is worth the effort for this change.

	Daniel Stenberg [:bagder] Assignee
	Comment 5 • 5 years ago

Here's a minor update to the patch, now using :: in front of all Mac API calls.

	Josh Aas
	Comment 6 • 5 years ago

Don't worry about it.

	Daniel Stenberg [:bagder] Assignee
	Comment 7 • 5 years ago

Here's the try run for this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=95e13f3c95b1

	Carsten Book [:Tomcat]
	Comment 8 • 5 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5e08af2e43

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 5 years ago

https://hg.mozilla.org/mozilla-central/rev/cd5e08af2e43

	Florian Bender
	Comment 10 • 5 years ago

Does this have an observable effect for OS X users? E.g. "Firefox now detects changes in network connectivity and behaves accordingly" (whatever "accordingly" is). If so, please mark as relnote? and explain – this is also valuable for MDN, I guess. Thanks!

	Daniel Stenberg [:bagder] Assignee
	Comment 11 • 5 years ago

It should have an observable effect. Things like switching to another wifi AP, going on/off a VPN or shutting down your laptop in one place and open it up at another etc where you have Firefox running and change network topology, should now be handled much better. IOW: what bug 939318 made for windows users...

	Eric Shepherd [:sheppy]
	Comment 12 • 5 years ago

What developer-facing changes are there here? This seems more like a user-focused change, both in terms of the descriptions I see here and looking over the code.

	Eric Shepherd [:sheppy]
	Comment 13 • 5 years ago

Ah, because the NS_NETWORK_LINK_DATA_CHANGED event is now correctly sent on Mac; was it working properly on other platforms already?

	Daniel Stenberg [:bagder] Assignee
	Comment 14 • 5 years ago

(In reply to Eric Shepherd [:sheppy] from comment #13)
> Ah, because the NS_NETWORK_LINK_DATA_CHANGED event is now correctly sent on
> Mac; was it working properly on other platforms already?

Yes. It is being sent correctly on Windows and Android, while Linux and FxOS are still pending (bug 1008091).