112366 - status message is often incorrect [eg. resolving host...]

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Darin Fisher]

this bug has bothered me for nearly a year.  tonight, i finally set out to track
it down.  turns out that the original code in nsBrowserStatusHandler.js is flat
out wrong.  it tries to prevent changing the status message too frequently, but
in doing so it ignores many of the status messages.  often, the last status
message will be "resolving host..." and this will stick for 400ms or whenever
the next status message arrives after 400ms has elapsed.  this is common, since
"resolving host..." is always the first status message sent out.

i looked at the code for updateStatusField() and it seems that it is already
smart about not updating the status message unless the status message has
actually changed.  this is good, and IMO probably makes the 400ms timeout
business unnecessary.  to test this theory, i added a dump statement to observer
how frequently the status message would be changed if i eliminated the 400ms
timeout.  it didn't appear to be called that frequently, so i think it might be
safe to just do away with the 400ms timeout.

since i have very little experience w/ tweaking our UI code, please let me know
if i'd be making a big mistake by ripping out this timeout.  attaching my patch...

Comment 1

[Darin Fisher]

Attachment: v1.0 patch

Comment 2

[John Morrison]

The big gotcha here is that you may slow down page-loading if you wind up 
updating too much. Maybe this is no longer the big deal that it was, but 
when hyatt set up this timeout back in March or so it was a _big_ win (like 
6 or 9%).

[That's not to say that I like the wrong messages, but if you do change this
make sure to test page load times. (Actually, since I have a bug assigned to 
me to try changing that timeout and see what happens now, bug me if you want 
a hand to test this)].

Comment 3

[Darin Fisher]

right.. i'm planning to run it through the page loader today.  it appears that
the code in onStatusChange has not been touched since v1.1 of
nsBrowserStatusHandler.js... however, that version is dated Apr 14, 2001... so i
guess there was a rewrite or something.  looks like the work was done for bug 46200.

Comment 4

[Brendan Eich [:brendan]]

If we need the timeout still, the fix is easy: if (this.statusTimeoutInEffect),
save aMessage and set a flag telling the timeout function to update the status
bar with the saved message when the timeout fires.

/be

Comment 5

[gordon]

right, that way "resolving host..." shouldn't be up for more than 400ms longer
than necessary.  Thanks for doing this Darin. Thank you Thank you Thank you
Thank you!

Comment 6

[Darin Fisher]

Attachment: v1.1 patch : brendan's suggested patch

ran ibench on this patch and didn't notice any impact on page load performance.

Comment 7

[Darin Fisher]

anyone care to r/sr this patch?

Comment 8

[jag (Peter Annema)]

Comment on attachment 59637 [details] [diff] [review]
v1.1 patch : brendan's suggested patch

r=jag

Comment 9

[Alec Flett]

Comment on attachment 59637 [details] [diff] [review]
v1.1 patch : brendan's suggested patch

I was worried about this affecting pageload time, but  it sounds like, from how
darin described this, that updateStatusField only updates the status when it
changes anyway
sr=alecf

Comment 10

[Darin Fisher]

fixed-on-trunk

Comment 11

[Darin Fisher]

*** Bug 105934 has been marked as a duplicate of this bug. ***