363618 - only add What's New tab on Software update restart

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Tracy Walker [:tracy]]

tested with updated from 2.0 to 2.0.0.1 on releasetest channel

Update in 2.0 can look kind of odd if the user had just their home page open when software update was run. The result of an update, because of session restore, is three tabs; Start Page, What's New and a second Start Page.

It's certainly not that big of a deal as normalcy is restored at the next browser session. It might look generally cleaner to offer the Welcome and Start pages on first install. Then, on any subsequent updates, add just the What's New tab.

Comment 1

[Marcia Knous [:marcia]]

I have this on Windows Vista as well testing the same jump that Tracy notes, it ends up looking a bit sloppy having two start pages.

Comment 2

[Paul Kim]

Tracy --

Is the startup behavior consistently that three tabs are displayed after
updating? Can you verify this for 1.5.x to 2.x updates as well? 

I ask because I've heard from Chris Beard that post 1.5->2.0 update, only two
tabs are being displayed (the What's New page, and the Firefox Start page).

We'll fix this but need to verify what's happening across version updates.

Thanks
Paul

Comment 3

[Tracy Walker [:tracy]]

it is consistent.  Posting steps (I should have done this originally, sorry)

1) clean install 2.0 (new profile)
2) launch (skipping step 3 will show 4 tabs after update)
3) shutdown, then restart to get to a single tab with the Firefox Start page.
4) Run Help | Check for updates... download the update to 2.0.0.1
5) relaunch when prompted.

tested behavior:
tab1 - Firefox Start page (Actually the User Home Page, which just happens to be the same as the start page)
tab2 - What's New Page (focused)
tab3 - Firefox Start page

expected results:
tab1 - User Home Page
tab2 - What's New Page (focused)

The bug is caused by the fact the session restore keeps any open tabs before restart and puts them back on restart. Then the additional two tabs are put in from the update. (not comment in step two above)

update from 1.5.0.x to 2.0.0.x will not show this problem, as it currently stands, because there isn't any session restore code in 1.5.0.x to remember open tabs.

minor update 2.0.0.x to 2.0.0.y should only add the What's New page (tab focused) to whatever the user had open prior to update.

Major update should display the users "home page" and the what's new page (tab focused). 

Comment 4

[Marcia Knous [:marcia]]

Just to chime in further, I did a quick test on the Mac with an existing profile, going from 2.0->2.0.0.1. Here are my steps and what I saw.

1. Launch 2.0 -> Get home pages and "Firefox Updated" page
2. Check for updates - update is found for 2.0.0.1. I apply it and Firefox restarts.
3. I now have Firefox Updated pages x2, as well as two sets of home pages.

We probably need to do something different for minor updates versus major updates in order to give a better user experience.

Comment 5

[Paul Kim]

(In reply to comment #4)
> Just to chime in further, I did a quick test on the Mac with an existing
> profile, going from 2.0->2.0.0.1. Here are my steps and what I saw.
> 
> 1. Launch 2.0 -> Get home pages and "Firefox Updated" page

Re: "home pages" -- do you have multiple pages specified as your home pages?

Also, are you seeing "Firefox Updated" each time you launch 2.0?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> Re: "home pages" -- do you have multiple pages specified as your home pages?

There's no need to have multiple home pages to reproduce this bug. When you relaunch, Firefox opens the "what's new" and your start page. In addition to that, session restore restores the last tabs you had open. Tracy is just saying that if the tab you had open when you installed the update happens to be the default start page, you'll get the start page twice: once due to session restore, and again because it's your home page. In other words, this only really affects users who upgrade their build while the default start page is showing.

Comment 7

[Paul Kim]

Adding beltzner, cbeard and mconnor to the party. 

Comment 8

[Tracy Walker [:tracy]]

 
> Also, are you seeing "Firefox Updated" each time you launch 2.0?

No, as expected restarts post update-restart display just the home page; no Firefox Updated (/2.0.0.1/whatsnew)

Basically, at software update restart, only add the Firefox Updated page (tab focused) to whatever is coming from the old application; major update - homepage(s), minor update - open tabs. But, this boils down to whether or not we really want to present the Firefox Start page as part of the relaunch-to-apply update experience. If we do want it, then this bug is invalid/won't-fix.

Comment 9

[Marcia Knous [:marcia]]

Yes, I did, and the only reason I was pointing this out is if someone has a large set of home pages bookmarked it would be even more ugly if this bug hangs around.

I don't see Firefox updated each time I launch, I see the same thing Tracy does - the situation clears after the first launch and I only see my set of homepages.

(In reply to comment #5)
> (In reply to comment #4)
> > Just to chime in further, I did a quick test on the Mac with an existing
> > profile, going from 2.0->2.0.0.1. Here are my steps and what I saw.
> > 
> > 1. Launch 2.0 -> Get home pages and "Firefox Updated" page
> 
> Re: "home pages" -- do you have multiple pages specified as your home pages?
> 
> Also, are you seeing "Firefox Updated" each time you launch 2.0?
> 

Comment 10

[Jay Patel [:jay]]

considering we are going to be pushing major update to all users in the near future, this bug would be nice to get fixed soon.  nominating for 1.8.1.3.

Comment 11

[Mike Connor [:mconnor]]

Throwing this over to Gavin, because he's the last one who touched that code. :)

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This patch hasn't been fully tested yet, but I'd like to throw it up for comments anyways, particularly for the sessionstore related parts.

1) Refactors some of the startup code to make it easier to see what's going on
2) Makes the defaultArgs getter check whether there is a pending session restore, and if so, only add the "override"/what's new page and not the homepage. It uses nsISessionStartup's "doRestore()" method for this, which I think should work reliably (other browser.js code that runs after this code already uses it for the same reason).
3) Adds a comment explaining why the override page even shows up at all, given the session store code in place to remove the default arguments when a session is restored. This confused me for a little while when I was looking into whether that was still necessary.

Simon, do you see any issues with this approach?

Comment 15

[Simon Bünzli]

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

Looks fine by me - and should even be quite branch safe. Thanks.

You might only want to consider replacing the |if (choice == ...)| bits with a switch clause as well, while you're at it.

Comment 16

[Asaf Romano (gone)]

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

>Index: browser/components/sessionstore/src/nsSessionStartup.js
>===================================================================

>+     * Note: this relies on the fact that nsBrowserContentHandler will return
>+     * a different value the first time it's getter is called after an update,
>+     * due to its needHomePageOverride() logic. We don't want to remove the
>+     * default arguments in the update case, since they include the "What's
>+     * New" page.
>+     *
>+     * Since we're garanteed to be at least the second caller of defaultArgs
>+     * (nsBrowserContentHandler calls it to determine which arguments to pass
>+     * at startup), we know that if the window's arguments don't match the
>+     * current defaultArguments, we're either in the update case, or we're
>+     * launching a non-default browser window, so we shouldn't remove the
>+     * window's arguments.
>+     */

nit: http://quotes.burntelectrons.org/2158

r=mano.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/sessionstore/src/nsSessionStartup.js 	1.17
mozilla/browser/components/nsBrowserContentHandler.js 	1.38

Comment 18

[Daniel Veditz [:dveditz]]

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

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Attachment: branch patch

Comment 20

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.21
mozilla/browser/components/sessionstore/src/nsSessionStartup.js 	1.2.2.10

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(The branch patch also included the typo fix from bug 375431)