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

RESOLVED FIXED in Firefox 26

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: dao, Assigned: gia)

Tracking

Trunk
Firefox 26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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?
(Reporter)

Comment 2

5 years ago
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

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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);

I hope it's good what I've done so far.
Attachment #785080 - Flags: review?(dao)
(Reporter)

Comment 5

5 years ago
(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?
(Assignee)

Comment 6

5 years ago
Created attachment 785551 [details] [diff] [review]
patch2

I updated the patch.
Attachment #785551 - Flags: review?(dao)
(Reporter)

Comment 7

5 years ago
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.
Attachment #785551 - Flags: review?(dao) → review-
(Reporter)

Updated

5 years ago
Attachment #785080 - Attachment is obsolete: true
Attachment #785080 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Assignee: nobody → georgiana.chelu93
(Assignee)

Comment 8

5 years ago
Created attachment 795099 [details] [diff] [review]
patch3

I hope it's ok now.
Attachment #785551 - Attachment is obsolete: true
Attachment #795099 - Flags: review?(dao)
(Reporter)

Comment 9

5 years ago
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
Attachment #795099 - Flags: review?(dao) → review-
(Assignee)

Comment 10

5 years ago
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.
Attachment #795099 - Attachment is obsolete: true
Attachment #795623 - Flags: review?(dao)
(Reporter)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 796301 [details] [diff] [review]
patch5
Attachment #795623 - Attachment is obsolete: true
Attachment #795623 - Flags: review?(dao)
Attachment #796301 - Flags: review?(dao)
(Reporter)

Comment 13

5 years ago
Comment on attachment 796301 [details] [diff] [review]
patch5

Thanks!
Attachment #796301 - Flags: review?(dao) → review+
(Assignee)

Comment 15

5 years ago
it looks great ! thank you for your patience!
https://hg.mozilla.org/mozilla-central/rev/900eaf8afe73
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
(Reporter)

Updated

5 years ago
Blocks: 910167

Updated

5 years ago
Depends on: 921639

Updated

4 years ago
Depends on: 961646
(Reporter)

Updated

2 months ago
Blocks: 1450559
You need to log in before you can comment on or make changes to this bug.