469739 - Add support for displaying Vista UAC shield icon

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Rain [:rain1] (inactive)]

Attachment: proposed patch, v1

According to the Windows Vista UX guidelines [1], at every UAC entry point one is supposed to display a UAC shield icon. Currently, Mozilla products don't follow this guideline as there isn't support in libpr0n to display such an icon.

This patch adds support to display UAC icons using a standard moz-icon URL. The URL is moz-icon://stock/uac-shield.

Since the Windows version of nsIconChannel didn't have any support for displaying stock icons, I've added generic support for it, and decoupled the file:// specific and the generic hIcon code. More parameters can easily be added as needed.

Most of the patch will require the Vista SDK, as SHGetStockIconInfo and SHSTOCKICONID are both new to Vista. People who don't compile using the Vista SDK shouldn't see a difference.

Stuart: are you an appropriate person to ask for review here? Is the parameter name fine? I also have no idea where this would be documented.

[1] http://msdn.microsoft.com/en-us/library/aa511445.aspx#guidelines

Comment 1

[Rain [:rain1] (inactive)]

Comment on attachment 353120 [details] [diff] [review]
proposed patch, v1

Changing r/sr? per stuart.

Comment 2

[Rain [:rain1] (inactive)]

Attachment: Screenshot of a button with the shield icon

This is how a button looks with the shield icon.

Comment 3

[Rain [:rain1] (inactive)]

Attachment: proposed patch, v1.01, added FreeLibrary [Checkin: Comment 7]

Oops, missed the FreeLibrary. Also fixed a couple of style issues.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 353207 [details] [diff] [review]
proposed patch, v1.01, added FreeLibrary
[Checkin: Comment 7]

>+  nsCAutoString stockIcon;
>+  iconURI->GetStockIcon(stockIcon);
>+  if (!stockIcon.IsEmpty())
>+    rv = GetStockHIcon(iconURI, &hIcon);
>+  else
>+#endif
>+    rv = GetHIconFromFile(&hIcon);

Is there any possibility that GetHIconFromFile will return a result when GetStockHIcon can't? If so, I'd rather this fall back rather than being an else condition. 

>+#include <windows.h>

I am 80% sure that including windows.h in a Mozilla .h file is frowned upon, but am willing to be shown to be wrong.

r+ as long as windows.h in a Mozilla header is okay; r- otherwise.

Comment 5

[Rain [:rain1] (inactive)]

Thanks for reviewing!

(In reply to comment #4)
> Is there any possibility that GetHIconFromFile will return a result when
> GetStockHIcon can't? If so, I'd rather this fall back rather than being an else
> condition.

No, I don't think so. According to the moz-icon "spec" <http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/icon/nsIIconURI.idl>, a url can be one of the two, not both.

> 
> >+#include <windows.h>
> 
> I am 80% sure that including windows.h in a Mozilla .h file is frowned upon,
> but am willing to be shown to be wrong.
> 
> r+ as long as windows.h in a Mozilla header is okay; r- otherwise.

FWIW: <http://mxr.mozilla.org/mozilla-central/search?string=%3Cwindows.h%3E&find=.*\.h&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central>

Comment 6

[Rain [:rain1] (inactive)]

Joe clarified that the patch was r+ him, as the header file touched in the patch is Windows only.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 353207 [details] [diff] [review]
proposed patch, v1.01, added FreeLibrary
[Checkin: Comment 7]

http://hg.mozilla.org/mozilla-central/rev/1a6d5321e253

Comment 8

[Rain [:rain1] (inactive)]

Comment on attachment 353207 [details] [diff] [review]
proposed patch, v1.01, added FreeLibrary
[Checkin: Comment 7]

Pretty low risk, no effect on existing code, and this + adding the shield to the updater button will be a nice visual integration win.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 353207 [details] [diff] [review]
proposed patch, v1.01, added FreeLibrary
[Checkin: Comment 7]

a191=beltzner

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/43db05dac257