Last Comment Bug 884585 - "Recently closed tabs" list should exclude about:newtab
: "Recently closed tabs" list should exclude about:newtab
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 24
Assigned To: Sid
:
: Mike de Boer [:mikedeboer]
Mentors:
Depends on: 918276
Blocks: 581937
  Show dependency treegraph
 
Reported: 2013-06-18 15:36 PDT by Sid
Modified: 2013-09-20 09:50 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Treat about:newtab just like about:blank (1.02 KB, patch)
2013-06-18 17:39 PDT, Sid
no flags Details | Diff | Splinter Review
Treat about:newtab just like about:blank (1.16 KB, patch)
2013-06-18 17:46 PDT, Sid
ttaubert: review+
adw: feedback+
Details | Diff | Splinter Review
Treat about:newtab just like about:blank v2 (1.44 KB, patch)
2013-06-19 20:08 PDT, Sid
adw: review+
Details | Diff | Splinter Review
Treat about:newtab just like about:blank v2 (1.60 KB, patch)
2013-06-20 09:41 PDT, Sid
ttaubert: review+
Details | Diff | Splinter Review

Description Sid 2013-06-18 15:36:26 PDT
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 2013-06-18 17:39:01 PDT
Created attachment 764476 [details] [diff] [review]
Treat about:newtab just like about:blank
Comment 2 Sid 2013-06-18 17:46:51 PDT
Created attachment 764481 [details] [diff] [review]
Treat about:newtab just like about:blank
Comment 3 Drew Willcoxon :adw 2013-06-19 11:22:41 PDT
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] 2013-06-19 12:20:04 PDT
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] 2013-06-19 12:21:23 PDT
I forgot to say, r=me with Drew's comment addressed, of course. Thanks!
Comment 6 Sid 2013-06-19 20:08:51 PDT
Created attachment 765161 [details] [diff] [review]
Treat about:newtab just like about:blank v2
Comment 7 Drew Willcoxon :adw 2013-06-19 20:19:17 PDT
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 2013-06-19 20:23:25 PDT
Thank you all!
Comment 9 Tim Taubert [:ttaubert] 2013-06-20 07:41:26 PDT
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 2013-06-20 09:41:48 PDT
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] 2013-06-20 11:05:13 PDT
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] 2013-06-20 11:07:28 PDT
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] 2013-06-20 15:00:54 PDT
https://hg.mozilla.org/mozilla-central/rev/80cd0ed2e33c

Note You need to log in before you can comment on or make changes to this bug.