723102 - [New Tab Page] Can't Hide/Show New Tab Page when closing left tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Virgil Dicu [:virgil] [QA]]

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] (inactive)]

Attachment: 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]]

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

[Jared Wein [:jaws] (please needinfo? me)]

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] (inactive)]

(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] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f2900ceb8202

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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]]

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]]

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] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20f0d78e1741

Comment 10

[Virgil Dicu [:virgil] [QA]]

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.