64704 - File:SaveAs with no file extension saves with ???? type instead of TEXT

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P4)

Description

[Mike Pinkerton (not reading bugmail)]

- go to any webpage
- select File:Save As to save the page to disk

Expected:
- file should have type 'TEXT', since it's html

Actual:
- file has type '????' which basically makes it useless

Comment 1

[Simon Fraser [no longer active]]

You're saving a file with no file extension, I presume.

Comment 2

[Mike Pinkerton (not reading bugmail)]

of course i am ;) it's a mac. what's a file extension? ;)

Comment 3

[Asa Dotzler [:asa]]

mscott or law maybe?

Comment 4

[Scott MacGregor]

I think file save as is bill. 

Comment 5

[Bill Law]

There is another bug that's very similar (relates to some other saved file not
launching the correct application when you open it after saving).  I think maybe
Paul Chen got that one 'cause it was a Mac thing.

Nominating this as nsbeta1 so I remember to go look into this...

Comment 6

[Paul Chen]

nav triage team:

At least take a look into this, p3, nsbeta1+

Comment 7

[Paul Chen]

nav triage team:

Now going to get to this one now. Removing nsbeta1+, marking future and p4

Comment 8

[Mike Pinkerton (not reading bugmail)]

i think a quick hack could be to change the default to 'TEXT' rather than '????'
since more often than not, the data will be 'TEXT'.

I'll take this over and at least do something sensible with it.

Comment 9

[Mike Pinkerton (not reading bugmail)]

would anyone object to me just changing the initialization of mType in
nsLocalFileMac from '????' to 'TEXT'?

Comment 10

[Simon Fraser [no longer active]]

Do we have a MIME type we can use in an IC lookup to get the file type?

Comment 11

[Conrad Carlen (not reading bugmail)]

The file type stuff I did is pretty stuck without a suffix or if the code which
creates the file doesn't call nsILocalFileMac::SetFileTypeFromMIMEType. 'TEXT'
would be a better default though.

Comment 12

[Mike Pinkerton (not reading bugmail)]

i perused the code and it really looks like we're up the creek if we don't have
an extension (no mime type is discernable). I could do one of two things:

1) init to 'TEXT' and leave everything else the same (nothing changes when
trying to set type with no extension)
2) leave default as '????' and set to 'TEXT' when we try to set type w/ no extension

I like (1) personally, but it's slightly less obvious...Conrad?

Comment 13

[Conrad Carlen (not reading bugmail)]

I like (1) too. Changing the default will have to affect on the current code
which will work if we have an extension and do nothing when we don't.
What really needs to happen (longer term) is we need to make more use of
nsILocalFileMac::SetFileTypeFromMIMEType
 from the sites which create and save files. I bet most of them know what type
they're creating/saving.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Index: mozilla/xpcom/io/nsLocalFileMac.cpp
===================================================================
RCS file: /m/pub/mozilla/xpcom/io/nsLocalFileMac.cpp,v
retrieving revision 1.51
diff -u -2 -r1.51 nsLocalFileMac.cpp
--- nsLocalFileMac.cpp	2001/01/30 05:03:38	1.51
+++ nsLocalFileMac.cpp	2001/01/30 23:35:14
@@ -790,5 +790,5 @@
 ,	mHaveFileInfo(PR_FALSE)
 ,   mFollowSymlinks(PR_FALSE)
-,   mType('????')
+,   mType('TEXT')
 ,   mCreator(kDefaultCreator)
 {

can i get an sr? r=pchen

Comment 15

[Simon Fraser [no longer active]]

sr=sfraser. but it would be nice to keep this bug open, and do a better fix at 
some point (using the MIME type to get a file type).

Comment 16

[Mike Pinkerton (not reading bugmail)]

after doing some cursory digging, it doesn't appear that anyone in the "file
save" code has the mime type nearby. the file code is a weakpoint of mine,
though, so I could be horribly mistaken.

landing the 'TEXT' change in the ctor, but leaving open and reassigning back to
necko since the mime type will need to be surfaced from netlib.

Comment 17

[Gagan]

Am going to let dougt investigate this for 0.9

Comment 18

[Doug Turner (:dougt)]

This belongs with who ever owns the file saving code as it is a normal case
which may happen.  If there is no extention, and the server (http) does not send
any content type, there is no way to know what kind of a file it is.  Using
'????' may be the right thing.  What do IE and Nav4x do?

In any case, nsIFile provides a way to set the mac creator/type info, and necko
provides the content type via the nsIChannel, and there is a MIME service for
getting the content type for an extension.

Reassigning to law.  

Comment 19

[Paul Chen]

Law won't have time during mozilla0.9, resetting target milestone

Comment 20

[Conrad Carlen (not reading bugmail)]

Are we sure this is still happening? In nsLocalFileMac, the file type is
initialized to 'TEXT' not '????' as was the case. Also,
SetOSTypeAndCreatorFromExtension() will not do anything if there is no suffix so
it should remain 'TEXT'

Comment 21

[Mike Pinkerton (not reading bugmail)]

we left this open for necko to be more intelligent about setting the file type 
from the mime type, rather  than relying only on the file extension. however, 
dougt kicked it back from necko-land so.....

Comment 22

[Simon Fraser [no longer active]]

Defaulting to TEXT is better, but not enough. Plenty of folks save GIFs and JPEGs 
with no file extensions.

Comment 23

[Paul Chen]

nav triage team:

Unless we get more info from the backend, like the mime type, the front end can't 
do anything intelligent about the content type, save for content sniffing.

Marking remind because we should log a bug on necko to pass the content type 
forward

Comment 24

[Simon Fraser [no longer active]]

And the bug number for that necko bug is....?

Comment 25

[Mike Pinkerton (not reading bugmail)]

1) log the bug on necko
2) mark that bug a dependency
3) when that bug is fixed, fix this one

