190409 - [FIXr]nsMIMEInfoImpl::GetApplicationDescription / GetDefaultDescription should be smarter

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In particular, if the description is not set, they should return the leafname of
the corresponding nsIFile...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: surprisingly few trivial uses...

Comment 2

[Philip K. Warren]

Comment on attachment 112489 [details] [diff] [review]
surprisingly few trivial uses...

>+  if (mPreferredAppDescription.IsEmpty()) {
>+    // Don't want to cache this, just in case someone resets the app
>+    // without changing the description....
>+    nsAutoString leafName;
>+    mPreferredApplication->GetLeafName(leafName);
>+    *aApplicationDescription = ToNewUnicode(leafName);

Are we sure at this position that mPreferredApplication has been set? What
would happen if we call GetApplicationDescription before
SetPreferredApplicationHandler?

>+  if (mDefaultAppDescription.IsEmpty()) {
>+    // Don't want to cache this, just in case someone resets the app
>+    // without changing the description....
>+    nsAutoString leafName;
>+    mDefaultApplication->GetLeafName(leafName);
>+    *aDefaultDescription = ToNewUnicode(leafName);

Same as above but for mDefaultApplication.

Would we benefit from doing an NS_ENSURE_ARG_POINTER in both of these functions
to make sure that the description passed is non null? I know we don't currently
do those checks, but it may be useful.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, yes.  We should be having a null-check there... I had them in an early
revision of the patch, but they got lost.  :(  So the conditional should look like:

>+  if (mPreferredAppDescription.IsEmpty() && mPreferredApplication) {

No, we should not be doing NS_ENSURE_ARG_POINTER.  At best, we could assert that
the pointer is non-null.  If it's null, someone has majorly violated XPCOM
calling  conventions.

Comment 4

[Philip K. Warren]

Comment on attachment 112489 [details] [diff] [review]
surprisingly few trivial uses...

Ok don't worry about the assertion/NS_ENSURE_ARG_POINTER stuff. Add the null
checks on mDefaultApplication/mPreferredApplication as shown in your last
comment and r=pkw.

Comment 5

[Darin Fisher]

Comment on attachment 112489 [details] [diff] [review]
surprisingly few trivial uses...

sr=darin

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: patch updated to include the null-checks

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.4a

Comment 8

[Chris Petersen]

Marking verified per last comments.