480893 - Session Restore, start new session does not follow startup page option

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Richard Biddle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090226 Firefox/3.1a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090226 Firefox/3.1a1pre

After selecting "New Session" from Session Restore dialog, the browser opens my homepage in a new tab, rather than following my configuration to open a blank tab on startup.

Reproducible: Always

Steps to Reproduce:
1. Set firefox to open a blank page on startup.
2. Click New Session from within session restore dialog

Actual Results:  
Opens home page url

Expected Results:  
Opens a blank tab

Comment 1

[Simon Bünzli]

Right, when browser.startup.page is 0, we should just load about:blank in startNewSession (cf. URL).

Comment 2

[Michael Kohler [:mkohler]]

Changed "URL" to http://hg.mozilla.org/mozilla-central/annotate/af782d5ec362/browser/components/sessionstore/content/aboutSessionRestore.js#l147 since this is the actual function we should edit. I doubt this has something to do with function onListKeyDown(aEvent).

Simon Bünzli: feel free to change "Assigned to" back to nobody if you don't want me to fix this.

Comment 3

[Michael Kohler [:mkohler]]

Attachment: Patch v1

Should be okay like that. Improvements always welcome!

Comment 4

[Simon Bünzli]

Comment on attachment 365493 [details] [diff] [review]
Patch v1

Thanks for the patch, Michael!

>+  var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+		     getService(Ci.nsIPrefService).getBranch("browser.");

If it's just for a single pref, there's no reason to get a subbranch first (see the restoreSingleTab function below). And please try not to sneak tabs into your code...

>+    getBrowserWindow().gBrowser.loadURI("about:blank", undefined, undefined);

There's no need to spell out |undefined|, omitting those parameters will leave them just as undefined. If you want to list all three parameters, pass the second and third as |null| instead.

r+=me with these two issues fixed.

Comment 5

[Michael Kohler [:mkohler]]

Attachment: Patch v1.1 : corrections from comment 4 applied

deleted the getBranch and omitted the second and third parameter.

Comment 6

[Simon Bünzli]

Comment on attachment 365510 [details] [diff] [review]
Patch v1.1 : corrections from comment 4 applied

> function startNewSession() {
>-  getBrowserWindow().BrowserHome();
>+  if (prefBranch.getIntPref("startup.page") == 0)

You still need a prefBranch, though - it's just that a root one will do as well (and the pref will then be "browser.startup.page").

Comment 7

[Michael Kohler [:mkohler]]

Attachment: Patch v1.2 : changes see comment 6

oops, I'm sorry..

Comment 8

[Simon Bünzli]

Comment on attachment 365514 [details] [diff] [review]
Patch v1.2 : changes see comment 6

>+  var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+                     getService(Ci.nsIPrefService).getBranch("");

You'll get a root branch if you directly request an Ci.nsIPrefBranch from the preferences-service (just look at the above mentioned restoreSingleTab function in the same file). BTW: As for alignment, we seem to prefer aligning getService with the Cc and not the bracket in sessionstore code...

Comment 9

[Michael Kohler [:mkohler]]

Attachment: Patch v1.3

Sorry for taking away that much time.

Comment 10

[Simon Bünzli]

Comment on attachment 365525 [details] [diff] [review]
Patch v1.3

Thanks again.

We probably also want a basic test for this: loading about:sessionrestore with browser.startup.page set to 0 and 1, both times clicking the "errorCancel" button and ensuring that once a blank page and the other time a non-blank one is loaded.

If you want to give it a try, that'd be great, otherwise I'll see what I can come up with once I've got some time to spare.

Comment 11

[Michael Kohler [:mkohler]]

(In reply to comment #10)
> If you want to give it a try, that'd be great, otherwise I'll see what I can
> come up with once I've got some time to spare.

I'll give it a try, but I can't promise to succeed.

Comment 12

[Michael Kohler [:mkohler]]

(In reply to comment #10)
> We probably also want a basic test for this: loading about:sessionrestore with
> browser.startup.page set to 0 and 1, both times clicking the "errorCancel"
> button and ensuring that once a blank page and the other time a non-blank one
> is loaded.

What if the user has set "about:blank" as his homepage?  (i.e. he doesn't know that he can choose "show blank tab" from the dropdown list)

Except of that, I guess, I've the test. But never tested yet..

Comment 13

[Michael Kohler [:mkohler]]

Oh, forget about my last comment.. I'm sorry.

Comment 14

[Michael Kohler [:mkohler]]

Sorry for asking, but how can I test this test?

I tried "TEST_PATH=browser/components/sessionrestore/test/browser_480893.js make -C fx-obj mochitest-plain" but that opens firefox and says: "/tests/browser/components/sessionrestore/test/browser_480893.js was not found."

Comment 15

[Simon Bünzli]

I'm not quite sure, but your path looks wrong as your SessionStore test should rather live in browser/components/sessionstore/test/browser/browser_480893.js . If that doesn't work, please ask on IRC for further instructions and maybe also update the documentation at <https://developer.mozilla.org/en/Browser_chrome_tests> for the next person to encounter your difficulties.

Comment 16

[Michael Kohler [:mkohler]]

Fixed that problem. Now the test still contains a few bugs so I will upload the new diff on Sunday.

Comment 17

[Michael Kohler [:mkohler]]

Attachment: Patch v1.4 (added test)

I hope this is okay. At least it passes.

Comment 18

[Simon Bünzli]

Comment on attachment 366355 [details] [diff] [review]
Patch v1.4 (added test)

The structure of the test looks good. I'm afraid that it passes only by accident, though:

>+  /** Test for Bug 480893 **/

You'll have to start your test with waitForExplicitFinish(), otherwise it passes as soon as the test() function has been run through (i.e. before the asynchronous event listeners are called the first time).

>+  let doc = tab.linkedBrowser.contentDocument;

That's the document loaded when the tab is first created, not however the tab's current document once that one has been overwritten (as you seem to expect). You'll have to get it anew after every load...

>+  // tab with browser.startup.page = 1
>+  gPrefService.setIntPref("browser.startup.page", 0);

1 or 0?

>+  tab.linkedBrowser.addEventListener("load", function(aEvent) {

Please remove the event listeners ASAP, otherwise it'll be triggered again for future load events (where you want a different event listener): this.removeEventListener("load", arguments.callee, true);

>+      is(tabURL, "about:blank",
>+         "doesn't show about:blank when browser.startup.page = 0");

Doesn't or _does_? Please indicate what's supposed to happen, not what didn't happen in the case of an error.

>+      tab.addEventListener("load", function(aEvent) {

linkedBrowser? For simplicity, you might want to
let browser = tab.linkedBrowser;
and then use that variable instead throughout the test.

>+          is(tabURL2, "resource:/browserconfig.properties",

That's certainly not the set homepage. I'd be fine with ensuring that the URL is neither "about:sessionrestore" nor "about:blank", though.

Thanks for giving it a(nother) shot.

Comment 19

[Michael Kohler [:mkohler]]

(In reply to comment #18) 
> >+  tab.linkedBrowser.addEventListener("load", function(aEvent) {
> 
> Please remove the event listeners ASAP, otherwise it'll be triggered again for
> future load events (where you want a different event listener):
> this.removeEventListener("load", arguments.callee, true);

when's ASAP?

Comment 20

[Simon Bünzli]

(In reply to comment #19)
> when's ASAP?

<http://www.dict.cc/?s=asap>: as the first statement in every event listener.

Comment 21

[Michael Kohler [:mkohler]]

Attachment: doesn't work..

Changed a few things, but now it doesn't work anymore. [need help]

Comment 22

[Simon Bünzli]

Comment on attachment 366373 [details] [diff] [review]
doesn't work..

Sweet, with one little change this test passes nicely.

>+  // tab with browser.startup.page = 0

Please be slightly more verbous as to what you're testing here.

>+  let tab = gBrowser.addTab("about:sessionrestore");

You'll have to select the tab at this point for its XUL bits to become active: gBrowser.selectedTab = ...

>+         "should show about:blank when browser.startup.page = 0");

What should...? And s/show/load/.

>+          let tabURL2 = doc.URL;

Minor nit: Is there a reason for creating a new variable?

r+=me with the nits fixed and once your test passes as expected.

Comment 23

[Michael Kohler [:mkohler]]

Attachment: Patch v1.6

Comment 24

[Michael Kohler [:mkohler]]

Attachment: Patch v1.7

This passes the test and I hope I've included all your nits.

Comment 25

[Simon Bünzli]

Comment on attachment 366665 [details] [diff] [review]
Patch v1.7

>+  // set browser.startup.page to 0
>+  gPrefService.setIntPref("browser.startup.page", 0);

I'm afraid, that's not exactly what I meant with "more verbous". Please comment on _what_ the two parts of your test do and not on _how_ they do it. Comments like this don't really help (and are at worst needlessly in the way).

>+    // click on the "Start New Session" button after about:sessionrestore is loaded
>+    doc.getElementById("errorCancel").click();

E.g. this comment is much more valuable, because what's happening isn't too obvious from reading the code only.

Try: "Test that starting a new session loads a blank page if Firefox is configured to display a blank page at startup (browser.startup.page = 0)" and something similar for the second half. The comments about backing up the original pref value and the one cited above can stay.

Comment 26

[Michael Kohler [:mkohler]]

Attachment: Patch v1.8

better now? ;)

Comment 27

[Simon Bünzli]

Comment on attachment 366679 [details] [diff] [review]
Patch v1.8

Looking good. Thanks again.

Comment 28

[Michael Kohler [:mkohler]]

Always welcome. wow, 9 patches for such a little thing..

Comment 29

[Michael Kohler [:mkohler]]

Attachment: merged (for checkin)

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a8173e5fd041

Comment 31

[Dão Gottwald [:dao]]

OS X 10.5.2 mozilla-central unit test:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_480893.js | Timed out

Backed out.

Comment 32

[Michael Kohler [:mkohler]]

Currently the test waits till the homepage is loaded. I've no idea how to go around that.

Comment 33

[Dão Gottwald [:dao]]

Don't load remote pages in tests, only local files.

Comment 34

[Simon Bünzli]

(In reply to comment #32)
Set the homepage e.g. to http://localhost:8888/ and reset it at the end of the test. If you do that, you can also verify that the homepage is indeed loaded in the second half of the test (instead of verifying that what's loaded isn't one of the about: pages).

Comment 35

[Michael Kohler [:mkohler]]

Attachment: Patch v1.10 (test works now)

This test timed out because of

> gPrefService.clearUserPref("browser.startup.page");

which threw a uncaught exception.

This is now corrected and I also set the homepage to localhost:8888.

Comment 36

[Simon Bünzli]

Comment on attachment 367480 [details] [diff] [review]
Patch v1.10 (test works now)

Two more nits for r+:

>+      // Test that starting a new session loads the homepage (set to http://localhost:8888)
>+      // if Firefox is configured to display a homepage at startup (browser.startup.page = 1)
>+      gPrefService.setCharPref("browser.startup.homepage", "http://localhost:8888");

Nit: Use a variable to hold that URL and reuse that variable later on (in case we ever want to change that homepage setting).

>+          if(gPrefService.prefHasUserValue("browser.startup.page"))

Nit: Add a space after the |if| (because it's not a function).

Comment 37

[Michael Kohler [:mkohler]]

Attachment: Patch v1.11

Comment 38

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/5771a8540ac6