182359 - Cookie manager: multiple windows can be opened

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Benjamin Mucci]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.3a) Gecko/20021127
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.3a) Gecko/20021127

If the cookie manager window is already opened and is in background, if I select
Tools > Cookie Manager > Manage Stored Cookie a new cookie manager window opens.

Reproducible: Always

Steps to Reproduce:
1. Open the cookie manager (Tools > Cookie Manager > Manage Stored Cookie)
2. Bring a Navigator window in foreground
3. Select again Tools > Cookie Manager > Manage Stored Cookie

Actual Results:  
Now you should have two Cookie Manager windows.

Expected Results:  
The first Cookie Manager windows should gets focus instead of having a new
Cookie Manager window.

MacOS 8.6

Comment 1

[Red Wolf]

Reproducible on Mac OS 9.1, Mozilla 1.2.1

Comment 2

[Tom Everingham]

confirming but I don't think this will cause any problems.  You can still manage
the cookie list properly.  Win NT trunk.

Comment 3

[Darin Fisher]

-> suresh

Comment 4

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.3) Gecko/20030312]

Bug still there.

Without testing, I would agree with comment 2:
*You may want to change the Severity from 'Normal' to 'Minor':
 Workaround is to close the new window and manually focus the previous one.

Same bug with:
*Tools: Form M., Image M., Password M.
 *You may want to update Summary to include all keywords;
  And possibly to change the Component.

No bug (== Allways uses one "window" only):
*Edit: Preferences...
*Tools: Download M., Web Dev. > JavaScript Console

This reminds me of ("unrelated") bug 172537 comment 3.

*NB: 'Tools > Popup M.' does not create a window icon in the task bar, nor allow
to focus back to the Browser window which was used to open it :-(
 *Should I file a new (I do not find an existing one) bug for this, or could it
be considered a part of the current bug ?

Comment 5

[benc]

Mozilla 1.f - all plats.
mvl on IRC pointed out that Cookie Manager has common code w/ Image Blocking,
but the other managers might be independent.

If you see that in other managers, you should file a bug against them in their
component.

If there is a common, underlying bug (some XUL template), then I'll mark this
bug as dependent.

Comment 6

[[e-mail]]

To do this for a non-modal window you first need to give it a windowtype
attribute. Having done this you can then use the window watcher to search for an
existing instance of the window:

function getMostRecentWindow( inType )
{
  const kWindowMediatorContractID = "@mozilla.org/appshell/window-mediator;1";
  const kWindowMediatorIID = Components.interfaces.nsIWindowMediator;
  const kWindowMediator = Components.classes[kWindowMediatorContractID]
.getService(kWindowMediatorIID);
  return kWindowMediator.getMostRecentWindow( inType );
}

If you think that the user might try to open the window twice in quick
succession (the windowtype won't be immediately valid after window.open) then
you need to save the window you have started to open as a flag and then clear it
when the window loads:

function toNewWindow(uri)
{
  // Recently opened one.
  if (uri in window)
    return;

  function newWindowLoaded(event) {
    delete window[uri];
  }
  window[uri] = window.openDialog(uri, "_blank", "chrome,all,dialog=no");
  window[uri].addEventListener("load", newWindowLoaded, false);
}

Alternatively if you need to be able to open the window with different
parameters you have a choice: either having found a window call a function to
update it, or close that window and open a new one, i.e.

function viewCookies() {
  var viewer = getMostRecentWindow( "manage:permissions" );
  if (viewer)
    viewer.close();

  window.openDialog(/* stuff */);
}

Hope this helps.

Comment 7

[Serge Gautherie (:sgautherie)]

Reply to comment 5: (on my comment 4)

*Popup Manager issue:
 *See now bug 198250 which someone else filed in the meantime.

*I filed the 3 following bugs: bug 199495 (F.M.), bug 199497 (I.M.), bug 199498
(Pa.M.).
 *I suspect a common underlying issue: but I did not find such a bug report, and
I don't have any clue on it to file one yet.

Comment 8

[Paul Wyskoczka]

Nominating...

Comment 9

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4a) Gecko/20030401]

Bug still there. (with v1.3 profile, at least)

Comment 10

[[e-mail]]

mvl, do you want to take this?

Comment 11

[Samir Gehani]

adt: nsbeta1-

Comment 12

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4b) Gecko/20030507]

Bug still there. (with v1.3 profile, at least)

Comment 13

[Dan Mellem]

There's a patch for the Download Manager in bug 131762. May be useful if both
managers are using similar code.

Comment 14

[David G King]

