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

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[David Hyatt]

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.

Comment 1

[Zach Lipton [:zach]]

This is a dup of 103097, copying information in there

*** This bug has been marked as a duplicate of 103097 ***

Comment 2

[Stuart Ballard]

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.

Comment 3

[Gervase Markham [:gerv]]

Well done, Stuart - you beat me to it :-) Let's not morph bug 103097 again.

Gerv

Comment 4

[Stuart Ballard]

I'll produce a patch for this if someone can tell me what I should compare
event.target to. Should it be equal to document.getElementById("appcontent") or
is there something else I can test for?

Comment 5

[David Hyatt]

The target will be a document.  You could compare it to
document.getElementById("appcontent").contentDocument.

Comment 6

[Tobias Burnus]

Suggest OS/Platform=all

Comment 7

[Tobias Burnus]

Suggest OS/Platform=all

Comment 8

[Stuart Ballard]

Attachment: Check event.target against getBrowser()

Comment 9

[Stuart Ballard]

The attached patch seems to be what you want.

Unfortunately, it doesn't *do* what you want. The event.target == 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().

Comment 10

[David Hyatt]

Sorry, i should have remembered that the <browser>s are anonymous content.  You
need to use event.originalTarget.

Comment 11

[Stuart Ballard]

Attachment: Check the right thing

Comment 12

[Stuart Ballard]

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 13

[David Hyatt]

sr=hyatt

Comment 14

[Nobody; OK to take it and work on it]

Comment on attachment 52258 [details] [diff] [review]
Check the right thing

r=Gerv.

Gerv

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 52258 [details] [diff] [review]
Check the right thing

a=asa for checkin to the 0.9.5 branch

Comment 16

[Christopher Hoess (gone)]

Checked in by Gerv to branch and trunk.

Comment 17

[HJ]

Did this patch improved the 5% page load hit? And if so, what is it now?