108635 - Should be possible to create a browser XBL widget w/o session & global history

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Scott MacGregor]

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.

Comment 1

[Scott MacGregor]

Attachment: the patch incl. an example change to messenger.xul

Comment 2

[Scott MacGregor]

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.

Comment 3

[jag (Peter Annema)]

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.

Comment 4

[Scott MacGregor]

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! =)

Comment 5

[jag (Peter Annema)]

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.

Comment 6

[Brendan Eich [:brendan]]

FWIW, I think I agree with mscott.  Sometimes the mighty if statement is better
than yet another super/subclass.

/be

Comment 7

[jag (Peter Annema)]

Alternatively, how about just one attribute: simple="true|false"?

Comment 8

[timeless]

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.

Comment 9

[David Hyatt]

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. :)

Comment 10

[Scott MacGregor]

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.

Comment 11

[jag (Peter Annema)]

"useHistory" is fine with me, but yes, that was the thought behind "simple".

Comment 12

[Scott MacGregor]

Attachment: reduced patch (uses just one attribute useHistory)

Comment 13

[Scott MacGregor]

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?

Comment 14

[jag (Peter Annema)]

I'm okay with this.

Comment 15

[jag (Peter Annema)]

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.

Comment 16

[jag (Peter Annema)]

Attachment: disableHistory instead of useHistory

Comment 17

[Scott MacGregor]

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. 

Comment 18

[jag (Peter Annema)]

Nope, go for it, that's why I posted it here.

Comment 19

[Scott MacGregor]

Comment on attachment 56990 [details] [diff] [review]
disableHistory instead of useHistory

r=mscott

Comment 20

[Joe Hewitt (gone)]

Comment on attachment 56990 [details] [diff] [review]
disableHistory instead of useHistory

sr=hewitt

Comment 21

[Scott MacGregor]

fix checked in. thanks for the help everyone.

Comment 22

[[e-mail] (gone - use [e-mail] instead)]

Whoops, I did a mass QA reassign to catch mscott's message display fixes. 
putting jrgm back on his bug ;-)