Closed
Bug 518805
Opened 15 years ago
Closed 8 years ago
meta refresh should not reload page when tab/window is not active (not visible) or in idle state
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: romaxa, Assigned: snorp)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 2 obsolete files)
6.73 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
pages with meta refresh tag will drain battery on mobile device. I think we should take care about this problem.
Comment 1•15 years ago
|
||
I'm not sure we want to change the default behavior here; I can see providing an option for certain embeddings to have this behavior.
Reporter | ||
Comment 2•15 years ago
|
||
I agree... for default browser it is not very good to disable it by default (Some page can be reloaded and trow some notification (sound, window manager) about new state).. But for mobile case it may cost you by dead phone... I see 2 ways: 1) use patch from bug 343515, and have some preference which will enable/disable suspend on Docshell->SetActive call. 2) Provide separate API for this suspension...
Comment 3•15 years ago
|
||
The idea is to stop all refresh timers when we are not active and restore all states when we are active again This patch should be apply on top of patch from bug 343515.
Attachment #407789 -
Flags: review?(bzbarsky)
Comment 4•15 years ago
|
||
Why do you need the presshell changes? What happened to the calling Freeze() idea?
Comment 5•15 years ago
|
||
(In reply to comment #4) > Why do you need the presshell changes? > > What happened to the calling Freeze() idea? These PresShell changes are part of patch from bug 343515, didn't delete everything. Sorry. Timers that I need to manipulate are in DocShell and they are private members there. ResumeRefreshURIs and SuspendRefreshURIs only stops and starts those timers. But after they are stopped new timers can be added in RefreshUri. So I need some flag that can tell if DocShell is active or not and to freeze all added timers.
Comment 6•15 years ago
|
||
This patch has better solution. All trash about PresShell is removed. Also should be applied upon patch from bug 343515.
Attachment #407789 -
Attachment is obsolete: true
Attachment #409126 -
Flags: review?(bzbarsky)
Attachment #407789 -
Flags: review?(bzbarsky)
Comment 7•15 years ago
|
||
Comment on attachment 409126 [details] [diff] [review] Handling meta refresh timers in background (better solution) >+++ b/docshell/base/nsDocShell.cpp Thu Oct 29 19:25:23 2009 +0200 > nsDocShell::SetIsActive(PRBool aIsActive) >+ // Resume refresh URIs for our child shells as well. This is fine, but should probably be done as part of bug 343515; it's needed for more than just refresh URIs. >@@ -4986,21 +5008,21 @@ nsDocShell::RefreshURI(nsIURI * aURI, PR >+ if (busyFlags & BUSY_FLAGS_BUSY || (!mIsActive && mDisableMetaRefreshInBackground)) { > // We are busy loading another page. Don't create the > // timer right now. Instead queue up the request and trigger the > // timer in EndPageLoad(). Perhaps the comment should now say: // We don't want to trigger the refresh right now. Instead, queue up the // request and trigger the timer in EndPageLoad or when we become active. ? >@@ -5799,22 +5821,23 @@ nsDocShell::EndPageLoad(nsIWebProgress * >+ if (mIsActive) >+ RefreshURIFromQueue(); That should be |mIsActive || !mDisableMetaRefreshInBackground|, no? >@@ -10195,20 +10218,24 @@ nsDocShell::Observe(nsISupports *aSubjec >+ rv = prefs->GetBoolPref("browser.meta_refresh_in_background.disabled", &tmpbool); >+ if (NS_SUCCEEDED(rv)) >+ mDisableMetaRefreshInBackground = tmpbool; >+ This change doesn't make sense. That block where you added the code is the block where we get notified that the error pages pref changed. If you think the meta_refresh_in_background pref should be live over a docshell lifetime, then we need to be observing it explicitly (so adding as an observer in Create(), and having a block in Observe() checking for it). Personally, I'm ok with this particular pref requiring a new docshell to be picked up. r=bzbarsky with the above issues fixed. My apologies for the lag here....
Attachment #409126 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 8•15 years ago
|
||
Egor please attach updated patch.
Assignee | ||
Comment 9•8 years ago
|
||
This is controlled by browser.meta_refresh_when_inactive.disabled, which is false (allow refreshes) on desktop, and true (disable refreshes) on Fennec
Attachment #8725907 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #409126 -
Attachment is obsolete: true
Comment 11•8 years ago
|
||
Nominating for tracking-fennec: we should get this to release ASAP to mitigate background bandwidth consumption, which has been a thorn in our sides for years.
Assignee: nobody → starkov.egor
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
OS: Linux → All
Hardware: Other → All
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/adfae83c382e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Blocks: background-data
Comment 14•8 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: Disables meta refresh on background web pages reducing Firefox's data usage [Suggested wording]: stopped data usage when Firefox is in the background and a page uses meta refresh [Links (documentation, blog post, etc)]:
Comment 15•8 years ago
|
||
Ioana would someone from the Softvision team verify this? Best to use a powerful phone or tablet as being killed in the background would affect this test. * Attach Firefox for Android to desktop devtools * Switch to the network monitor * Open http://www.lefigaro.fr/flash-actu/ in Firefox for Android * ~5 to 7 MB of data should be transferred * due to meta refresh tag the page will reload ~5 to ~7 MB every 15 min * If the screen is on with firefox as the active app, lefigaro as the active tab and 15 min have passed the page should reload * If the screen is off with firefox as the active app, lefigaro as the active tab and 15 min have passed no data should be consumed * If the screen is on with firefox as the active app, lefigaro as the background tab and 15 min have passed no data should be consumed by lefigaro * If the screen is on with firefox as the background app (switch to gmail or similar) , lefigaro as the active tab and 15 min have passed no data should be consumed by lefigaro
Flags: needinfo?(ioana.chiorean)
Comment 16•8 years ago
|
||
Reducing background bandwidth use sounds great, tracking this for 46
status-firefox46:
--- → affected
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8725907 [details] [diff] [review] Don't do meta refreshes when backgrounded Approval Request Comment [Feature/regressing bug #]: None [User impact if declined]: Unnecessary refreshes when Fennec is backgrounded, resulting in increased data usage and wasted battery. [Describe test coverage new/current, TreeHerder]: Nightly, aurora [Risks and why]: Low [String/UUID change made/needed]: None
Flags: needinfo?(snorp)
Attachment #8725907 -
Flags: approval-mozilla-beta?
Comment 19•8 years ago
|
||
Comment on attachment 8725907 [details] [diff] [review] Don't do meta refreshes when backgrounded Should improve battery life by reducing repaints. This should make it into the beta 4 build.
Attachment #8725907 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/56f6fe01291f
Comment 21•8 years ago
|
||
I don't think a risk assessment of "low" is ever reasonable for docshell. Looking at the patch carefully again, I realized that we missed a case: what happens when the docshell goes active before EndPageLoad?
Flags: needinfo?(snorp)
Comment 22•8 years ago
|
||
Also, seems to me that what you may want to do is avoid doing the refresh, not avoid starting the timer, right? Not entirely clear. Oh, and while I was reading the diff again, "whenver" is not a word.
Comment 23•8 years ago
|
||
This may be a case where I could have asked for a 2nd review or just said no to uplifting this far. We can still back this out and do more verification, or just not aim for 46 as we have plenty of risk already.
Comment 24•8 years ago
|
||
Wes can you back this out on mozilla-beta before I build beta 4? I have cold feet. We could give ourselves a bit more time to figure out ways to do manual testing for this on both fennec and desktop, and for snorp to address the issues bz brought up. I don't expect aurora android users to report issues on this. How might we go looking for problems or confirm this is working correctly?
Flags: qe-verify+
Flags: needinfo?(wkocher)
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #21) > I don't think a risk assessment of "low" is ever reasonable for docshell. Reasonable. > > Looking at the patch carefully again, I realized that we missed a case: what > happens when the docshell goes active before EndPageLoad? I think that's handled, no? We should hit this[0] and enable the timer. [0] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7547
Flags: needinfo?(snorp)
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #22) > Also, seems to me that what you may want to do is avoid doing the refresh, > not avoid starting the timer, right? Not entirely clear. IMHO it's cleaner to avoid starting the timer, since it could fire every second and keep the CPU awake. > > Oh, and while I was reading the diff again, "whenver" is not a word. Yeah, that's fixed in the version I pushed.
Comment 27•8 years ago
|
||
> I think that's handled, no? We should hit this[0] and enable the timer.
No, the problem is that we will enable it immediately when going active instead of waiting for EndPageLoad like we're supposed to, as far as I can tell.
Backed out in https://hg.mozilla.org/releases/mozilla-beta/rev/fcdacbff7d1d
Flags: needinfo?(wkocher)
Updated•8 years ago
|
Flags: needinfo?(snorp)
Updated•8 years ago
|
Attachment #8725907 -
Flags: approval-mozilla-beta+
Added to Fx47 (beta) release notes.
Comment 30•8 years ago
|
||
Doesn't look like there's anything for snorp to do here. Bug 1265769 was filed for the follow-up issue in comment 27.
Flags: needinfo?(snorp)
Updated•3 years ago
|
Flags: needinfo?(ioana.chiorean)
You need to log in
before you can comment on or make changes to this bug.
Description
•