Closed Bug 1024614 Opened 10 years ago Closed 10 years ago

Send NS_NETWORK_LINK_DATA_CHANGED events on Android

Categories

(Core :: Networking, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
fennec 34+ ---

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 939318 added some stuff that allows listeners to respond to a NS_NETWORK_LINK_TOPIC CHANGED event. We need to send this appropriately on Android.
Assignee: nobody → snorp
tracking-fennec: ? → 32+
Status: NEW → ASSIGNED
Attached patch Send network link change events (obsolete) — Splinter Review
Attachment #8445247 - Flags: review?(blassey.bugs)
Comment on attachment 8445247 [details] [diff] [review]
Send network link change events

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

As discussed on IRC, I'm concerned that we'll miss link changes where the type doesn't change, like moving from one WiFi AP to another or switching networks on a dual SIM phone
Attachment #8445247 - Flags: review?(blassey.bugs) → review-
:bagder has some wip that builds a checksum for the windows networking state (current addresses and interfaces, etc..).. you might coordinate

he's on vacation for a bit but I'm thinking of bug 939318
Flags: needinfo?(daniel)
(In reply to Patrick McManus [:mcmanus] from comment #3)
> :bagder has some wip that builds a checksum for the windows networking state
> (current addresses and interfaces, etc..).. you might coordinate
> 
> he's on vacation for a bit but I'm thinking of bug 939318

One thing Brad and I considered was just sending the change event whenever Android tells us there was a change. Would that cause any bad stuff if nothing important did actually change? Presumably it would just cause a few extra pings?
Attachment #8445247 - Attachment is obsolete: true
Attachment #8445347 - Flags: review?(mcmanus)
Attachment #8445347 - Flags: review?(blassey.bugs)
Attachment #8445347 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8445347 [details] [diff] [review]
Send network link change events

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

I think for the moment this is fine, but a lot of it going to depend on exactly how bagder handles the notifications - so he should consider this and any changes before his other code lands.. I know it will lead to clearing of some pretty important caches, so we want to avoid that if e.g. the wifi signal just blips on and off as it goes in and out of your pocket.
Attachment #8445347 - Flags: review?(mcmanus) → review+
I think it is fine to send the event more often rather than too seldom (if there's ever such a choice), but I added a checksum for the Windows backend in Bug 939318 since its notifying system is very trigger-happy and caused a large amount of events to happen without me seeing any "interesting" info changing. I'm sure that will differ on specific backend system functionality. And again, triggering CHANGED a little too often will "just" hurt some caches and some performance but will be better than missing a network change event that then instead will end up with stalls and annoyed users.
Flags: needinfo?(daniel)
tracking-fennec: 32+ → ?
tracking-fennec: ? → 34+
This looks like we try to call FlushCache() on a null nsHostResolver. :bagder can you have a look?
Flags: needinfo?(daniel)
Daniel/Patrick, whoever can get to this first :)
Attachment #8496103 - Flags: review?(mcmanus)
Attachment #8496103 - Flags: review?(daniel)
Flags: needinfo?(daniel)
Comment on attachment 8496103 [details] [diff] [review]
Guard against null resolver when flushing DNS cache

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

I'm gonna give a drive-by r+ here, taking Daniel's review because it's late in Europe :)
Looks like mHostResolver can be null if we've previously been told to go offline. So, null checking seems like the right thing to do.
Attachment #8496103 - Flags: review?(daniel) → review+
Comment on attachment 8496103 [details] [diff] [review]
Guard against null resolver when flushing DNS cache

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

steve's review is fine
Attachment #8496103 - Flags: review?(mcmanus)
https://hg.mozilla.org/mozilla-central/rev/ea3b44a4b97a
https://hg.mozilla.org/mozilla-central/rev/473717699910
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.