Closed Bug 777445 Opened 7 years ago Closed 7 years ago

Network activity indicator for B2G

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: timdream, Assigned: michal)

References

Details

(Keywords: feature, productwanted, Whiteboard: [LOE:L][WebAPI:P3])

Attachments

(1 file, 4 obsolete files)

Status bar comes with an loading indication

https://github.com/mozilla-b2g/gaia/issues/2565

It is currently listen to the mozbrowserloadstart and mozbrowserloadend events of the foreground app to show it. The problem is that for local package apps the loading icon will blink for ~0.2s which is annoying and might scare the Brazilian away.

UX wishes the indication representation the real network activity of the foreground app. Is it possible to have such event? Does these event results any real differences? What is "Loading" generally means for a browser?
We have a 0.2s delay of the loading indication as a workaround. If that's not enough for v1 this platform feature will be a blocker.
blocking-basecamp: --- → ?
AFAIK, the spinner on Firefox stops when we got a load event. Chrome might cheat a bit and stop at some point. When is mozbrowserloadend fired?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #0)
> which is annoying and
> might scare the Brazilian away.
> 

What the fuck is that supposed to mean?

> UX wishes the indication representation the real network activity of the
> foreground app. Is it possible to have such event? Does these event results
> any real differences? What is "Loading" generally means for a browser?

It's really hard to know what "real network activity" is due to caching.

What if we just delay starting the "spinner" animation for a little bit?
> which is annoying and might scare the Brazilian away.

I believe Tim is alluding to Brazilian C-class data cost sensitivity. It was a major theme of research results. We want to do everything we can to communicate data consumption to users. So an accurate network activity indicator would be ideal.

> What if we just delay starting the "spinner" animation for a little bit?

That is the current work-around solution, implemented by Tim.
(In reply to Josh Carpenter [:jcarpenter] from comment #4)
> > which is annoying and might scare the Brazilian away.
> 
> I believe Tim is alluding to Brazilian C-class data cost sensitivity. It was
> a major theme of research results. We want to do everything we can to
> communicate data consumption to users. So an accurate network activity
> indicator would be ideal.

Yes. Sorry about that, I don't meant to be rude. That's the impression I was given about our target users.

> 
> > What if we just delay starting the "spinner" animation for a little bit?
> 
> That is the current work-around solution, implemented by Tim.

Yes. That's already done on the Gaia side.
Thanks for the explanation, that makes more sense :).
Summary: Notwork activity indication for mozbrowser? → Network activity indication for mozbrowser?
Whiteboard: [blocked-on-input Chris Lee]
I don't understand this bug. You shouldn't be able to navigate to a packaged app in the browser at all. The fact that you currently can is a bug but will be fixed before we ship.

Given that, is this WONTFIX?
> I don't understand this bug. You shouldn't be able to navigate to a packaged app in the browser at 
> all. The fact that you currently can is a bug but will be fixed before we ship.

I don't think that's what this bug is about.

They just want to know when an app is accessing the network (perhaps only during page loads).
(In reply to Justin Lebar [:jlebar] from comment #8)
> They just want to know when an app is accessing the network (perhaps only
> during page loads).

Any app?  Should this indication be present for all browser network access, too?
> Any app?  Should this indication be present for all browser network access, too?

We'd get that for free with any implementation strategy I can think of.
Now I'm even more confused :)

Is what's requested an API to allow apps to know if they are accessing network so that they can render in-app UI?

Or is what's being requested UI in the gaia notification-bar indicating when network access is happening?

Or is what's requested in-browser UI indicating when any webpage is accessing network (including things like XHR requests and websocket activity)?
> Or is what's being requested UI in the gaia notification-bar indicating when network access is happening?

This. :)

As per Tim's original post:

> UX wishes the indication representation the real network activity of the foreground app.

iOS represents this with a spinner in the Notification bar. It's a nice little feature, especially for our launch market. 

FWIW I would not call it a Basecamp-blocker, if we're in cutting-to-the-bone mode. We already have a kludgy solution in place via Tim.
These two things are not the same:

> 1) UI in the gaia notification-bar indicating when network access is happening?

and

> 2) indication of real network activity of the foreground app.

Which do you want?  Foreground only, or anywhere on the phone?
"UI in the gaia notification-bar. . ." + ". . .[indicating] real network activity of the foreground app."

Foreground app only.
Ok, I would say this shouldn't be a blocker then. I think it would be very nice to be able to get per-app information about which apps are currently causing network activity to happen which gaia can then use to determine what should be rendered. But it's not something we should block on IMO.
Summary: Network activity indication for mozbrowser? → Network activity indication for foreground app.
Let's be clear that this market of user are *very* cost-sensitive esp when it comes to data. 

Foreground app network activity indicator I think seems reasonable.  Is this what this bug is about?  (I think so...)
Chris: yes, this is about network activity for the foreground app. Mark it as a blocker if you think it should be.
blocking-basecamp: ? → +
Tim, can you talk a little bit more about the current "hacky" implementation? Based on our previous conversations, may impression was it would be accurate enough for v1? Especially with a 2 sec delay we implemented to filter out false positives from local app loads (forgive my hand-wavy description!)

