905325 - about:networking - make refreshing feature only request data for active tab

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Robert Bindar]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130627161625

Steps to reproduce:

We should refresh data only for the active tab. Now refreshing feature requests data for all the tabs, not just for the active one causing a lack of performance.

Comment 1

[Robert Bindar]

Attachment: refresh_active_tab.patch

Comment 2

[Valentin Gosu [:valentin] (he/him)]

As we discussed today, maybe it's better if the Refresh button refreshes all of the tabs, while the auto-refresh checkbox only refreshes the current tab.
The reason being that when you use the refresh button, you might probably want to get a "snapshot" of the current state.

Comment 3

[Robert Bindar]

Attachment: independent refresh v2

Refresh button produces an overall refresh and auto-refresh checkbox only refresh the active tab.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 793588 [details] [diff] [review]
independent refresh v2

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

::: toolkit/content/aboutNetworking.js
@@ +131,5 @@
>      document.getElementById("confpref").addEventListener("click", confirm);
>    }
>  
> +  for (let id in gRequestNetworkingData)
> +    requestNetworkingData(id);

This is used in the click handler as well and could be moved to a separate function like refreshAllNetworkingData() or the like. Maybe you could also name it requestAllNetworkingData() and rename the existing function to requestNetworkingDataForTab().

@@ +151,5 @@
>    let refr = document.getElementById("refreshButton");
> +  refr.addEventListener("click", function() {
> +    for (let id in gRequestNetworkingData)
> +      requestNetworkingData(id);
> +  });

refr.addEventListener("click", requestAllNetworkingData);

@@ +187,5 @@
> +  if (autoRefresh.checked) {
> +    clearInterval(autoRefresh.interval);
> +    autoRefresh.interval = setInterval(function() {
> +      requestNetworkingData(button.value);
> +    }, REFRESH_INTERVAL_MS);

This too should be moved to a separate function as it just repeats the code from above.

Comment 5

[Robert Bindar]

Attachment: independent refresh v3

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 793966 [details] [diff] [review]
independent refresh v3

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

r=me with the two minor issues addressed.

::: toolkit/content/aboutNetworking.js
@@ +140,2 @@
>  
>    document.getElementById("autorefcheck").addEventListener("click", function() {

When initializing, we should check for checkbox.checked. When reloading the page with F5 after clicking the checkbox, it will actually stay checked on the next page load and we should set the interval right at the start.

@@ +191,5 @@
> +function setAutoRefreshInterval(checkBox) {
> +    let active_tab = document.querySelector(".active");
> +    checkBox.interval = setInterval(function() {
> +      requestNetworkingDataForTab(active_tab.id);
> +    }, REFRESH_INTERVAL_MS);

Indentation is a little off here.

Comment 7

[Robert Bindar]

Attachment: independent refresh v4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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