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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Paul Stone]

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

Comment 1

[Paul Stone]

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

[u88484]

(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.

Comment 3

[Paul Stone]

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!

Comment 4

[Scott MacGregor]

too late in the release process to look at this. 

Comment 5

[Alfred Kayser]

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.

Comment 6

[u88484]

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?

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 9

[Asaf Romano (gone)]

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.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch

Comment 11

[Asaf Romano (gone)]

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

Comment 12

[Dão Gottwald [:dao]]

(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.

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch

Comment 14

[Asaf Romano (gone)]

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

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/71ab8f14af55

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 399421 [details] [diff] [review]
patch

a192=beltzner

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aaf08cbbbafb

Comment 18

[Nochum Sossonko [:Natch]]

This is only supposed to disable the per-window throbber on the bottom right, not the per-tab pie throbber, is that correct?

Comment 19

[Dão Gottwald [:dao]]

This is supposed to disable all activity indicators.

Comment 20

[Nochum Sossonko [:Natch]]

Strange, when trying to verify this bug, I was still seeing the per-tab throbbers fire.

Comment 21

[Dão Gottwald [:dao]]

The bfcache isn't used for pages that listen to the unload event.

Comment 22

[Nochum Sossonko [:Natch]]

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?

Comment 23

[Dão Gottwald [:dao]]

(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.

Comment 24

[Dão Gottwald [:dao]]

(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