Open Bug 1037075 Opened 10 years ago Updated 2 years ago

Need event or notification signalling initiation of restoring a session

Categories

(Firefox :: Session Restore, defect)

defect

Tracking

()

UNCONFIRMED

People

(Reporter: u462496, Unassigned)

References

Details

Attachments

(3 files, 2 obsolete files)

Currently sessionstore sends a notification upon completion of restoring of a browser session or window state, eg, sessionstore-windows-restored, sessionstore-browser-state-restored.  However, there is no event or notification sent upon its initiation.

Bug 615394 seems to have attempted to deal with this problem, however the SSWindowStateBusy and SSWindowStateReady events are fired on setTabValue (maybe others that do not affect the window state).  Not sure if this was intentional or just a bug.  Nevertheless, this throws a big kink in trying to determine when window state/browser state is being restored.  It seems like the changes made in resolving this bug were specifically for Panorama.

I am asking that there would events/notifications sent which would signal the initiation of updating a browser state or window state, which might occur during the course of a session via an addon such as Session Manager.  ie, counterpart notifications for sessionstore-windows-restored and sessionstore-browser-state-restored, which would signal the initiation of actions which result in sending of those notifications.  I believe this would be a more general approach.

This is a need which I have encountered in developing various addons which has left me the task of applying some hackish (and possibly unreliable) strategies in order to ignore TabOpen/TabClose events when inappropriate.  One such addon is one I am currently developing to be a possible replacement to Panorama (see bug 836758).
correction: I stated setTabValue, I meant setTabState.
Perhaps an alternate solution would be to add a property to the event object which would tell the kind of action which initiated the SSWindowStateBusy and SSWindowStateReady events, eg, "setTabState", "setWindowState", "setBrowserState".
sending a notification early on in ssi_restoreWindow seems to work, eg "sessionstore-windows-begin-restore"
Switching to untriaged in hope it will get some attention.  The need for this is becoming very important to me.  I believe my request is very reasonable and logical, and helpful to future implementation, and the fix for this would be trivial.
Component: Session Restore → Untriaged
Flags: needinfo?(dteller)
Component: Untriaged → Session Restore
Flags: needinfo?(dteller)
Flags: needinfo?(dteller)
I'm not completely sure I understand. Do you want to bypass session restoration completely?
Flags: needinfo?(dteller) → needinfo?(ttaubert)
Not sure why I'm ni?'ed here. Guess we need some more information and examples why this would be needed.
Flags: needinfo?(ttaubert) → needinfo?(allassopraise)
(In reply to David Rajchenbach Teller [:Yoric] (away until August 20th - use "needinfo") from comment #5)
> I'm not completely sure I understand. Do you want to bypass session
> restoration completely?

Yes and no.  In light of the original description of this bug, no.  My original request was only asking for notifications when a session or window restore begins occurring and ends, that are less ambiguous than what is currently available.  sessionstore-windows-restored and sessionstore-browser-state-restore notifications are clear, but they only occur at the end; there are no counterparts for the beginning.  SSWindowStateBusy occurs at the beginning, but is too ambiguous, as the same event is fired when browser state, window state, or tab state restore begins.  So that is what I was asking when originally filing this bug.

David, I believe your question comes from our discussion on the dev-extension@mozilla.lists.org list which is not documented here.  They involve my needs for Virtual Tabs addon (see bug 906076). In answer to that question, there are instances where I would like to go a step further and intercept a particular session restore and stop tabs from loading.  Even further than that, I would like to be able to intercept then replace the session that would normally be loaded with a modified version, and allow the restore to continue with the modified state.

These are my requests and my justifications for them:

1) original request, unambiguous event/notifications for marking the beginning and end of a session or window restore.

2) event notification of start of session or window restore with the ability to intercept the restore and stop it.

3) same as 2, only rather than stopping it, replace the session with a modified session state.

I view #1 as a very reasonable request, and generic.  Notifications/events are already being sent, signalling that justification has been felt for them, however the ambiguity makes them less than ideal.  Why not clean this up a bit?  It seems to me that the changes involve would be trivial.

I view #2 and #3 as a bit less generic, and admittedly specific to my own addon Virtual Tabs (still under development). However my justification is that if event dispatching code is in place anyway, the additional code needed would also be trivial.  And also, who knows that this feature would be of benefit in other circumstances, such as session management implementations.  I am wanting to strike while the iron is hot, while sessionstore is undergoing a revamp.

If it is appropriate, I will report details of how my further requests would be implemented in Virtual Tabs addon in this bug.

I don't know if my additional requests warrant filing a new bug, please advise.