> It is currently listen to the mozbrowserloadstart and mozbrowserloadend events of the foreground app to show it.
Component: General → Networking
OS: Mac OS X → All
Product: Boot2Gecko → Core
Hardware: x86 → All
Could this perhaps be changed to *all* network activity and not just for the foreground app?
Whiteboard: [blocked-on-input Chris Lee] → [blocked-on-input Chris Lee] [LOE:L]
Jason: This seems like a very large chunk of work. I.e. measuring whether a specific app is using the network as well as having events for when a given app starts/stops using the network.

Would it be significantly easier to not do this on a per-app basis and instead just have an indicator for whether the network is being used?
Assignee: nobody → jduell.mcbugs
> Could this perhaps be changed to *all* network activity and not just for the foreground app?

I'm open to this model. It might even be a better fit for the needs of the target market, who will be sensitive to _any_ network activity, and not just the foreground app's. My concern with this approach is the potential for user confusion and distraction. A status indicator that blinks in and out constantly, potentially without any user input (eg: as result of background process) has the potential to annoy an baffle. 

Can someone give me a sense of the frequency of network activity from System and/or background-ed apps? Is this something we could quickly implement and dogfood before making a final product call?
(In reply to Josh Carpenter [:jcarpenter] from comment #21)
> > Could this perhaps be changed to *all* network activity and not just for the foreground app?
> 
> I'm open to this model. It might even be a better fit for the needs of the
> target market, who will be sensitive to _any_ network activity, and not just
> the foreground app's. My concern with this approach is the potential for
> user confusion and distraction. A status indicator that blinks in and out
> constantly, potentially without any user input (eg: as result of background
> process) has the potential to annoy an baffle. 

This is *exactly* what Android does and this is just a matter of UX to make it not annoying the user (and they succeed in that I believe): the indicator has to be small enough so you notice it but don't get distracted when you are focused somewhere else.

I think the idea of indicating foreground app network activity would give a false sense of control to the users. If I understand it correctly, this is copy-pasted from iOS model but I would bet iOS do not allow apps to do network access when in the background (they are pretty restrictive about background activities AFAIK). If we allow apps to do network access in the background as we currently do, apps will be able to do whatever they want while in the background without the user noticing and, worse of it is that the user would believe that the app isn't using the network at all because the indicator will not be present.
> This is *exactly* what Android does and this is just a matter of UX to make it not 
> annoying the user (and they succeed in that I believe): the indicator has to be small 
> enough so you notice it but don't get distracted when you are focused somewhere else.

I started typing "No way, Android doesn't do this!", but then I realized that it shows little up/down arrows next to the 3G icon.  It's indeed very subtle.  :)
> This seems like a very large chunk of work.

We have existing mechanisms for monitoring HTTP traffic: nsIHttpActivityDistributor and/or nsIWebProgress.  Those give you the channel, from which you can get the AppID if you want this per-app.

Alas, we don't have any mechanism right now for getting traffic notifications for Websockets traffic, nor AFAICT ftp.   We could add those on a per-protocol basis, or create some new protocol-blind "network being used" event.
Which implementation is easier?

1. Indicate foreground app network activity only
2. Indicate all network activity (System+Apps)

I'd rather have something than nothing, and would be happy with either.
Easiest: Monitor HTTP traffic only (foreground app or global, just about equally as easy, presuming it's easy enough to map from a channel's appID to whether it's in the foreground app)

More work: include FTP and websockets traffic (either by adding monitoring for them individually, or hooking socket transport to provide some protocol-agnostic "network in use" notification).  Oh, and I suppose JS TCP Socket traffic too (Mail app).

Note that HTTP should be the vast % of traffic, so for a first cut it's not a bad approximation for total traffic IMO.
I'm worried that showing just the http traffic will make people think that it's an accurate indicator, and then be upset when some app has been using mostly TCP-sockets or WebSockets causing charges without it having been shown in the network indicator.
> I'm worried that showing just the http traffic will make people think that it's an accurate indicator, and then be upset when some app has been using mostly TCP-sockets or WebSockets causing charges without it having been shown in the network indicator.

Agreed. Inaccurate indicator may be worse than none at all. I've asked our research team whether existing devices in the market offer an indicator like this. Market expectations may help us make a product call here on whether to push for inclusion in v1, or punt to v2.

> Note that HTTP should be the vast % of traffic, so for a first cut it's not a bad approximation for total traffic IMO.
 
cyee, what do you think?
clee for input (mistakenly asked Casey in previous comment)
This has been marked a blocker.  Also LOE of L which concerns me slightly. 

I agree whichever implementation path is easier we should take, but we shouldn't indicate network activity if it's inaccurate.  

My understanding from UR is that network activity is very important given how sensitive these users are.  

What else do we need here to move forward from Product/UX?
Whiteboard: [blocked-on-input Chris Lee] [LOE:L] → [LOE:L]
> ...whichever implementation path is easier we should take, but we shouldn't indicate network activity if it's inaccurate. . . What else do we need here to move forward from Product/UX?

Agree 100%. I believe we're good to go from Product and UX perspective.
if the bug dependency chain leading up to bug 784575 become basecamp blockers we'll have metrics for all the network protocols.  So the HTTP-only scenario would be moot in that case.   Of course we can still decide not to make per-app measurements a blocker...
Hmm, actually the IDL I've got for bug 784575 may not be the right solution for this, as it's designed to report network traffic fairly infrequently (my thought was just once, at channel completion time).   Let me think if we need to change that IDL, or use something else.

Speaking of which: I have an implementation question for UI peeps.  For the throbber so far, assuming we use nsIHttpActivityDistributor, we've had a "window" for the throbber to animate: i.e. a START and then an END event.  For some network protocols (Websockets, JS TCP) that model doesn't apply, and instead you get something more like one-time events that say "packet sent or received", without any notion of whether there's more traffic coming, or if the network pipe may be silent for a while after that.   I assume that's still OK if we can translate that into "throb for a little while".
Jason: I would say it's up to you if you think we are able to do this on a per-app basis, or if we should just shoot for having a "network activity is happening somewhere" API.

Note that in either case we need a JS API exposed to web pages (well, webapps really) which provides the caller with whatever information we're planning on exposing.


Yes, I would say it's ok if we for "socket like" protocols simply animate the "throbber" for a few seconds after a packet has been sent/received.
> Yes, I would say it's ok if we for "socket like" protocols simply animate the "throbber" for a few seconds after a packet has been sent/received.

If that's the best we can do, then yes, that's sounds acceptable for v1.
(In reply to Jonas Sicking (:sicking) from comment #34)
> Jason: I would say it's up to you if you think we are able to do this on a
> per-app basis, or if we should just shoot for having a "network activity is
> happening somewhere" API.

What would a "per-app basis" give us? As said in comment 22, doing foreground app only would be a wrong indication. If we do background and foreground apps, there is no reason to do that as a per-app basis AFAICT.
> What would a "per-app basis" give us? As said in comment 22, doing foreground app only would be a wrong indication. If we do background and foreground apps, there is no reason to do that as a per-app basis AFAICT.

The value of a foreground-only approach is the 1-to-1 relationship between the user's input and the indicator's feedback. It's scope is narrow (my current activity) versus broad (the entire system's activity). It's handy when I initiate an action that may take a while to process, such as loading a new batch of emails. The appearance of the indicator tells me that my inputs have produced a response, that the connection is working, and that the app is processing my request. I glance at my iPhone's activity indicator constantly during daily usage.

As an aside, I believe iOS makes it's Network activity indicator optional for devs.

On the other hand, a system-wide approach provides a high-level perspective of all device network activity. It's great if I want a god's eye perspective, but it can't be relied upon to provide feedback of my immediate actions. System-wide is probably more valuable to users who are highly-conscious of data consumption and to monitor all activity. And/or for users of an OS that enables background app network use (as FxOS does, apparently). 

Downsides include increased visual noise of a "randomly-appearing" indicator, and the lack of actionable data it provides. It tells me there's a fire, but not which house it's in. If we go with this approach we can mitigate the former with visual design, and perhaps the later with v2 additions (eg: show network activity indicators alongside individual apps in Task Switcher view?).

I hope that helps explain the thinking? 

As I said, I think both approaches have their own utility, and I'm fine with either for v1.
(In reply to Josh Carpenter [:jcarpenter] from comment #37)
> > What would a "per-app basis" give us? As said in comment 22, doing foreground app only would be a wrong indication. If we do background and foreground apps, there is no reason to do that as a per-app basis AFAICT.
> 
> The value of a foreground-only approach is the 1-to-1 relationship between
> the user's input and the indicator's feedback. It's scope is narrow (my
> current activity) versus broad (the entire system's activity). It's handy
> when I initiate an action that may take a while to process, such as loading
> a new batch of emails. The appearance of the indicator tells me that my
> inputs have produced a response, that the connection is working, and that
> the app is processing my request. I glance at my iPhone's activity indicator
> constantly during daily usage.

I think what you are describing here isn't a network activity indicator, it's an activity indicator. It's a way for an app to say "I'm doing something". This is different from the network usage IMO because network usage might happen when there is no apparent activity.
> I think what you are describing here isn't a network activity indicator, it's an activity indicator. It's a way for an app to say "I'm doing something". This is different from the network usage IMO because network usage might happen when there is no apparent activity.

You're quite right. Sometimes the foreground app Network Activity indicator _does_ fire without any immediate user input, slightly diminishing the exact 1-to-1 relationship I described. But in my experience it's either _usually_ very recent user inputs that are driving the activity, or _quite recent_ ones. And it's also the foreground app that's been monitored, so again, the context does feel immediate.

Again, I'm happy either way for v1. What's the latest on our options?
per comment 22 I'm changing this to indicate network activity in general, not just foreground app.  It's what UX folks seem to want, and easier to code as well.
Summary: Network activity indication for foreground app. → Network activity indicator for B2G
Here's an IDL that control whether and how often we send notifications that the network is active.

Note that this uses a model where we signal based on actual network activity (TCP traffic occurring) rather than the "logical" model that the throbber has historically used (where we throb from "request opened" until onload() fires, or something close to that).  I think that's what people expect from an indicator we overlay on the wifi signal--they care about actual network traffic.  But asking for UI feedback to make sure the UI team is happy with this.

:mcmanus, so my plan here is to hook the core socket transport thread DoPollIteration logic to set some new 'mTrafficOccured' variable if a read/write/connect (but not just a timeout) has happened during the Poll of all sockets, and set a timestamp when that happens, and if timestamp - mLastNotificationSentAt > blipIntervalMilliseconds, sent a notification.   I was thinking I could put that logic here:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSocketTransportService2.cpp?rev=a16372ce30b5#624

Does that seem about right?  I doubt UI will want this notification more often than every 1/10th of a second, so we don't need super-fine granularity here, and I want to keep the overhead down.  I'm expecting that this loop won't generally take very large amounts of time.

I'm a little fuzzy on whether the above loop will catch socket connection in all cases, given the attachSocket() logic in nsSocketTransportService.cpp.  Is it safe to assume network traffic has happened (or is imminent) when AttachSocket is called?  Do we need to do something earlier than this to capture SSL handshakes, etc.?

DNS: we could check and fire the network activity notification off when we issue DNS, but of course until we have our own resolver we won't know when/if network activity actually takes place.  I'm not sure which is the right tradeoff: blip the network activity UI early to show network traffic (but sometimes falsely show activity if the network is down or DNS is cached), or don't blip and make it seem like it "takes longer" for the network to be active than it really is.  Either is probably OK given the use case here.  I'd lean toward issuing the blip.
Attachment #658733 - Flags: ui-review?(jcarpenter)
Attachment #658733 - Flags: feedback?(mcmanus)
Whiteboard: [LOE:L] → [LOE:L][WebAPI:P3]
Keywords: feature
> I think that's what people expect from an indicator we overlay on the wifi signal--they care about actual network traffic.  But asking for UI feedback to make sure the UI team is happy with this.

That's perfect. 

One point of clarification: this will apply to both data and WiFi connections—whichever is active. It is not just WiFi only. I suspect you know that, but just wanted to confirm.

> I doubt UI will want this notification more often than every 1/10th of a second, so we don't need super-fine granularity here, and I want to keep the overhead down.

That's more than enough. On the front end I imagined we would cut down on visual noise and potential false positives by making the indicator "lazy". eg: Only show indicator after 500 milliseconds of sustained network activity. And once it's visible, only hide after 500 milliseconds of sustained inactivity. That should help prevent it flickering in and out, while maintaining an acceptable level of accuracy.

Thanks Jason!
Hmm, looks like Tim may have already implemented that lazy approach I just described: https://github.com/mozilla-b2g/gaia/pull/3830
(In reply to Jason Duell (:jduell) from comment #41)
> Created attachment 658733 [details] [diff] [review]
> IDL for network activity notification control

> 
> :mcmanus, so my plan here is to hook the core socket transport thread
> DoPollIteration logic to set some new 'mTrafficOccured' variable if a
> read/write/connect (but not just a timeout) has happened during the Poll of
> all sockets, and set a timestamp when that happens, and if timestamp -
> mLastNotificationSentAt > blipIntervalMilliseconds, sent a notification.   I
> was thinking I could put that logic here:
> 
>   
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/
> nsSocketTransportService2.cpp?rev=a16372ce30b5#624
> 
> Does that seem about right?  

I think that will approximately work, but its really just a heuristic. Just because a write handler is called doesn't mean anything is written. (less the case for a read handler). but the handlers should only be called with temporal locality to the real activity so it ought to work.

You will catch things like socket closes of idle persistent connections. If you filter that out as non-sustained activity at a higher level that would be ok I suppose.

The more direct approach would be a NSPR IO layer that could catch and report read/write/connect explicitly.
Attachment #658733 - Flags: feedback?(mcmanus)
I received an "Outstanding requests" ping on this bug, and I see that I'm required to do a UI review on Jason's 9/5 patch. This process is completely new to me, so I will ping timdream to see if he can give an assist (load code onto his device and shoot a video, for example)
Jason, any chance you can shoot a quick vid of this new behavior in action? Or were my responses in comment 42 sufficient?
Comment on attachment 658733 [details] [diff] [review]
IDL for network activity notification control

Josh: comment 42 was fine.

I can get the network stack part of this done in a short amount of time, but I'll need someone to write the part of the code that 1) sets nsPISocketTransportService.lipIntervalMilliseconds and 2) receives the "network-activity-blip" nsIObserverService notifications and makes the network indicator "throb" in whatever way the UI folks want it to.  Do we have someone to take that part?  If not I'll spend my time on other B2G bugs...
Attachment #658733 - Flags: ui-review?(jcarpenter)
(In reply to Jason Duell (:jduell) from comment #47)
> Comment on attachment 658733 [details] [diff] [review]
> IDL for network activity notification control
> 
> Josh: comment 42 was fine.
> 
> I can get the network stack part of this done in a short amount of time, but
> I'll need someone to write the part of the code that 1) sets
> nsPISocketTransportService.lipIntervalMilliseconds and 2) receives the
> "network-activity-blip" nsIObserverService notifications and makes the
> network indicator "throb" in whatever way the UI folks want it to.  Do we
> have someone to take that part?  If not I'll spend my time on other B2G
> bugs...

Someone from the Gaia team can do 2).

I'm not sure to understand what are the requirement for 1). Do you need just a pref that set it at startup or something like that?
Assignee: jduell.mcbugs → michal.novotny
> I'm not sure to understand what are the requirement for 1). Do you need just a 
> pref that set it at startup

The IDL I gave would be used by getting the nsPISocketTransportService service and setting the blipIntervalMilliseconds attribute.   But I think you're right--this would be even easier to just make a preference.  Let's call it "network.activity.blipIntervalMilliseconds".

Michal: if the pref isn't set (or == 0), then don't send the notifications (and don't hook NSPR I/O).

As mentioned from talking to mcmanus, it looks like the better way to implement this is via the NSPR IO layer stuff, i.e. something like

   http://www.mozilla.org/projects/nspr/reference/html/priofnc.html#19896

where we hook read and write in order to capture when we're reading/writing bytes from sockets.
Josh: apparently some phones (iPhone?) have separate throbbers for download/upload.  So we're thinking of sending out 2 different notifications in necko, so we can show separate download/upload throbber UI at some point if desired.  Of course for now we can have one throbber that throbs for either event.  Sound OK?
> So we're thinking of sending out 2 different notifications in necko, so we can show separate download/upload throbber UI at some point if desired.  Of course for now we can have one throbber that throbs for either event.  Sound OK?

That's perfect, Jason. We can get granular in v2, and go with a unified throbber in v1.
Can anyone confirm with me on the scope of this bug? Is there a DOM API or event available to web content when this bug is landed, or someone need to patch b2g/chrome/content/shell.js and relay whatever message (observer?) to Gaia System app?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #52)
> Can anyone confirm with me on the scope of this bug? Is there a DOM API or
> event available to web content when this bug is landed, or someone need to
> patch b2g/chrome/content/shell.js and relay whatever message (observer?) to
> Gaia System app?

Some love to b2g/chrome/content/shell.js is needed. There will be an observer message "network-activity-blip" that needs to be translated and forward to the system app. This message will happen periodically while there is some network activity.

+ * Network activity indicator: we send out NS_NETWORK_ACTIVITY_BLIP_TOPIC no
+ * more than every blipIntervalMilliseconds if the network is currently active
+ * (i.e. we're sending/receiving TCP socket data).
+ */
+#define NS_NETWORK_ACTIVITY_BLIP_TOPIC  "network-activity-blip"
Do we have anyone working on the Gaia side here?  We're working on the code that triggers the "network-activity-blip" notification.
By the way, I have good experience with the following API behavior:

- have only addObserver and removeObserver methods on the notification service
- when a callback registers the first time let there be a contract that it gets the notification immediately (in an async fashion) about the activity state ; this way we don't need any sync "getState" attribute to init the indicator
- then only changes will be notified (no periodic updates, only state change updates)

But this is no more then a suggestion...
> no periodic updates, only state change updates

So you're suggesting that we have both a "network-active" and "network inactive" notification, and a caller can assume that the network is active once they get the former until they get the latter?  I think that will be more work for the necko layer (we'd need to reset "inactive" timer for all socket activity?).  The IDL I created for my approach lets the UI select how often it gets notifications, and it doesn't sound like they want it super-often (every 100ms or even less frequent), so I'm not worried about overhead of notifications (IIUC fewer notifications is the main benefit of doing it your way).   So it seems to be easiest to stick with the "blip" notifications rather than a pair of state transition notifications.
Just to give a preview of what the user will see, here is the new icon from Peter. 

https://www.dropbox.com/sh/6ggrsj0wd047tp1/j_fU3CxEgz

There are two icons there. The twin-triangles-flashing is the network activity one.
(In reply to Jason Duell (:jduell) from comment #57)
> So it seems to be easiest to stick with the "blip"
> notifications rather than a pair of state transition notifications.

My suggestion has also a benefit of:
- no need for additional timer
- no need to stop this timer when going to sleep

And I personally don't see a benefit of blipping from necko layer with mostly the same notification all the time.  The logic to decide on whether we do something or not will be the same I believe.  But my comment was just a suggestion.
Depends on: 794316
(In reply to Honza Bambas (:mayhemer) from comment #59)
> My suggestion has also a benefit of:
> - no need for additional timer
> - no need to stop this timer when going to sleep

I find the current approach easier to implement. Btw. it doesn't need any timer.
Honza, I'm not opposed to comment 56 in theory but I don't have a sense of how the implementation works--how for instance do we know when it's time to flag a websocket that's been sending data as "inactive", unless we use a timer?  And time of course is rather short...
For information, I will work on the DOM glue for that. Likely, an event saying that there was a network activity. I will wait for the actual Necko patch to be attached though.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #658733 - Attachment is obsolete: true
Attachment #665224 - Flags: review?(jduell.mcbugs)
Comment on attachment 665224 [details] [diff] [review]
patch v1

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

This looks good to me with a few questions and one important fix (adding the pref to b2g.js--see below).

I've looked over the whole patch, but I'm new to the NSPR IOLayer stuff, so I'd like either mcmanus or bsmith to look over this too (picking mcmanus for now because he'll wake up earlier and we only have a day to land this).  If they're both too busy to review I'm OK with landing this w/o additional review.  It seems simple enough.

::: modules/libpref/src/init/all.js
@@ +3745,5 @@
> +
> +// Minimum delay in milliseconds between network activity notifications (0 means
> +// no notifications). The delay is the same for both download and upload, though
> +// they are handled separately.
> +pref("network.activity.blipIntervalMilliseconds", 0);

We need to add this pref to b2g/app/b2g.js with a non-zero value.  For now let's set it to 250ms.  I've gotten some pushback about how often to send the notifications for overhead reasons, and I've told cjones/sicking 500ms, but looking at comment 42 it seems like the UI folks would like to see multiple notifications within 500ms so they can only throb if there's been continuous notifications in that period (I don't actually know if that's a sensible approach, but in any case hopefully we don't need notification more than 4x/second).

I think we can get away with not setting this in all.js--only b2g cares about this, and we do the right thing (disable notifications) if the pref is not set.  I'm not sure what the 'best practice' is for setting variables we don't really need. (If we do set in all.js, we'd probably also want to set this to 0 in mobile/android/app/mobile.js, and mobile/xul/app/mobile.js).

::: netwerk/base/public/nsPISocketTransportService.idl
@@ +40,5 @@
> +
> +%{C++
> +/*
> + * Network activity indicator: we send out these topics no more than every
> + * blipIntervalMilliseconds if the network is currently active (i.e. we're

Add to comment:

...no more than every blipIntervalMilliseconds (as set by the "network.activity.blipIntervalMilliseconds" preference: if 0 no notifications are sent) if the network is currently active...

::: netwerk/base/src/nsNetworkActivityMonitor.cpp
@@ +213,5 @@
> +  mLayerMethods.recv       = nsNetMon_Recv;
> +  mLayerMethods.send       = nsNetMon_Send;
> +  mLayerMethods.recvfrom   = nsNetMon_RecvFrom;
> +  mLayerMethods.sendto     = nsNetMon_SendTo;
> +  mLayerMethods.acceptread = nsNetMon_AcceptRead;

Do we want to count '.connect' as network activity?  Seems like it's not a bad idea.  I'm ok with doing that in a followup, and I could probably be persuaded that we don't need to do it (since we don't use significant bandwidth until we're actually read/write-ing: OTOH it might be nice for users to see the 'throb' as soon as they launch a request, etc.  Of course to do that properly we'd need to hook DNS, which is definitely out of scope for now).

I assume we don't use '.transmitfile' in the browser?  (The only calls using it are in nss, and nsSSLIOLayerMethods sets it to invalid)--bsmith probably knows if we ever hit it.

@@ +220,5 @@
> +  if (!mObserverService)
> +    return NS_ERROR_FAILURE;
> +
> +  mBlipInterval = PR_MillisecondsToInterval(blipInterval);
> +  mLastNotificationTime[kUpload] = PR_IntervalNow() - mBlipInterval - 1;

Why the -1 here?  I would assume you'd want to set to a value that would allow immediate notification, but it seems like you're adding the -1 to make that not happen.

@@ +255,5 @@
> +nsresult
> +nsNetworkActivityMonitor::DataInOut(Direction direction)
> +{
> +  if (gInstance) {
> +    gInstance->DataInOut_Internal(direction);

doesn't seem like DataInOut_Internal is big enough to be a separate function?  Given how often this will be called, it would be nice to avoid another function call overhead--either merge to one function or make _Internal an inline function?

@@ +264,5 @@
> +void
> +nsNetworkActivityMonitor::DataInOut_Internal(Direction direction)
> +{
> +  {
> +    MutexAutoLock lock(mLock);

Do we need a lock?  Are the NSPR I/O functions going to be called re-entrantly?  I would have guessed that the socket transport thread would handle them serially.
Attachment #665224 - Flags: review?(mcmanus)
Attachment #665224 - Flags: review?(jduell.mcbugs)
Attachment #665224 - Flags: review+
Comment on attachment 665224 [details] [diff] [review]
patch v1

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

I really like how non-invasive this code is!

a general word on the IO layers as that was what I was asked to particularly look at - they are implemented exactly as I figured it would be when I suggested this approach. on the other hand, I've never added a whole layer myself (just messed around with existing ones) so take it for what its worth :)

If you agree we can remove the lock, fixup the namespace/naming issues, and file the apropos followon bugs then r=mcmanus

::: modules/libpref/src/init/all.js
@@ +3745,5 @@
> +
> +// Minimum delay in milliseconds between network activity notifications (0 means
> +// no notifications). The delay is the same for both download and upload, though
> +// they are handled separately.
> +pref("network.activity.blipIntervalMilliseconds", 0);

I'm in favor of setting to 0 in all.js - hidden variables kind of suck when people do want to go change them; I learned that when people mis-spelled a dnsprefetch variable in about:config

Its my understanding that mobile should just inherit the all.js value if not defined locally, so you only need to define it in b2g and all

::: netwerk/base/src/Makefile.in
@@ +65,5 @@
>  		nsDNSPrefetch.cpp \
>  		RedirectChannelRegistrar.cpp \
>  		nsPreloadedStream.cpp \
>  		nsStreamListenerWrapper.cpp \
> +		nsNetworkActivityMonitor.cpp \

new files are not prefixed with ns.. so just NetworkActivityMonitor

::: netwerk/base/src/nsNetworkActivityMonitor.cpp
@@ +10,5 @@
> +#include "nsPISocketTransportService.h"
> +#include "nsThreadUtils.h"
> +#include "mozilla/Services.h"
> +
> +using namespace mozilla;

and namespace mozilla::net

@@ +213,5 @@
> +  mLayerMethods.recv       = nsNetMon_Recv;
> +  mLayerMethods.send       = nsNetMon_Send;
> +  mLayerMethods.recvfrom   = nsNetMon_RecvFrom;
> +  mLayerMethods.sendto     = nsNetMon_SendTo;
> +  mLayerMethods.acceptread = nsNetMon_AcceptRead;

don't need transmitfile

this code doesn't count either connect() or FIN (the 0 on recv) both of which are network activity. But I think that's ok for now - the UI layer is going to require constant stream of activity anyhow to throb. I agree a followup bug should count both those cases.

@@ +220,5 @@
> +  if (!mObserverService)
> +    return NS_ERROR_FAILURE;
> +
> +  mBlipInterval = PR_MillisecondsToInterval(blipInterval);
> +  mLastNotificationTime[kUpload] = PR_IntervalNow() - mBlipInterval - 1;

michal please add a comment, but jason I think the code is right.

assume now = 1000 and interval = 50, this sets lastTime to 949.. so if we get a IO event @1000 the delta is 51 (> 50) so the notification happens. PR_interval should make all the wrapping work out. But the notification test is for >= interval anyhow so the -1 is redundant and maybe confusing?

@@ +264,5 @@
> +void
> +nsNetworkActivityMonitor::DataInOut_Internal(Direction direction)
> +{
> +  {
> +    MutexAutoLock lock(mLock);

I agree with jason.. it would make more sense to me to remove the lock and replace it with an assertion about being on the socketthread

::: netwerk/base/src/nsNetworkActivityMonitor.h
@@ +13,5 @@
> +#include "prinrval.h"
> +
> +class nsIObserverService;
> +
> +class nsNetworkActivityMonitor

new code needs to be in namespace mozilla::net

@@ +36,5 @@
> +  void DataInOut_Internal(Direction direction);
> +  void PostNotification(Direction direction);
> +
> +  static nsNetworkActivityMonitor * gInstance;
> +  mozilla::Mutex                    mLock;

I don't think we need this

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +461,4 @@
>          tmpPrefService->AddObserver(SEND_BUFFER_PREF, this, false);
> +
> +        int32_t blipInterval = 0;
> +        rv = tmpPrefService->GetIntPref(BLIB_INTERVAL_PREF, &blipInterval);

doing it only out of init() instead of updateprefs() means the pref is not configurable without a restart. rather than making the change right now (as activity monitor looks like it would have to change too) can you file a followup bug to deal with that later?
Attachment #665224 - Flags: review?(mcmanus) → review+
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to Patrick McManus [:mcmanus] from comment #65)
> Its my understanding that mobile should just inherit the all.js value if not
> defined locally, so you only need to define it in b2g and all

I've followed Patrick's suggestion. Jason, are you OK with it?


> this code doesn't count either connect() or FIN (the 0 on recv) both of
> which are network activity. But I think that's ok for now - the UI layer is
> going to require constant stream of activity anyhow to throb. I agree a
> followup bug should count both those cases.

I'm not sure that we really want to cover these cases. E.g. when writing to the net we will also sooner or later receive ACK, but we notify only upload activity since IMO we are not interested in control packets. If we really want to notify all network activity, then it makes no sense to differentiate between upload and download. Also it is really hard (or impossible) to do it reliably, e.g. first call to PR_Read() that returns 0 means that FIN packet was received, but any further call to PR_Read() on the same descriptor returns 0 without any network activity. Sure, I know that it doesn't make sense to call PR_Read() again on a closed descriptor, but it is a valid call...


> @@ +220,5 @@
> assume now = 1000 and interval = 50, this sets lastTime to 949.. so if we
> get a IO event @1000 the delta is 51 (> 50) so the notification happens.
> PR_interval should make all the wrapping work out. But the notification test
> is for >= interval anyhow so the -1 is redundant and maybe confusing?

Right, the -1 was redundant. I removed it.


> @@ +264,5 @@
> > +void
> > +nsNetworkActivityMonitor::DataInOut_Internal(Direction direction)
> > +{
> > +  {
> > +    MutexAutoLock lock(mLock);
> 
> I agree with jason.. it would make more sense to me to remove the lock and
> replace it with an assertion about being on the socketthread

Agree. My plan was to call this function from other places like DNS thread, but with the current code is the lock not necessary.


> ::: netwerk/base/src/nsSocketTransportService2.cpp
> @@ +461,4 @@
> >          tmpPrefService->AddObserver(SEND_BUFFER_PREF, this, false);
> > +
> > +        int32_t blipInterval = 0;
> > +        rv = tmpPrefService->GetIntPref(BLIB_INTERVAL_PREF, &blipInterval);
> 
> doing it only out of init() instead of updateprefs() means the pref is not
> configurable without a restart. rather than making the change right now (as
> activity monitor looks like it would have to change too) can you file a
> followup bug to deal with that later?

Do we really want to do it? As Jason described in comment #49, we don't hook the NSPR IO layer if the preference is not set or it is 0. So if it is activated later after startup there is no way to hook existing descriptors. I.e. it is trivial to implement listening for the pref changes but for the price that we hook the IO layer all the time.
Attachment #665224 - Attachment is obsolete: true
(In reply to Michal Novotny (:michal) from comment #66)
> Created attachment 665595 [details] [diff] [review]
> patch v2
> 
> (In reply to Patrick McManus [:mcmanus] from comment #65)
> > Its my understanding that mobile should just inherit the all.js value if not
> > defined locally, so you only need to define it in b2g and all
> 
> I've followed Patrick's suggestion. Jason, are you OK with it?
> 
> 
> > this code doesn't count either connect() or FIN (the 0 on recv) both of
> > which are network activity. But I think that's ok for now - the UI layer is
> > going to require constant stream of activity anyhow to throb. I agree a
> > followup bug should count both those cases.
> 
> I'm not sure that we really want to cover these cases. E.g. when writing to
>

I think SYN and FIN are both pretty interesting.

SYN can happen all by itself (and repeatedly) if a service isn't connecting.. and that's going to have battery implications, potentially security/privacy implications (hey - I'm communicating!), and (to a lesser extent) bandwidth implications. So the throbber is interesting to me there.

Likewise, a FIN a long time after data transfer has stopped means your radio transitioned from a low power to a high power event to handle that. Engineering wise that's really interesting (and bad!).. But you're right that it might not really mean anything helpful to a user.

that's why I say file the bug and the UX team can sort out what it wants (maybe nothing) later.


> > 
> > doing it only out of init() instead of updateprefs() means the pref is not
> > configurable without a restart. rather than making the change right now (as
> > activity monitor looks like it would have to change too) can you file a
> > followup bug to deal with that later?
> 
> Do we really want to do it? As Jason described in comment #49, we don't hook
> the NSPR IO layer if the preference is not set or it is 0. So if it is
> activated later after startup there is no way to hook existing descriptors.
> I.e. it is trivial to implement listening for the pref changes but for the
> price that we hook the IO layer all the time.

I wouldn't make it dynamic for a connection for this reason, but dynamic for the session (i.e. new connections). Connections are pretty short lived compared to the session. You could even easily clear the persistent connection table on the pref being changed.
Comment on attachment 665595 [details] [diff] [review]
patch v2

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

OK, this looks ready to go.  Do we have a plan for the NSPR dependency (i.e. should we land bug 794316 on trunk so we can land this too before tomorrow, or is there some plan to import a newer NSPR?  794316 hasn't even landed in NSPR yet, so I'm assuming we should land it in m-c and then land this).

> Its my understanding that mobile should just inherit the all.js value if not
> defined locally, so you only need to define it in b2g and all

I checked--Patrick is right about this, so we're good.

> I think SYN and FIN are both pretty interesting.

I do think an upload blip for connect() and a download blip for revc=0 are worth doing in a followup.  Possibly even now?  It looks like it would only take a few minutes to add, and you can +r/me on it w/o another review cycle.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +461,4 @@
>          tmpPrefService->AddObserver(SEND_BUFFER_PREF, this, false);
> +
> +        int32_t blipInterval = 0;
> +        rv = tmpPrefService->GetIntPref(BLIB_INTERVAL_PREF, &blipInterval);

I'm fine with only reading the pref at startup.  The use case for users changing it dynamically at runtime seems weak, and IMO the complexity to support it isn't worth it.  The UI folks can restart their browser until they get the right value, and then it's effectively hard-coded.  Geeky users who really want to change the pref can always reboot their phone (I'm not sure we've even going to give them access to about:config for the device?).  

For now let's add a note to the pref in b2g/all.js saying "this pref is only read once at startup: a restart is required to enable a new value".
Attachment #665595 - Flags: review+
mounir: I think maybe it's best to split out the nsIObserver->WebAPI stuff into another bug so we can land this sooner.  Please do that if it's ok with you.
Blocks: 795136
(In reply to Jason Duell (:jduell) from comment #68)
> OK, this looks ready to go.  Do we have a plan for the NSPR dependency (i.e.
> should we land bug 794316 on trunk so we can land this too before tomorrow,
> or is there some plan to import a newer NSPR?  794316 hasn't even landed in
> NSPR yet, so I'm assuming we should land it in m-c and then land this).

It was landed in NSPR, see http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=prlayer.c&branch=3.22&root=/cvsroot&subdir=mozilla/nsprpub/pr/src/io&command=DIFF_FRAMESET&rev1=3.21&rev2=3.22
I have no idea when/if this appears in m-c, so I'm going to land it in m-c. Stop me, if I shouldn't do it...
Attachment #665595 - Attachment is obsolete: true
(In reply to Jason Duell (:jduell) from comment #69)
> mounir: I think maybe it's best to split out the nsIObserver->WebAPI stuff
> into another bug so we can land this sooner.  Please do that if it's ok with
> you.

I opened bug 795136 yesterday.
https://hg.mozilla.org/mozilla-central/rev/de06aeb3c7f3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 795711
You need to log in before you can comment on or make changes to this bug.