sessionStore starts observe browser.sessionstore.max_tabs_undo only after tab closed

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: tabmix.onemen, Assigned: AMoz)

Tracking

11 Branch
Firefox 27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=ttaubert][lang=js])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Bug 542032 add lazy getter for _max_tabs_undo.

The lazy getter for _max_tabs_undo trigger only by onTabClose
So, until user closed the first tab in a session, any changes to browser.sessionstore.max_tabs_undo have no effect on closed tab list

Reproducible: Always

Steps to Reproduce:
1. start Firefox, make sure browser.sessionstore.max_tabs_undo > 0, if not set it and restart Firefox.
2. Open a bunch of tab, close few tabs.
3. Restart Firefox.
4. set browser.sessionstore.max_tabs_undo to 0 (zero)

Actual Results:  
History > Recently Closed Tabs list contained the tabs you closed before

Expected Results:  
Recently Closed Tabs list should be empty after you set browser.sessionstore.max_tabs_undo to 0 (zero)
What would be the best solutions to fix this? I think this is one of the disadvantages of lazy getters.
Clearing by setting the pref was never really a supported way of clearing out your closed tabs, so I'm not sure what we should do here, if anything. It's possible (if a bit awkward) to do via the API. Maybe the better path would be to improve the sessionstore API to make this easier.
(Reporter)

Comment 3

5 years ago
Just move the addObserver out of the lazy getters

+ this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
  XPCOMUtils.defineLazyGetter(this, "_max_tabs_undo", function () {
-   this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
    return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
  });

+ this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
  XPCOMUtils.defineLazyGetter(this, "_max_windows_undo", function () {
-   this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
    return this._prefBranch.getIntPref("sessionstore.max_windows_undo");
  });
I think it would be best if we do it like for gDebbugingEnabled here:

http://hg.mozilla.org/mozilla-central/file/3d20597e0a07/browser/components/sessionstore/src/SessionStore.jsm#l523

Read and store the current pref value, and then register the observer.
Whiteboard: [good first bug][mentor=ttaubert][lang=js]
(Reporter)

Comment 5

5 years ago
this link doesn't point to gDebbugingEnabled
It does for me...
(Assignee)

Comment 7

5 years ago
I would like to work on this bug. I have resumed to bugzilla after a very long time. So might take some time to grasp it. I would request you to help me getting started. Thanks !
Hey Amod, that's great to hear. I gave a hint on how this can be done in comment #4. Should there be any questions feel free to ask here or on IRC.
Sorry, I don't understand. Is that a question? Do you need more information from me? Please feel free to ask. Thanks!
(Assignee)

Comment 11

5 years ago
Created attachment 811543 [details] [diff] [review]
patchv1

Still I am bit unclear with the concept. Forgive me in case of out of bounds code.
Attachment #811543 - Flags: feedback?(ttaubert)
Comment on attachment 811543 [details] [diff] [review]
patchv1

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

Thanks, Amod! I annotated your patch below:

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +537,5 @@
>        gDebuggingEnabled = this._prefBranch.getBoolPref("sessionstore.debug");
>      }, false);
>  
>      XPCOMUtils.defineLazyGetter(this, "_max_tabs_undo", function () {
> +      gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

We want to save the value of 'sessionstore.max_tabs_undo' to this._max_tabs_undo, like:

this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

@@ +538,5 @@
>      }, false);
>  
>      XPCOMUtils.defineLazyGetter(this, "_max_tabs_undo", function () {
> +      gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +      Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);

This line and the line above should be outside of the lazy getter. In fact, we want to remove the lazy getter. Also, we don't want to register 'this' as the observer but an anonymous function (like on line 536 above) that updates the value stored in this._max_tabs_undo.
Attachment #811543 - Flags: feedback?(ttaubert)
(Assignee)

Comment 13

5 years ago
Created attachment 811546 [details] [diff] [review]
patchv2
Attachment #811543 - Attachment is obsolete: true
Attachment #811546 - Flags: feedback?(ttaubert)
Comment on attachment 811546 [details] [diff] [review]
patchv2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +530,5 @@
>  
>    _initPrefs : function() {
>      this._prefBranch = Services.prefs.getBranch("browser.");
>  
> +    this._max_tabs_undo = this._prefBranch.getBoolPref("sessionstore.debug");

No, this needs to stay gDebuggingEnabled.

@@ +541,2 @@
>        return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
>      });

This is just leftover code and can be removed now.

@@ +541,3 @@
>        return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
>      });
> +    gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

This line should be modifying this._max_tabs_undo.

@@ +541,4 @@
>        return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
>      });
> +    gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);

The 'this' in here is the observer argument. We need to pass an anonymous function that modifies this._max_tabs_undo. You can look at line 536-538 to see how that can be done.
Attachment #811546 - Flags: feedback?(ttaubert)
(Assignee)

Comment 15

5 years ago
Created attachment 811552 [details] [diff] [review]
patchv3

Explanation why i used this:

>  Services.prefs.addObserver("browser.sessionstore.max_tabs_undo", () => {
>   this._max_tabs_undo=this._prefBranch.getBoolPref("sessionstore.max_tabs_undo");
>    }, false);

The anonymous function should modify the value of  | this |, thats why i took the | this._max_tabs_undo | to the left side of the assignment. Rest i followed the same given in line 536-538
Attachment #811546 - Attachment is obsolete: true
Attachment #811552 - Flags: feedback?(ttaubert)
Comment on attachment 811552 [details] [diff] [review]
patchv3

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

Thanks Amod. This looks a lot better but we're not quite there yet :)

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +537,5 @@
>        gDebuggingEnabled = this._prefBranch.getBoolPref("sessionstore.debug");
>      }, false);
>  
> +
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

This pref is an 'int'. You need to use getIntPref().

Did you try to run the test suite? Or just Firefox with your patch? The parenthesis don't match here.

@@ +538,5 @@
>      }, false);
>  
> +
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    //Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);

Nit: that commented out code should be removed

@@ +539,5 @@
>  
> +
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    //Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);
> +    

Nit: please remove the white space left on the line here

@@ +541,5 @@
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    //Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);
> +    
> +    Services.prefs.addObserver("browser.sessionstore.max_tabs_undo", () => {
> +    this._max_tabs_undo = this._prefBranch.getBoolPref("sessionstore.max_tabs_undo");

This pref is an 'int'. You need to use getIntPref().

Nit: this line should be a tad more indented, i.e. two spaces

@@ +547,5 @@
>  
>      XPCOMUtils.defineLazyGetter(this, "_max_windows_undo", function () {
>        this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
>        return this._prefBranch.getIntPref("sessionstore.max_windows_undo");
>      });

While we're at it, can you please do the same for _max_windows_undo?
Attachment #811552 - Flags: feedback?(ttaubert) → feedback+
(Assignee)

Comment 17

5 years ago
Created attachment 811966 [details] [diff] [review]
patchv4

modified all the suggested changes.
Attachment #811552 - Attachment is obsolete: true
Attachment #811966 - Flags: review?(ttaubert)
Comment on attachment 811966 [details] [diff] [review]
patchv4

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

Another thing I noticed: on line 1771, can you please change

  curWinState._closedTabs.splice(this._prefBranch.getIntPref("sessionstore.max_tabs_undo"), curWinState._closedTabs.length);

to:

  curWinState._closedTabs.splice(this._max_tabs_undo, curWinState._closedTabs.length);

Thanks!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +544,5 @@
> +
> +    this._max_windows_undo = this._prefBranch.getIntPref("sessionstore.max_windows_undo");
> +    Services.prefs.addObserver("browser.sessionstore.max_windows_undo", () => {
> +      this._max_windows_undo = this._prefBranch.getIntPref("sessionstore.max_windows_undo");
> +    }, false);

Sorry, totally my fault. I forgot that we actually need to handle changes to these preferences in that we might need to truncate the list of tabs and windows to restore. The easiest way to fix this is to just add 'this' as the observer like we had it before, that way onPrefChange() handles value changes for us:

  this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
  this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);

Something like this should work.
Attachment #811966 - Flags: review?(ttaubert)
(Assignee)

Comment 19

5 years ago
Created attachment 812165 [details] [diff] [review]
patchv5

please let me know in case of additional changes. :)
Attachment #811966 - Attachment is obsolete: true
Attachment #812165 - Flags: review?(ttaubert)
Comment on attachment 812165 [details] [diff] [review]
patchv5

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +537,5 @@
>        gDebuggingEnabled = this._prefBranch.getBoolPref("sessionstore.debug");
>      }, false);
>  
> +    this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
> +    Services.prefs.addObserver("browser.sessionstore.max_tabs_undo", () => {

We can remove the 'Services.prefs.addObserver' line. But we need to keep the line that sets the property. It was included in the previous patch but you unfortunately removed it. We need that back :)

The 'this._prefBranch.addObserver' line should ideally come after the line that sets the property. Same thing for max_windows_undo below.
Attachment #812165 - Flags: review?(ttaubert)
(Assignee)

Comment 21

5 years ago
Created attachment 812769 [details] [diff] [review]
patchv6

I am extremely sorry for the mistake.. Even I dont believe that I made such stupid mistake. This patch should be correct.
Attachment #812165 - Attachment is obsolete: true
Attachment #812769 - Flags: review?(ttaubert)
Comment on attachment 812769 [details] [diff] [review]
patchv6

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

Thanks! Sorry for the late response, I was out for a while after the Summit. Can you please prepare the patch for checkin? [1]

[1] http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Attachment #812769 - Flags: review?(ttaubert) → review+
Assignee: nobody → amod.narvekar
Status: NEW → ASSIGNED
Flags: needinfo?(amod.narvekar)
(Assignee)

Comment 23

5 years ago
Created attachment 820271 [details] [diff] [review]
bug-758138-fix

I am unable to set myself as the author of the patch. 
here i have pastebin-ed my .hgrc file contents: http://pastebin.mozilla.org/3311273
Attachment #812769 - Attachment is obsolete: true
Attachment #820271 - Flags: review?(ttaubert)
Flags: needinfo?(amod.narvekar)
Comment on attachment 820271 [details] [diff] [review]
bug-758138-fix

Why are you unable to do that? Using 'hg qref -u "Your Name <email@domain.org>"' should work, no?

Also, please add 'r=ttaubert' at the end of the patch description. Thanks!
Attachment #820271 - Flags: review?(ttaubert)
(Assignee)

Comment 25

5 years ago
Created attachment 821131 [details] [diff] [review]
bug-758138-fix
Attachment #820271 - Attachment is obsolete: true
Attachment #821131 - Flags: review?(ttaubert)
Comment on attachment 821131 [details] [diff] [review]
bug-758138-fix

Thank you, Amod! No need to review this again.
Attachment #821131 - Flags: review?(ttaubert)
https://hg.mozilla.org/integration/fx-team/rev/9f6f1acf4290

Thanks a lot for your contribution, Amod! I just pushed this to the fx-team branch, from there on it should be merged to our main branch (mozilla-central) soon and will then be in Nightly.
https://hg.mozilla.org/mozilla-central/rev/9f6f1acf4290
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
(Assignee)

Comment 29

5 years ago
Thank you very much Tim. Had a great time working with you. :)Looking forward to work with you again.
You need to log in before you can comment on or make changes to this bug.