Last Comment Bug 723102 - [New Tab Page] Can't Hide/Show New Tab Page when closing left tab
: [New Tab Page] Can't Hide/Show New Tab Page when closing left tab
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on:
Blocks: 723832
  Show dependency treegraph
 
Reported: 2012-02-01 07:08 PST by Virgil Dicu [:virgil] [QA]
Modified: 2012-03-08 23:27 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v1 (2.23 KB, patch)
2012-02-03 09:17 PST, Tim Taubert [:ttaubert]
jaws: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Virgil Dicu [:virgil] [QA] 2012-02-01 07:08:40 PST
Mozilla/5.0 (Windows NT 5.1; rv:13.0a1) Gecko/20120201 Firefox/13.0a1

1. Open Firefox Nightly with clean profile.
2. Enable New tab feature (browser.newtab.url--->about:newtab)
3. Restart.
4. Open two new tabs.
5. Close the left one while the second one is focused.
6. Select the Hide/Show New tab Page button in the remaining tab.

Actual result: No action is performed when the button is selected.
Comment 1 Tim Taubert [:ttaubert] 2012-02-03 09:17:52 PST
Created attachment 594211 [details] [diff] [review]
patch v1

Simple patch. I accidentally the definition of 'self'. So this was undefined and didn't properly unregister the page.
Comment 2 Dão Gottwald [:dao] 2012-02-03 10:32:10 PST
Comment on attachment 594211 [details] [diff] [review]
patch v1

>+    function unload() gAllPages.unregister(this);

unload doesn't want to return anything, so:

function unload() { gAllPages.unregister(this); }
Comment 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-02-03 14:20:07 PST
Comment on attachment 594211 [details] [diff] [review]
patch v1

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

Looks good to me. Make the change that Dao recommended and then r=me. Thanks for adding a test :)
Comment 4 Tim Taubert [:ttaubert] 2012-02-04 02:54:15 PST
(In reply to Dão Gottwald [:dao] from comment #2)
> unload doesn't want to return anything, so:
> 
> function unload() { gAllPages.unregister(this); }

Fixed.

https://hg.mozilla.org/integration/fx-team/rev/f2900ceb8202
Comment 5 Tim Taubert [:ttaubert] 2012-02-05 04:37:58 PST
https://hg.mozilla.org/mozilla-central/rev/f2900ceb8202
Comment 6 Tim Taubert [:ttaubert] 2012-02-09 13:30:45 PST
Comment on attachment 594211 [details] [diff] [review]
patch v1

[Approval Request Comment]
User impact if declined: Close new tab pages will be held alive.
Risk to taking this patch (and alternatives if risky): Very low.
String changes made by this patch: None.

Needed for New Tab Page testing. Fixes whole page leaks.
Comment 7 Chris Lee [:clee] 2012-02-09 15:41:01 PST
Alex, this sounds like a pretty key bug we also want to take on Aurora by pref'ing on New Tab per bug 716538.
Comment 8 Alex Keybl [:akeybl] 2012-02-09 15:57:11 PST
Comment on attachment 594211 [details] [diff] [review]
patch v1

[Triage Comment]
Low risk, in support of testing for bug 716538. Approving for Aurora 12.
Comment 9 Tim Taubert [:ttaubert] 2012-02-09 16:22:11 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/20f0d78e1741
Comment 10 Virgil Dicu [:virgil] [QA] 2012-02-21 06:35:03 PST
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a2) Gecko/20120220 Firefox/12.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a2) Gecko/20120221 Firefox/12.0a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a2) Gecko/20120220 Firefox/12.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:13.0a1) Gecko/13.0a1 Firefox/13.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/13.0 Firefox/13.0a1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/13.0 Firefox/13.0a1

Verified on today's Aurora and today's Nightly with steps in comment 0 on Ubuntu 11.10, Mac OS 10.7 and Windows 7.

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