Closed Bug 1079385 Opened 6 years ago Closed 6 years ago

Send NS_NETWORK_LINK_DATA_CHANGED event on Mac OS X

Categories

(Core :: Networking, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bagder, Assigned: bagder)

References

Details

(Keywords: dev-doc-needed, relnote, Whiteboard: [lame-network])

Attachments

(1 file, 1 obsolete file)

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)
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.
Attachment #8509336 - Flags: feedback?(joshmoz)
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.
Attachment #8509336 - Flags: feedback?(joshmoz) → feedback+
(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...
(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.
Flags: needinfo?(joshmoz)
Here's a minor update to the patch, now using :: in front of all Mac API calls.
Attachment #8509336 - Attachment is obsolete: true
Don't worry about it.
Flags: needinfo?(joshmoz)
Attachment #8512597 - Flags: review?(joshmoz)
Depends on: 1090170
Attachment #8512597 - Flags: review?(joshmoz) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd5e08af2e43
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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!
Keywords: dev-doc-needed
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...
Keywords: relnote
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.
Ah, because the NS_NETWORK_LINK_DATA_CHANGED event is now correctly sent on Mac; was it working properly on other platforms already?
(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).
You need to log in before you can comment on or make changes to this bug.