Closed
Bug 108309
Opened 23 years ago
Closed 23 years ago
[PATCH]Need SaveState/RestoreState equivalent for content
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: john, Assigned: john)
References
Details
(Keywords: memory-footprint, perf, Whiteboard: [adt2])
Attachments
(2 files, 2 obsolete files)
77.73 KB,
patch
|
rods
:
review+
jst
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
947 bytes,
patch
|
bryner
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•23 years ago
|
||
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?
Comment 2•23 years ago
|
||
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.
Assignee | ||
Comment 3•23 years ago
|
||
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.
Assignee | ||
Comment 4•23 years ago
|
||
*** Bug 111446 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 5•23 years ago
|
||
*** Bug 111136 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 6•23 years ago
|
||
I really *want* to get this in by 1.0. I do not know if I will have time,
however. Let us see.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
Updated•23 years ago
|
Keywords: mozilla1.0,
perf
Comment 7•23 years ago
|
||
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.
Target Milestone: mozilla1.0 → mozilla1.1
Assignee | ||
Comment 8•23 years ago
|
||
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.
Assignee | ||
Comment 9•23 years ago
|
||
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).
Keywords: footprint
Assignee | ||
Comment 10•23 years ago
|
||
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.
Comment 11•23 years ago
|
||
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?
Assignee | ||
Comment 12•23 years ago
|
||
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.
Assignee | ||
Comment 13•23 years ago
|
||
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.
Comment 14•23 years ago
|
||
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.
Assignee | ||
Comment 15•23 years ago
|
||
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.
Comment 16•23 years ago
|
||
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.
Comment 17•23 years ago
|
||
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.)
Assignee | ||
Comment 18•23 years ago
|
||
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).
Comment 19•23 years ago
|
||
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.
Assignee | ||
Comment 20•23 years ago
|
||
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.
Comment 21•23 years ago
|
||
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.
Comment 22•23 years ago
|
||
> 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. :-)
Assignee | ||
Comment 23•23 years ago
|
||
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.
Assignee | ||
Comment 24•23 years ago
|
||
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.
Attachment #73490 -
Attachment is obsolete: true
Comment 25•23 years ago
|
||
nsbeta1+. This is a blocker for XBL form controls.
Updated•23 years ago
|
Target Milestone: mozilla1.1alpha → mozilla1.0
Updated•23 years ago
|
Summary: Need SaveState/RestoreState equivalent for content → [PATCH]Need SaveState/RestoreState equivalent for content
Comment 26•23 years ago
|
||
Comment on attachment 74799 [details] [diff] [review]
Patch
r=rods
Attachment #74799 -
Flags: review+
Comment 28•23 years ago
|
||
- 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
Assignee | ||
Comment 29•23 years ago
|
||
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.
Assignee | ||
Comment 30•23 years ago
|
||
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;
}
Attachment #74799 -
Attachment is obsolete: true
Comment 31•23 years ago
|
||
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
Attachment #76065 -
Flags: superreview+
Comment 32•23 years ago
|
||
Comment on attachment 76065 [details] [diff] [review]
Patch v1.1
r=rods
Attachment #76065 -
Flags: review+
Comment 33•23 years ago
|
||
Comment on attachment 76065 [details] [diff] [review]
Patch v1.1
a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76065 -
Flags: approval+
Assignee | ||
Comment 34•23 years ago
|
||
On checkin last night there was a 60ms pageload spike. Investigating.
Assignee | ||
Comment 35•23 years ago
|
||
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 36•23 years ago
|
||
Comment on attachment 76865 [details] [diff] [review]
Pageload Regression Patch
r=bryner
Attachment #76865 -
Flags: review+
Comment 37•23 years ago
|
||
Comment on attachment 76865 [details] [diff] [review]
Pageload Regression Patch
sr=jst
Attachment #76865 -
Flags: superreview+
Comment 38•23 years ago
|
||
Can we close this bug out now?
Assignee | ||
Comment 39•23 years ago
|
||
Fix checked in the night of 3/30.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•23 years ago
|
QA Contact: madhur → tpreston
You need to log in
before you can comment on or make changes to this bug.
Description
•