166636 - Move Save/Restore Out of Layout

Status: NEW

(Core :: Layout: Form Controls, defect)

Description

[John Keiser (jkeiser)]

Save/Restore State (Layout History State) has a number of problems that need to
be addressed:

- there are two mechanisms for triggering save/restore, content and layout
(maintainability)
- it has a hashtable of hashtables to store state when it only needs one
hashtable (bloat)
- layout mechanism for triggering save/restore iterates over the entire tree to
see who wants to get saved/restored.  (Pageload, though it probably doesn't show
up on typical pageload tests which don't trigger save/restore.)

If we use only one mechanism (content) for triggering save/restore, we should
deal with all these problems (the pageload problem will only be lessened a bit,
but it will be lessened).

Comment 1

[John Keiser (jkeiser)]

I have been thinking about a general mechanism to do this.  It seems to me that
scrollframe stuff does in fact belong in layout, so you need to have frames able
to restore.  This does not, however, mean we need two different mechanisms and
times to save and restore.

Here are the goals I see:
- centralize code (we have two mechanisms now and the code is scattered all over)
- reduce tree iteration (we currently call both save and restore for every frame
in the entire tree just to handle scrollbars)
- reduce bloat (we store an unnecessary hash-per-element)
- don't restore *quite* so early (we restore immediately on element creation
when many things aren't yet flushed)

The new idea centers around an nsSaveRestoreManager class, in content, that
handles the timing of save/restore for content and of save for frames, and which
holds the actual hash of data.  Here is the general idea now:

1. Make restore state happen as an nsIDocumentObserver when ContentAppended /
ContentInserted occur rather than currently where we do it immediately on
element creation (this fixes bugs such as bug 147322 and bug 139568).  This
observer would iterate over the elements and call nsIContent->RestoreState() on
them (it could check their tagnames, as we do currently, but the benefit of that
in that instance is dubious--we will probably have a pageload win without it
given that we are removing the save / restore calls for all frames and it makes
things harder to maintain in the future--i.e. to add state for other elements).

2. Make save state happen when ContentRemoved occurs.

3. Move state key generation out of nsFrameManager into this new observer
(probably a service called nsSaveRestoreManager).  Remove CaptureFrameState and
friends altogether.

4. Allow frames to register themselves against nsSaveRestoreManager for
save/restore (this will happen, for example, when nsStateRestoreObserver hits).
 Specifically upon creation, scroll frame will restore itself (calling into
nsStateRestoreObserver and asking for state) and register itself for save with
RegisterSaveableFrame().  When the page is unloaded, nsSaveRestoreManager will
call saveable->Save().  Given that only scrollframes save and restore state at
this point, the inevitable array of observers will not be hard to maintain.

5. Have content elements register themselves for both save and restore the way
frames do; this should give us a very real pageload win, removing the last tree
iteration we would have to do.  If there are a lot of form controls, however,
there will be a big redundant array of them sitting in nsSaveRestoreManager
during the life of the page.  I would bet money that we will still have a bloat
win overall for this patch since we're getting rid of those hashtables.  I am a
bit torn over this one, but I think overall that it is a good idea.

6. Call nsSaveRestoreManager::DocumentUnloaded() or some such when it is time to
save state.  Probably an observer will do here, but I'm not sure which one I
need yet.  Something like onUnload would be ideal.  Maybe having
nsSaveRestoreManager itself capture onUnload would be the right way to do it.  I
would very much like to remove all semblance of save and restore state out of
both DocShell and CSSFrameConstructor and use standard mechanisms instead.

7. Store state for an element as an nsISupports rather than as a hashtable.  Let
elements create and use their own implementations of save/restore state.  Select
already does this, and we would save many many bytes in most cases.  Most other
elements only save a single thing.  This saves us the size of a *populated*
nsHashtable for each element.

8. Throw a "-moz-restore" event when a restoration changes the value on any
element.  Or some event.  We really need to do this, I'm tired of half-assed
solutions that make it impossible for the developer to tell what we're doing
with his page.  This would at least allow the developer to catch restores and
ensure his page's data integrity.  We could have a "has_restore_handlers" flag
on the document or some such so that we avoid the overhead of throwing this most
of the time.  (Note that we don't need to have this event be cancellable because
the developer can already use autocomplete="off" to disable state save/restore.)

Thoughts?

Comment 2

[Boris Zbarsky [:bzbarsky]]

A few things to consider:

1) Toggling display of something scrollable should persist scroll position (eg a
   div with overflow:scroll).  We get this right currently.
2) Toggling display of an iframe should persist scroll position of the document
   inside (we get this wrong currently)

Comment 3

[John Keiser (jkeiser)]

OK, revised spec for how the New World will work:

WHAT
- Each history entry has an nsIDocumentState associated with it.  This
represents the extra state of the document necessary to get it back close to the
state it is in when the user left the page.  It is a hash of string keys ->
nsISupports.  The string keys will be transparent to the user of the class, to
whom the interface will look like a hash of nsIContent -> nsISupports.

The nsIDocumentState also has a place for general document-level
state--currently focused element and main scrollbar position (if it proves
necessary) can be stored there.

