Closed
Bug 845893
Opened 12 years ago
Closed 12 years ago
window.close() on a private window doesn't pass through warnAboutClosingWindow
Categories
(Firefox :: Private Browsing, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: jdm, Assigned: jdm)
References
Details
Attachments
(1 file)
Therefore we don't get any last-pb-context-exiting warning.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #719103 -
Flags: review?(ttaubert)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → josh
Comment 2•12 years ago
|
||
At some point we should probably stop having more than 285 ways of closing windows. :(
Comment 3•12 years ago
|
||
Comment on attachment 719103 [details] [diff] [review]
Make window.close() on a private chrome window trigger last-pb-context-exiting.
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>+window.addEventListener('DOMWindowClose', function windowShouldClose(ev) {
>+ // Usually window.close() on a chrome window is not blockable. If it's a
>+ // private window, however, we want to prevent potential dataloss.
>+ if (PrivateBrowsingUtils.isWindowPrivate(window) &&
>+ !warnAboutClosingWindow()) {
>+ ev.preventDefault();
>+ }
>+ window.removeEventListener('DOMWindowClose', windowShouldClose, false);
You don't actually want to remove this listener, do you? Otherwise it looks to me like the second window.close() call for a given window will be broken.
Assignee | ||
Comment 4•12 years ago
|
||
Oops, in my head there was an early return in the if block.
Comment 5•12 years ago
|
||
(In reply to :Ehsan Akhgari from comment #2)
> At some point we should probably stop having more than 285 ways of closing
> windows. :(
Right, and it seems like this bug wants to add another one.
As far as I remember, window.close isn't handled (e.g. no warnings for the user, no browser-lastwindow-close-* notifications) because UI code shouldn't be calling it in the first place. So, what code is calling window.close?
Comment 6•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Ehsan Akhgari from comment #2)
> > At some point we should probably stop having more than 285 ways of closing
> > windows. :(
>
> Right, and it seems like this bug wants to add another one.
Yeah, that was my first thought, too. It's bad the we can get into strange states when some code calls window.close() but I think this should maybe be handled by PrivateBrowsingUtils? Can't we make that keep track of how many private windows there are and send a notification if necessary?
I know that this is more like some tools instead of a service that wants to keep track but OTOH these last-pb-context-exiting/exited notification seem actually weird to me now that we have per-window private browsing.
Assignee | ||
Comment 7•12 years ago
|
||
If everyone agrees that we don't deal with window.close, then we can just WONTFIX this. I was calling it in a test and lost an hour to this problem.
Comment 8•12 years ago
|
||
(In reply to comment #5)
> (In reply to :Ehsan Akhgari from comment #2)
> > At some point we should probably stop having more than 285 ways of closing
> > windows. :(
>
> Right, and it seems like this bug wants to add another one.
>
> As far as I remember, window.close isn't handled (e.g. no warnings for the
> user, no browser-lastwindow-close-* notifications) because UI code shouldn't be
> calling it in the first place. So, what code is calling window.close?
Erm I thought this is being triggered by a web page calling window.close(), is that not the case?
Comment 9•12 years ago
|
||
(In reply to comment #7)
> If everyone agrees that we don't deal with window.close, then we can just
> WONTFIX this. I was calling it in a test and lost an hour to this problem.
No, hold on. Unless I'm missing something, we should absolutely fix this bug, as content can close the windows it has opened.
Assignee | ||
Comment 10•12 years ago
|
||
I believe this is only a problem when calling close on a chrome window object.
Comment 11•12 years ago
|
||
(In reply to comment #10)
> I believe this is only a problem when calling close on a chrome window object.
Ah ok. In that case... maybe we can just wontfix this? :/
Updated•12 years ago
|
Attachment #719103 -
Flags: review?(ttaubert)
Comment 12•12 years ago
|
||
Please reopen if you disagree.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•