The link toolbar as written right now is impacting both startup time and new window time because of the way it was architected. There is XUL that is always parsed, CSS that is always parsed, and about 900 lines of JS code that is always loaded and compiled at startup. The link toolbar should be an empty shell of a toolbar until the first page with links is encountered. At that time an attribute should be set on the toolbar that causes it to be bound to an XBL binding for the link toolbar that will then load in the JS and use a scoped stylesheet and anonymous content to represent the buttons on the toolbar. This will avoid virtually all of the overhead that the link toolbar currently incurs and return us to the situation we were in before this new toolbar landed.

Won't fastload more-or-less have the same effect? Btw, I've been saying for a long time that the link toolbar should be triggered from XBL on <link> tags rather than by JS code triggered in onload, but that's blocked by bug 59701 because XBL bound to an html <link> tag would be untrusted. I don't know if that's related to this bug, however - it might fix some of the issues raised here, but probably not all of them.

PS: could you give some information about exactly what degree of performance impact this is having? I can't imagine it's *that* severe - I've been running with the toolbar including autohide for months and never noticed a particular slowdown in app launch time or new windows opening, although I admit I don't open new windows often.

FastLoad is a win, but it doesn't make the fastloaded data free to load. So hyatt is right to argue that all costs (reduced by FastLoad or not) should accrue not at startup, but when you first load a page that needs the linkbar. /be

The impact is probably around 2-3% if I had to hazard a guess. That's why I'm not proposing anything drastic like a backout. This is just something to do in the future to make the feature a little cleaner.

BTW, I like the idea of the <link> triggering the showing of the toolbar, and I can tell you exactly how to do it using XBL without having to solve the security issues. Bind links that will cause the toolbar to show to some XBL that fires a custom DOM event that then bubbles up into the chrome. The toolbar (rather than listening for load events) could listen for this custom event instead and then manage the addition of the link/updating itself etc. when it gets this event).

This is how tabs in the tabbrowser update their titles. I made documents fire a custom DOMTitleChanged event that the tabbrowser listens for to update its tab titles. You can see the relevant code in tabbrowser.xml (for the listener part) and nsDocument::SetTitle (for the event firing part).

Hyatt, I don't think the feature should be backed out, but Gerv, et al.: I do think this bug should be fixed ASAP. It wasn't the last cookie I ate that made me fat. Startup is not slow due to any one cause. We need to stop adding costs to the startup critical path, and start removing costs. /be

Hyatt: That's ingenious :) Wouldn't your custom event also be available to remote code, though, so a malicious page could fire one manually? (Okay, I guess that's not actually a security hole since the event can't do anything the page couldn't do by modifying its own DOM anyway, but it still seems wrong...) Also, can events like this still propagate before the document is fully loaded or after its onunload has fired? If I get a chance (no promises! real life is busy atm) I'll take a look into the part of this bug that involves using XBL bound to <link> to populate the toolbar instead of an onload handler. I think that would take some of the costs out of the critical path (mostly on page-load rather than new-window), but nowhere near as much as Hyatt's original proposal. Should this be a separate bug? As a preliminary question that affects how I would implement it: is it true that given an XBL binding on a tag in an HTML document, events are guaranteed to happen in the following order: xbl-constructor, document.onload, document.onunload, xbl-destructor?

Stuart, no worries on the malicious page front, since the event's target could be checked to ensure that it was a valid link element. You'll have to do that to populate the toolbar anyway.

Stuart, sure, the DOMTitleChanged DOM event that i use to update tab titles fires way before the page has loaded. In fact, this will make the Link toolbar kick ass, because it will be capable of incrementally populating the toolbar, even well before the page is loaded!

Let's file a separate bug for the <link> element using XBL to fire a custom DOM event. This bug should just cover the conversion of the toolbar itself to XBL in order to lazy-load the methods/properties and the CSS and XUL.

This is behind the ~5% slowdown in page loadtimes on win98 and linux (Mac is kind of lost in the noise). I've verified the initial test results, and get the same sort of slowdown. I've also tested / backed-out+tested / reapplied+tested /backed-out-again+tested, and it is clearly this change that is the cause. Okay, I'll play bad cop: this should be backed out until the new solution is worked out (and tested). "If you keep eating cookies, you'll never see your toes".

I concur with jrgm. It's one thing to slow down new window and startup by 2-3%, but hitting page load times for 5% is unacceptable. This could be alleviated by eliminating the load listener and using custom DOM events and forcing the relevant <link> elements themselves to fire the events up to the listening toolbar.

I'm the bad mozilla.org cop (donuts don't work on me). If we've failed to stop regressions in the past, it's because their signal in jrgm's test looked like noise, until it piled up over too long an interval for anyone to play bonsai hook detective and pin cvsblame on any one checkin, or day's worth of checkins. Here we have a smoking gun. We have to stop regressing, now -- or we won't make any kind of performant 1.0, soon or ever. I have complete confidence in the interested parties being able to hack along the lines hyatt has outlined, with hyatt's help, to restore a linkbar that does not cost startup and page-load performance until you hit a page with link tags. Let's back out, get it right, and land it for 0.9.6. Gerv, what do you say? /be

