220773 - Cancel overwrites data if an existing mime type is added or changed in helper applications

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Daniel de Wildt]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20030916
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20030916

A warning popup dialog opens if you try to add a mime type that already exists.
The new entered data will overwrite the existing mime information even if this
dialog is canceled.

Reproducible: Always

Steps to Reproduce:
1. Open Preferences->Navigator->Helper Applications
2. Add a new file type
3. Enter an none existing mime type with some description and an action.
4. Enter the same mime type is in 3. with an other description and/or action.
5. An dialog appears, saying that this MIME type exists and asking if I want to
replace it.
6. Press cancel.

Actual Results:  
The field 'file type details' for the file type shows the data entered in step 4.

Expected Results:  
The field should show the data entered in step 3.

Comment 1

[Matthias Versen [:Matti]]

The panels itself doesn't belong into "preferences"

Comment 2

[Daniel de Wildt]

Bug 201692 has nearly the same description, with the exception that the bug
described there is only reproducible when creating an existing entry.


*** This bug has been marked as a duplicate of 201692 ***

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

no, this bug is about another cancel button

Comment 4

[Daniel de Wildt]

This bug still exists in
Mozilla/5.0 (Windows; U; WinNT4.0; de-AT; rv:1.6) Gecko/20040113

The data of an existing mime type are also overwritten if you press cachel when
you try to change a mime type to an existing mime type.

The check for an existing mime type and the confirmation dialogs are opened in
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/components/prefwindow/resources/content/pref-applications-edit.xul&rev=1.44&root=/cvsroot&mark=328-347#328
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/components/prefwindow/resources/content/pref-applications-new.js&rev=1.26&root=/cvsroot&mark=108-121#108

Comment 5

[Boris Zbarsky [:bzbarsky]]

Shouldn't we "return" after the window.close() or something?

Comment 6

[Daniel de Wildt]

Seems to be the same as bug 88429.
The fix in that bug corrects adds the return in pref-applications-new.js
The javascript executed here is in pref-applications-edit.xul (says the
javascript debugger). I don't have CVS access on my computer, so I couldn't
create a patch. But I think it's a one (or three) liner like the patch in bug 88429.

Comment 7

[Daniel de Wildt]

I modified my local comm.jar and the following changes works fine. I replaced
the marked lines in
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/components/prefwindow/resources/content/pref-applications-edit.xul&rev=1.44&root=/cvsroot&mark=336-337#328
by

if (!replace)
{
  window.close();
  return false;
}

Comment 8

[Boris Zbarsky [:bzbarsky]]

Daniel, you may want to look at http://www.gerv.net/software/patch-maker/ --
that lets you create patches based on modifications to your jars without having
CVS access...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Daniel's proposed change

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140372 [details] [diff] [review]
Daniel's proposed change

sr=bzbarsky

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 140372 [details] [diff] [review]
Daniel's proposed change

r=biesi

however, would it suffice to just "return true"?

Comment 12

[Daniel de Wildt]

I don't know exactly when which result is expected from onAccept, but in two
other cases the function also a returns 'false' if the values are not changed
(missing values and internal handled mime types). At this moment onAccept only
returns true if the changes are done. 
So I think 'return false' would make sense.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

what I meant was: Does a return value of "true" mean "close this window" while
false mean "don't close"? because in that case, the window.close() call would
not be needed.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 140372 [details] [diff] [review]
Daniel's proposed change

neil confirmed that "return true" means "close this window", false mean "don't
close". I'm retracting my review.

Comment 15

[Daniel de Wildt]

Attachment: Patchmaker patch with change as mentioned in comment 11

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 140405 [details] [diff] [review]
Patchmaker patch with change as mentioned in comment 11

sorry for being unclear... I meant remove window.close and use return true
instead.

Comment 17

[Daniel de Wildt]

Attachment: Patchmaker patch with change as mentioned in #16

Tested and works.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 140413 [details] [diff] [review]
Patchmaker patch with change as mentioned in #16

thanks. r=biesi

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140413 [details] [diff] [review]
Patchmaker patch with change as mentioned in #16

sr=bzbarsky

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in.