18 years ago
Summary: 5 load listeners registered when one would do → 5 load listeners registered when one could do
Nominating for RTM as search is broken without this fix.
Uh, how is search broken? This bug is only about the performance that could be gained by replacing the 5 separate load listeners with one load listener that would then call these 5 functions.
Robert sez, "Just try doing a search in Mozilla. Sometimes you'll get each search result duplicated several times, sometimes the results end up disappearing... basically, its unusable." This would be bad. A major loss of functionality. Matt, can you find out what's going on here?
Assignee: don → matt
Priority: P3 → P1
Whiteboard: [rtm need info]
> Uh, how is search broken? This patch... The proposed patch has the following line in it: if (event.target == window._content.document) which helps prevent search results from reloading endlessly. This was all covered in bug # 51211.
OK, rjc has nicely sent a patch. Matt, can you review this and approve it?
Whiteboard: [rtm need info] → [rtm need info]Fix in hand
Please note that 51211 was broken into SEVERAL bugs. This bug is ONLY about the performance that could be gained by replacing the 5 load listeners with one. This is actually an ENHANCEMENT. Bug 51211 was closed, and the fix was that image load event no longer travels to chrome. This should have fixed most of the problems with search panel. Update dom/src/base/nsGlobalWindow.cpp and see if the search is better... Bug 55414 is about filtering also the frame load events. That is what if (event.target == window._content.document) would be doing, and it should be handled in bug 55414.
I've been running this in my tree. Seems fine and works fine. sr=rjc Looking for another reviewer.
As I implied in my previous critique of this patch, I have concerns that this rewrite might change the semantics if one of the called handler functions raises an exception. That's probably not important, and I don't even know how the current listener-calling code behaves. So as not to be a party-pooper, I'll go ahead and say "r=law" anyway.
I am still saying you are discussing the wrong thing in this bug. If search results are reloading too many times, that is bug 55414. This bug is only about the performance gained by replacing 5 event listeners with one.
This is silly... both bugs are the same exact patch. Let's get this fix checked in(!) instead of playing bugzilla games.
Yeah, it is the same patch (I did explain that the patch contains other stuff). If the patch goes in as is, the other bug should be marked fixed as well. But it is really two different bugs, and I was specifically asked by PDT to make separate bugs. Two decisions need to be made. If the answer is yes on both bugs I have no reasons to complain, other than that that wrong thing is being discussed in this bug ;)
Fix in hand, reviewed and approved, so this is "rtm+".
Whiteboard: [rtm need info]Fix in hand → [rtm+]Fix in hand, reviewed and approved
Don't understand what's going on. Is there a patch associated with this bug? Also, how much of a performance improvement are we talking about? Is this user perceptable? Marking need info
Whiteboard: [rtm+]Fix in hand, reviewed and approved → [rtm+ need info]Fix in hand, reviewed and approved
Patch: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=15371 Nobody has measured performance gain. The patch also contains the proposed fix for bug 55414. It seems that frame loads(?) are messign up the search sidebar, maybe something else as well.
PDT: As Heikki alludes to with bug # 55414, internet searches are horked until this is fixed.
Re-marking as "plus" now that the information is available.
Whiteboard: [rtm+ need info]Fix in hand, reviewed and approved → [rtm+]Fix in hand, reviewed and approved
The unquantified performance enhancement is certainly [rtm-]. However, it's important that internet searching work, so marking [rtm++] for the absolute minimum set of work which fixes search.
Whiteboard: [rtm+]Fix in hand, reviewed and approved → [rtm++]Fix in hand, reviewed and approved
fixed in branch. monday will check into trunk.
updating status whiteboard to show that branch has been fixed
Whiteboard: [rtm++]Fix in hand, reviewed and approved → [rtm++]Fix in hand (fixed on branch 10/13)
Did this make it to the trunk, ever? (I bet you can get away with the whole fix, though I'm not sure that registering four additional load observers has any meaningful effect.)
Umm..what are we going to do here? This bug has been sitting at rtm++ for 8 days now.
Oops Got sidetracked. I'll check this into the tree today.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
rubberstamp vrfy from the branch perspective...
...and a rubberstamp vrfy from the trunk pov.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.