- Each PresShell has an nsLayoutState associated with it.  This is a place where
frames store their state when they go away so that they can retrieve it later
(particularly useful for display: none and other nuances).  It is a hash of
nsIContent -> nsISupports.  For our history implementation, however, the layout
state must be saved and restored, so therefore we will simply have the
nsLayoutState functions store state in the nsIDocumentState.  This class exists
because in a hypothetical Gecko implementation without history, you need this
functionality anyway but cannot go to the history entry.

WHEN
- the document state is *created* on first use.
- the document state is *destroyed* when the history entry dies.
- the document state is *cleared* on unload (after users have caught the unload
event).
- as elements are flushed into the document, their nsIContent::RestoreState()
method is called and they can retrieve their state.
- on load, radios make sure that their state is consistent (bug 133330, may or
may not be fixed here)

SCROLL FRAMES
- on destruction, scroll frames put their data into the nsLayoutState.
- on Init(), scroll frames retrieve their data from the nsLayoutState.

I have a patch, but it needs tweaking to look more like the above.

Comment 4

[John Keiser (jkeiser)]

Attachment: WIP Restore

OK, I'm going to do this as separate parts.  The DocShell changes scare me, I
think they will scare me less if I fix everything else first.

Breakdown
1. Make restore happen on flush instead of DoneCreatingElement (fixes Hotmail
and other friends)
2. Replace nsLayoutHistoryState with nsDocumentState / nsLayoutState combo
3. Make save happen on unload notification

Comment 5

[Daniel Bratell]

Comment on attachment 106640 [details] [diff] [review]
WIP Restore

Should this be reviewed.

Comment 6

[Daniel Bratell]

Comment on attachment 106640 [details] [diff] [review]
WIP Restore

Should this be reviewed?

Comment 7

[John Keiser (jkeiser)]

No.  It's a work in progress and doesn't work yet.  I have put it on hold
temporarily for other bugs and some thinking.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsDocumentState should (eventually) store the current selection and also the
current alternative style sheet.

Why can't page authors use DOM mutation events to detect changes, including
restore changes?

I think DocumentStateType should be extensible. (Make it just an nsIAtom*?)
E.g., suppose we added the ability to resize elements on a browser page
(something I've wanted). We'd like the size annotation to be attached to the
content by nsDocumentState. We don't need to hardcode it for scrollbars. This
wouldn't seem to add any implementation complexity.

Why do we need frames and content to register for save/restore? For restore, we
can simply poke the content and its primary frame for which we have any saved
state. For save, frames and content can save their state as they're torn down.

In your patch, we don't really need nsCSSFrameConstructor::InitFrame, do we?
Can't we just call frame->Init directly?

Comment 9

[John Keiser (jkeiser)]

> nsDocumentState should (eventually) store the current selection and also the
> current alternative style sheet.

Agreed.  There's lots of things we should add.  I was thinking that one could
set these kinds of global things in there by sending in content = null.  The API
seems flexible enough to store a lot of stuff.

> Why can't page authors use DOM mutation events to detect changes, including
> restore changes?

DOM mutation events will not show when the scrollbar changes or when the value
of the input changes, and for backwards compatibility we can't fire onchange
(maybe we could fire onscroll though, not sure).

> I think DocumentStateType should be extensible. (Make it just an nsIAtom*?)
> E.g., suppose we added the ability to resize elements on a browser page
> (something I've wanted). We'd like the size annotation to be attached to the
> content by nsDocumentState. We don't need to hardcode it for scrollbars. This
> wouldn't seem to add any implementation complexity.

I'd be fine with making DocumentStateType extensible, but the reason I put it in
the central enum is, I didn't want anyone making conflicting strings--the same
string for the two different purposes.  And the type needs to be small so as to
not waste space.

> Why do we need frames and content to register for save/restore? For restore,
> we can simply poke the content and its primary frame for which we have any
> saved state. For save, frames and content can save their state as they're torn
> down.

Frames rarely need to do it, so I deemed it would be better for performance (and
possibly even bloat) if only the frames that need it--currently
scrollframes--just registered.  Content needs to do it more often, in my
experience, so 

> In your patch, we don't really need nsCSSFrameConstructor::InitFrame, do we?
> Can't we just call frame->Init directly?

We can, but it seems to me that InitFrame() is exactly the kind of place where
you're going to want to do other kinds of initialization on the frame.  We could
potentially do that later instead, I'm not particularly attached.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> DOM mutation events will not show when the scrollbar changes or when the value
> of the input changes

Scrollbars yes, but do authors really want to track that? If they do, we could
use onscroll.

I'd forgotten that form values aren't actually reflected in the content model.
That is so bogus.

> I didn't want anyone making conflicting strings--the same
> string for the two different purposes.  And the type needs to be small so as
> to not waste space.

I think nsIAtom* is OK for both of those. Just use a naming convention for the
atom strings like "nsGfxScrollFrame::scrollposition".

> Frames rarely need to do it

Sure. But what about this: to restore the state, just send a notification to
each nsIContent and its frames which we find in the stored state. For saving, we
could have each frame update the saved state whenever the state changes. E.g.,
every time we scroll we update the saved scroll position. The overhead of these
updates should be small.

Comment 11

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 12

[Carlos Alén Silva]

Is this bug still relevant?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Somewhat.  Some of the issues from comment 0 have been resolved, but some have not.