402839 - Modal dialogs should use Linux's native icons

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Michael Ventnor]

In our efforts to feel more native on every platform, we should use as many features that the OS provides. GNOME on Linux provides main icons for almost any type of modal dialog. We should probably use those since the ones we have now are a bit too shiny to fit in. Patch coming up.

Comment 1

[Michael Ventnor]

Attachment: Patch part A

This is the C++ patch. The libpr0n part fixes an awful macro where it converts 8-bit integers to 16-bit integers then converts the result back to an 8-bit integer. This MAY also fix the case where dialog-sized stock icons are occasionally borked. It seems to from here.

The nsPromptService part provides a new class to allow us to use GNOME's authentication icon on the WWW-authenticate dialogs.

Comment 2

[Michael Ventnor]

Attachment: Patch part B

This is the toolkit CSS part to use the icons. It had to be separate because Core doesn't let me do additional review...

Comment 3

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

Comment on attachment 287670 [details] [diff] [review]
Patch part A

Since I had to look up the arithmetic rules in C++ for the nsIconDecoder change, as well as write a test program, I really would prefer that you don't make that change.

Anyway, why would that possibly fix anything? Your code is equivalent to the original.

r=biesi on the promptservice change

Comment 4

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

Comment on attachment 287670 [details] [diff] [review]
Patch part A

I meant, r+sr=biesi on the promptservice change.

Comment 5

[Michael Ventnor]

(In reply to comment #3)
> (From update of attachment 287670 [details] [diff] [review])
> Since I had to look up the arithmetic rules in C++ for the nsIconDecoder
> change, as well as write a test program, I really would prefer that you don't
> make that change.
> 
> Anyway, why would that possibly fix anything? Your code is equivalent to the
> original.

Its inefficient. Converting 8-bit integers to 16-bit integers then back to 8-bit integers. I don't know if its the placebo effect, but ever since that change I haven't had a problem with dialog-sized stock icons. Before they would rarely, and at random, return a corrupt image. It doesn't seem to happen now.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 287671 [details] [diff] [review]
Patch part B

Hrm, why do we specify width & height here?

Comment 7

[Michael Ventnor]

(In reply to comment #6)
> (From update of attachment 287671 [details] [diff] [review])
> Hrm, why do we specify width & height here?
> 

We did that on winstripe too. Its just that GNOME's icons are slightly larger. We need to specify those because without them the image height flexes with the text of the dialog, and we can't have that.

Comment 8

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

(In reply to comment #5)
> Its inefficient. Converting 8-bit integers to 16-bit integers then back to
> 8-bit integers. I don't know if its the placebo effect, but ever since that
> change I haven't had a problem with dialog-sized stock icons. Before they would
> rarely, and at random, return a corrupt image. It doesn't seem to happen now.

The conversion to a wider integer is needed for correctness, and in any case, per C++'s arithmetic rules, all integers smaller than "int" get converted to int before doing the calculation.

Comment 9

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

(and since *out is an 8-bit integer, you also need the conversion back to that, whether explicitly or implicitly)

Comment 10

[Michael Ventnor]

Attachment: Patch part A - windowwatcher only

Comment 11

[Asaf Romano (gone)]

Comment on attachment 287671 [details] [diff] [review]
Patch part B

r=mano

Comment 12

[Reed Loden [:reed]]

Part A:
Checking in embedding/components/windowwatcher/src/nsPromptService.cpp;
/cvsroot/mozilla/embedding/components/windowwatcher/src/nsPromptService.cpp,v  <--  nsPromptService.cpp
new revision: 1.38; previous revision: 1.37
done

Part B:
Checking in toolkit/themes/gnomestripe/global/global.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/global.css,v  <--  global.css
new revision: 1.9; previous revision: 1.8
done

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 287671 [details] [diff] [review] [details])
> > Hrm, why do we specify width & height here?
> We did that on winstripe too. Its just that GNOME's icons are slightly larger.

Is there a reason 40x40 was chosen for the width & height here?

This size causes really gross-looking icons, scaled down from 48x48 to 40x40, on Ubuntu 8.10, 8.04 and presumably many other Linux OSes.  (see bug 463938 comment 5)