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