Soon (I hope) I will provide a patch demonstrating my requests.
Flags: needinfo?(allassopraise)
See Also: → 836758
Patch for sending event upon start and stop of ssi_restoreWindow method. Provides the listener with the intended restore state. Allows opportunity for listener to cancel the restore, or restore instead to a supplied state.
revision of previous patch bug_1037075_SessionStore.jsm_patch_events_for_start_stop_ssi_restoreWindow.diff
Attachment #8485518 - Attachment is obsolete: true
Updated patch, first version, event only - good
Attachment #8539801 - Flags: feedback?(dteller)
Updated patch, second option, intended restore state passed to listener - better
Attachment #8539802 - Flags: feedback?(dteller)
Updated patch, third option, second option plus option to replace intended restore state with a different state, or cancel restore altogether.
Attachment #8485555 - Attachment is obsolete: true
Attachment #8539803 - Flags: feedback?(dteller)
I updated the previous patch with a little cleaner code, and also provided two lesser options.  The most detailed option (ability to change state or cancel) would be my most preferred however.
I'll try and look at the code in January.
(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #14)
> I'll try and look at the code in January.

thank you
(In reply to Allasso Travesser from comment #7)
> I am wanting to strike while the iron is hot, while
> sessionstore is undergoing a revamp.

So to be clear, sessionstore is not currently undergoing a revamp. We did make a lot of modifications months ago to make it work with e10s but that work is mostly finished besides a few bugs that might pop up. sessionstore is not a priority for us at all right now.

> I view #2 and #3 as a bit less generic, and admittedly specific to my own
> addon Virtual Tabs (still under development).

I think adding an API to satisfy a very specific need is problematic, we would want APIs to be more generic. You also should not forget that implementing and especially maintaining an API does not come without cost. Once we add something people will start to rely on it and we have to carry that decision around for years. We apply the same reasoning even when thinking about adding a simple binary preference to switch something on or off. Just a simple preference can add significant overhead.

> I view #1 as a very reasonable request, and generic.  Notifications/events
> are already being sent, signalling that justification has been felt for
> them, however the ambiguity makes them less than ideal.  Why not clean this
> up a bit?  It seems to me that the changes involve would be trivial.

I think all the events and notifications currently sent by sessionstore are less than ideal and the product of the lack of time when building Firefox 4 and the desire to ship Panorama/Tab Groups. I would love to see a lot less notifications/events.

I would rather like to see a reasonable method/API implemented although I currently can't come up with a use case that justifies being able to stop the restoration of a single tab, or modify the state that is going to be restored.
Firstly a question: given your patches for lazifying instantiation of xul:browser, is this API still useful?

I believe that #1 is reasonable with the current design of Session Restore + Tab Groups, but, as Tim points out, every such API will make migrating away from the current design harder, and I would really like us to do perform such a migration once we figure out how, so if we can do without this API, I would prefer.

I'm not a big fan of #2 or #3, as I feel they expose too much internal state.
Flags: needinfo?(allassopraise)
Attachment #8539801 - Flags: feedback?(dteller) → feedback+
Attachment #8539802 - Flags: feedback?(dteller) → feedback-
Attachment #8539803 - Flags: feedback?(dteller) → feedback-
> Firstly a question: given your patches for lazifying instantiation of
> xul:browser, is this API still useful?
regarding patch #1, there is code in two of my addons which could have benefited from this, where I wanted to trigger certain operations from tab events, but not during a window restore.  I have worked around this with some kludgy code and guesswork, but a notification of the actual event in question would be ideal.  In the current API, there are events fired, but they are ambiguous, as there is no way to distinguish between a window restore due to a session restore, and an individual tab restore, which may possibly be called by an addon via setTabState.  The fact that this has come up more than once for me makes me wonder if other addon developers might benefit.

regarding patches #2 and #3, yes, these would be specific to the original "Virtual Tabs" addon that I began developing which sparked my patches for lazy instantiation of xul:browser.  Nevertheless the very idea of being able to manipulate a session restore seems useful to me though I don't have any use cases other than the "Virtual Tabs" addon.  I do appreciate Tim giving me insight on the ongoing responsibility involved in adding a new API.  Being an addon dev, I'm admittedly myopic to the largeness of the app and what it takes to maintain it.

In light of the fact that the lazy instantiation of xul:browser approach is invasive and must be taken incrementally and cautiously, it will also presumably take some time to implement it to the full degree I would like to see in regard to gains in resources.  Patches #2 and #3 seemed like a quick fix-for-now which would allow me to offer an intermediate solution in the "Virtual Tabs" addon.

Nevertheless, the "Virtual Tabs" addon is in no way a replacement for lazy instantiation of browser.  It is extremely invasive (its "tabs" are true "virtual" tabs which completely take the place of unloaded tabbrowser tabs) and by it's nature rules out the use of any normal tab-related addon.  It has it's own "tab" management features which attempt to emulate some features of the popular tab addons, but as you can see will most likely be limited to a narrow base of power users whose concern for performance and memory usage is predominant.
Flags: needinfo?(allassopraise)
I have a suggestion for a low-impact option for this bug:  How about if we simply set a property on the SSWindowStateBusy event when the event is fired from windowRestore, which a listener can test for?
> I have a suggestion for a low-impact option for this bug:  How about if we
> simply set a property on the SSWindowStateBusy event when the event is fired
> from windowRestore, which a listener can test for?
.. but perhaps, as you say, even this will make migrating away harder.
I may be wrong, but I have the impression that the biggest gain we will obtain is making xul:browser lazy, which can be done without adding new APIs. If I'm right, I believe that we should concentrate on getting that feature landed, and only then see how we want things to proceed.

> Nevertheless, the "Virtual Tabs" addon is in no way a replacement for lazy instantiation of browser.  It is extremely invasive (its "tabs" are true "virtual" tabs which completely take the place of unloaded tabbrowser tabs) and by it's nature rules out the use of any normal tab-related addon.  It has it's own "tab" management features which attempt to emulate some features of the popular tab addons, but as you can see will most likely be limited to a narrow base of power users whose concern for performance and memory usage is predominant.

I hope that, once lazy xul:browser has landed, we can proceed and progressively find ideas on how to bring about some or all of the other optimizations from Virtual Tabs without breaking stuff. But, as mentioned, let's not get ahead of ourselves.
David - comment #21:

I agree.
Time to review?
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: