898732 - Use SessionStore object instead of getting the nsISessionStore service in browser.js and tabbrowser.xml

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Bug 898308 added the SessionStore object to browser.js. We can use that instead of getting the nsISessionStore service. For instance, this code:

> let ss = Cc["@mozilla.org/browser/sessionstore;1"].
>          getService(Ci.nsISessionStore);
> ss.setNumberOfTabsClosedLast(window, numberOfTabsToClose);

should just be:

> SessionStore.setNumberOfTabsClosedLast(window, numberOfTabsToClose);

browser.js and tabbrowser.xml can be found at <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/>.

Comment 1

[Kyle Colle-Tripp]

Hi there, I'd like to work on this project, but I'm relatively new to all of this and would need some help getting started. Can you give me a hand learning the ropes?

Comment 2

[Dão Gottwald [:dao]]

I'd suggest starting with <https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial>. There's also an #introduction IRC channel on irc.mozilla.org where you can ask any questions.

Comment 3

[Kyle Colle-Tripp]

Hi Dão, I've actually been assigned a different bug to work on by a friend in order to learn the ropes. I'll have to resign this one for the time being.

Again, sorry to do this to you.

Comment 4

[Georgiana [:gia]]

Attachment: patch1

I replaced nsISessionStore service with SessionStore object, but I am not sure what to do in this case: 

let ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
         getService(Components.interfaces.nsISessionStore);

I hope it's good what I've done so far.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Georgiana [:gia] from comment #4)
> Created attachment 785080 [details] [diff] [review]
> patch1
> 
> I replaced nsISessionStore service with SessionStore object, but I am not
> sure what to do in this case: 
> 
> let ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
>          getService(Components.interfaces.nsISessionStore);

You should remove these lines and replace the references to 'ss' with 'SessionStore'. Does this make sense to you?

Comment 6

[Georgiana [:gia]]

Attachment: patch2

I updated the patch.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 785551 [details] [diff] [review]
patch2

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -2304,24 +2304,20 @@ function PageProxyClickHandler(aEvent)
>  */
> function BrowserOnAboutPageLoad(doc) {
>   if (doc.documentURI.toLowerCase() == "about:home") {
>     // XXX bug 738646 - when Marketplace is launched, remove this statement and
>     // the hidden attribute set on the apps button in aboutHome.xhtml
>     if (getBoolPref("browser.aboutHome.apps", false))
>       doc.getElementById("apps").removeAttribute("hidden");
> 
>-    let ss = Components.classes["@mozilla.org/browser/sessionstore;1"].
>-             getService(Components.interfaces.nsISessionStore);
>-    if (ss.canRestoreLastSession &&
>+    if (SessionStore.canRestoreLastSession &&
>         !PrivateBrowsingUtils.isWindowPrivate(window))
>       doc.getElementById("launcher").setAttribute("session", "true");
> 
>-
>-

Something went wrong in your local repository. The two blank lines you're removing here don't actually exist in the remote repository.

>@@ -6290,40 +6286,40 @@ function undoCloseTab(aIndex) {
>   let numberOfTabsToUndoClose = 0;
>   if (Number.isInteger(aIndex)) {
>     if (SessionStore.getClosedTabCount(window) > aIndex) {
>       numberOfTabsToUndoClose = 1;
>     } else {
>       return tab;
>     }
>   } else {
>-    numberOfTabsToUndoClose = ss.getNumberOfTabsClosedLast(window);
>+    numberOfTabsToUndoClose = SessionStore.getNumberOfTabsClosedLast(window);
>     aIndex = 0;
>   }

In the context of this change, you can see 'if (SessionStore.getClosedTabCount(window) > aIndex) {', but that doesn't exist either -- it's 'if (ss.getClosedTabCount(window) > aIndex) {' in the remote repository.

Comment 8

[Georgiana [:gia]]

Attachment: patch3

I hope it's ok now.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 795099 [details] [diff] [review]
patch3

>@@ -6401,17 +6392,17 @@ var gIdentityHandler = {
>       gNavigatorBundle.getString("identity.encrypted2");
>     this._encryptionLabel[this.IDENTITY_MODE_IDENTIFIED] =
>       gNavigatorBundle.getString("identity.encrypted2");
>     this._encryptionLabel[this.IDENTITY_MODE_UNKNOWN] =
>       gNavigatorBundle.getString("identity.unencrypted");
>     this._encryptionLabel[this.IDENTITY_MODE_MIXED_DISPLAY_LOADED] =
>       gNavigatorBundle.getString("identity.mixed_display_loaded");
>     this._encryptionLabel[this.IDENTITY_MODE_MIXED_ACTIVE_LOADED] =
>-      gNavigatorBundle.getString("identity.mixed_active_loaded");
>+      gNavigatorBundle.getString("identity.mixed_active_loaded2");
>     this._encryptionLabel[this.IDENTITY_MODE_MIXED_DISPLAY_LOADED_ACTIVE_BLOCKED] =
>       gNavigatorBundle.getString("identity.mixed_display_loaded_active_blocked");
>     return this._encryptionLabel;
>   },
>   get _identityPopup () {
>     delete this._identityPopup;
>     return this._identityPopup = document.getElementById("identity-popup");
>   },

This doesn't belong in this patch.

Otherwise this looks fine, except that the patch fails to apply. Is your local tree up to date? Did you create the diff of your changes against the repository tip?

patching file browser/base/content/browser.js
Hunk #1 FAILED at 2303
Hunk #2 FAILED at 2559
Hunk #4 FAILED at 6271
3 out of 7 hunks FAILED -- saving rejects to file browser/base/content/browser.js.rej
patching file browser/base/content/tabbrowser.xml
Hunk #2 FAILED at 1637
1 out of 5 hunks FAILED -- saving rejects to file browser/base/content/tabbrowser.xml.rej
abort: patch failed to apply

Comment 10

[Georgiana [:gia]]

Attachment: patch4

I fortgot to update the local tree. The patch looks ok now, I hope it won't fail again.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Georgiana [:gia] from comment #10)
> Created attachment 795623 [details] [diff] [review]
> patch4
> 
> I fortgot to update the local tree. The patch looks ok now, I hope it won't
> fail again.

Seems like that tree update didn't go well. This patch still produces exactly the same failures noted in comment 9.

Comment 12

[Georgiana [:gia]]

Attachment: patch5

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 796301 [details] [diff] [review]
patch5

Thanks!

Comment 14

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/900eaf8afe73

Comment 15

[Georgiana [:gia]]

it looks great ! thank you for your patience!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/900eaf8afe73