Closed Bug 898732 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: dao, Assigned: gia)

References

Details

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

Attachments

(1 file, 4 obsolete files)

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/>.
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?
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.
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.
Attached patch patch1 (obsolete) — Splinter Review
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)
(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?
Attached patch patch2 (obsolete) — Splinter Review
I updated the patch.
Attachment #785551 - Flags: review?(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.
Attachment #785551 - Flags: review?(dao) → review-
Attachment #785080 - Attachment is obsolete: true
Attachment #785080 - Flags: review?(dao)
Assignee: nobody → georgiana.chelu93
Attached patch patch3 (obsolete) — Splinter Review
I hope it's ok now.
Attachment #785551 - Attachment is obsolete: true
Attachment #795099 - Flags: review?(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
Attachment #795099 - Flags: review?(dao) → review-
Attached patch patch4 (obsolete) — Splinter Review
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)
(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.
Attached patch patch5Splinter Review
Attachment #795623 - Attachment is obsolete: true
Attachment #795623 - Flags: review?(dao)
Attachment #796301 - Flags: review?(dao)
Comment on attachment 796301 [details] [diff] [review]
patch5

Thanks!
Attachment #796301 - Flags: review?(dao) → review+
it looks great ! thank you for your patience!
https://hg.mozilla.org/mozilla-central/rev/900eaf8afe73
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Blocks: 910167
Depends on: 921639
Depends on: 961646
Blocks: 1450559
You need to log in before you can comment on or make changes to this bug.