If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Don't show progress indicators when using bfcache to go to another page (causes slowdown)

VERIFIED FIXED in Firefox 3.7a1

Status

()

Firefox
General
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: Paul Stone, Assigned: dao)

Tracking

(Blocks: 1 bug, {perf, verified1.9.2})

unspecified
Firefox 3.7a1
perf, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8rc1 -
blocking-firefox3 -
blocking-firefox3.6 -
wanted-firefox3 +
wanted-firefox3.6 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: [TSnap] calm-ui)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051001 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051001 Firefox/1.4.1

When the status bar is enabled, back/forward using bfcache does not work as fast
as when it is disabled. 

On my 1Ghz Athlon, this makes the difference between bfcache being instant and
having a slight delay between clicking back and the page appearing. I narrowed
the problem to the progress meter by using DOMI to delete the progress meter
widget. This removed the delay.

This could be related to bug 279465, however I filed this bug separately to
highlight the effect it has on bfcache. The whole point of this feature is that
is it fast - this bug makes it slower.

Reproducible: Always

Steps to Reproduce:
1. Have status bar enabled
2. Go back/forward using bfcache

Actual Results:  
Back/forward not as blazingly fast as it could be

Expected Results:  
Progress meter visibility should not affect speed of bfcache
(Reporter)

Updated

12 years ago
Keywords: perf

Updated

12 years ago
Blocks: 274784
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

12 years ago
Depends on: 279465
(Reporter)

Comment 1

12 years ago
A potential workaround for this could be to not show the progress meter if a
page is being retrieved from bfcache. It doesn't actually do anything useful in
this case - it just flashes up and disappears again.

Comment 2

12 years ago
(In reply to comment #1)
> A potential workaround for this could be to not show the progress meter if a
> page is being retrieved from bfcache. It doesn't actually do anything useful in
> this case - it just flashes up and disappears again.

ya but browsing all together is sped up dramatically on slower machines with no
progress meter, be it regular browsing or using back/forward.

Updated

12 years ago
Flags: blocking1.8.1?
(Reporter)

Comment 3

12 years ago
I mentioned that if I removed the progress meter widget by deleting it with
DOMI, it removed the delay... that actually seems not to be the case. I'll put
that observation down to the placebo effect. 

Anyway, disabling the status bar does definitely remove the delay, but I'll
leave the diagnosis to someone who knows a bit more about Mozilla's internals
than I do - sorry for the bugspam!

Updated

12 years ago
Flags: blocking1.8.1? → blocking1.8rc1?

Comment 4

12 years ago
too late in the release process to look at this. 
Flags: blocking1.8rc1? → blocking1.8rc1-

Comment 5

12 years ago
This bug is still valid, when the summary is changed to:
don't do the progress bar when using bfcache to go to another page.

Updated

10 years ago
Flags: blocking-firefox3?
Summary: progress meter on statusbar causes bfcache slowdown → Don't do the progress bar when using bfcache to go to another page (Causes slowdown)
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]

Comment 6

9 years ago
Has anyone taken a look at the numbers (again?) since bug 279465 was fixed?  Is there still an improvement by not showing the progress bar when retrieving a page from bfcache?

Updated

8 years ago
Blocks: 481360
(Assignee)

Comment 7

8 years ago
Created attachment 398924 [details] [diff] [review]
patch
Assignee: nobody → dao
Attachment #398924 - Flags: review?(mconnor)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Updated

8 years ago
Blocks: 514915
(Assignee)

Updated

8 years ago
Flags: blocking-firefox3.6?
Summary: Don't do the progress bar when using bfcache to go to another page (Causes slowdown) → Don't show progress indicators when using bfcache to go to another page (causes slowdown)
Whiteboard: [TSnap] calm-ui
Connor, if you can't get to this review quickly, please propose an alternate. Would like it for 3.6, though won't block on it.
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
Comment on attachment 398924 [details] [diff] [review]
patch

I don't think it's a good idea to lie about mIsBusy, code wise. It may lead to random and totally unreproducible bugs, esp. for extensions. Here's some code from tabbrowser.xml for example:

