The default bug view has changed. See this FAQ.

"Recently closed tabs" list should exclude about:newtab

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Sid, Assigned: Sid)

Tracking

Trunk
Firefox 24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 581937
(Assignee)

Updated

4 years ago
Component: Untriaged → Session Restore
(Assignee)

Comment 1

4 years ago
Created attachment 764476 [details] [diff] [review]
Treat about:newtab just like about:blank
Attachment #764476 - Flags: review?
(Assignee)

Comment 2

4 years ago
Created attachment 764481 [details] [diff] [review]
Treat about:newtab just like about:blank
Attachment #764476 - Attachment is obsolete: true
Attachment #764476 - Flags: review?
Attachment #764481 - Flags: review?

Comment 3

4 years ago
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, ..."
Attachment #764481 - Flags: review?(ttaubert)
Attachment #764481 - Flags: review?
Attachment #764481 - Flags: feedback+

Updated

4 years ago
Assignee: nobody → sidrabbit
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: "Recently closed tabs" list keep about:newtab tabs → "Recently closed tabs" list should exclude about:newtab
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!
Attachment #764481 - Flags: review?(ttaubert) → review+
I forgot to say, r=me with Drew's comment addressed, of course. Thanks!
(Assignee)

Comment 6

4 years ago
Created attachment 765161 [details] [diff] [review]
Treat about:newtab just like about:blank v2
Attachment #764481 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #765161 - Flags: review?(ttaubert)

Comment 7

4 years ago
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.
Attachment #765161 - Flags: review?(ttaubert) → review+

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 8

4 years ago
Thank you all!
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
Keywords: checkin-needed
(Assignee)

Comment 10

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

Thanks for the tip. Is it OK now?
Attachment #765161 - Attachment is obsolete: true
Attachment #765427 - Flags: review?
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!
Attachment #765427 - Flags: review? → review+
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
https://hg.mozilla.org/mozilla-central/rev/80cd0ed2e33c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

4 years ago
Depends on: 918276
You need to log in before you can comment on or make changes to this bug.