Closed Bug 103097 Opened 23 years ago Closed 23 years ago

UI for the HTML <link> element should be faster and enabled

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: burnus, Assigned: bugzilla)

References

Details

(Keywords: perf)

Attachments

(4 files, 7 obsolete files)

Trivial patch to possibly improve performance
(deleted), patch
gerv
: review+
asa
: superreview+
asa
: approval+
Details | Diff | Splinter Review
Fix indentation.
(deleted), patch
Details | Diff | Splinter Review
Abstraction is your friend.
(deleted), patch
jag+mozilla
: review+
jst
: superreview+
Details | Diff | Splinter Review
Patch updated per blake's comments
(deleted), patch
sballard
: review+
sballard
: superreview+
Details | Diff | Splinter Review
In bug 87428 the UI for the link element was checked in, but it got backed out because it was to slow (bug 103082). a) Workaround: Undo the patch: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=52027 b) Solution: Make it faster and then undo the patch. Concerns in bug 103082: ------------------- The links toolbar is causing a 2-3% impact on startup/new window and a 5% slowdown on page load times. The page load hit in particular is unacceptable. This bug is tracking the disabling and removal of the toolbar from navigator.xul until such time as the feature can be demonstrated not to slow page load times. ------------------- For other outstanding bugs, see meta bug 103053.
Blocks: 103053
Adding hyatt and gerv (and self). Per discussion in bug 102992, the way to go on this seems to be to make the link toolbar be triggered by a custom event fired by the Link element whenever one is encountered in a document. Then there's no hit at all if the page contains no links, and also the toolbar can begin populating immediately rather than only once the page has fully loaded. If Hyatt (or someone else) can produce the patch to fire an event when <link> is constructed, destructed or changed (is that right?), then I'll take a look into making the necessary changes on the frontend. Btw, how much overhead on page load is acceptable? My current design thoughts would require something like this on pageload: this.flag = true if (this.a != this.b) { dosomething(); } where this.a *is* equal to this.b in the common case (no links) and both are booleans. Would that still be too much pageload overhead?
Under what circumstances will a <link> elt be added to the toolbar. Let me know what the attr values etc. need to be and I will give you a patch to fire the custom event only in those cases. Also, I still don't see why you need a load listener at all. Could you clarify what you need it for? I do see that you would need an unload listener, since you want to flush the links bar on an unload, but I don't really see why you need a load listener.
The load listener is needed because we want to keep the toolbar around until the next page (or at least it's <head> element) has loaded, in case it has links too. It would be very bad UI-wise for the toolbar to flash out of existence and then back in again. So my plan is to handle it as follows: on link create: - show the toolbar if it's not already shown - add the link into the toolbar on page load: - hide the toolbar if there are no links in it and it's shown - set a flag which I'll use below on page unload: - clear all the links out of the toolbar, if any, but don't hide it - clear the flag that was set on page load on link delete: - IF the flag is set (ie we're between load and unload): - remove the link from the toolbar - hide the toolbar if it's now empty on link modify: - change the link in the toolbar Note that the link delete and link modify cases are harder than the others because in the current implementation it's hard (I think) to go from a link to it's associated toolbar item. Also they don't need to be done right away because they're additional functionality over the current implementation. But even the minimal case needs link create, load, and unload. Note also that both load and unload can have all their functionality except setting the flag protected by if (fastbooleancondition) {...} so they really should be fast to invoke.
Making the <head> fire a custom event that let you know it had finished loading would enable you to hide the toolbar much more quickly without having to hook into the onload (which could fire much much later and cause you to keep the toolbar open for way longer than you need to).
Point, go for it :) As far as which variations on Link actually correspond to toolbar items, take a look at http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/linkToolbarHandler.js#197 You can use a C++ified version of this to determine which links to load.
Let's just get the event for any kind of <link> or <meta http-equiv="Link">. We don't want to have to go back and modify the event firing code when we decide we want to support alternate stylesheets on the link toolbar (bug 103062).
I'm not sure I agree: A lot of documents have stylesheets attached through <link> that don't use it for anything else. We don't want to add too much (if any) overhead for these documents. Filtering them out below the C++/java boundary is probably a good thing.
Yeah, I like what Stuart said about just copying his JS code into the C++.
(just a niggle - it's not my code, I can't take credit: all I wrote was the autohide feature. I can't remember whose code it actually is though - about a half dozen people contributed to it over 3 years)
This isn't the biggy, by any means, but it might give a significant speedup by dropping out of the potentially expensive doClear() code if there's nothing to clear. No promises, but it can't hurt.
Comment on attachment 52093 [details] [diff] [review] Trivial patch to possibly improve performance r=gerv. Gerv
Attachment #52093 - Flags: review+
sr=hyatt
*** Bug 103223 has been marked as a duplicate of this bug. ***
pasting info from 103233 (just duped to this bug) from 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.
I un-duped bug 103223: This bug is already pushing the limits of how many separate lines of attack should be in one bug (mostly my fault for adding a patch that wasn't related to the main plan of attack, sorry). Let's keep this one working on the custom-dom-events approach and getting the toolbar enabled by default, and leave bug 103223 for the specific issue therein (which is a dependency of this bug, probably)
Depends on: 103223
No longer depends on: 103223
Comment on attachment 52093 [details] [diff] [review] Trivial patch to possibly improve performance (adding hyatt's sr to patch status) a=asa (on behalf of drivers) for checkin to 0.9.5.
Attachment #52093 - Flags: superreview+
Attachment #52093 - Flags: approval+
Attachment 52093 [details] [diff] checked in. This bug has good perf info in it, so I'm leaving it open for now. Gerv
I agree with leaving the bug open until both of the following happen: 1) The main plan of attack as described in this bug is implemented - that is, use custom events fired by C++ code from Hyatt to trigger the toolbar, removing the need to trawl for HEAD, and 2) The sitenavbar is turned to either "on" or "autoshow" by default on the trunk.
Hyatt, I thought of a problem with just using a custom event from <head> rather than the standard load event: What about documents (like images and text/plain) that don't have a <head>? We can either add <head> to their DOMs or use both that and onload, at a slight performance penalty to pages that do have heads.
hyatt, one comment on the c++-zation of the conditions on Links. The code in the JS is: if (!this.rel) return true; if (this.isStylesheet()) return true; if (this.isIcon()) return true; The C++ version should be more like: if (!this.rel && !this.rev) return; if (this.isStylesheet()) return; if (this.isIcon()) return; If you look in the JS, the constructor for that object actually sets the "rel" attribute based on the "rev" attribute. Let's not break <link rev="made"> here. :)
--> XP Apps
Assignee: mpt → pchen
Component: User Interface Design → XP Apps
Depends on: 102992, 103223
QA Contact: zach → sairuh
Blocks: 103417
Handing this to sballard for now.
Assignee: pchen → sballard
QA Contact: sairuh → claudius
Hyatt, are you still planning on providing the patch to produce events on link element creation and at the end of the head element, per your comment on 2001-10-04? If so, do you have any ETA for this? (I promised in a newsgroup posting that I would produce a patch for this bug within 24 hours of getting a build with such a patch - I'd love to get the opportunity to see if I can fulfil that promise :) ) If you aren't planning on doing this, do you know anyone else who might have the ability and inclination to do this? (I don't know C++ and I can only just get Mozilla to compile ;) )
Yeah, I'll try to get to this tonight. I need this for my icon support using the <link> tag (I need all the same events, like knowing when the <head> has loaded, knowing when a <link rel=icon> has been added, etc.)
See also patch attached to bug 102992
Attachment #56690 - Attachment is obsolete: true
Attached patch Fix indentation. (deleted) — — Splinter Review
Attached patch Abstraction is your friend. (deleted) — — Splinter Review
Comment on attachment 56694 [details] [diff] [review] Abstraction is your friend. sr=jst You should add a prototype for CreateAndDispatchEvent() tho, IIRC the mac doesn't like calling functions it doesn't know anything about.
Attachment #56694 - Flags: superreview+
Duh, CreateAndDispatchEvent() is in the class decl, i.e. no proto needed.
Comment on attachment 56694 [details] [diff] [review] Abstraction is your friend. r=jag
Attachment #56694 - Flags: review+
Checked in. The events are called "DOMLinkAdded" and "DOMLinkRemoved". They fire when a <link> element with a non-empty rel attribute other than "stylesheet" is added/removed from a doc.
the events should also be fired if rel= and rev= change.
I think we can make do with changes of the rel attribute, any UI we'd create for the rev attribute could be lazily loaded.
Hyatt, Thanks for the patch! :) I think there are still a few more events needed, however: - This patch doesn't seem to cover the "end of <head>" event. - We need to get an event for <link rev="made"> which means you'll need to check for non-empty "rev" as well as non-empty "rel" (your call on whether to check specifically for "made" or just pass up all "rev" values - I can imagine possibly displaying other types in the future, too: rev="next" <==> rel="previous", etc). - Long-term, we'll need "link changed" events too (at least when rev or rel change).
choess: If by "lazily loaded" you mean "you won't see it unless it's there when the document loads", I suppose we *could* do that, but why bother? It's not like it slows things down much to check the "rev" value as well as "rel". This is C++ code, after all, and it won't even be called unless the rev attribute actually gets modified. It's not exactly performance-critical code :)
*** Bug 110987 has been marked as a duplicate of this bug. ***
Attached patch Preliminary patch to use DOMLinkAdded event (obsolete) (deleted) — — Splinter Review
This is a work-in-progress patch which comes part way to implementing the performance improvement discussed in this bug. It implements: - Use DOMLinkAdded to trigger toolbar creation. - No pageload perf hit *at all* if there are no links on the page (the load and unload handlers only get added when the toolbar is visible) - Future-proofed: will behave correctly when DOMHeadLoaded event is added for when the <head> element finishes loading. This is necessary to get the full effect of the desired behavior. The following still need to be done: - Fix regression: The current patch causes the toolbar not to activate if it is switched off and then on, because it no longer has a way to scan the document for links. I need to add something like this back in just for when the toolbar is activated from the menu. - Use DOMLinkRemoved rather than unload to clear the toolbar, so that links removed by the DOM correctly disappear. - The "decorator" pattern appears to mean that if the href of a link is changed by the DOM, the new one won't be honored. Similarly for the title of the link. This should be fixed. - It'd be nice to integrate something to work correctly with tabs into this patch. - Still need the HeadLoaded event to get the full effect of the patch, and the LinkChanged event to cover all possible DOM manipulations. The patch can be checked in without all of these, but at least the first two or three should be done. I'm continuing to work on this tonight.
Attachment #59016 - Flags: needs-work+
Attached patch Updated patch without regression (obsolete) (deleted) — — Splinter Review
This patch fixes the regression from the previous patch and also a couple of strict js warnings. I may not be able to work on this any more for a while, but this patch is in a state that's ready for r/sr, I hope. I'd also like to see whether this patch actually causes zero pageload hit - it should be zero hit on pages that don't use the toolbar, regardless of the view->sitenavbar setting (ie even when the toolbar is on autoshow). It might also be a slightly smaller hit even on pages that *do* use the toolbar, but I can't be 100% certain of that. Note also that this patch is a net reduction of 32 lines of code, which should be a tiny cut in the 3% window.open hit that the toolbar causes (although bug 102992 needs to be fixed to make any more substantial dents in that 3%). In conclusion - looking for r/sr on this. Also looking for Hyatt (or someone) to add the DOMHeadLoaded event to get the full benefit, but that can be done in a later checkin.
Attachment #59016 - Attachment is obsolete: true
adding jrgm to cc. jrgm, do you think you could run some pageload tests with this patch? Specifically, I want to make sure that: 1) This patch doesn't cause any performance regression over the current state if the "site navigation bar" is set to "hide always" in both cases. 2) With this patch, having the site nav bar set to "show only as needed" or "show always" is just as fast as "hide always" IF the pages being tested do not cause the toolbar to be displayed. 3) The performance on pages which *do* cause the toolbar to be displayed is not significantly worse with this patch than without it. Numbers 1 and 2 are essential, number 3 isn't unless the effect is really bad. Ideally I'd like to see no hit on the daily pageload graphs even with the toolbar enabled, but that depends on how many of the pages in that test use the toolbar.
I'll try to get some tests Sat or Sunday [but I may not get to it till Monday, cause I think I've overpromised at this point :-]. Just to be clear: I need to apply http://bugzilla.mozilla.org/attachment.cgi?id=59017 to the current trunk (i.e., I don't need anything other than that).
Correct - and thanks! I made the patch against 0.9.6 but I don't think any of the affected files have changed since. If it doesn't apply cleanly let me know in the bug.
I applied the patch to r=, some comments: 1) + dump("added - " + element); bad, evil, keep the console clean and lean 2)+ if (event.originalTarget != window._content.document) If I open a tab with a bugzilla query, close that tab, I get a js error: Error: window._content has no properties Source File: chrome://navigator/content/linkToolbarOverlay.js Line: 87 You probably want to make sure window._content exists :)
Grr. tabs suck. What's it doing firing an event without window._content set up in the first place? I'll try to take a look at that tonight but it might not happen until the weekend - things are pretty busy at home atm. Thanks for looking at the patch.
Attached patch Patch addressing doron's concerns (obsolete) (deleted) — — Splinter Review
This is the same as the previous patch, except for: 1) Remove the dump statement - oops! 2) Add a check for whether window._content is defined at all before comparing against it in the unload handler. Although I don't quite understand how you got the error you did, this theoretically should avoid it. Note that the patch still doesn't cause the link toolbar to work anywhere near correctly with tabs - that's outside the scope of this bug (it's bug 102905), and I plan on working on it after I get xblification done. This patch just hopefully avoids an actual js error from tabs.
Attachment #59017 - Attachment is obsolete: true
Adding choess in a desperate bid to get r/sr on this. Any of Gerv, choess, hyatt, doron or tim should be able to review this - or anybody else who wants to try. pweddy pwease?
Oh, and jrgm, any news on pageload tests with this patch?
Attached patch Patch addressing fabian's partial review (obsolete) (deleted) — — Splinter Review
This patch: - changes window._content.document to getBrowser().contentDocument throughout - with the side-effect that getBrowser() should always be defined so we can avoid the null check. - Adds a comment on why deactivate() doesn't check toolbarActive but does check hasItems. - Sets a variable length to optimize the loop in fullSlowRefresh(). - Uses HTMLLinkElement instead of Component.interfaces... Fabian, anything else? (or anyone else wanna review? ;) )
Attachment #59647 - Attachment is obsolete: true
Comment on attachment 59976 [details] [diff] [review] Patch addressing fabian's partial review r=fabian with the comments on IRC
Attachment #59976 - Flags: review+
Attached patch Patch addressing fabian's remaining issue (obsolete) (deleted) — — Splinter Review
This patch replaces list.item(i) with list[i] for perf reasons. It is otherwise identical to the previous patch.
Attachment #59976 - Attachment is obsolete: true
Comment on attachment 60570 [details] [diff] [review] Patch addressing fabian's remaining issue r=fabian (carried over from previous patch - this one just addresses his remaining issue)
Attachment #60570 - Flags: review+
Updating status to indicate that I'm working on this bug; also setting TM to 0.9.7 because the patch is ready and just needs perf testing and sr. Hopefully by getting it onto the 0.9.7 radar it won't be just me trying to chase it up :)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
As far as I know this patch just needs sr= and a= to get checked in, but I don't have time to go chasing these up right now - my day job is really hectic. (Perf testing would be nice too, but doesn't seem to be going to happen - and since the toolbar is turned off by default it doesn't matter too much. There's no way that this patch will impact performance when the toolbar is turned off, except for a possible minute speedup of window-open due to reduction in code size). If anyone is willing to go and chase up sr= and a= for me in time for 0.9.7, I'd be most grateful; if not, I'll triage this back to 0.9.8 later today or tomorrow.
=> 0.9.8, sorry (if anyone wants to pick this up and run with it for 0.9.7 they're still welcome to, of course, but this is an acknowledgement that I personally won't be able to)
Target Milestone: mozilla0.9.7 → mozilla0.9.8
*** Bug 102886 has been marked as a duplicate of this bug. ***
*** Bug 110438 has been marked as a duplicate of this bug. ***
Question for reviewy-type people: The current patch has a fullSlowRefresh method which is vaguely equivalent to the old patch's doRefresh method. The main difference, apart from details about how the individual links are actually handled, is that the new method (fullSlowRefresh) calls document.getElementsByTagName("link"), where the old method (doRefresh) identified the head element and then scanned for links only within that. Obviously the new method is slower, but this is mitigated by the fact that it now only gets called when the toolbar is first enabled rather than on every pageload. The new method also will catch <link> elements found outside the <head>, in case the document's html is bad. The question is whether this increase in coverage is worth it. It seems likely that fullSlowRefresh will, at least short term, have to be called when tabs are switched (bug 102905). It will also probably be called when the first link-enabled page is encountered, once the toolbar has been converted to xbl (bug 102992). So perhaps it should be optimized (to the extent that the previous code was) after all? Are there really any realistic cases where a <link> might be encountered outside the <head>? Is it acceptable that, in such cases, the link might appear in the toolbar initially but then disappear later (if, say, you switch tabs and then switch back)? Is it acceptable that the link might not appear at all if you turn the toolbar on while viewing that page, but then show up when you hit reload? Okay, well that's my brain dump. My instinct now is suggesting that I *should* go with the old optimized but lower-coverage version, which is the opposite of what my instinct said when I wrote the patch. I'll sleep on it, and perhaps provide an updated patch - but any suggestions would be much appreciated.
<warning I am not a reviewer, this is just my own observation and opinion> Mozilla currently will cause all <link> elements to go into the first and only <head> element in the DOM tree when parsed via the HTML parser, but when it goes through the xml parser (as xhtml) the <link> elements appear where they are declared (and if one declares 2 <head> elements you get 2 in the DOM as well). Personally I don't think that <link>'s outside of the first <head> elements should be supported in this case as the w3 html standard doesn't allow it, and if it speeds it, up all the better. OTOH mozilla currently does not seem to have any restriction on elements that should only work when contained in the <head> element xhtml when parsed via the xml parser (or html parser for that matter as it stuffs everything into the head.). Questions: Does the site navigation toolbar work in xhtml? Does this bug depend on 102992 or is it the other way round (or is there no depend/block relationship at all)? The latest patch supersedes hyatt's patch right?
Keywords: mozilla0.9.8, patch
Answers to your questions: 1) I haven't specifically tested with xhtml but I know of no reason why it shouldn't work; the DOM functions used are the same in both cases, after all. 2) There's no direct depend/block relationship with bug 102992 (convert link toolbar to xbl) - that bug primarily applies to startup and new-window time, while this bug covers pageload time. However, both bugs do operate on the same code, so much of the code touched by this patch will also be touched by the patch to 102992, when it appears (xbl is giving me problems at the moment). I think this patch is worth checking in in the meantime, because it gives an improvement *now* and the *logic* of these changes will still need to be incorporated into bug 102992 when that patch is written. 3) Hyatt's patch was checked in some time ago - my patch depends on it. His patch was at the C++ level and provides the necessary event-firing; my patch consumes that event and acts accordingly. For the full effect of this patch, we also need another C++ level patch - one that fires when the </head> tag is encountered (explicitly or implicitly). But this patch works fine without it and will simply improve its behavior once that event gets fired. As far as the scanning for links is concerned, I think I agree that non-standard xhtml (shouldn't that be an oxymoron?) is a small enough set of pages that I won't worry about it. I'll produce an updated patch with fullSlowRefresh optimized the way doRefresh used to be.
Attached patch Patch with fullSlowRefresh optimized as before (obsolete) (deleted) — — Splinter Review
This patch optimizes fullSlowRefresh to scan for the <head> and only scan for <link> elements inside that. This should mean that, even though it's still accurately described as "slow" compared to the usual procedure of getting events, it's okay to call this semi-frequently (I expect to be using this on tab-switch, for example). This patch also reverts the change from HTMLLinkElement back to Components.interfaces.nsIDOMHTMLLinkElement, because apparently HTMLLinkElement (while it appeared to work) was actually allowing non-link elements through, which would then give a js warning on the .href check below. I hope that, since the updated version of fullSlowRefresh is pulled practically verbatim from old code that was in the tree, the review from fabian still stands. I'm still hoping for sr on this so that it can be checked in.
Attachment #60570 - Attachment is obsolete: true
Attachment #63018 - Flags: review+
*** Bug 104074 has been marked as a duplicate of this bug. ***
Keywords: perf
Hyatt, are you still planning on writing the DOMHeadLoaded (or whatever) patch? Is there a separate bug for that or will the patch be appearing in this bug?
Updated per blake's comments - specifically, all the guard-ifs are now combined into one big humungous if. Also removed a comment that's no longer pertinent with this patch.
Attachment #63018 - Attachment is obsolete: true
Comment on attachment 63989 [details] [diff] [review] Patch updated per blake's comments got sr=blake from irc
Attachment #63989 - Flags: superreview+
Attachment #63989 - Flags: review+
Moving to the correct component, and reassigning to the owner of that component to do the checkin.
Assignee: sballard → blaker
Status: ASSIGNED → NEW
Component: XP Apps → XP Apps: GUI Features
QA Contact: claudius → sairuh
The latest patch (attachment 63989 [details] [diff] [review]) has been checked in. Checking in linkToolbarHandler.js; /cvsroot/mozilla/xpfe/browser/resources/content/linkToolbarHandler.js,v <-- linkToolbarHandler.js new revision: 1.3; previous revision: 1.2 done Checking in linkToolbarItem.js; /cvsroot/mozilla/xpfe/browser/resources/content/linkToolbarItem.js,v <-- linkToolbarItem.js new revision: 1.3; previous revision: 1.2 done Checking in linkToolbarOverlay.js; /cvsroot/mozilla/xpfe/browser/resources/content/linkToolbarOverlay.js,v <-- linkToolbarOverlay.js new revision: 1.8; previous revision: 1.7 done
QA Contact: sairuh → claudius
Leaving this bug open to address two remaining issues: 1) We need a DOMHeadLoaded event. 2) The toolbar should be enabled by default. Number 2 could perhaps be moved to a separate bug, but number 1 definitely belongs here. The patch that was checked in will take advantage of the DOMHeadLoaded event if it's fired.
QA Contact: claudius → sairuh
QA Contact: sairuh → claudius
This bug is big enough as it is. I filed bugs 119087 and 119088 for those two issues. Let's track them separately and mark this bug fixed.
Blocks: 119088
Depends on: 119087
I agree. Marking fixed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Blocks: 71668
Product: Core → Mozilla Application Suite
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: