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

RESOLVED FIXED in mozilla0.9.7

Status

SeaMonkey
UI Design
RESOLVED FIXED
17 years ago
13 years ago

People

(Reporter: Scott MacGregor, Assigned: Scott MacGregor)

Tracking

({perf})

Trunk
mozilla0.9.7
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
Created attachment 56686 [details] [diff] [review]
the patch incl. an example change to messenger.xul
(Assignee)

Comment 2

17 years ago
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.
Blocks: 22960
Status: NEW → ASSIGNED
Keywords: nsbeta1, perf
Target Milestone: --- → mozilla0.9.7

Comment 3

17 years ago
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.
(Assignee)

Comment 4

17 years ago
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

17 years ago
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

Comment 7

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

Comment 8

17 years ago
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.
QA Contact: sairuh → jrgm

Comment 9

17 years ago
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. :)


(Assignee)

Comment 10

17 years ago
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

17 years ago
"useHistory" is fine with me, but yes, that was the thought behind "simple".
(Assignee)

Comment 12

17 years ago
Created attachment 56808 [details] [diff] [review]
reduced patch (uses just one attribute useHistory)
(Assignee)

Updated

17 years ago
Attachment #56686 - Attachment is obsolete: true

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Comment 13

17 years ago
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

17 years ago
I'm okay with this.

Comment 15

17 years ago
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

17 years ago
Created attachment 56990 [details] [diff] [review]
disableHistory instead of useHistory
(Assignee)

Comment 17

17 years ago
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

17 years ago
Nope, go for it, that's why I posted it here.
(Assignee)

Comment 19

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

r=mscott
Attachment #56990 - Flags: review+
(Assignee)

Updated

17 years ago
Attachment #56808 - Attachment is obsolete: true

Comment 20

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

sr=hewitt
Attachment #56990 - Flags: superreview+
(Assignee)

Comment 21

17 years ago
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
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.