See bug 103082. I performed a minimal excision, and only turned off the inclusion of the overlay for now.

Actually I recommend not even using XBL for the firing of the custom DOM event. Make it fast, don't waste time with XBL, and just fire the event from the C++ element (since we already have a class for it).

In that case it's certainly beyond my hacking abilities and I'll have to defer to someone more knowledgeable :(

I think bug 103097 would be a good place to do the page-load work since the page-load-time regression was what eventually backed us out. We'll keep this bug for just the new-window and startup improvement, per Hyatt's original submission. I've added gerv and hyatt to that bug, anyone else interested should hop on over.

Somehow I get a feeling that the Link Toolbar is being held to a higher standard than other stuff. (For example, many folks were reluctant to back out "What's Related?" despite the privacy hole.) I agree that performance is important, but it seems to me that keeping this in the tree makes it easier to contribute improvements.

Why do we need custom DOM events? Why can't we just attach a DOMNodeInserted listener to the <head> element? We need this anyway in case links are added programmatically from JS.

What if a <link> is inserted into the <body> via JS? A custom DOM event fired from the C++ implementation of the <link> node would be fired on programmatic creation too, of course.

bz: then people will learn not to create certain invalid documents by DOM manipulation. ;)I'm adding 103060 (jwbaker's bug on DOMNodeInserted) as a dependency; also, won't we need to be listening for changes in the attributes of link elements?

Listening for changes in the link elements would be a lot easier if DOMSubtreeModified mutation events were actually implemented. Bug 74218.

Do not beat the mutation event drum. Mutation events will slow down page load even more than anything else that the link toolbar is already doing. You want a custom DOM event that is specific to <link> elements and that fires only for <link> elements, so you don't waste time filtering out all the noise (or taking the global mutation event hit that occurs the moment you use a single mutation event listener).

Also, Henri, the link toolbar is absolutely not being held to a higher standard. Any feature that causes a 5% spike in page load times should be backed out. We've been strict about backing out obvious page load spikes for months.

Fine I won't beat it but we need to get the events from both <link> and <meta>.

Two thoughts about the links toolbar: 1. It should not be included in released builds, but should be made available as a package (jar file) that people can drop into their build if they want the functionality. 2. It should be reimplemented as a sidebar panel, thus avoiding the constant resizing of the content area as it is shown and hidden.

Those would have been good suggestions three years ago when the design discussion took place, but I think it's a little late for dramatic changes. In any case, equivalent functionality in the sidebar would require significantly more of the window area than the current toolbar requires. As for the separate JAR, that doesn't seem like a terribly good idea unless you also want to implement other standard HTML features in separate JARs. Perhaps we could just ship with a capability-free browser and then you can add the JARs you want to just support the <p> and <ul> elements only.

I'm not in full agreement with either point. 1) Could be said for a lot of things, but this seems like a pretty fundamental navigation tool that people might not realize they 'want' until they've had it a while! It's a matter of finally making good use of navigation data that some people have been building into their pages practically since year dot. 2) You avoid the resize by having the toolbar always-there or always-not-there. There's already a menu option for that. You might /almost/ as validly argue that the regular forward, backward, stop etc. buttons should go in the sidebar (well, that's an /interesting/ thought).

Not to mention that it's already been available as an xpi for many months but with little effect - because, of course, nobody used it which meant hardly any sites support it, which means that the benefit for everyone is minimized. This feature is only useful if it's widely adopted by websites, which will only happen if it's widely available to users. And I'd like to echo previous comments that this has been under design for 3 years and that you already *can* have it stay on or off with no content-area resizing. Also, a fix to bug 103097 as is currently under discussion would (almost) guarantee the toolbar appearing before any content ever does, which would minimize this effect even further (you should never need a reflow due to the toolbar appearing).

Regarding Simon Fraser's seccond point on 2001-10-04 11:39 I generally browse with the sidebar disabled. It would be annoying to have to have it open jsut to use this feature. At least with the toolbar I have it there ready to use when a site supports it. I wouldn't even know if a site used <link> in a sidebar, without having the sidebar open "Just incase", and that's not worth the space the sidebar takes.

Note: I think binding XBL to <link> elements is a red herring. Firing events from C++ code that bubble to JS code is a much safer and neater solution IMHO. One of the problems of XBL on <link> is that authors can override it.

This bug being assigned to me might indicate to people that I plan to fix it. This is not the case - I expect the Links toolbar community to pick up the torch here. This is one of the high priorities for turther development of this feature, so if you get this bugmail, please consider whether you are the person :-) Reassigning bug to owner and QA contact of selected component. Gerv

I'll take it

I've done a little work on this; I can't commit to anything because my time availability is very sporadic, but if I do get a chance I'll post my results in this bug. I have some preliminary code at home that's a long way from working, that would also address bug 102990. But it has some bizarre results like rendering all the page content, including the toolbar, in an approx 200x100 pixel area in the top left corner of the content area, and I never got past that behavior onto the stuff that would really deal with this bug.

I horked up a patch to fire the custom event but I don't THINK it works, because adding a handler for DOMLinkInserted to linkToolBarOverlay.js did nothing. I hope one of you sharp fellows can set me straight.

Jeffrey, did you intend to attach this to bug 103097? That's the one that requires custom events.

0.9.6 is out the door.

Given that (if I read the status report right) the Links toolbar just got turned off completely for a 3% startup perf gain, this bug just got a whole lot more important. Gerv

Did hyatt's patch from 103097 get checked in or not?

Hyatt's patch is in, because my recent patch to bug 103097 relies on it and works fine against the 0.9.6 release. If you plan on working on this bug, please incorporate the changes in that patch, because it would be silly to take a 3% new window improvement instead of a 5% pageload improvement. Gerv, I think you may be misreading the status report - the following are the parts that mention the sitenavbar: http://mozilla.org/xpapps/progress/2001-11-16.html is the url for this status report btw. "* Bill Law shortened startup by 3% by removing links toolbar" "Bill Law (law): 1. Measured "Cost of Chrome" for: a. entire status bar: removing this saved 3.7% of start up time b. links toolbar: removing this saved 2.94% of start up time (even though it isn't used). c. status text/progressmeter: removing this saved only 0.36% (so it seems much of the status bar cost is paid even though this portion of it is disabled)" Since it seems unlikely that law removed the status bar or the progress meter, and no bug numbers are quoted for removing it, I don't think it's been removed. I also doubt that it could have got r/sr for removal this quickly without any of us being aware of it until it was mentioned in a status report. Can anyone confirm whether the linktoolbar (sorry sitenavbar) is in the latest nightlies? This bug is still urgent though. If I do get any chance to work on moz this weekend, I'll work on this one rather than the other linktoolbar stuff I had been planning on doing.