at no time should this bug be resolved.

Comment 26

[Kathleen :Brade]

Necko bug is #75758

Comment 27

[Bill Law]

Handing off to Samir

Comment 28

[Samir Gehani]

Attachment: Set the file type and creator based on Internet Config settings for a given MIME type.

Comment 29

[Simon Fraser [no longer active]]

Looks good, except that you leak contentType. Use an nsXPIDLCString, and 
getter_Copies.

Comment 30

[Samir Gehani]

Attachment: Patch that no longer leaks contentType.

Comment 31

[Samir Gehani]

law, please r.
sfraser, please sr.

Thanks!

Note, this fixes the case where the content type is known because the nsIChannel 
exposes this info.  The other pending problem, possibly much more scarcely 
encountered, is that when the content type is not supplied by the server, as in 
the case of a bad http server or in the case of a normal ftp server, then we need 
to sniff for at least a handful of "well known" MIME types and set the creator 
and type info accordingly.  The latter issue is being tracked by necko bug 75758 
since necko will do the sniffing and setting of the content type.  Once necko 
does the setting of the content type the attached fix should cause the correct 
creator and type to be set on the mac.  Hence, this should complete the xpfe work 
for setting the creator and type on a saved file on the mac.

Comment 32

[Simon Fraser [no longer active]]

sr=sfraser

Comment 33

[Conrad Carlen (not reading bugmail)]

nsLocalFileMac already has SetFileTypeFromMIMEType. Couldn't that be used
instead of putting al the IC code in here?

Comment 34

[Samir Gehani]

Conrad,
I recall you mentioning nsILocalFileMac::SetFileTypeFromMIMEType() but it only 
sets the file type, not the creator.  After poking lxr, I realize there are no 
clients of this API, yet.  Hence changing this API may be feasible.  But I'm not 
sure if that will work for the embedding folks, i.e., if this API is frozen.  In 
such a case we could always create a wrapper in nsILocalFileMac that does both 
and calls in to SetFileTypeFromMIMEType() for the file type.  

Comment 35

[Bill Law]

1. I think the current state-of-the-art for using services eschews:
+            NS_WITH_SERVICE(nsIInternetConfigService, icService, 
+                NS_INTERNETCONFIGSERVICE_CONTRACTID, &rv);
and recommends this, instead:
    nsCOMPtr<nsIInternetConfigService> icService(do_GetService
(NS_INTERNETCONFIGSERVICE_CONTRACTID, &rv));

2. You need to wrap all the added code with "if (mInputChannel)...".  If the 
user hits the Cancel button as the data stops coming in, then mInputChannel 
could theoretically get zeroed out by nsStreamXferOp::Stop before OnStopRequest 
is executed.  There may be error cases where that could happen, too, I suppose.

With those changes, r=law.  Good work, and thanks for pitching in.

Comment 36

[Samir Gehani]

Attachment: Patch rev 3 with law's suggestions including handling the case where the user cancelled.

Comment 37

[Conrad Carlen (not reading bugmail)]

Samir, go ahead and change SetFileTypeFromMIMEType. nsILocalFileMac does not
have the "FROZEN" comment at the top. It would save some duplication of code.

Comment 38

[Samir Gehani]

Conrad,
Also note that SetFileTypeFromMIMEType() doesn't actually set the file type on 
the file, merely the member var mType.  So the next patch will include the 
following changes:
1> Change the API name to SetFileTypeAndCreatorFromMIMEType().
2> Get the file creator as well.
3> Actually set the type and creator (call into SetFileTypeAndCreator()).

Comment 39

[Samir Gehani]

Attachment: Patch rev 4 using nsILocalFileMac::SetFileTypeAndCreatorFromMIMEType() per Conrad's suggestion.

Comment 40

[Samir Gehani]

law, please r again.
sfraser, please sr again.

Thanks!

Comment 41

[Simon Fraser [no longer active]]

Minor comment:
+            if (contentType.get() && macFile)
you might want to check for an empty contentType string here as well.
Otherwise, sr=sfraser

Comment 42

[Conrad Carlen (not reading bugmail)]

Thank you - keeps the non-XP code smaller. r=ccarlen

Out of curiosity, what's the advantage in calling HasMappingForMIMEType before
the call to FillInMIMEInfo? Wouldn't the 2nd return an error if the 1st would
have returned false?

Comment 43

[Samir Gehani]

Attachment: Patch rev 5 with sfraser's suggestion to check for contentType being an empty string.

Comment 44

[Samir Gehani]

Attachment: Patch rev 6 eliminating redundant HasMappingForMIMEType() call per ccarlen's suggestion.

Comment 45

[Bill Law]

Nitpicking, but I find
   0 != strcmp(contentType.get(), "")
kind of verbose.  Why not just
   *contentType.get()
?

Either way, r=law.

Comment 46

[Samir Gehani]

Fixed law's last suggestion in local tree (not reattaching patch unless 
explicitly requested).

Comment 47

[Samir Gehani]

Fix checked in.