235502 - nsIMIMEService, nsIMIMEInfo should use nsACString/nsAString instead of string/wstring

Status: RESOLVED FIXED

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

Description

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

that would reduce the number of needed allocations

Comment 1

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

I think I have a patch...
cc'ing platform maintainers so they are aware of this, as the patch touches
platform-specific nsOSHelperAppService.

Comment 2

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

turns out I forgot some files, so you'll have to wait a bit longer for the patch :)

Comment 3

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

Attachment: patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143113 [details] [diff] [review]
patch

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>+NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(const nsACString& aMIMEType, const nsACString& aFileExt, nsIMIMEInfo **_retval) 
>+      if (aMIMEType.Equals(APPLICATION_OCTET_STREAM, nsCaseInsensitiveCStringComparator()) != 0)

What's with the != 0 thing?  You just want !Equals(), right?

>+NS_IMETHODIMP nsExternalHelperAppService::GetTypeFromExtension(const nsACString& aFileExt, nsACString& aContentType) 

>+  const nsCString& flatExt = PromiseFlatCString(aFileExt);

I'd prefer to use nsAFlatCString& here.... Just in case the "nsAFlatC?String is
just nsC?String" thing gets changed.  There's no drawback to doing that, right?
 Check with Darin, I guess -- I have no really strong views on this.

>Index: uriloader/exthandler/nsExternalHelperAppService.h