Yes, it's still there. It was a test. It was only a test. We now return you to your regularly scheduled programming.

I'm working on this bug, but XBL is fighting back and currently I'm losing ;) If I ever get a patch that anywhere near works, I'll post it here. If anyone's interested I can post the very incomplete and nonfunctional patch I have so far, but I won't bother unless specifically requested because of it's horribly nonworking state.

sballard took it. obviously this is not a blocker and i'm moving it out one more stone.

Since timeless needs something likse this for his project, I played around with this and got a semi-working version. (currently it activates the link toolbar only, no population). Wish I had read this before, would have saved me some effort :) I currently use hyatt's tabbrowser.xml <link> handling code. He checks if it was a faviicon, and if not, dismissed it (return;). I simply added a method that then activates the toolbar. sballard: do you need this? We also need to make the toolbar tab aware

I think it would be better (read: cleaner) if you added your own link listener instead of trying to reuse the tabbrowser.xml one. I believe tabbrowser nor browser shouldn't have to know about the site navigation toolbar.

Progress report: I finally got a chance to work on this a little further. I now have an xblified toolbar that loads on demand (ie on the first link-enabled site) and displays the toolbar in the right circumstances, more-or-less, and activates some of the buttons. It's a thoroughly hacky job at this point: it relies heavily on jssubscript-loader, and is designed to be minimally different from the old code. Most of it needs significant reworking, quite apart from the fact that it doesn't actually work once activated. In other words, it's not at a point to even post the code here yet, let alone consider checking in. Just thought that anybody interested in this bug might be interested to know that some progress has been made on it. Still a long way to go, but this is a significant milestone, IMHO at least :)

More progress: The toolbar actually *works* in some circumstances now, although it's still a long way from working on in all circumstances. Major brokennesses are: - The fullSlowRefresh function doesn't want to work, which is required on the first page that uses the toolbar and also when switching it from "never" to any other state. Thus it doesn't work the first time you visit a link-using page in a new window, although it does approximately work after that. - Only the top-level buttons work at the moment; the menus don't. This I know how to fix, at least. When I get both of these issues fixed I'll post a patch for testing. There are also a bunch of other major issues: - The code is based on the toolbar as it stood at 0.9.6. All fixes since then need to be merged. - The code is ugly as hell, mostly because for a first pass I wanted to make minimal changes to the existing code. Once I have it working reliably, there are a lot of cleanups I want to make. - The code still relies a lot on document.getElementById("linktoolbar"). I'd like to arrange for the code to be independent of whether it is bound to an element with id of "linktoolbar" or not. Anyway, at least progress is being made towards this bug :)

This shouldn't block a "page-loading perf issues" bug. The linktoolbar pageload hit has already been addressed by bug 103097. This is a window-open and startup perf hit only - is there a meta-bug for that?

startup -> bug 7251 new window -> bug 49141

adding those to dependencies :)

my guess this is not going to make mozilla0.9.8? So mozilla0.9.9 ?

