92750 - After modifying a new composer document, quitting the app doesn't ask to save changes

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Chris Petersen]

Build: 2001072905
Platform: Mac OS X
Expected Results: Save as dialog should appear
What I got: Application quits without prompting to save

Steps to reproduce:

1) Launch Mozilla.

2) Select Composer from the Tasks menu.

3) In the new document, type some text.

4) Press Command-Q to quit.

Comment 1

[Chris Petersen]

Appears to be Mac OS X only issue since I can't reproduce under Mac OS 9.1.

Comment 2

[rubydoo123]

giving this to Simon since it is Mac and cc brade

Comment 3

[Simon Fraser [no longer active]]

What's up here guys?

Comment 4

[Simon Fraser [no longer active]]

Here's what I think is happening here.

Command-Q is picked up by the Mac OS X menu, not our File menu. Someone probably 
hooked that up to call appShellService::Quit(), which closes all open windows. 
However, this code path bypasses all the 'dirty window' logic that lives in the 
JS.

I think a similar thing would happen if you sent Mozilla a 'quit' Apple Event.

So, the real issue is that there are ways into the Quit code path that bypass the 
dirty window check logic. Since that lives in JS, we need something we can call 
from C++ that can call that JS logic. Or we move that logic into C++. I think the 
correct way to handle this would be to have quitting happen via a cmd_quit 
command, and have a way to execute this command from C++.

This is not my bug.

Comment 5

[Mike Pinkerton (not reading bugmail)]

actually on osx, the quit menu item in the app menu sends a 'quit' apple event, 
IIRC.

Comment 6

[Chris Petersen]

*** Bug 100647 has been marked as a duplicate of this bug. ***

Comment 7

[Conrad Carlen (not reading bugmail)]

Attachment: a JS component to do this

Comment 8

[Conrad Carlen (not reading bugmail)]

Something I did for something else and haven't put to use yet: One way to make
JS callable from C++ is to wrap it in a little JS component. Then, from JS or
C++ it's equally easy to just create an instance and call its 1 method. Really I
just wanted to make a JS component but it could be useful.

Comment 9

[Simon Fraser [no longer active]]

This is data loss. We need to consider it for the branch.

Comment 10

[Simon Fraser [no longer active]]

This bug also bites on Mac OS 9. If you shut down your computer with an open 
modified composer window (or mail composer) up, we'll quit without prompting you 
to save it. The reason is the same -- going through the Quit Apple Event code 
doesn't do the dirty window stuff.

Comment 11

[Conrad Carlen (not reading bugmail)]

So, is the JS component approach reasonable? If so, it's pretty easy to hook up.

Comment 12

[Dan M]

Yes, please, if you have it ready to go. I believe that this bug will evaporate 
if I fix bug 84256, and that's the general solution I still want to try. I won't 
have it ready any time soon, though, and this bug is a data loss problem, so 
please take the JS component route for now.

Comment 13

[Conrad Carlen (not reading bugmail)]

Huh, I just found this: bug 99318 with this comment:

It also fixes another (albeit non-turbo-specific) problem,
that if the user chose to quit and then, say, pressed Cancel in the `Do you want
to save before existing' dialog of Composer, remaining windows didn't close.

AFAIK, if you press Cancel in a Save/Don't Save/Cancel dialog while quitting, it
stops the app from quitting. Isn't that standard behavior the world over?

Comment 14

[lchiang]

Sujay or petersen - can you see if this is a regression from 6.1?  The bug was
reported around the time of 6.1 ship, but not sure if it was trunk or branch at
that time. Thanks.

Comment 15

[Conrad Carlen (not reading bugmail)]

Attachment: full patch

Comment 16

[Conrad Carlen (not reading bugmail)]

The patch uses the nsCloseAllWindows component from globalOverlay.js as well as
where the Quit AppleEvent is handled so we have the same logic used everywhere.
I did make the behavior what I think is standard - if you cancel from a save
changes dialog, it stops quitting.

Dan, Simon - Can you review?

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 50871 [details] [diff] [review]
full patch

looks good, r=pink

Comment 18

[Conrad Carlen (not reading bugmail)]

Attachment: update to full patch

Comment 19

[Conrad Carlen (not reading bugmail)]

The update has two things:
1) The new license - I did that to all the new files but no need posting a new
diff for that detail
2) Instead of enumerating the windows in the order they were created (wrong,
visually distracting, and non-standard), I'm now using
nsIWindowMediator::GetZOrderDOMWindowEnumerator to close them from front to back.

Dan - can you think of any strangeness that closing them front-to-back may cause? 

Comment 20

[Blake Ross]

You're correct, that is standard behavior.  Please ensure that Composer closes 
when in turbo and quitting, when you have and do not have changes to save.

Comment 21

[Simon Fraser [no longer active]]

Comment on attachment 50887 [details] [diff] [review]
update to full patch

sr=sfraser

In the long term, I don't like this solution; it seems wrong to have a component whose job it is to close all windows; that task should lie with appshell or something. I think, longer term, the window closing logic should move into appshell C++ code (where we close windows without asking, from C++).

Comment 22

[Conrad Carlen (not reading bugmail)]

Simon, I agree. File a bug on me for it. I need a better way of doing this soon.

Comment 23

[Dan M]

(Conrad: The z-order enumerators should work fine, used in a loop closing 
windows. It'll be an ugly show behind the curtain, but it should work. If not, it 
should be easy to spot and shouldn't be tough to fix.)

Comment 24

[Judson Valeski]

plussing. Conrad can you ETA this?

Comment 25

[Chris Petersen]

Confirming issue also occurs in the OS X 6.1 preview release.

Comment 26

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 27

[Conrad Carlen (not reading bugmail)]

Attachment: smaller patch

Comment 28

[Conrad Carlen (not reading bugmail)]

In the last minute, I found that, since a new js component was being added, the
packages had to be updated. This convinced me that adding this component to all
platforms was wrong. This bug was only on the Mac and the js component is a
temporary measure. Just getting this thing in the build so it could be called
from globaloverlay.js meant changing 4 makefiles and packages. Yuck. The latest
patch is the same code but only used on the Mac from C++.

Comment 29

[Simon Fraser [no longer active]]

Comment on attachment 51255 [details] [diff] [review]
smaller patch

sr=sfraser

Comment 30

[Kathleen :Brade]

Comment on attachment 51255 [details] [diff] [review]
smaller patch

r=brade

Comment 31

[Conrad Carlen (not reading bugmail)]

Fix checked into branch. I'll do trunk before monday.

Comment 32

[Conrad Carlen (not reading bugmail)]

Checked into trunk this morning - bugzilla was down.

Comment 33

[Chris Petersen]

Verifying on the Oct 2 branch OS X build.

Comment 34

[Chris Petersen]

Verified on Oct 3 trunk build (2001-10-03-21)