235505 - Make nsIMIMEInfo::MIMEType readonly

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

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

Reasoning: The default application is now immutable. So one instance of a
mimeinfo will always have the same default application, even though the mimetype
can be changed. But this new mime type may actually have a different default
application. I'd like to avoid that situation.

Comment 1

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

(this requires a few changes to the helper application preference code)

Comment 2

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

hm, comment 1 is actually not true, I wonder why...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Because it doesn't use MIME infos.  It's eeeevil.

How do we plan to make it work once it _does_ use MIME infos?

Comment 4

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

(In reply to comment #3)
> Because it doesn't use MIME infos.  It's eeeevil.

ew, right, forgot about that...

> How do we plan to make it work once it _does_ use MIME infos?

get a new mimeinfo from the mimeservice for the new type.
changing the mime type should be a rare thing, I imagine...

Comment 5

[Boris Zbarsky [:bzbarsky]]

That works, I guess.  As long as we keep the extensions writable...

Comment 6

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

note to self: exthandler sets the mimetype in these places: (with a small patch)
GetMIMEInfoForExtensionFromDS
GetFromTypeAndExtension:
  DS lookup via extension
  Extras lookup via extension
GetMIMEInfoForExtensionFromExtras

Comment 7

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

Attachment: something along this lines, maybe (DO NOT CHECK IN)

I'm thinking of something like this.

needs a bit of work - GetTypeFromExtension needs to look at the RDF datasource
as well; and beos and mac won't compile with this patch.

Comment 8

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

Attachment: like this

ok, this should work

Comment 9

[Boris Zbarsky [:bzbarsky]]

It might take me a week or so to get to this...  I spent a good chunk of
yesterday reviewing.  :(

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149107 [details] [diff] [review]
like this

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>+PRBool nsExternalHelperAppService::GetTypeFromDS(const nsACString& 

Wouldn't it be better to return the actual error codes instead of turning them
all into PR_FALSE?

This is fine either way, in any case.

>Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp

>@@ -263,21 +267,22 @@ nsOSHelperAppService::GetMIMEInfoFromOS(
>+    nsMIMEInfoMac* mimeInfoMac = new nsMIMEInfoMac();
>+    if (!mimeInfoMac)
>       return nsnull;
>-    NS_ADDREF(mimeInfo);
>+    NS_ADDREF(mimeInfoMac);
> 
>     if (!aMIMEType.IsEmpty())
>-      mimeInfo->SetMIMEType(aMIMEType);
>+      mimeInfoMac->SetMIMEType(aMIMEType);

Why not just pass aMIMEType to the constructor?

r=bzbarsky with that.  Sorry this took so long...

Comment 11

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

(In reply to comment #10)
> Wouldn't it be better to return the actual error codes instead of turning them
> all into PR_FALSE?
> 
> This is fine either way, in any case.

hmm, the callers don't really care about the exact error code...

> Why not just pass aMIMEType to the constructor?

ah, yeah, good point.  I really should add a ctor that takes a const nsACString&.

Comment 12

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

Attachment: patch v2

Comment 13

[Darin Fisher]

Comment on attachment 150027 [details] [diff] [review]
patch v2

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+  NS_ConvertUTF8toUCS2 extension(aExtension);

nit: use NS_ConvertUTF8toUTF16 instead.


>Index: uriloader/exthandler/nsMIMEInfoImpl.cpp

> nsMIMEInfoBase::~nsMIMEInfoBase()
> {
> }

nit: no need to code the dtor right?


sr=darin

Comment 14

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

(In reply to comment #13)
> >Index: uriloader/exthandler/nsMIMEInfoImpl.cpp
> > nsMIMEInfoBase::~nsMIMEInfoBase()
> > {
> > }
> 
> nit: no need to code the dtor right?

as I want it to be virtual, I do need an impl; I'll put it inline in the .h file.

Comment 15

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

checked in