566593 - Add-ons Manager window shows multiple times

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Worcester12345]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100517 SeaMonkey/2.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100517 SeaMonkey/2.1a2pre

When going to the Add-ons Manager, then to another window, then to Add-ons Manager again, it opens a new window instead of changing to the already open one.

Reproducible: Always

Steps to Reproduce:
1.Open Add-ons Manager
2.Switch to another window
3.Open Add-ons Manager

Actual Results:  
This will open a second Add-ons Manager window.

Expected Results:  
It should switch to the already open Add-ons Manager window.

No additional information.

Comment 1

[Justin Wood (:Callek)]

Thank you for the report, It is a known issue I plan to address. But having this as an open bug saves me a step anyway!

Comment 2

[Robert Kaiser]

Could we just make goAbout() implement the logic Firefox has in switchToTabHavingURI()? I think that probably all about pages want this - we can make it listen to a param, though, of course.

Comment 3

[Justin Wood (:Callek)]

requesting blocking; (I am not sure if we need this to block a2, or if it can slip to a3)

Comment 4

[erpman1]

(In reply to comment #3)
> requesting blocking; (I am not sure if we need this to block a2, or if it can
> slip to a3)

too late for it to block a2 as SM 2.1a2 had been already released a few days ago.  SM 2.1a2 also has the multiple addon manager window problem as I've checked myself.

Comment 5

[Robert Kaiser]

That's why it's not marked as blocking a2, but blocking a3 instead.

Comment 6

[Worcester12345]

Was this fixed? I just checked in on my bugs, and don't really remember seeing this lately.

Build identifier: Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b2pre) Gecko/20100720 SeaMonkey/2.1a3pre

Comment 7

[Justin Wood (:Callek)]

I have not done any work [yet] to prevent this, but its really now, "Addons manager tab shows multiple times". Though this is still on my plate.

Comment 8

[Misak Khachatryan]

Also, IMHO addon manager tab shoud be focused when opened, regardless of tab settings.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #8)
> Also, IMHO addon manager tab shoud be focused when opened, regardless of tab
> settings.

+1. If the Add-on Manager was still a window and we opened that in the background that wouldn't make sense either, would it? Opening a tab in the background means that the user might want to look at it later. I doubt many people would like to do that with something they opened from the menus (directly or via a shortcut, once that is implemented).

Comment 10

[Worcester12345]

Feel free to migrate or change this bug as you please. You may even close it, as I don't experience it as far as I can tell any more.

Comment 11

[Ian Neal]

Attachment: port switchToTabHavingURI to SM patch v0.1

This patch:
* Ports switchToTabHavingURI to SM
* Changes toEM to use switchToTabHavingURI

There may still be a few bugs left to iron out...

Comment 12

[Ian Neal]

Attachment: port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

Changes since v0.1:
* Use newURI instead of makeURI so it works from Error Console

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 459659 [details] [diff] [review]
port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

