Send NS_NETWORK_LINK_DATA_CHANGED event on Mac OS X

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bagder, Assigned: bagder)

Tracking

({dev-doc-needed, relnote})

Trunk
mozilla36
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lame-network])

Attachments

(1 attachment, 1 obsolete attachment)

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)
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.
Attachment #8509336 - Flags: feedback?(joshmoz)

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.
Attachment #8509336 - Flags: feedback?(joshmoz) → feedback+
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...
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.
Flags: needinfo?(joshmoz)
Assignee

Comment 5

5 years ago
Here's a minor update to the patch, now using :: in front of all Mac API calls.
Attachment #8509336 - Attachment is obsolete: true

Comment 6

5 years ago
Don't worry about it.
Flags: needinfo?(joshmoz)
Assignee

Updated

5 years ago
Attachment #8512597 - Flags: review?(joshmoz)
Assignee

Updated

5 years ago
Depends on: 1090170

Updated

5 years ago
Attachment #8512597 - Flags: review?(joshmoz) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd5e08af2e43
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

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!
Keywords: dev-doc-needed
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...
Assignee

Updated

5 years ago
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?
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).
You need to log in before you can comment on or make changes to this bug.