Closed Bug 103223 Opened 22 years ago Closed 22 years ago

Link Toolbar doesn't check target of load/unload events!


(SeaMonkey :: General, defect)

Not set


(Not tracked)



(Reporter: hyatt, Assigned: gerv)




(1 file, 1 obsolete file)

Correct me if I'm wrong, but it looks like the link toolbar is reexecuting its
load/unload code for every load without checking the target.  This means that
the handler code will execute for iframe banner ads and for subframes, which
means it will be executing multiple times for many pages!

This probably has a lot to do with the 5% page load hit.
Blocks: 103053
Keywords: perf
This is a dup of 103097, copying information in there

*** This bug has been marked as a duplicate of 103097 ***
No longer blocks: 103053
Closed: 22 years ago
Keywords: perf
Resolution: --- → DUPLICATE
I disagree this is a dup - let's make it a dependency instead. There are lots of
ways we can improve performance here, and bug 103097 already has two distinct
approaches going on in it (because I put a trivial "low-hanging fruit" patch in
there that's separate from the main focus of that bug just to avoid the overhead
of filing one)

Also, 103097 already has two separate criteria for "fixedness" - the performance
needs to be increased AND the toolbar needs to be re-enabled by default. Let's
not add another one. Let's keep this bug for this issue so that it can be closed
when this particular issue is fixed.
Resolution: DUPLICATE → ---
Blocks: 103097
Well done, Stuart - you beat me to it :-) Let's not morph bug 103097 again.


No longer blocks: 103097
I'll produce a patch for this if someone can tell me what I should compare to. Should it be equal to document.getElementById("appcontent") or
is there something else I can test for?
The target will be a document.  You could compare it to
Blocks: 103053
Suggest OS/Platform=all
Suggest OS/Platform=all
OS: Windows 2000 → All
Hardware: PC → All
The attached patch seems to be what you want.

Unfortunately, it doesn't *do* what you want. The == getBrowser()
test always passes, even for loads inside frames. This seems to be because we're
getting a load event fired on appcontent itself, rather than on the document
inside it. I'm not sure why that should be, but in a way it's good - a page
shouldn't be able to preventBubble() it's load and prevent chrome from seeing it.

So the question is how to determine the target *document* of the event, as
opposed to the target of the event which will always be getBrowser().
Sorry, i should have remembered that the <browser>s are anonymous content.  You
need to use event.originalTarget.
Attachment #52245 - Attachment is obsolete: true
This works as intended - ready for r/sr.

Up to you guys (Gerv/Hyatt/anyone else interested) as to whether to go for a= on
this one or leave it until 0.9.6. It's simple and probably gives a worthwhile
perf benefit, but it's not a showstopper IMO.
Comment on attachment 52258 [details] [diff] [review]
Check the right thing


Attachment #52258 - Flags: superreview+
Attachment #52258 - Flags: review+
Comment on attachment 52258 [details] [diff] [review]
Check the right thing

a=asa for checkin to the 0.9.5 branch
Attachment #52258 - Flags: approval+
Checked in by Gerv to branch and trunk.
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Did this patch improved the 5% page load hit? And if so, what is it now?
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.