// If the new tab is busy, and our current state is not busy, then
// we need to fire a start to all progress listeners.
if (this.mCurrentTab.hasAttribute("busy") && !this.mIsBusy) {

Let's rather introduce showBusyUI.
Attachment #398924 - Flags: review?(mconnor) → review-
(Assignee)

Comment 10

8 years ago
Created attachment 399349 [details] [diff] [review]
patch
Attachment #398924 - Attachment is obsolete: true
Attachment #399349 - Flags: review?(mano)
Attachment #399349 - Flags: review?(mano) → review-
Comment on attachment 399349 [details] [diff] [review]
patch

You misunderstood my comments here and on irc. I don't care about the busy attribute, it's intended for style, and we don't need a new attribute for "restoring" pseudo-state. It's mIsBusy/isBusy that shouldn't be changed.

> 
>   onProgressChange: function (aWebProgress, aRequest,
>                               aCurSelfProgress, aMaxSelfProgress,
>                               aCurTotalProgress, aMaxTotalProgress) {
>-    if (aMaxTotalProgress > 0) {
>+    if (aMaxTotalProgress > 0 && this.isBusy) {
>       // This is highly optimized.  Don't touch this code unless
>       // you are intimately familiar with the cost of setting
>       // attrs on XUL elements. -- hyatt

why is this change
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> >   onProgressChange: function (aWebProgress, aRequest,
> >                               aCurSelfProgress, aMaxSelfProgress,
> >                               aCurTotalProgress, aMaxTotalProgress) {
> >-    if (aMaxTotalProgress > 0) {
> >+    if (aMaxTotalProgress > 0 && this.isBusy) {
> >       // This is highly optimized.  Don't touch this code unless
> >       // you are intimately familiar with the cost of setting
> >       // attrs on XUL elements. -- hyatt
> 
> why is this change

Because it's not clear to me that we'll never get a status update when restoring. We don't want to update the status meter in that case.
(Assignee)

Comment 13

8 years ago
Created attachment 399421 [details] [diff] [review]
patch
Attachment #399349 - Attachment is obsolete: true
Attachment #399421 - Flags: review?(mano)
Comment on attachment 399421 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -3909,64 +3909,67 @@ var XULBrowserWindow = {
>   onLinkIconAvailable: function (aBrowser) {
>     if (gProxyFavIcon && gBrowser.userTypedValue === null)
>       PageProxySetIcon(aBrowser.mIconURL); // update the favicon in the URL bar
>   },
> 
>   onProgressChange: function (aWebProgress, aRequest,
>                               aCurSelfProgress, aMaxSelfProgress,
>                               aCurTotalProgress, aMaxTotalProgress) {
>-    if (aMaxTotalProgress > 0) {
>+    if (aMaxTotalProgress > 0 && this._busyUI) {

Add that comment over here.

r=mano otherwise. Thanks for the quick update
Attachment #399421 - Flags: review?(mano) → review+
(Assignee)

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/71ab8f14af55
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
(Assignee)

Updated

8 years ago
Attachment #399421 - Flags: approval1.9.2?
Comment on attachment 399421 [details] [diff] [review]
patch

a192=beltzner
Attachment #399421 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 17

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aaf08cbbbafb
status1.9.2: --- → beta1-fixed
Keywords: checkin-needed
This is only supposed to disable the per-window throbber on the bottom right, not the per-tab pie throbber, is that correct?
(Assignee)

Comment 19

8 years ago
This is supposed to disable all activity indicators.
Depends on: 517272
Strange, when trying to verify this bug, I was still seeing the per-tab throbbers fire.
(Assignee)

Comment 21

8 years ago
The bfcache isn't used for pages that listen to the unload event.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091012 Minefield/3.7a1pre
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b1pre) Gecko/20091012 Namoroka/3.6b1pre

V. Fixed.

(In reply to comment #21)
> The bfcache isn't used for pages that listen to the unload event.

That must have been the issue I saw previously. There still is a small visual blur when switching pages, the tab gets a blank page favicon before switching to the other sites favicon, worth a new bug?
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
(Assignee)

Comment 23

8 years ago
(In reply to comment #22)
> That must have been the issue I saw previously. There still is a small visual
> blur when switching pages, the tab gets a blank page favicon before switching
> to the other sites favicon, worth a new bug?

Yep.
(Assignee)

Comment 24

8 years ago
(In reply to comment #23)
> (In reply to comment #22)
> > That must have been the issue I saw previously. There still is a small visual
> > blur when switching pages, the tab gets a blank page favicon before switching
> > to the other sites favicon, worth a new bug?
> 
> Yep.

filed bug 521967
You need to log in before you can comment on or make changes to this bug.