The patch in Bug #131762
(http://bugzilla.mozilla.org/attachment.cgi?id=79221&action=view) looks simple
and clean. I vote for moving the code that was added to nsDownloadManager.cpp to
the equivalent Cookie Manager program (with the appropriate changes of course).

If I can get some of my own outstanding patches cleared up, I might even be able
to help with this.

Comment 15

[Dan M]

This patch:

Index: extensions/cookie/resources/content/cookieOverlay.js
===================================================================
RCS file: /cvsroot/mozilla/extensions/cookie/resources/content/cookieOverlay.js,v
retrieving revision 1.14
diff -u -r1.14 cookieOverlay.js
--- extensions/cookie/resources/content/cookieOverlay.js	26 Mar 2003 02:02:42
-0000	1.14
+++ extensions/cookie/resources/content/cookieOverlay.js	25 Aug 2003 14:25:53 -0000
@@ -27,7 +27,7 @@
 }
 
 function viewCookies() {
- 
window.openDialog("chrome://communicator/content/wallet/CookieViewer.xul","_blank",
+ 
window.openDialog("chrome://communicator/content/wallet/CookieViewer.xul","_cookieManager",
                     "chrome,resizable=yes", "cookieManager");
 }  

is much simpler and will suffice. It has its drawbacks but it's a technique used
throughout the codebase.

If you do go with the WindowMediator version,

+  nsCOMPtr<nsIWindowMediator> wm =
do_GetService("@mozilla.org/rdf/datasource;1?name=window-mediator", &rv);
+  if (NS_FAILED(rv)) return rv;

oy! just do

nsCOMPtr<nsIWindowMediator> wm(do_GetService(NS_WINDOWMEDIATOR_CONTRACTID));
if (wm) {
  ...
}

Comment 16

[Darin Fisher]

-> me

danm: what are the drawbacks of the _cookieManager approach?

Comment 17

[Dan M]

I thought chrome and user script shared the same namespace, so if a window
opened by a webpage happened to have the same name, they'd collide. But it looks
like that's not happening. I wonder if someone fixed that. Nifty.

However my solution ends up reloading the cookie manager window when you
reselect its menu item, and this seems to clear out the dialog's contents.
Alright I take it back, the windowname method won't suffice. Guess you have to
go with the WindowMediator version in this case. That's a shame. The windowname
method is much simpler and supposed to work. Looks like it could stand a little
more tweaking, though.

Comment 18

[Darin Fisher]

danm: thanks for the info... care to attach a patch? ;-)

Comment 19

[Dan M]

Attachment: use windowmediator to focus extant cookie viewer

Yeah, if you don't mind waiting for it.

The reason reloading an extant cookie manager window clears out its contents is
bug 25040. Even were that fixed though, it seems suboptimal to reload the
window. So Neil's plan (comment 6) is probably best. However I don't think you
need to worry about the load listener or parameter change cases.

Since the cookie and image viewers are both the same XUL, a simple plan to
focus the most recently used window of that type will ... well, you know. Seems
best to me to keep track with both windowtype and name, as in the attached
patch. And while I was there I also reformatted to 80-space lines (*love* those
unGUI editors) and got rid of the two var declarations up top. They should be
consts but that doesn't work for some reason that I'll let you worry about if
you want. They were unused, anyway.

Comment 20

[Dan M]

There is a problem with the above patch. Should the user already have the Cookie
Manager open and then click the Privacy icon that becomes visible in the
browser's statusbar after a cookie is rejected, this patch will prevent the
Cookie Manager from hiding the Privacy icon and forcing its own Status column to
be visible (and sorting on it). Heh heh. Back to you, Bob.

Comment 21

[[e-mail]]

I've since seen how Help does it. If you have more than one Help source file
(perhaps you install an extension with its own help, such as xulmine) then each
help file opens in its own window, but if you request further help then the
correct window is reused. I think it should be possible to adapt the Help code
to resolve this issue too.

Comment 22

[[e-mail]]

Attachment: Possible patch

Comment 23

[[e-mail]]

Comment on attachment 132722 [details] [diff] [review]
Possible patch

great stuff! r=me

Comment 24

[Dan M]

That's identical in function to my patch. It's the same, except you
differentiate between the different kinds of viewers using window.arguments
instead of window.name. Comment 20 still applies.

Comment 25

[[e-mail]]

hmm, won't it open a new window with the correct "fromIcon" argument?

it might not be neat, but i'm ok with that for now... i don't really like that
icon anyway, hopefully we'll kill it soon :)

Comment 26

[Dan M]

Oh. You're so right. That does make it slightly different from my patch. However
that doesn't get rid of the problem; it makes the problem different. With this
patch, then, you still can have those two copies of the Cookie Manager dialog
open simultaneously.

Comment 27

[Darin Fisher]

Comment on attachment 132722 [details] [diff] [review]
Possible patch

sr=darin

Comment 28

[[e-mail]]

Fix checked in.

Comment 29

[benc]

V/fixed:
mozilla 1.84a, mac os x.

Still brings up multiple versions as described by DanM in #26. New bug?