Unfortunately the way my time availability is turning out right now it seems extremely unlikely that this will even make mozilla1.0. I'm setting the TM accordingly based on what I personally will be able to do. It's been suggested that if this can't be fixed for 1.0, the linktoolbar might be backed out altogether. Nothing definite has been decided yet, but the possibility is out there. At some point this weekend I'll post the work I've begun on this. I haven't made any progress since my last status report (Comment #49) so anyone who picks this up will have a lot to do. But it might be necessary to do it if we want to keep the toolbar for mozilla1.0 :( If anyone does want to pick this up please comment in the bug or email me. I'll be available to give tips or discuss my current implementation and why it's done the way it is, and what I want to still accomplish. Perhaps some of the cleanup issues can be deferred and, once fully working, a hacky version of this could be landed on the 1.0 *branch* while we work towards a full clean version for the trunk? I don't know.

Okay, here goes with posting my work-in-progress. If anyone is interested in working on this, please be aware of the contents of comment #49 on this bug, and also post here before starting in case I can think of any pointers to give. I'm going to attach three files: The first is a diff, and the other two are new files. I've been working on this with patchmaker and so the locations for the new files are given in terms of location in the "chrome" directory of a finished build. They are content/navigator/linkToolbarUI.js (a replacement for linkToolbarOverlay.js) and content/navigator/linkToolbarBindings.xml (the XBL implementation itself). The most important thing to be aware of is that all linktoolbar fixes since 0.9.6 still need to be backported into this codebase, because the majority of the code is moved around into new files which won't automatically patch. Oh, and I'm adding choess to the cc list in a vague attempt to make sure that anyone who might possibly be interested in helping out here is at least aware of the issues. Okay, patch and new files coming up...

If this doesn't work by Wednesday someone might back out the links toolbar and that will really ruin my day.

I actually don't want to get involved in this again. Drivers don't want this for 1.0. Let them drop it for a few milliseconds in new window performance. Galeon already has a links toolbar. Why should I care.

> Drivers don't want this for 1.0. Where did you get that idea? Have there been mails I haven't seen? drivers@mozilla.org will assess any patch, on its merits, for 1.0. If you can fix the perf problems with a one-liner, the links toolbar is in. If it requires a total rewrite, it's probably out. But, in between, we can't tell until you produce a patch. The closer the patch is to 1.0, the smaller and lower-risk it has to be. So why not make one ASAP and try your luck? If it doesn't get in, fine - we check it in after we branch. Gerv

The patch applies with a bit of fuzz, but the toolbar doesn't work at all. "linkToolbarUI is undefined", sayeth STDERR.

Hmm, it worked (ish) for me last time I tested it, but that was a while ago. linkToolbarUI is (as you might guess) defined and set up in linkToolbarUI.js which is (should be) included from linkToolbarOverlay.xul. Perhaps this will give you enough of a start on debugging? One thing you could try is commenting out the fullSlowRefresh() call in the Bindings.xml <constructor> - this is theoretically needed, but when I was testing it caused problems. Without that call, the toolbar would fail to show up for the first page that used it, but work fine for the second and subsequent pages. The patch right now only supports toolbar buttons, not menus; to fix that you need to change the code that populates the variable "hooks" in LinkToolbarHandler.createItemForLinkType. This needs to recursively descend the node tree rather than just looping across the variable "nodes". I wish I could offer to provide real-time debugging assistance, but it wouldn't be fair to make any such commitment right now because I probably can't honor it. I'll certainly try to provide answers in as close to realtime as I possibly can in bugzilla, and if I ever do get any time availability in the evenings I'll make myself available on IRC. If there's any help I can give in explaining how the patch is supposed to be working, let me know either here or by email.

I've filed bug 138496 to back out the linktoolbar as a result of this bug not getting fixed for 1.0. I don't like doing this, but it seems inevitable and I think it would be wrong of me to let the linktoolbar stay in just because all of the drivers forgot about it, when I knew they wanted it backed out and didn't forget about it. If you don't like this, discuss alternatives in bug 138496 or come up with a miracle fix to this bug for the 1.0 timeframe.

Patch exists, but needs a little work. What remains to be done in this bug? Shouldn't we open separate issues for all the separate issues that remain? Can you sketch out in pseduocode what remains to be done? I strongly vote in favor of fixing this for 1.0. This is the API freeze release. We need the links toolbar as a key part of our arsenal of features that the competition doesn't have. It's Mozilla1.0+. Why isn't the target milestone set at 1.0? This is major severity.

topembed? you got to be kidding me.

Yeah. Didn't mean to include topembed. Sorry, and thanks for catching that.

It has a patch that *doesn't work*. The patch keyword is inappropriate. It has nothing to do with embedding. And Netscape would be crazy to even think about including it in anything like it's current state. The current patch needs probably a workday's worth of solid hacking to make it minimally usable (ie "as good as the current state of the code"). If you want to get this bug fixed, HELP FIX IT. Don't try to magic problems away by setting inappropriate keywords on bugs. A better solution if you really care about 1.0 would be to investigate the XPI solution. That's something that's actually achievable if a single person with moderate knowledge was willing to spend a few hours hacking at it, and a bit of politicking to get it checked in. But needless to say, throwing keywords around and yelling is much easier.

Oh, and it's mozilla1.0+ because drivers have indicated they'd like to see this fixed for mozilla1.0. That doesn't mean that somebody's magically going to turn up and actually do the work to fix it. As was made clear in comments 54 through 60, I don't have the time to do that, and I made it very clear that I hoped somebody else would step up. Nobody did. It's not up to drivers to force people to fix bugs. It's up to the *people who care about those features*. I care about this feature enough to have worked on it numerous times (I saved it from the initial backout way back in 0.9.5 by helping to eliminate the pageload perf hit when the toolbar was disabled, and wrote the fix for bug 103097 to eliminate the pageload hit the rest of the time, and I wrote the patch in this bug). Regardless, I recognize that *I have no right to complain* that it's being backed out, because I wasn't able to do enough to solve all the problems that were considered vital. So don't complain at me. And don't bitch in my bugs. Either FIX THEM, or find somebody who has the time and skills and persuade or bribe them to fix them. I'm as upset as you are, but *please*, use your righteous indignation productively or not at all, because bugs with too high a noise-to-signal ratio are almost guaranteed to get ignored.

Renominating for nsbeta1. This bug is not currently assigned. Maybe someone in Netscape could make a contribution to the bug. Admittedly, that might push the fix back to 1.0.1, but that's all right. I can't help out personally, because I do not understand how to fix the bug, and I don't have coding skills. I find it a little baffling when people ask for comments on whether a bug should be fixed or a feature backed out, and then criticize someone who says he wants the bug to be fixed.

I don't think I asked for opinions on whether fixing this bug was a good idea, either here or in bug 138496. *Obviously*, fixing the bug would be preferable to backing out the feature. What I said was: "If you don't like this, discuss alternatives in bug 138496 or come up with a miracle fix to this bug for the 1.0 timeframe." And over in bug 138496 I said: "If you have a problem with this, don't complain to me or to drivers, either fix bug 102992 or come up with the changes to make this an optional XPI." Nobody *wants* to see the toolbar backed out. Everyone would love to see this bug fixed. If that can be done, great (Netscape don't care about the linktoolbar feature, and sometimes seem to be actively opposed to it - they're unlikely to commit a programmer to fix it). You can leave the nsbeta1 nomination I guess, but having seen Netscape's reaction to several linktoolbar related bugs, I feel safe in saying it'll make no difference. Discussing realistic alternatives is fine (probably should be done in bug 138496 rather than here, though, unless the alternative involves converting the linktoolbar to XBL). Simply saying that you don't want the world to be the way the world clearly *is*, is redundant. Sorry if you interpreted my earlier statements as requesting that kind of comment. And sorry if I was harsh - I'm as pissed off that this is happening as anyone could possibly be, as I've invested quite a lot of time into the toolbar, and that's probably making me snappy. Sorry.

Is this bug dependent on bug 119087?

Nope. It will take advantage of it automatically when it's done, just as the current code does, but it can be implemented to still be a vast improvement over the current code even without it.

adt: nsbeta1-

Just wondering the latest status on this one.

The status is that someone needs to do it. No one has been found so far who's willing to.

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

The SeaMonkey team will not be devoting any resources to this. If you want to work on this bug and drive a patch into the tree please open a NEW bug.