In doing performance analysis on message display, I'm discovering that we are creating and spending a non trivial amount of time in mork opening up global history. It should be possible to create a browser XBL widget and to specify attributes for disabling session and global history. Patch coming up.
Created attachment 56686 [details] [diff] [review] the patch incl. an example change to messenger.xul
Setting to 097 this isn't necessary for 096. Here's my suggested patch to browser.xml which I submit for jag and hewitt's approval / suggestions. I added 2 attributes to the browser tag: useSessionHistory and useGlobalHistory both default to true unless someone explicitly decides to over ride them. Then in the constructor for the browser tag, we don't create the history objects unless they are enabled for this browser element. Maybe they should be called enableSessionHistory and enableGlobalHistory instead? Marking this bug as a blocker of 22960.
Status: NEW → ASSIGNED
Keywords: nsbeta1, perf
Target Milestone: --- → mozilla0.9.7
Isn't there a way to specify default values of attributes on the <content> tag? As an alternative, how about extending the browser object, and moving the default creation of global and session history into that? For backward compatibility I think we could still call that <browser>, and call the base class <browserbase> or some such. Then mail/news could extend browserbase and add its own things if needed.
It seems like over kill to me, to create a new XBL tag for mail to subclass just to avoid global & session history. It's also not clear why you draw a line at the history objects between a base and an extended binding. Why just those 2 values? Just because I want a browser without those 2 attributes? If you call the base binding <browser-base> and the extended binding used by browser: <browser>, then it's not clear how the extended binding differentiates itself from the base binding. History seems like an arbitrary line and the tag names wouldn't cleanly explain that line. Just my two cents anyway! =)
Oh, I'm sure there are other things we could split on, e.g. support for goBack, goForward, canGoBack, canGoForward, goHome, homePage, ... I guess what I'm advocating is that we create a simple type, perhaps <frame> then (I tried not to use that, but I guess I should have) for simply displaying a single page, and extend that with whatever's necessary to do the full browser thing. Mail/News wouldn't have to extend this <frame> (or whatever good name we come up with), it could just use it directly if that's all it needs.
FWIW, I think I agree with mscott. Sometimes the mighty if statement is better than yet another super/subclass. /be
Alternatively, how about just one attribute: simple="true|false"?
mscott: one of the things i really don't like about mozilla mail v. netscape4 mail is that alt-left doesn't work :-( ... but that's a bit offtopic i guess. "<frame>" is probably a bad name since html already has a meaning for it, there were plans for a <navigator> but that's too high a level to be the lowest defining history support.
Seems ok to me. Without making a base binding, we could even default the use*** values to false if the tag name is iframe or something. In other words, you can both have your cake and eat it too. :)
jag, to answer your suggestion of using a single attribute for this. That sounds like a good idea. Although calling it "simple" leaves some ambiguity as to what simple means. In this case, it means turning off both session and global history but the name of the attribute doesn't imply that. What about "useHistory"? Or do we want to intentionally make the attribute name generic like "simple" with the thought that in the future if the browser tag grows more complex, other things may get hooked into that attribute as well? Although if that is the case then in the future we may end up breaking it up anyway. But for now, if it's just these 2 attributes I wouldn't turn browser into 2 separate tags. I don't think we have critical mass for it IMHO.
"useHistory" is fine with me, but yes, that was the thought behind "simple".
Created attachment 56808 [details] [diff] [review] reduced patch (uses just one attribute useHistory)
are we okay with this solution of adding a single attribute to the brower widget? If so, I'll start the formal review process. If not, do we need to continue discussing adding a new tag?
I'm okay with this.
Comment on attachment 56808 [details] [diff] [review] reduced patch (uses just one attribute useHistory) >Index: browser.xml >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/browser.xml,v >retrieving revision 1.8 >diff -u -w -r1.8 browser.xml >--- browser.xml 2001/10/27 05:34:33 1.8 >+++ browser.xml 2001/11/07 00:19:20 >@@ -218,6 +218,21 @@ > onget="return this.webNavigation.document;" > readonly="true"/> > >+ <!-- option to disable use of history --> >+ <property name="useHistory" >+ onset="return this.setAttribute('useHistory', val);" >+ onget="return this.getAttribute('useHistory') == 'true';"/> >+ I don't think we'll want this property unless you also want to hook up code that kills session history and global history, or creates those instances. General comment: the setter should return |val| so one can do e.g. with property foo: var foo = bar.foo = createFoo(); >@@ -240,12 +255,18 @@ > // wire up session history > // XXXdwh On a dynamic skin switch, we should be checking our box object to obtain > // the session history. >- this.webNavigation.sessionHistory = Components.classes["@mozilla.org/browser/shistory;1"].createInstance(Components.interfaces.nsISHistory); > >+ this.ifSetAttribute("useHistory", true); >+ useHistory = this.getAttribute("useHistory"); I guess I'd rather turn this around and use |disableHistory| so we'll only have to pay the cost of the additional attribute in the non-default case. Patch coming up.
Jag, do you mind I submit your patch to use disableHistory instead of useHistory for reviews and super reviews? I'd like to check it in today when the tree opens along with some other message display performance work I've been working on this week.
Nope, go for it, that's why I posted it here.
Comment on attachment 56990 [details] [diff] [review] disableHistory instead of useHistory r=mscott
Attachment #56990 - Flags: review+
Comment on attachment 56990 [details] [diff] [review] disableHistory instead of useHistory sr=hewitt
Attachment #56990 - Flags: superreview+
fix checked in. thanks for the help everyone.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
QA Contact: jrgm → stephend
Whoops, I did a mass QA reassign to catch mscott's message display fixes. putting jrgm back on his bug ;-)
QA Contact: stephend → jrgm
You need to log in before you can comment on or make changes to this bug.