>+  function switchIfURIInWindow(aWindow) {
>+    if (!("gBrowser" in aWindow))
>+      return false;

To me this might be concerning. Has this been tested when opened from our shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.

I suspect it would only work when opened in Navigator, and it should work from at the _least_ Mailnews. I'd argue tools->Addons should _always_ work for us when able to be selected. So I'm not sure a direct port from firefox is possible here.

Comment 14

[Ian Neal]

(In reply to comment #13)
> Comment on attachment 459659 [details] [diff] [review]
> port switchToTabHavingURI to SM with newURI patch v0.1a
> 
> >+  function switchIfURIInWindow(aWindow) {
> >+    if (!("gBrowser" in aWindow))
> >+      return false;
> 
> To me this might be concerning. Has this been tested when opened from our
> shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.
> 
> I suspect it would only work when opened in Navigator, and it should work from
> at the _least_ Mailnews. I'd argue tools->Addons should _always_ work for us
> when able to be selected. So I'm not sure a direct port from firefox is
> possible here.

Yes, I have tested from all the listed "shipped-mini-applications" even for mailnews using -mail on startup and it works. One of the things I tried before posting my first patch on here.

Comment 15

[Robert Kaiser]

(In reply to comment #13)
> Comment on attachment 459659 [details] [diff] [review]
> port switchToTabHavingURI to SM with newURI patch v0.1a
> 
> >+  function switchIfURIInWindow(aWindow) {
> >+    if (!("gBrowser" in aWindow))
> >+      return false;
> 
> To me this might be concerning. Has this been tested when opened from our
> shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.

If you read through to code to see when this function is actually called, you'll see that this is perfectly fine after all.

Comment 16

[Justin Wood (:Callek)]

(In reply to comment #15)
> (In reply to comment #13)
> > Comment on attachment 459659 [details] [diff] [review] [details]
> > port switchToTabHavingURI to SM with newURI patch v0.1a
> > 
> > >+  function switchIfURIInWindow(aWindow) {
> > >+    if (!("gBrowser" in aWindow))
> > >+      return false;
> > 
> > To me this might be concerning. Has this been tested when opened from our
> > shipped-mini-applications. e.g. MailNews, Chatzilla, Composer, Venkman, etc.
> 
> If you read through to code to see when this function is actually called,
> you'll see that this is perfectly fine after all.

Bah right, stupid nested functions threw me for a loop on my quick-skim.  I also suspect this may fix: Bug 508223 (after that bugs prefs land) due to noticing the difference in http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/extensions/test/browser/head.js?mark=90-90,97-97#88

Comment 17

[Justin Wood (:Callek)]

Comment on attachment 459659 [details] [diff] [review]
port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

nit add to this patch: 
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js?mark=7706-7708#7690

>+  // This can be passed either nsIURI or a string.
>+  if (!(aURI instanceof Components.interfaces.nsIURI))
>+    aURI = Services.io.newURI(aURI, null, null);

I like this use of the Services module rather than using makeURI like firefox does

>+  if (aOpenNew) {
>+    let browser = openUILinkIn(aURI.spec, "tab");

I'm not convinced actually that openUILinkIn is what we want (we might want to force-focus the tab, similar to Firefox here since it is obvious in most cases the user WANTS to see the addon manager right away), but frankly its how we handle it today, so sounds good anyway :-)

>+    if (aCallback) {
>+      // let browser = tabWindow.selectedBrowser;

nit: kill this comment entirely.

Otherwise good to land!

Comment 18

[Justin Wood (:Callek)]

(In reply to comment #17)
> Comment on attachment 459659 [details] [diff] [review]
> port switchToTabHavingURI to SM with newURI patch v0.1a
> 
> nit add to this patch: 
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js?mark=7706-7708#7690
> 

actually we don't have an impl of isTabEmpty() yet. And http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#6700 looks like it would need a review on our side, so you can ignore this one for now, at your discretion.

Comment 19

[Ian Neal]

Comment on attachment 459659 [details] [diff] [review]
port switchToTabHavingURI to SM with newURI patch v0.1a [Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/adc6cec598be

Comment 20

[Ian Neal]

(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 459659 [details] [diff] [review] [details]
> > port switchToTabHavingURI to SM with newURI patch v0.1a
> > 
> > nit add to this patch: 
> > http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js?mark=7706-7708#7690
> > 
> 
> actually we don't have an impl of isTabEmpty() yet. And
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#6700
> looks like it would need a review on our side, so you can ignore this one for
> now, at your discretion.

I'm not sure that we would want to delete the previous empty tab, but as you said that would need a review to implement isTabEmpty(). At the moment, I don't think we need it, but if you think either/both is required, please log bug(s) for it/them. I would say outside scope of this bug though.

Comment 21

[Robert Kaiser]

Please make sure you file a followup on isTabEmpty() and using it in that function. I suppose that's another of the tab API compat issues we want to fix for 2.1 if possible.

Comment 22

[Philip Chee]

> +      browser.addEventListener("pageshow", function(event) {
> +        if (event.target.location.href != aURI.spec)
> +          return;
> +        browser.removeEventListener("pageshow", arguments.callee, true);

Doesn't this mean that in one code path, the event listener is never removed?

Comment 23

[Justin Wood (:Callek)]

(In reply to comment #22)
> > +      browser.addEventListener("pageshow", function(event) {
> > +        if (event.target.location.href != aURI.spec)
> > +          return;
> > +        browser.removeEventListener("pageshow", arguments.callee, true);
> Doesn't this mean that in one code path, the event listener is never removed?

No, this is here to catch the pageshow event that might fire on the about:blank of a new tab, once we get the addonmgr url (or really whatever url we want) we remove the handler.