>+  virtual already_AddRefed<nsIMIMEInfo> GetMIMEInfoFromOS(const nsCString& aMIMEType,
>+                                                          const nsCString& aFileExt,

Same here.... and for the subclasses.

>Index: uriloader/exthandler/nsMIMEInfoImpl.cpp
>+nsMIMEInfoBase::SetFileExtensions(const nsACString& aExtensions)

>+        nsCString ext( extList.get(), breakLocation );

Can't that be a Substring?

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

>+already_AddRefed<nsIMIMEInfo> nsOSHelperAppService::GetMIMEInfoFromOS(const nsCString& aMIMEType, const nsCString& aFileExt, PRBool *aFound)

>-  if (aMIMEType && *aMIMEType && PL_strcasecmp(aMIMEType, APPLICATION_OCTET_STREAM) != 0) {
>+  if (aMIMEType.Equals(APPLICATION_OCTET_STREAM, nsCaseInsensitiveCStringComparator())) {

You want !Equals(), no?

r=bzbarsky with those changes.

Comment 5

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

(In reply to comment #4)
> I'd prefer to use nsAFlatCString& here.... Just in case the "nsAFlatC?String is
> just nsC?String" thing gets changed.  There's no drawback to doing that, right?
>  Check with Darin, I guess -- I have no really strong views on this.

darin: is there any chance that nsAFlatString will no longer be an nsString at
some point?

I kinda prefer nsCString&, because it makes clear what the type really is...
especially as nsAFlatCString used to have much less methods than nsCString...

Comment 6

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

Attachment: patch v2

ok, darin convinced me to pass nsACString to nsOSHelperService instead of
nsCString, so nsCString vs nsAFlatCString becomes moot.

Comment 7

[Darin Fisher]

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

>Index: htmlparser/src/COtherDTD.cpp
>Index: htmlparser/src/nsExpatDriver.cpp

changes in these files are for the EqualsWithConversion bug.


>Index: layout/html/base/src/nsObjectFrame.cpp

>+    rv = mimeService->GetTypeFromURI(uri, typeStr);
>+    if (NS_FAILED(rv) || typeStr.IsEmpty())
>       return PR_FALSE;

does GetTypeFromURI ever return NS_OK and an empty result?


>Index: mailnews/mime/src/mimemoz2.cpp

>     nsCOMPtr<nsIMIMEService> mimeFinder (do_GetService(NS_MIMESERVICE_CONTRACTID, &rv));
>+    if (NS_SUCCEEDED(rv) && mimeFinder) {

sort of silly to check both |rv| and |mimeFinder| here.  i think this code
should just do away with checking |rv| since it does not do anything with
|rv| in the failure case.


>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>-  const char* mimetype;
...
>-  mimetype = aMimeType;
>+  nsCAutoString mimetype;
>+  if (aMimeType)
>+    mimetype = aMimeType;

can you use a nsDependentCString here instead?	the Rebind method
could be used, right?


>     {
>-      nsXPIDLCString mt;
>-      res = ms->GetTypeFromURI(aURL, getter_Copies(mt));
>+      nsCAutoString mt;
>+      res = ms->GetTypeFromURI(aURL, mt);
>       if(NS_SUCCEEDED(res))
>         mimetype = mt;
>     }
>   }

actually, nevermind... i wonder how this code ever worked before.
|mt| would have gone out of scope leaving |mimeType| referencing
a deallocated string buffer.

ok, keep the nsCAutoString.


>Index: netwerk/mime/public/nsIMIMEInfo.idl

>-    void SetFileExtensions(in string aExtensions);
>+    void SetFileExtensions(in ACString aExtensions);

how about making this interface use interCaps as well?	or are you
worried about busting some JS extensions?  we'd really want to 
interCap the entire interface before freezing it.


>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

> nsresult nsExternalAppHandler::Init(nsIMIMEInfo * aMIMEInfo,
>+                                    const nsCString& aTempFileExtension,

can aTempFileExtension be a |const nsCSubstring&| type instead?  it can
so long as you don't need it to be null-terminated.


>+  const nsAFlatCString& flatExt = PromiseFlatCString(aFileExt);

you could use |const nsCString&| here instead.	i don't think there's
much value in using nsAFlatCString since it is just a typedef for
nsCString.  eventually, i might even try to eliminate nsAFlatC?String.

>+    return GetTypeFromExtension(
>+             Substring(specStr, extLoc + 1, specStr.Length() - extLoc - 1),
>+             aContentType);

use |Substring(specStr, extLoc + 1)| instead... Substring is going to
normalize the arguments anyways, so you might as well save yourself
some code.


>Index: uriloader/exthandler/nsExternalHelperAppService.h

>   nsExternalAppHandler * CreateNewExternalHandler(nsIMIMEInfo * aMIMEInfo,
>+                                                  const nsCString& aFileExtension,

same question here: can you use |const nsCSubstring&| instead?


>-  nsresult FillTopLevelProperties(const char * aContentType,
>+  nsresult FillTopLevelProperties(const nsCString& aContentType,

ditto.


>-  nsresult Init(nsIMIMEInfo * aMIMEInfo, const char * aFileExtension,
>+  nsresult Init(nsIMIMEInfo * aMIMEInfo, const nsCString& aFileExtension,

ditto (oh, wait.. i already mentioned this one!) ;-)


sr=darin w/ nits picked


thanks so much for doing this!!!

Comment 8

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

(In reply to comment #7)
> changes in these files are for the EqualsWithConversion bug.

er, oops, indeed

> does GetTypeFromURI ever return NS_OK and an empty result?

probably not, I changed it to just check NS_FAILED(rv) instead.

> sort of silly to check both |rv| and |mimeFinder| here.  i think this code
> should just do away with checking |rv| since it does not do anything with
> |rv| in the failure case.

ok. this is just how the code originally was there...

> actually, nevermind... i wonder how this code ever worked before.

yeah, I also wondered that...

> how about making this interface use interCaps as well?	or are you
> worried about busting some JS extensions?  we'd really want to 
> interCap the entire interface before freezing it.

I didn't want to break C++ and JS components in a single checkin ;)
I'd prefer that to be done in a separate bug. I filed Bug 239394 for this.

Comment 9

[Darin Fisher]

ok

Comment 10

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

Attachment: all issues fixed

Comment 11

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

checked in.

Comment 12

[[e-mail]]

-  nsCAutoString fileExt( ext );       
+  nsDependentCString fileExt( ext );       
Ext isn't null-checked, so this crashes when the file has no extension.

Comment 13

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

Attachment: patch

hm, indeed. that would crash.

Comment 14

[Darin Fisher]

Comment on attachment 146290 [details] [diff] [review]
patch

why not do this:

nsDependentCString fileExt(ext ? ext : "");

Comment 15

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

Comment on attachment 146290 [details] [diff] [review]
patch

good idea

Comment 16

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

Attachment: what darin said

Comment 17

[Boris Zbarsky [:bzbarsky]]

I've taken the liberty of checking in that patch so tomorrow's builds don't
crash all over.

Comment 18

[Greg K.]

Did this cause crasher bug 240748?