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

RESOLVED FIXED in mozilla0.9.1

Status

SeaMonkey
UI Design
P4
major
RESOLVED FIXED
17 years ago
13 years ago

People

(Reporter: Mike Pinkerton (not reading bugmail), Assigned: Samir Gehani)

Tracking

Trunk
mozilla0.9.1
PowerPC
Mac System 9.x

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Fix in hand)

Attachments

(6 attachments)

(Reporter)

Description

17 years ago
- 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

17 years ago
You're saving a file with no file extension, I presume.
Summary: File:SaveAs saves with ???? type instead of TEXT → File:SaveAs with no file extension saves with ???? type instead of TEXT
(Reporter)

Comment 2

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

Comment 3

17 years ago
mscott or law maybe?
Assignee: asa → mscott
Component: Browser-General → XP Apps

Comment 4

17 years ago
I think file save as is bill. 
Assignee: mscott → law

Comment 5

17 years ago
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...
Keywords: nsbeta1

Comment 6

17 years ago
nav triage team:

At least take a look into this, p3, nsbeta1+
Priority: -- → P3
Whiteboard: nsbeta1+

Comment 7

17 years ago
nav triage team:

Now going to get to this one now. Removing nsbeta1+, marking future and p4
Priority: P3 → P4
Whiteboard: nsbeta1+
Target Milestone: --- → Future
(Reporter)

Comment 8

17 years ago
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.
Assignee: law → pinkerton
Target Milestone: Future → mozilla0.8
(Reporter)

Comment 9

17 years ago
would anyone object to me just changing the initialization of mType in
nsLocalFileMac from '????' to 'TEXT'?
Status: NEW → ASSIGNED

Comment 10

17 years ago
Do we have a MIME type we can use in an IC lookup to get the file type?
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.
(Reporter)

Comment 12

17 years ago
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?
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.
(Reporter)

Comment 14

17 years ago
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

17 years ago
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).
(Reporter)

Comment 16

17 years ago
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.
Assignee: pinkerton → gagan
Status: ASSIGNED → NEW

Comment 17

17 years ago
Am going to let dougt investigate this for 0.9
Assignee: gagan → dougt
Target Milestone: mozilla0.8 → mozilla0.9

Comment 18

17 years ago
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.  
Assignee: dougt → law

Comment 19

17 years ago
Law won't have time during mozilla0.9, resetting target milestone
Target Milestone: mozilla0.9 → ---
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'
(Reporter)

Comment 21

17 years ago
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

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

Comment 23

17 years ago
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
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → REMIND

Comment 24

17 years ago
And the bug number for that necko bug is....?
(Reporter)

Comment 25

17 years ago
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.
Status: RESOLVED → REOPENED
Resolution: REMIND → ---

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+
Target Milestone: --- → mozilla0.9.1

Comment 26

17 years ago
Necko bug is #75758
Depends on: 75758

Comment 27

17 years ago
Handing off to Samir
Assignee: law → sgehani
Status: REOPENED → NEW
(Assignee)

Comment 28

17 years ago
Created attachment 32086 [details] [diff] [review]
Set the file type and creator based on Internet Config settings for a given MIME type.

Comment 29

17 years ago
Looks good, except that you leak contentType. Use an nsXPIDLCString, and 
getter_Copies.
(Assignee)

Comment 30

17 years ago
Created attachment 32090 [details] [diff] [review]
Patch that no longer leaks contentType.
(Assignee)

Comment 31

17 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: Fix in hand

Comment 32

17 years ago
sr=sfraser
nsLocalFileMac already has SetFileTypeFromMIMEType. Couldn't that be used
instead of putting al the IC code in here?
(Assignee)

Comment 34

17 years ago
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

17 years ago
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.
(Assignee)

Comment 36

17 years ago
Created attachment 32100 [details] [diff] [review]
Patch rev 3 with law's suggestions including handling the case where the user cancelled.
Samir, go ahead and change SetFileTypeFromMIMEType. nsILocalFileMac does not
have the "FROZEN" comment at the top. It would save some duplication of code.
(Assignee)

Comment 38

17 years ago
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()).
(Assignee)

Comment 39

17 years ago
Created attachment 32173 [details] [diff] [review]
Patch rev 4 using nsILocalFileMac::SetFileTypeAndCreatorFromMIMEType() per Conrad's suggestion.
(Assignee)

Comment 40

17 years ago
law, please r again.
sfraser, please sr again.

Thanks!

Comment 41

17 years ago
Minor comment:
+            if (contentType.get() && macFile)
you might want to check for an empty contentType string here as well.
Otherwise, sr=sfraser
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?
(Assignee)

Comment 43

17 years ago
Created attachment 32176 [details] [diff] [review]
Patch rev 5 with sfraser's suggestion to check for contentType being an empty string.
(Assignee)

Comment 44

17 years ago
Created attachment 32177 [details] [diff] [review]
Patch rev 6 eliminating redundant HasMappingForMIMEType() call per ccarlen's suggestion.

Comment 45

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

Either way, r=law.
(Assignee)

Comment 46

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

Comment 47

17 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.