Currently we only do SaveState and RestoreState on frames. What this means is that, if we create a frame for content, destroy it (SaveState), change the content, create another frame for the content (SaveState), the changes in between are lost. This is bad. Plus, having it only on frames means that changeable elements that have display: none are neither restored on load or saved on unload.

There is a serious question here: when do we actually want to save and restore form controls? Currently we are doing it on frame destroy and create, respectively. Clearly this is not sufficient. I think it's safe to say we should save controls on document unload, probably after the unload handler. (On element destroy is not soon enough; elements are destroyed asynchronously some time after the next page is loaded--and the "next page" could be the current page. If the element is loaded) Here are some times when we could restore controls: 1. As soon as is humanly possible (as soon as the tag is read and the Content object is created). 2. Immediately after the content element is fully created (for <select> this is after </select>) 3. Immediately before document onLoad handlers fire (after document is loaded but before JS) 4. Immediately after document onLoad handlers fire (after document is loaded and user JS goes off) I believe we should restore at point (1), and here's why: I think restore needs to happen as soon as humanly possible to give the user or web developer maximum time to react to the value. If the web developer thinks the value of the textbox is "A" in an embedded <script> (and does things in his script accordingly), it should not change to "B" later on because the restore() was fired late. This is a data integrity issue. This means, for example, that with a <select> we need to give the restore command immediately upon creation of the element (even before the options) so that the <select> can use the restored values to select options immediately instead of the default values. For an <input> we pass the restore as soon as the object is created (and set the value as such); when they try to set VALUE we allow them to set the attribute, but we do not set .value--we leave that at the value it was restored to. The *only* case where this will not work well is where people are adding and subtracting radio buttons and <option>s. But in those cases we aren't working right anyway now, and without some system to validate what the options looked like on Save(), we can't possibly know if the options we're restoring to the select are the proper ones. We just know option numbers or, possibly better, option values. Comments?

The hook for calling RestoreState() should IMO be in the content sink, it should call RestoreState() as elements are created/inserted into the document. The hook for SaveState() is ::SetDocument(nsnull) in the form control classes, that will be called when we leave the page, or when an element is taken out of the document.

Heck, I didn't even think of the SetDocument(nsnull) as a hook. Hopefully this is called before the parents are removed from the document and before the child is removed from the parent. As long as that is so (I'll look into it tonight), I think it's a great place for it. The more I think about it, the absolute best way to do restore/save is to use the following rules: (A) Restore immediately when the NAME attribute is set (or whatever attribute is used to retrieve the value from presentation state) (B) However, do not restore if the value that would be restored has ever been set or retrieved. Removing the second rule would be OK, as it would make a small number of cases not work but leave the vast majority happy, and it would make Save/Restore a lot easier to code. My objections to restoring when the element is added into the document are based on this principle: The value of the control may be initialized without the web developer's knowledge (as in RestoreState) but it should not *change* without the web developer's knowledge. If the web developer uses the value or sets .value before it is restored, then he has a reasonable expectation that the value not change after that. We should write things to either eliminate or at least reduce to near-zero such cases, because web developers are probably not going to test their pages against restore. A couple of examples of what could happen. If the web developer does this: <SCRIPT> var input = document.createElement("INPUT"); input.setAttribute("name", "dynamicInput"); input.value = "this is the value I want to see when the document is created"; document.form.appendChild(input); </SCRIPT> Then it will overwrite .value on Restore(). I do not think this is what we want. If the developer does this: <SCRIPT> var input = document.createElement("INPUT"); input.setAttribute("NAME", "myInput"); input.setAttribute("VALUE", "B"); ... if(input.value == "B") { document.form.inputOnPage.value = "value is B!"; } document.form.appendChild(input); if(input.value == "B") { document.form.inputOnPage2.value = "value is B!"; } </SCRIPT> Then input.value will be different between the two inputs on restore but not on load. Using the rules above will prevent both these problems and pretty much all others that will crop up (except with dynamic <select>s, which I have written off as a lost cause). The second rule is needed in the strange case that the user sets .value before doing .setAttribute("NAME"). Another thing we need to deal with: - Is there a way we can add a directive similar to the no-cache directives, that is a no-restore directive? Restore will mess up some JavaScript-heavy pages no matter what (onLoad may depend on the initial value), so web developers should have a way to turn it off entirely. I would sort of prefer a CSS directive so the developer can turn it off control by control, but that probably *does* interfere with existing standards. Could y'all CC others who you think have a stake in Restore/SaveState()? I think we're going to end up completely defining how it will work in the future, and I'm not sure who else needs to have a say in it.

*** Bug 111446 has been marked as a duplicate of this bug. ***

*** Bug 111136 has been marked as a duplicate of this bug. ***

I really *want* to get this in by 1.0. I do not know if I will have time, however. Let us see.

Moving to Moz1.1 to get it off the Moz1.0 radar. John if you get through your nsbeta1+ bugs please add nsbeta1 to nominate it and I'll move it back to Mozilla1.0.

