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)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: burnus, Assigned: bugzilla)

References

Details

(Keywords: perf)

Attachments

(4 files, 7 obsolete files)

334 bytes, patch
gerv
: review+
Details | Diff | Splinter Review
2.44 KB, patch
Details | Diff | Splinter Review
2.31 KB, patch
jag+mozilla
: review+
Details | Diff | Splinter Review
10.40 KB, 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.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. ***
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) — 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.
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?
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+
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.
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: