283606 - make nsOSHelperAppService::GetApplicationDescription get a friendly description

Status: RESOLVED FIXED

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

Description

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

unfortunately, I missed this in my review of bug 252189...

anyway, that bug added a way to get a friendly app description;
GetApplicationDescription should be able to use that.

Comment 1

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

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

>Index: uriloader/exthandler/win/nsOSHelperAppService.cpp
>+  nsAutoString typeName(utf16Scheme);

Why do you need this?  GetDefaultAppInfo should take a const nsAString as its
first arg.

>+  GetDefaultAppInfo(typeName, _retval, getter_AddRefs(app));

Isn't |typeName| supposed to be something like "pngfile" or whatever?  Does
passing something like "mms" here really do the right thing?  I guess this is
looking for the command under the same registry key as GetDefaultAppInfo, so it
must be...

sr=bzbarsky with the extra nsAutoString nixed.

Comment 3

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

> GetDefaultAppInfo should take a const nsAString as its first arg.

it doesn't... hmm, but yeah, it should. I'll make a new patch.

Comment 4

[Darin Fisher]

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

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

>+  const NS_ConvertUTF8toUTF16 utf16Scheme(aScheme);

A URI scheme is restricted to ASCII, so you should be able to
use NS_ConvertASCIItoUTF16 without any loss what-so-ever.


>+  nsCOMPtr<nsIFile> app;
>+  nsAutoString typeName(utf16Scheme);
>+  GetDefaultAppInfo(typeName, _retval, getter_AddRefs(app));

Why is the first parameter to GetDefaultAppInfo non-const?  The
implementation in this file certainly does not seem to modify
the string.  Can you change it to be const and so avoid having
to copy utf16Scheme?


>+  nsAutoString cmdString(utf16Scheme);

If you must copy utf16Scheme, then it seems like you should only
have to do so once.  You could avoid the third copy with code
like this:

    nsAutoString &cmdString = utf16Scheme;

But, I'd recommend avoiding all copies if possible.

Comment 5

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

Attachment: copy once

Comment 6

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

Attachment: don't copy

Can I append to an NS_ConvertASCIItoUTF16 string?

Comment 7

[Darin Fisher]

> Can I append to an NS_ConvertASCIItoUTF16 string?

Yes, it is just a subclass of nsAutoString :-)

Comment 8

[Darin Fisher]

Comment on attachment 192192 [details] [diff] [review]
don't copy

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

>+  NS_ConvertASCIItoUTF16 utf16Scheme(aScheme);
>+
>+  nsCOMPtr<nsIFile> app;
>+  GetDefaultAppInfo(utf16Scheme, _retval, getter_AddRefs(app));
>+
>+  if (!_retval.Equals(utf16Scheme))
>+    return NS_OK;
>+
>+  // Fall back to full path
>+  utf16Scheme.AppendLiteral("\\shell\\open\\command");
>   nsresult rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                             utf16Scheme,
>                              nsIWindowsRegKey::ACCESS_QUERY_VALUE);

In situations such as this, I'd be tempted to give the
string object a generic name such as "buf" since it is
not always used to represent a protocol scheme.

r=darin

Comment 9

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

Comment on attachment 192192 [details] [diff] [review]
don't copy

I'll rename the variable when checking in

This patch changes the application name in the dialog that appears when
launching an external application for a protocol to be a nice name instead of
just the filename. should be low risk; certainly limited to to that dialog.

Comment 10

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

Attachment: patch for checkin

Comment 11

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

trunk:
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <-- 
nsOSHelperAppService.cpp
new revision: 1.64; previous revision: 1.63
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.h,v  <-- 
nsOSHelperAppService.h
new revision: 1.25; previous revision: 1.24
done

MOZILLA_1_8_BRANCH:
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <-- 
nsOSHelperAppService.cpp
new revision: 1.63.4.1; previous revision: 1.63
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.h,v  <-- 
nsOSHelperAppService.h
new revision: 1.24.4.1; previous revision: 1.24
done