Remove unneeded SessionSaver.clearLastSaveTime()

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

If SessionStore.initializeWindow() were a little more intelligent we wouldn't need that function. Let's remove it and simplify the SessionSaver code.
Comment on attachment 8622386 [details] [diff] [review]
0001-Bug-1174666-Remove-unneeded-SessionSaver.clearLastSa.patch

Review of attachment 8622386 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/SessionStore.jsm
@@ +912,5 @@
>  
>        // restore a crashed session resp. resume the last session if requested
>        if (aInitialState) {
> +        // Don't write to disk right after startup.
> +        SessionSaver.updateLastSaveTime();

This is something of a hack. I fully support it, but please mention that you're resetting the clock [to avoid writing immediately].

@@ -938,5 @@
>          // Nothing to restore, notify observers things are complete.
>          Services.obs.notifyObservers(null, NOTIFY_WINDOWS_RESTORED, "");
> -
> -        // The next delayed save request should execute immediately.
> -        SessionSaver.clearLastSaveTime();

Mmmh... What did that do, exactly? And why don't we need it anymore?
Attachment #8622386 - Flags: review?(dteller) → feedback+
(In reply to David Rajchenbach-Teller [:Yoric] (away June 22 - July 7th, use "needinfo") from comment #2)
> >        // restore a crashed session resp. resume the last session if requested
> >        if (aInitialState) {
> > +        // Don't write to disk right after startup.
> > +        SessionSaver.updateLastSaveTime();
> 
> This is something of a hack. I fully support it, but please mention that
> you're resetting the clock [to avoid writing immediately].

This isn't something new I'm adding, I'm just moving it. Will update the comment.

> @@ -938,5 @@
> >          // Nothing to restore, notify observers things are complete.
> >          Services.obs.notifyObservers(null, NOTIFY_WINDOWS_RESTORED, "");
> > -
> > -        // The next delayed save request should execute immediately.
> > -        SessionSaver.clearLastSaveTime();
> 
> Mmmh... What did that do, exactly? And why don't we need it anymore?

This was setting lastWriteTime=0, basically reverting the updateLastSaveTime() call from above. If we just move that to the proper branch we don't need it.
Attachment #8625555 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/8d99fb177495
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.