Closed Bug 149841 Opened 19 years ago Closed 19 years ago

It should NOT be possible to open cert manager twice at the same time

Categories

(Core Graveyard :: Security: UI, defect, P3)

Other Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
Future

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

Attachments

(1 file, 2 obsolete files)

Go to Edit/Prefs/Security/Certificates. Click "manage certificates". The
certificate manager opens. Move away the new window, but do not close it. Find
the open prefs window. Click again on "manage certificates".

Actual behaviour: Another certificate manager window will open. If you modify
one window by deleting or importing certs, the other open cert cert window will
not disappear. (In addition, it will most like crash, when you delete a cert in
one window, and try to work with that cert in the other open window).

Expected behaviour: When you click the "manager certificates" button again,
there should not open another window. Instead, the already open window should
come to the front.
Forgot the word NOT in the summary, adding.
Summary: It should be possible to open cert manager twice at the same time → It should NOT be possible to open cert manager twice at the same time
Keywords: nsbeta1
-> me
Assignee: ssaux → kaie
Keywords: nsbeta1nsbeta1+
Attached patch Patch v1 (obsolete) — Splinter Review
Javi, can you please review?
Comment on attachment 93001 [details] [diff] [review]
Patch v1

does calling focus really make it so that you can't open a new one?  Shouldn't
we look at making the window modal instead?
Calling focus is just a secondary thing to make it work. I do it to bring an
already open window to front.

The real trick is the changed window.open() call. By giving the window a name,
subsequent calls to window.open() will re-use the previous window, and not open
a second one.
Comment on attachment 93001 [details] [diff] [review]
Patch v1

In that case, r=javi
Comment on attachment 93001 [details] [diff] [review]
Patch v1

marking patch as has-review
Attachment #93001 - Flags: review+
Alec, can you please review?
Comment on attachment 93001 [details] [diff] [review]
Patch v1

hmm.. can't you just make this modal? I'm not sure how focusing the window will
have the desired effect.
Alec, I believe it was a design decision to make the certificate non modal.

I was not involved in that decision, it happened before I worked on PSM. We
would need to find the people involved in the original decision, but I believe
making it modal is not wanted.

My patch is just a small effort to make the existing behaviour more robust.

Alec, if you know of a function "bringToFront()", please let me know. I could
not find any, but it would probably more sense. At least on Linux, this function
brought the window to front when hidden behind other windows.

In any way, the major motivation is to avoid that certificate manager gets
opened multiple times.
all I mean is that the focus stuff in the patch doesn't do anything - newly
opened windows already have focus, and this doesn't prevent me from opening a
second window.

Ah, now I see why it appears that you avoided opening a new window! It has
nothing to do with the w.focus(). The reason is that the patch adds the window
target in openCertManager() - so that when you call window.open, the window will
re-open in the same chrome window..a good test would be to change something in
the window without saving anything, and then try to open the window again - what
you should find is that any changes you made were reset.

I think you're going to have to cache the result of window.open or something,
and clear it when the window closes. You could also try the window cycling code
that the taskbar uses (the mozilla taskbar, not the windows taskbar) to find the
most recently used window of a given type.
*** Bug 162034 has been marked as a duplicate of this bug. ***
I agree. Probably the most similar code I can re-use will be the logic for the
preferences window, since it is modeless, too.
Priority: -- → P3
Target Milestone: --- → Future
Attached patch patch (obsolete) — Splinter Review
this patch finds the most resent opened window and focuses on it.
Attachment #115351 - Flags: review?(kaie)
Comment on attachment 115351 [details] [diff] [review]
patch

Great patch, thanks!
I already tried it, it works for me.
r=kaie
Comment on attachment 93001 [details] [diff] [review]
Patch v1

marking my earlier patch as obsolete
Attachment #93001 - Attachment is obsolete: true
Attachment #115351 - Flags: review?(kaie) → review+
Attached patch patchSplinter Review
only modifies the comment a little since I directly copied the code from the
preference window code.
Attachment #115351 - Attachment is obsolete: true
Comment on attachment 115353 [details] [diff] [review]
patch

take over r=kaie and seeking sr=?
Attachment #115353 - Flags: superreview?(alecf)
Attachment #115353 - Flags: review+
Comment on attachment 115353 [details] [diff] [review]
patch

sr=alecf
Attachment #115353 - Flags: superreview?(alecf) → superreview+
checked in. Thanks!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Verified fixed.
Status: RESOLVED → VERIFIED
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.