374503 - Status bar doesn't clear when page finishes loading

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Mark Mentovai]

Tested on the 1.8[.1] branch as far back as 2006120105.  Tested on the current 1.8[.1] branch build, 2007031904; the current trunk build, 2007031822; and even the current 1.8.0 branch build, 2007031901.

The status bar doesn't always clear when a page finishes loading.

STR:
1. Remove the cache, profile directory, and plist.
2. Start Camino.
2. Dismiss the "default browser?" dialog if it shows up because Launch Services went behind your back and reset the default browser to Safari again.
3. Type "google.com" in the location bar and hit RETURN.
4. Wait for google.com to load.
5. When google.com finishes loading, the status bar still says "Transferring data from www.google.com…".  Expected behavior: it shouldn't say anything, it should be clear.  Hovering over a link causes the link text to show in the status bar, but onmouseout, the status text still changes back to the evil message instead of clearing.  (On the 1.8.0 branch, the status bar DOES clear after onmouseout.)
6. Reload.  The status bar's fixed text will change to "Read www.google.com" or something else inappropriate.
7. Quit and try again.  Now, the text will be "Read www.google.com" the first time around.

I usually keep my home page set to about:blank.  That's when I first noticed this.

Comment 1

[Stuart Morgan]

(In reply to comment #0)
> (On the 1.8.0 branch, the status bar DOES clear after onmouseout.)

That's more or less a bug that I fixed (a side-effect of us letting link mouseover beat out obnoxious JS changes to the status bar text), but it sounds like the underlying bad behavior predates my change (yay! :).

Comment 2

[Mark Mentovai]

Why didn't we get those side effects on the trunk and 1.8 branch, too?

Comment 3

[Stuart Morgan]

Yeah, that wasn't very clearly stated. Rewinding and trying again:

> (On the 1.8.0 branch, the status bar DOES clear after onmouseout.)

The fact that the status bar was cleared by mousing on and off a link was a bug, which I fixed for 1.8 and trunk as part of the fix allowing mouseovers to beat out obnoxious JS in the status bar.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Simplest STR:

Set "New Window" not to load home page.  Open new window.  Type www.google.com and visit.

It might be useful if someone with a few spare cycles could find a more detailed regression range here (unless I'm misunderstanding the cause of the bug, i.e., that Stuart's fix for status bar priority only exposed the bug)....

Comment 5

[Mark Mentovai]

Sounds like Stuart's fix only exposed the portion of the bug that caused the evil status bar text to return after onmouseout on a link.

Comment 6

[Mark Mentovai]

I'm seeing the bug in builds as far back as 2005-01-01.  Didn't hit up archive for earlier builds.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I have bad news...I can reproduce this (using comment 4) as far back as archive goes, 2003-03-06-07/"Camino" 0.6 (although it seems somewhat finicky in that build, working the first window but not in subsequent windows).  I didn't check every build, just sporadically, so it's possible it did work at some point....

I wonder if that is why we made "Document: Done" show up (which was then removed in bug 246112)?

Comment 8

[Stuart Morgan]

What's going on is that there's no status message change corresponding to "everything is done", so what we do is clear it in onLoadingCompleted:. In some cases, though, we get status messages saying we are loading things *after* onLoadingCompleted:; I'm pretty this usually (always?) corresponds to resources loaded via JS, since for example we get one on the Camino homepage every time the mini-screenshot rotates.

I think what we need to do here is keep an array of all nsIRequest objects we get starts for, remove each one as we get the end for it, and clear the status string every time that array empties. It looks from some traces I took of a couple cases like that should work.

I'll put together a fix soon, and I think we may want to take it for 1.1 since this feels like a regression in many cases because of my other fix that prevents accidental clearing of the stuck message.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Nominating per comment 8.

Comment 10

[Stuart Morgan]

Attachment: fix

Comment 11

[Stuart Morgan]

Since there's a patch now, moving to the 1.1 list so it stays on the radar.

Comment 12

[Mark Mentovai]

+} EStatusPriority;

Need the E?

+  if ([mLoadingResources containsObject:resourceIdentifier])

When's this false?  If never, we can optimize by getting rid of the set and just maintaining a count.

Comment 13

[Stuart Morgan]

(In reply to comment #12)
> +} EStatusPriority;
> 
> Need the E?

I was just following the format some of the other Camino enums use. I'm not wild about it, so I'm glad to change if that's not the format we actually want to use.

> +  if ([mLoadingResources containsObject:resourceIdentifier])
> 
> When's this false?  If never, we can optimize by getting rid of the set and
> just maintaining a count.

In my testing there were cases where a count wouldn't work. Sometimes more than one end came for the same request (although that was probably filterable by other flags in the state), but the big one IIRC was that after changing to a new page while another was loading I would at least sometimes get completion notices for resources from the old page.

Comment 14

[Mark Mentovai]

Comment on attachment 261558 [details] [diff] [review]
fix

r=me then.  Personally, I don't like the E either, and I didn't think that it was the prevailing style, but I've been wrong before.

Comment 15

[Stuart Morgan]

Comment on attachment 261558 [details] [diff] [review]
fix

I can change the enum name on check-in

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 261558 [details] [diff] [review]
fix

+typedef enum {

the |typedef| shouldn't be needed in C++. You should just be able to do 

enum Foo { ... }

sr=pink

Comment 17

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH with the enum changes.