669077 - Cleanup sessionstore tests suite. Ports bug 667202 and syncs head.js with FF

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Attachment: patch

This almost trivial cleanup.

Comment 1

[[e-mail]]

Comment on attachment 543678 [details] [diff] [review]
patch

>-    gPrefService.setIntPref("browser.sessionstore.max_tabs_undo",
>+    Services.prefs.setIntPref("browser.sessionstore.max_tabs_undo",
>                             test_state.windows[0]._closedTabs.length);
Nit: ideally you would reindent lines like this.
(I just picked on this one, there are other cases)

>+  let newWindow;
>   function windowObserver(aSubject, aTopic, aData) {
>     if (aTopic == "domwindowopened") {
>-      let newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
>+      newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
What's this change for?

Comment 2

[Misak Khachatryan]

Attachment: patch, nits fixed

(In reply to comment #1)
> Comment on attachment 543678 [details] [diff] [review] [review]
> patch
> 
> >-    gPrefService.setIntPref("browser.sessionstore.max_tabs_undo",
> >+    Services.prefs.setIntPref("browser.sessionstore.max_tabs_undo",
> >                             test_state.windows[0]._closedTabs.length);
> Nit: ideally you would reindent lines like this.
> (I just picked on this one, there are other cases)
> 

Fixed all that found.

> >+  let newWindow;
> >   function windowObserver(aSubject, aTopic, aData) {
> >     if (aTopic == "domwindowopened") {
> >-      let newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> >+      newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> What's this change for?

Don't know exactly, but without this change test fails on my machine. Seems scope stuff.

Comment 3

[[e-mail]]

Comment on attachment 543899 [details] [diff] [review]
patch, nits fixed

> function test_setBrowserState() {
>   // We'll track events per window so we are sure that they are each happening once
>   // pre window.
>   let windowEvents = {};
>   windowEvents[getOuterWindowID(window)] = { busyEventCount: 0, readyEventCount: 0 };
> 
>   // waitForBrowserState does it's own observing for windows, but doesn't attach
>   // the listeners we want here, so do it ourselves.
>+  let newWindow;
>   function windowObserver(aSubject, aTopic, aData) {
>     if (aTopic == "domwindowopened") {
>-      let newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
>+      newWindow = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
>       newWindow.addEventListener("load", function() {
>         newWindow.removeEventListener("load", arguments.callee, false);
>+
...
>+    newWindow.removeEventListener("SSWindowStateBusy", onSSWindowStateBusy, false);
>+    newWindow.removeEventListener("SSWindowStateReady", onSSWindowStateReady, false);
So, it turns out that there are three functions involved here, which is why newWindow needs to be declared higher up. To make it clearer, I think it would be helpful to declare newWindow at the beginning of test_setBrowserState.

Comment 4

[Misak Khachatryan]

Pushed with nit fixed:

http://hg.mozilla.org/comm-central/rev/2ffa1a8f3463