884585 - "Recently closed tabs" list should exclude about:newtab

Status: RESOLVED FIXED

Description

[Sid]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130618 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130618031335

Steps to reproduce:

1. Start Nightly
2. Open New Tab
3. Close the New Tab
4. History > Recently Closed Tabs


Actual results:

"Recently closed tabs" listed New Tab (which is about:newtab by default)


Expected results:

Should not

Comment 1

[Sid]

Created attachment 764476 [details] [diff] [review]
Treat about:newtab just like about:blank

Comment 2

[Sid]

Created attachment 764481 [details] [diff] [review]
Treat about:newtab just like about:blank

Comment 3

[Drew Willcoxon :adw]

Comment on attachment 764481 [details] [diff] [review]
Treat about:newtab just like about:blank

Thanks for the patch!

This looks fine to me, but I'd like to run it by Tim first.  Tim, you're working on SessionStore now, right?

>diff --git a/browser/components/sessionstore/src/SessionStore.jsm b/browser/components/sessionstore/src/SessionStore.jsm
>--- a/browser/components/sessionstore/src/SessionStore.jsm
>+++ b/browser/components/sessionstore/src/SessionStore.jsm
>@@ -4052,18 +4052,19 @@ let SessionStoreInternal = {
>    * @returns boolean
>    */
>   _shouldSaveTabState: function ssi_shouldSaveTabState(aTabState) {
>     // If the tab has only the transient about:blank history entry, no other
>     // session history, and no userTypedValue, then we don't actually want to
>     // store this tab's data.

Could you please update this comment now that about:newtab is excluded too?  For example, "If the tab has only a transient about: history entry, ..."

Comment 4

[Tim Taubert [:ttaubert]]

Comment on attachment 764481 [details] [diff] [review]
Treat about:newtab just like about:blank

Review of attachment 764481 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

Comment 5

[Tim Taubert [:ttaubert]]

I forgot to say, r=me with Drew's comment addressed, of course. Thanks!

Comment 6

[Sid]

Created attachment 765161 [details] [diff] [review]
Treat about:newtab just like about:blank v2

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 765161 [details] [diff] [review]
Treat about:newtab just like about:blank v2

Review of attachment 765161 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating it.  I'll carry over Tim's r+, r=ttaubert.

Comment 8

[Sid]

Thank you all!

Comment 9

[Tim Taubert [:ttaubert]]

Sid, can you please format your patch so that we can check it in? It currently lacks the necessary header information, like the patch author and patch name. Here's a great blog post that describes how to prepare your patch for "checkin-needed":

http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 10

[Sid]

Created attachment 765427 [details] [diff] [review]
Treat about:newtab just like about:blank v2

Thanks for the tip. Is it OK now?

Comment 11

[Tim Taubert [:ttaubert]]

Comment on attachment 765427 [details] [diff] [review]
Treat about:newtab just like about:blank v2

Review of attachment 765427 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this looks great. Thank you!

Comment 12

[Tim Taubert [:ttaubert]]

I landed this on the fx-team branch, it will be in Nightly by roughly tomorrow:

https://hg.mozilla.org/integration/fx-team/rev/80cd0ed2e33c

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80cd0ed2e33c