581242 - The Addons Manager should open in the current tab if that tab is blank.

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Hawken Rives]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3pre) Gecko/20100722 Minefield/4.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3pre) Gecko/20100722 Minefield/4.0b3pre

I usually forget that the addons manager opens itself in a new tab, so I end up making a new tab first, then opening it. However, the addons manager then opens itself into yet another tab. It would be nice if it could use the current tab if the current tab is blank.

Reproducible: Always

Steps to Reproduce:
1. Open a new tab.
2. Open the Add-ons Manager
Actual Results:  
You now have at least two tabs open, one that is blank and one with the addons manager.

Expected Results:  
You would only have one tab, the one with the addons manager.

Comment 1

[ratman]

Confirm this bug exists in trunk builds with Windows, likely exists in Linux as well; not an issue in 3.6 branch since this is a new feature.

Comment 2

[Michael Kohler [:mkohler]]

Yes, and "Release Notes" in the Help Menu should act the same!

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dao, is this a tabbrowser or add-ons manager bug?

Comment 4

[Dão Gottwald [:dao]]

It is not an add-ons manager bug.

Comment 6

[Sindre Dammann]

Attachment: Proposed patch + test

Proposed patch: switchToTabHavingURI opens aURI in current tab when aURI isn't already open and the current tab is empty.

The test confirms that:
1. Tab doesn't change when calling BrowserOpenAddonsMgr() and current tab is empty.
2. about:addons loads in current tab

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 462410 [details] [diff] [review]
Proposed patch + test

This patch is randomly using \t, please use spaces instead. Also, "if(" should be "if (" and "someFunction( foo )" should be "someFunction(foo)".

Comment 8

[Sindre Dammann]

Attachment: Patch v2

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 462445 [details] [diff] [review]
Patch v2

>+    if (isTabEmpty(gBrowser.selectedTab))
>+      gBrowser.selectedBrowser.loadURI(aURI.spec, null, null);

", null, null" can be omitted.

>+ * Portions created by the Initial Developer are Copyright (C) 2009

2010?

>+  gBrowser.selectedBrowser.addEventListener("load", function() {
>+      let browser = blanktab.linkedBrowser;
>+      is(browser.currentURI.spec, "about:addons", "about:addons should load into blank tab.");
>+      gBrowser.removeTab(blanktab);
>+      finish();
>+    }, true);

nit: reduce the indentation in the last five lines by two spaces each

Looks good overall. Thanks!

Comment 10

[Sindre Dammann]

Attachment: Patch v3

Comment 11

[Dão Gottwald [:dao]]

This needs approval2.0+ before it can land.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 462492 [details] [diff] [review]
Patch v3

a=me

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/66474ee346e7

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/c2e0bf771eeb

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So what should be the correct behavior if the Add-ons Manager is already open and you repeat the steps from comment 0? The blank tab you have selected gets closed and the Add-ons Manager tab focused. Do we really want to close the blank tab?

Comment 16

[Hawken Rives]

(In reply to comment #15)
> So what should be the correct behavior if the Add-ons Manager is already open
> and you repeat the steps from comment 0? The blank tab you have selected gets
> closed and the Add-ons Manager tab focused. Do we really want to close the
> blank tab?

Yes, you do. We already have a prior implementation of this, the switch-to-tab-via-awesome-bar. If you type the name of a tab into a blank tab, it will switch to that tab and close the blank tab. The user therefore might already expect this behavior.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sounds reasonable. Marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So what about situations when a notification is queued and the icon is visible in the notification bar? In such a case we do not re-use the current blank tab. Gavin, what would be the correct behavior here?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #18)
> So what about situations when a notification is queued and the icon is visible
> in the notification bar? In such a case we do not re-use the current blank tab.
> Gavin, what would be the correct behavior here?

Filed it as bug 592472.