I have thought of a place to do this that suits us pretty well. SetDocument(). When removing the control from the document, save state. When adding it to a document, restore state. This should not take long to do. Nominating for nsbeta1. This blocks XBL form controls, which cannot save/restore state without it.

I have this totally working in my tree hooked up to SetDocument(). It has a minimal impact on pageload (save/restore code is only executed on form controls) and at the same time I cleaned up *when* we store values so that we don't waste so much memory saving values unnecessarily. This should be a highly noticeable footprint improvement in browsing sessions with lots of history. The one problem I'm running into is radio buttons. If you check the first radio button in a group but a later one has the CHECKED property, the later one will be checked and uncheck the one you already restored. I can think of no really clean, efficient way to handle this; the closest I've come to it is to store a boolean for an entire radio group that indicates whether it has been restored or not. If it has, you uncheck new radios that come into the group. We'd have to go through and unset this boolean for all radio groups when the page has finished loading, however, so that setAttribute("CHECKED") will work properly afterward. Hmm, it's possible we could set some boolean state while we're in SetDocument() that passes the same information to SetForm(). That might be optimal, I'll look into it. Another possibility is to restore radios at a later date, but I don't like that because it is inconsistent and will probably be inefficient. It could be made more efficient by storing a hash in the document of content that wants to be restored ... when you get added to the document you ask it to restore you when it has finished loading. I don't like that though, I'd rather restore early so that JS can screw around with the restored values. The footprint and performance benefits may be worth it to get this in and leave radio buttons saving/restoring in the frame. It would also sort-of unblock XBL form controls (it would make them a lot more bearable).

Posting the patch I made up tonight until I can decide what to do here. This is a work in progress; it even has printfs. The only thing that should *not* work, however, is radio button restoration.

Just trying to understand your comment in bug 128989 -- does the work in this patch lay the groundwork for later on allowing any random bound element to expose an API that will automatically add it to session history?

No sir. Only those who implement nsIFormControl. A general mechanism is still desirable, but this has lower performance impact for 1.0 and will not be hard to *change* to a general mechanism when the time comes. I am skirting the issue of having the browser do yet another QI on every element in the content tree when it creates the elements.

Erm, missed "lay the groundwork." This does in fact make it easier to implement a general mechanism for *all* content to save/restore history, but only because it provides an example. Most of the hooks for save/restore are already there for content in nsIFormControl. It will not be terribly hard to rip this code out of SetDocument() and move it into the parser when the time comes, I just don't really feel the desire to add that extra QI into the sink yet.

If we need to have a generic mechanism for doing this then that belongs in nsIContent, not in its own interface since implementing a new interface on all elements would either bloat every element instanca, or slow us down when QI:ing to this new interface if we go the tearoff route.

The thing is, most elements don't have state to save. A virtual function call is definitely less expensive than a QI though, so maybe we'll just go that route.

Right, a stubbed out virtual function costs us way less in time and memory useage than a new interface on the elements that want to [re]store state.

cool, so long as we're not making it hard for ourselves in the future :-) (regarding the specifics: this has to be extensible at the XBL level; I don't know what that means with regard to whether we use a virtual function or a QI.)

The save/restore as we are envisioning it here has to do with content. XBL Form Controls do not extend content but instead get bound to it. I am not sure if that is what you mean by "must be extensible by XBL," since I am not familiar with the breadth of our XBL usage. If you mean XBL needs a hook for save/restore, you could maybe call the XBL function from nsIContent::Save/RestoreState (assuming that is the way I go here, I'm leaning towards it).

I mean that I should be able to say, in my CSS: div { -moz-binding: url(foo.xml#bar); } ...and in foo.xml have a binding bar that hooks into session history state saving and form submission.

Hmmm ... I would say separate bug. We're hooking up to content here, and the performance implications of hooking it up to XBL bindings are nontrivial. As are other implications ... is the XBL guaranteed to be bound by the time the parser finishes parsing the element? At any rate, all we do here is allow any content object to take Save/RestoreState. If we want to eventually forward that on to XBL it can be done, but it's orthogonal.

The XBL won't be bound to the element until style is applied, which most of the time happens later than when the parser is done building an element.

> is the XBL guaranteed to be bound by the time the parser finishes parsing the > element The binding could be changed on the fly through the DOM (e.g. changing a <div> from acting like a radio button to acting like a text field). I agree that this is a separate bug. I am just making sure we all realise the eventual aim and that we are not making our life harder on the long term. :-)

Yeah, XBL is going to really be a different paradigm ... I don't think we should do Save/Restore for XBL, but rather onBoundToContent() / onUnboundFromContent(), which is the time you'd really do it in XBL form controls anyway.

This patch works for all elements. It adds DoneCreatingElement() to nsIContent and calls that for input and button in the content sink when all attributes are added ... for textarea and select, it calls DoneAddingChildren() on their respective interfaces. These are the triggers that the elements use to save. SetDocument(nsnull) is used to restore. This patch also reduces memory consumption of history in general; we were saving information when it was equal to the default for the control (when getAttribute("value") == .value) and that is a waste of time. Now only radio, checkbox and select does that. checkbox doesn't have to, but for various reasons radio and select do.

nsbeta1+. This is a blocker for XBL form controls.

Comment on attachment 74799 [details] [diff] [review] Patch r=rods

adt2 per adt triage

- In nsIContent.h: + * If you also need to determine whether the parser is the one creating your + * element (generally only CloneNode can do otherwise) then add a boolean Elements can also be created through the DOM by calling document.createElement[NS](). - In nsHTMLContentSink.cpp: - rv = NS_NewHTMLInputElement(aResult, aNodeInfo); + rv = NS_NewHTMLInputElement(aResult, aNodeInfo, PR_TRUE); You can't unconditionally pass PR_TRUE here, MakeContentObject() (which is where this change is) is called from the sink when the parser creates content objects, but it's also called from NS_CreateHTMLElement(), which is called by the HTML documents when script calls document.createElement[NS](). - In nsFrameManager.cpp, in KeyAppendSep(). This isn't your change, but I saw it in the diff context: aKey.Append(">"); Use aKey.Append('>') in stead, it's faster. - In nsHTMLInputElement::SaveState(): + // Only save if checked != defaultChecked (bug 62713) + //if (checked != defaultChecked) { Huh? Intentially commented out, or a mistake? If intentional, why even leave the commented out code and the comment there? - Also (this occures a few times in the same method): + nsAutoString value; + GetValue(value); + // XXX Should use nsAutoString above but ConvertStringLineBreaks + // requires mOwnsBuffer! + rv = nsLinebreakConverter::ConvertStringLineBreaks( + value, The XXX comment suggests that the |value| string needs to be a nsString and not an nsAutoString as you made it. Is the comment bugus? - Same thing in nsHTMLTextAreaElement Have a look at that and you'll have sr=jst

createElement(): fixed. checked still initializes properly when you do createElement() and .setAttribute("checked"), which is the best indication that this is working. checked != defaultChecked: fixed. We should always save for radio and we don't save for checkbox unless the value is different from defaultValue (this was the reason for it being commented out). Always saving for radio prevents us from thinking that one of the false radio buttons is not saved and therefore initializing its CHECKED attribute, turning off the button that was actually restored. nsLinebreakConverter / nsAutoString: comment removed. The conversion from nsString to nsAutoString must have been unconscious, but I am assured it is all right to use nsAutoString anywhere you use nsString, which I did not know when I wrote that comment so long ago. At the time I thought I would have to change the argument type to nsAString.

Fixed all this and more! I saw a few asserts and got rid of them: - NS_ENSURE_TRUE is too strong a word in nsGenericHTMLElement::GetLayoutHistoryAndKey. In anonymous content, the key returned from GenerateStateKey will be blank and rv will be NS_OK. So I made it an if() return. We don't save/restore such things anyway. I did the same with a previous NS_ENSURE_TRUE. NS_ENSURE_TRUE is kind of like assertion; if it ever *could* happen in the normal course of events, you don't want it in an ENSURE_TRUE. - There was an assertion when trying to restore image elements--of course, they weren't found in the elements map. nsHTMLInputElement::DoneCreatingElement now only tries to restore element types that can be restored. - made a pure cleanup that's been bothering the hell out of me, that is ancillary to this patch (I'll remove the change on checkin if r=/sr= so desire): - nsCOMPtr<nsIListControlFrame> listFrame(do_QueryInterface(frame)); - if (listFrame) { - if (!IsClickingInCombobox(aMouseEvent)) { - return NS_OK; - } - } else { - if (!IsClickingInCombobox(aMouseEvent)) { - return NS_OK; - } + if (!IsClickingInCombobox(aMouseEvent)) { + return NS_OK; }

Comment on attachment 76065 [details] [diff] [review] Patch v1.1 Do we want to add an assert or warning to the empty stubbed out DoneCreatingElement() methods? Either way, sr=jst

Comment on attachment 76065 [details] [diff] [review] Patch v1.1 r=rods

Comment on attachment 76065 [details] [diff] [review] Patch v1.1 a=asa (on behalf of drivers) for checkin to the 1.0 trunk

On checkin last night there was a 60ms pageload spike. Investigating.

I found the pageload regression. The problem is one that we came up against the first time I tried to move save/restore to content in bug 34297, namely that for some strange reason the form is not in the form map. At the time I put a hack in to flush the content sink and try to get the form, and it wasn't working but it looks like we put it in anyway (probably figured It Couldn't Hurt). Removing that hack (and keeping the other hack I put in at the time, which simply assumes that if it can't find the form in the list of forms, that it's the next one and simply hasn't been put in yet) fixes the Tp regression totally. And it stands to reason that's where the real problem lay here. http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&file=nsFrameManager.cpp&rev2=1.91&rev1=1.90 is where the hacks got put in.

Comment on attachment 76865 [details] [diff] [review] Pageload Regression Patch r=bryner

Comment on attachment 76865 [details] [diff] [review] Pageload Regression Patch sr=jst

Can we close this bug out now?

Fix checked in the night of 3/30.

Verfied patch checked into lxr.mozilla.org