233461 - [GNOME] use of stock images

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Pierre Chanial]

The stock images are the icons that are shared between gtk applications. They
usually come with a label, a direction (ltr,rtl). But this bug is likely to only
cover the use of stock images in our dialog buttons.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Stealing bug.  I've got some semblance of a hack of this working in my tree.  I
need to clean it up quite a bit and stuff though, but should have a patch
forthcoming RSN.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

BTW, a work-in-progress patch is at
http://people.redhat.com/caillon/patches/mozilla/icons/icons.diff

I still have some stuff to do, including finishing support for all the stuff I
claim to in the icon uri/channel stuff.  Thanks to mconnor who implemented the
firefox frontend changes.

Comment 3

[Mike Connor [:mconnor]]

Attachment: not-building aviary version of patch

this is a work in progress, I'm working on backporting the moz-icon stuff that
biesi implemented, which this depends on, but I'm running into build issues. 
Putting this here for the moment.

Comment 4

[piers]

Does this bug cover the use of gnome-icon-theme for icons too when available?

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

No.  This is for GtkStockIcons.  Isn't GnomeIconTheme deprecated anyway?

Comment 6

[James Henstridge]

The GnomeIconTheme API is deprecated in Gnome 2.6 in favour of the GtkIconTheme
API found in GTK 2.4.

The GtkIconTheme API can be used to look up both stock icons and file type
icons.  However, most people still use the higher level APIs to create stock images.

Looking at the patch, it seems like it only handles dialog button size icons,
and only for the NORMAL state.  It'd be nice if we could get at other sized
icons (eg. toolbar or menu sized icons), and for other states (prelight,
disabled, etc).

It'd be nice if this allowed the creation of a native icon theme for mozilla or
firefox that pulled all the toolbar icons from the theme.  This would give
access to all the accessibility themes available with Gnome.

Comment 7

[Mike Connor [:mconnor]]

that's our eventual goal, but that's in the longer term.  Just using what we
have in this patch is a drastic improvement in getting the GNOME feel right.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Aviary patch at
http://people.redhat.com/caillon/patches/mozilla/gtk-stock-icons/gtk2-stock-icons.patch

Comment 9

[Pierre Chanial]

caillon: this is beautiful. Is it ready for reviews?

Comment 10

[Marco Pesenti Gritti]

Attachment: Add an icon channel for gnome icon themes

Comment 11

[Marco Pesenti Gritti]

Attachment: Add icons to the dialogs buttons

Comment 12

[Marco Pesenti Gritti]

Attachment: gnomestripe browser theme using several icons from gnome-icon-theme

Comment 13

[Marco Pesenti Gritti]

Comment on attachment 164389 [details]
gnomestripe browser theme using several icons from gnome-icon-theme

I think the gnomestripe browser theme should probably be off by default with
--enable-gnome-theme configure option. Marketing concerns apart, we will
probably want to use icons that are available only in recent versions of
gnome-icon-theme.

Comment 14

[Marco Pesenti Gritti]

Note that the above patches are heavily based on caillon one. I just finalized
his work.

Comment 15

[Mike Connor [:mconnor]]

sweet!

I'm assuming the change to /browser/config/mozconfig was not intended for this
patch.  The build-fu to turn on the Gnome theme is definitely needed, though.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Makefile changes for attachment 164389

Comment 17

[Marco Pesenti Gritti]

Comment on attachment 164387 [details] [diff] [review]
Add icons to the dialogs buttons

Please ignore the mozconfig changes.

Comment 18

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

Comment on attachment 164384 [details] [diff] [review]
Add an icon channel for gnome icon themes

can you make a trunk patch for this? the configure.in changes will probably
look quite different there (maybe none needed)

since ff 1.0 is about to ship afaik, I don't think this will land there anyway

modules/libpr0n/decoders/icon/nsIIconURI.idl

you must change the IID of interfaces you change

+   * The stock icon size requested from the OS.
+   */
+   readonly attribute ACString stockIconSize;

why is a size a string?

+   * The stock icon state requested from the OS.
+   */
+   readonly attribute ACString stockIconState;

what's a "stock icon state"?


hm... given the nsIconChannel part of the patch, which already exists on the
trunk, I'd prefer to review a trunk patch which only shows the differences to
it

Comment 19

[Marco Pesenti Gritti]

Attachment: Add an icon channel for gnome icon themes (remerged with head)

Comment 20

[Marco Pesenti Gritti]

> +   * The stock icon size requested from the OS.
> +   */
> +   readonly attribute ACString stockIconSize;
> 
> why is a size a string?

Gtk has logical size for icons (like menu, toolbar, small toolbar). So it cant
be an integer, I think it could be an enum though if you prefer.
 
> +   * The stock icon state requested from the OS.
> +   */
> +   readonly attribute ACString stockIconState;
> 
> what's a "stock icon state"?

Gtk renders icon differently depending on the widget state. For now we support
two states normal and disabled (for insensitive toolbar buttons for example).

> hm... given the nsIconChannel part of the patch, which already exists on the
> trunk, I'd prefer to review a trunk patch which only shows the differences to
> it
> 

Sure, I didnt know part of that patch was already on head.

Comment 21

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

historical note ;) that was not so much a part of this patch - it was bug 225148

Comment 22

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

Comment on attachment 165117 [details] [diff] [review]
Add an icon channel for gnome icon themes (remerged with head)

ah, I see that the idl actually documents what icon sizes and states are -
missed it before, as it's at the top of the file, rather than where the
attribute is...

modules/libpr0n/decoders/icon/nsIIconURI.idl
+   * This interface derives from nsIURI, to provide additional information
+   * about moz-icon URIs.  These URIs

that last sentence seems a bit unfinished :-)

+   * <stock-image> is of the format:	stock/<icon-name>

the BNF above doesn't seem to mention <stock-image> anywhere...

+   *	Description: If integer, this is the desired size in square pixels of
the icon
+   *		     Else, use the OS default for the specified keyword
context.

is it possible to use a keyword for non-stock images?

+   *	Parameter:   contentType
+   *	Values:      <mime-type>
+   *	Description: A valid mime type for the icon.

isn't it actually a content-type of the data?
i.e. the content-type you want an icon _for_


this does not document the state parameter




Is it worth using atoms for the size?

a diff -p and a bit more context would've been nice...

modules/libpr0n/decoders/icon/nsIconURI.cpp

+      spec.Append(nsPrintfCString("%s", size_string));

now really, you don't need sprintf to convert a string to a string. just do:
spec.Append(size_string);

+    if (mStockIconState) {
+      spec += NS_MOZ_ICON_DELIMITER;

this means if I specify both a size and a state, that this will turn it into
?size=...?state=..., instead of separating it by '&'...
it also loses the content type... that's ok probably. maybe the idl should
mention that for stock images, the content type is ignored?

(same comment about printfcstring for state)

(sorry... IDL Documentation is somewhat important to me)

hm, if someone wanted to improve this code, they should change
extractAttributeValue to take an ACString& rather than forcing heap
allocation... and maybe make that pass some substring type instead, so that it
need not copy at all
(not a condition for review+ ;) )

+    if (!strncmp("//stock/", mDummyFilePath.get(), 8))

hm, you could use StringBeginsWith... but that's somewhat less pleasant to use,
requiring NS_LITERAL_CSTRING. (bug 267081 would fix that)
feel free to ignore this paragraph ;)

I wonder if invalid sizes/states should return NS_ERROR_MALFORMED_URI (instead
of silently ignoring them)

while you're here... please replace nsCRT::strncmp with strncmp, or at least be
consistent ;) (you use strncmp for "//stock", and nsCRT::strncmp for the rest).
nsCRT::str* is kinda scheduled to go away (bug 105482 / bug 124536)

+  aFileExtension = nsDependentCString(fileExt);

while touching this, you might want to remove the nsDependentCString here

modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp
I trust that you/caillon did the gtk stuff right, since I don't know the apis
here really...

don't you need to clean up gProtoWindow/gStockImageWidget at some point though?

+    gIconFactory = gtk_icon_factory_new();
+    gtk_icon_factory_add_default (gIconFactory);
+    g_object_unref(gIconFactory);

space before '(' on the second line is inconsistent with the style of this
file; and why can you unref here? does add_default increase the refcount?

+    NS_WARNING(iconSizeString.get());

maybe make this "Invalid icon size"

hmm, so I _have_ to specify a size for a stock icon? can you document that in
the idl? :-)

can't your ensure_* functions fail? (out of memory or something)

Comment 23

[Marco Pesenti Gritti]

Attachment: icon module (2)

Comment 24

[Marco Pesenti Gritti]

> +   *	Description: If integer, this is the desired size in square pixels of
> the icon
> +   *		     Else, use the OS default for the specified keyword
> context.
> 
> is it possible to use a keyword for non-stock images?

I changed the interface so that logical sizes and states apply also to not
stock-images. Even if this is not fully implemented, I think it could be useful
in the future, so it's better to keep the interface flexible.
In the gtk implementation the size keyword works for non-stock images. What is
left is integer size for stock images and states for non stock images. Those are
not immediately useful, so I did put XXX in the code.
 
> Is it worth using atoms for the size?

I think it is, given we need to validate states and logical sizes in
nsIconURI.cpp. You'd have to add a bunch of strcmp.
 
> +      spec.Append(nsPrintfCString("%s", size_string));
> 
> now really, you don't need sprintf to convert a string to a string. just do:
> spec.Append(size_string);

Hah, not sure how this was there. Prolly leftover from some previous attempts.
/me blames caillon :)
 
> don't you need to clean up gProtoWindow/gStockImageWidget at some point though?
>

Right, I added a Shutdown and called it from module dtor.
gStockImageWidget will be destroyed automatically because it's a child.

(I'm unreffing gIconFactory but that is wrong, please ignore it.)

> +    gIconFactory = gtk_icon_factory_new();
> +    gtk_icon_factory_add_default (gIconFactory);
> +    g_object_unref(gIconFactory);
> 
> space before '(' on the second line is inconsistent with the style of this
> file; and why can you unref here? does add_default increase the refcount?

Yeah add_default g_object_ref it.

> +    NS_WARNING(iconSizeString.get());
> 
> maybe make this "Invalid icon size"
> 
> hmm, so I _have_ to specify a size for a stock icon? can you document that in
> the idl? :-)

I changed the code to fallback to the menu size in the case it's invalid. That's
to be consistent with how we handle integer sizes.
 
> can't your ensure_* functions fail? (out of memory or something)
> 

GObject _new is guaranteed to return not NULL, so I dont think they can fail.

Comment 25

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

Comment on attachment 165575 [details] [diff] [review]
icon module (2)

modules/libpr0n/decoders/icon/nsIIconURI.idl
hmm, I'm not sure the:
   * XXXcaa document or reference to all 76 (yes, 76) of them.
comment should've been removed. what if I want to implement this for a
non-gnome platform? for those cases, a list would be nice. not to mention for
people wanting to use stock icons. referring to some gtk api docs might be a
possibility too.

modules/libpr0n/decoders/icon/nsIconURI.cpp
+    spec.Append(nsPrintfCString("%d", mSize));

note that there's AppendInt (oh, I see that you copied it. feel free to leave
as nsPrintfCString if you want)

modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp
+  GtkIconSize icon_size = moz_gtk_icon_size(iconSizeString.get());
+  if (icon_size == GTK_ICON_SIZE_INVALID) {

hmm, it doesn't look like this function will ever return _INVALID anymore...

    gtk_icon_size_lookup (icon_size, &size, NULL);

please remove the space before ( here, to be consistent with the rest of the
file

+    gtk_icon_size_lookup (icon_size, &size, NULL);

passing NULL is ok here?
http://developer.gimp.org/api/2.0/gtk/gtk-Themeable-Stock-Images.html#gtk-icon-
size-lookup doesn't explicitly allow it...

>(I'm unreffing gIconFactory but that is wrong, please ignore it.)

why is it?

oh, just noticing something else:
+ensure_stock_image_widget()  has 4 spaces of indentation, contrary to the rest
of the file, which uses 2.

I kinda wonder how a _new function can guarantee it won't return null when
malloc returns null. but ok.

Comment 26

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

> a list would be nice

although looking at the names of these icons, maybe it would be better to
specify "the list of available names is platform-dependent"?

Comment 27

[Marco Pesenti Gritti]

Attachment: icon module (3)

Comment 28

[Marco Pesenti Gritti]

(In reply to comment #25)
> (From update of attachment 165575 [details] [diff] [review])
> modules/libpr0n/decoders/icon/nsIIconURI.idl
> hmm, I'm not sure the:
>    * XXXcaa document or reference to all 76 (yes, 76) of them.
> comment should've been removed. what if I want to implement this for a
> non-gnome platform? for those cases, a list would be nice. not to mention for
> people wanting to use stock icons. referring to some gtk api docs might be a
> possibility too.

I'm not yet sure if we can make these platform independent. It's hard to say
without actually trying to integrate it with another platform. So I'm leaving
this undecided for now and I put a comment about it in the idl.

> +    gtk_icon_size_lookup (icon_size, &size, NULL);
> 
> passing NULL is ok here?
> http://developer.gimp.org/api/2.0/gtk/gtk-Themeable-Stock-Images.html#gtk-icon-
> size-lookup doesn't explicitly allow it...

Yeah, I checked the code. This is not always documented.
 
> >(I'm unreffing gIconFactory but that is wrong, please ignore it.)
> 
> why is it?

Because we are already unreffing after add_default.

> I kinda wonder how a _new function can guarantee it won't return null when
> malloc returns null. but ok.
> 

If you are interested, there is some discussion about it in this thread.

http://mail.gnome.org/archives/gtk-devel-list/2001-November/msg00643.html

Comment 29

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

   * XXXcaa Should these considered platform dependent or can we share and
document them?

this now being your comment not caillon's, I think you should not put his
initials there :-)

Comment 30

[Marco Pesenti Gritti]

Heh ok, I will change that. I didnt actually figure out what caa was meaning, I
thought to some suffix used in idls documentation XXX ;)

Comment 31

[Mike Connor [:mconnor]]

Comment on attachment 164387 [details] [diff] [review]
Add icons to the dialogs buttons

>+++ toolkit/components/passwordmgr/resources/content/passwordManager.xul	

>-            <button id="removeAllSignons"
>+            <button id="removeAllSignons" icon="remove"

"clear" is more appropriate for Remove All, this is already done elsewhere

>-            <button id="removeAllRejects"
>+            <button id="removeAllRejects" icon="remove"

Same here


Index: toolkit/locales/en-US/chrome/global/finddialog.dtd
>===================================================================

>-<!ENTITY cancelButton.label "Cancel">
>+<!ENTITY closeButton.label "Close">

We need to keep Cancel for the non-UNIX case, or decide to make it Close
cross-platform and turf the #ifdefs in finddialog.xul.	Probably best to just
restore this line.

>Index: extensions/cookie/resources/content/cookieAcceptDialog.js
>===================================================================

>+  // hook up GNOME stock icons where implemented
>+  document.getElementById("ok").setAttribute("icon","accept");
>+  document.getElementById("cancel").setAttribute("icon","cancel");
>+  document.getElementById("Button2").setAttribute("icon","accept");
>+  document.getElementById("disclosureButton").setAttribute("icon","properties");
>+

This is fine as a stopgap, but I really hate the cookie dialog UI code from
seamonkey.

r=me with those changes.

Comment 32

[Takanori MATSUURA]

Attachment: updated version of attachement 164387

only fitted to the latest trunk.

marco, follow this attachement please.

Comment 33

[Takanori MATSUURA]

Attachment: updated version of attachement 165681

only fitted to the latest trunk.

marco, follow this attachement please.

Comment 34

[Takanori MATSUURA]

Attachment: live bookmark patch from Fedora Core Rawhide

Is this also needed?

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Marco already has the latest patches including the live bookmarks patch and a
few more changes in his tree, with the review comments addressed as well.

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

1) looks like there are some alignment issues on various buttons, e.g. OK, etc.
bryner will explain in detail. 
2) changes to the browser's theme buttons (those designed by Stephen Horlander)
should not be part of a theme called "GNOMEStripe" - we use the "*Stripe" name
for themes that use that specific icon set applied to a theme designed for a
given platform's l&f. 
3) the stock image sections that apply to OK/Cancel buttons etc enhance
gnomestripe's integration with GNOME and should be checked in upon review. 
4) whether or not the changes to the browser's theme buttons as designed by
Stephen Horlander etc are a different story - more of a policy decision relating
to hosting other themes in cvs, best to talk to brendan. 

Comment 37

[Marco Pesenti Gritti]

I landed the icon module patch:

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1404; previous revision: 1.1403
done
Checking in configure;
/cvsroot/mozilla/configure,v  <--  configure
new revision: 1.1380; previous revision: 1.1379
done
Checking in modules/libpr0n/decoders/icon/nsIIconURI.idl;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIIconURI.idl,v  <--  nsIIconURI.idl
new revision: 1.7; previous revision: 1.6
done
Checking in modules/libpr0n/decoders/icon/nsIconModule.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconModule.cpp,v  <-- 
nsIconModule.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in modules/libpr0n/decoders/icon/nsIconURI.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp,v  <--  nsIconURI.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in modules/libpr0n/decoders/icon/nsIconURI.h;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconURI.h,v  <--  nsIconURI.h
new revision: 1.8; previous revision: 1.7
done
Checking in modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp,v  <-- 
nsIconChannel.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in modules/libpr0n/decoders/icon/gtk/nsIconChannel.h;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/gtk/nsIconChannel.h,v  <-- 
nsIconChannel.h
new revision: 1.2; previous revision: 1.1
done

Comment 38

[Ben Goodger (use ben at mozilla dot org for email)]

The patch to Icon broke EXE icons in the form: 

moz-icon://file///C:/foo/bar.exe?size=16

on Windows... Please fix it or I'll have to revert the changes when I land the
new Preferences UI. 

Comment 39

[Marco Pesenti Gritti]

Attachment: fix moz-icon://file:///... like uris

Comment 40

[Marco Pesenti Gritti]

Attachment: fix moz-icon://file:///... like uris

Comment 41

[Marco Pesenti Gritti]

I assume you meant:

moz-icon://file://C:/foo/bar.exe?size=16

The attached patch should fix it, I cant test on windows though.

Comment 42

[Christopher Blizzard (:blizzard)]

Comment on attachment 173062 [details] [diff] [review]
fix moz-icon://file:///... like uris

sr=blizzard

Comment 43

[Marco Pesenti Gritti]

Comment on attachment 173062 [details] [diff] [review]
fix moz-icon://file:///... like uris

I checked in this one.
moz-icon://file://C:/foo/bar.exe?size=16 like uris should be back working.

Comment 44

[Marco Pesenti Gritti]

Attachment: button icons + alignement fix

This is the same as the already reviewed attachment 164387 [details] [diff] [review], remerged to trunk,
with review comments addressed.

I've also fixed the alignment issue. Now both icon/text are centered. For
gnomestripe I added a 2 pixels spacing between them, like in native gtk
buttons.
(These changes are in button.xml/button.css)

I'm not sure what is the deal with other platforms. Is current alignment
correct or should we have icon/text centered for those too? What about spacing?

Comment 45

[Marco Pesenti Gritti]

Some screenshots to make the alignement difference more clear:

Before my patch:

http://gnome.org/~marco/firefoxicons-1-1.png
http://gnome.org/~marco/firefoxicon1.png

After my patch:

http://gnome.org/~marco/firefoxicon2.png
http://gnome.org/~marco/firefoxicons-1-2.png

Comment 46

[Christopher Blizzard (:blizzard)]

bryner, have you had time to look at this?

Comment 47

[Mike Connor [:mconnor]]

Comment on attachment 174378 [details] [diff] [review]
button icons + alignement fix

As a note, the browser prefwindow stuff is soon to be obsoleted due to the
prefbranch landing.  Not a big deal, its trivial to just add the icon
attributes once the branch lands.

>--- toolkit/components/printing/content/printdialog.xul	5 Dec 2004 23:29:22 -0000	1.4
>+++ toolkit/components/printing/content/printdialog.xul	15 Feb 2005 13:37:55 -0000

>-          <button id="properties" label="&propertiesButton.label;" oncommand="displayPropertiesDialog();"/>
>+          <button id="properties" label="&propertiesButton.label;" icon="properties" oncommand="displayPropertiesDialog();"/>

please break this properly, should be more like this:

	  <button id="properties" label="&propertiesButton.label;" 
		  icon="properties" oncommand="displayPropertiesDialog();"/>


>-          <button id="chooseFile" label="&chooseButton.label;" oncommand="onChooseFile()"/>
>+          <button id="chooseFile" label="&chooseButton.label;" icon="open" oncommand="onChooseFile()"/>

similar nit here

>Index: toolkit/content/widgets/wizard.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/wizard.xml,v
>retrieving revision 1.17
>diff -u -r1.17 wizard.xml
>--- toolkit/content/widgets/wizard.xml	19 Jan 2005 01:57:32 -0000	1.17
>+++ toolkit/content/widgets/wizard.xml	15 Feb 2005 13:38:02 -0000
>@@ -64,9 +64,19 @@
>           if (this.onFirstPage) {
>             this.canRewind = false;
>             this.setAttribute("firstpage", "true");
>+#ifdef XP_UNIX
>+#ifndef XP_MACOSX
>+            this._backButton.setAttribute('hidden', 'true');
>+#endif
>+#endif
>           } else {
>             this.canRewind = true;
>             this.setAttribute("firstpage", "false");
>+#ifdef XP_UNIX
>+#ifndef XP_MACOSX
>+            this._backButton.setAttribute('hidden', 'false');
>+#endif
>+#endif
>           }
>                     
>           if (this.onLastPage) {
>@@ -328,7 +338,11 @@
>          var btn = document.getAnonymousElementByAttribute(this._wizardButtons, "dlgtype", aName);
>          if (btn) {
>            btn.addEventListener("command", this["_"+aName+"Func"], false);
>-           btn.setAttribute("label", this._bundle.GetStringFromName("button-"+aName));
>+#ifdef XP_UNIX
>+#ifndef XP_MACOSX
>+           btn.setAttribute("label", this._bundle.GetStringFromName("button-"+aName+"-gnome"));
>+#endif
>+#endif

this is a problem, we won't have a label set if the conditions aren't met. 
Since OS X shouldn't have the labels either, lets go with a bit more generic
solution, and fix 271554 in the process.

I think this patch is also missing the wizard.properties changes needed to make
this work.  Please change button-*-gnome to button-*-unix here and in the
properties file, and change this to just plain #ifdef XP_UNIX #else (original
line so Windows gets the old strings) #endif

>            this["_"+aName+"Button"] = btn;
>          }
>          return btn;
>@@ -478,16 +492,22 @@
>         <xul:separator class="wizard-buttons-separator groove"/>
>         <xul:hbox class="wizard-buttons-box-2">
>           <xul:spacer flex="1"/>
>-          <xul:button class="wizard-button" dlgtype="back"/>
>+#ifdef XP_UNIX
>+          <xul:button class="wizard-button" dlgtype="cancel" icon="cancel"/>
>+          <xul:spacer style="width: 24px"/>
>+#endif
>+          <xul:button class="wizard-button" dlgtype="back" icon="go-back"/>
>           <xul:deck class="wizard-next-deck" anonid="WizardButtonDeck">
>             <xul:hbox>
>               <xul:button class="wizard-button" dlgtype="finish" default="true" flex="1"/> 
>             </xul:hbox>
>             <xul:hbox>
>-              <xul:button class="wizard-button" dlgtype="next" default="true" flex="1"/> 
>+              <xul:button class="wizard-button" dlgtype="next" icon="go-forward" default="true" flex="1"/> 
>             </xul:hbox>
>           </xul:deck>
>-          <xul:button class="wizard-button" dlgtype="cancel"/> 
>+#ifdef XP_WIN
>+          <xul:button class="wizard-button" dlgtype="cancel" icon="cancel"/> 
>+#endif

please make this an #ifndef XP_UNIX instead.  Otherwise, if neither XP_UNIX or
XP_WIN are set, we have no cancel button... (and yes, I think you inherited
this from my original patch)

once I see these changes, including the missing wizard.properties file, we
should be good to go.

Comment 48

[Marco Pesenti Gritti]

Attachment: button icons (comments adressed)

Looks like you did already check in the wizard.properties changes:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=wizard.properties&branch=AVIARY_1_0_20040515_BRANCH&root=/cvsroot&subdir=mozilla/toolkit/locales/en-US/chrome/global&command=DIFF_FRAMESET&rev1=1.1.2.1&rev2=1.1.2.2

I made the requested changes...

Comment 49

[Marco Pesenti Gritti]

Comment on attachment 175283 [details] [diff] [review]
button icons (comments adressed)

This is once again bitrotten, because the prefs dialog changes :/
I will fixup the patch and submit for review.

Comment 50

[Marco Pesenti Gritti]

Attachment: button icons (remerged)

This doesnt include the preferences changes. There are large changes, so that
part needs to be redone. I will submit a separate patch for that later but I
want to get this before it goes bitrotten too.

Comment 51

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 175916 [details] [diff] [review]
button icons (remerged)

>+  // hook up GNOME stock icons where implemented

Can we just get rid of the word GNOME? (several times) Its pretty generic in
design...

Comment 52

[Mike Connor [:mconnor]]

Comment on attachment 175916 [details] [diff] [review]
button icons (remerged)


>Index: browser/components/bookmarks/content/addBookmark2.xul
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/bookmarks/content/addBookmark2.xul,v
>retrieving revision 1.18
>diff -u -r1.18 addBookmark2.xul
>--- browser/components/bookmarks/content/addBookmark2.xul	17 Feb 2005 00:17:12 -0000	1.18
>+++ browser/components/bookmarks/content/addBookmark2.xul	1 Mar 2005 09:43:44 -0000
>@@ -59,6 +59,8 @@
>         buttonlabelextra2="&button.newfolder.label;" buttonaccesskeyextra2="&button.newfolder.accesskey;"
> #ifdef XP_UNIX
>         buttonlabelaccept="&acceptButton.label;"
>+        buttoniconaccept="add"
>+        buttoniconextra2="open"
> #endif

lets move this outside of the #ifdef, actually, so third-party themes can use
the icon attributes as widely as possible.

>Index: extensions/cookie/resources/content/cookieAcceptDialog.js
>+  // hook up GNOME stock icons where implemented
>+  document.getElementById("ok").setAttribute("icon","accept");
>+  document.getElementById("cancel").setAttribute("icon","cancel");
>+  document.getElementById("Button2").setAttribute("icon","accept");
>+  document.getElementById("disclosureButton").setAttribute("icon","properties");
>+

What caillon said.  Something like s/GNOME stock/button/ should be fine.

>Index: toolkit/content/finddialog.xul
>     <vbox flex="1">
>       <button id="btnFind" label="&findButton.label;" accesskey="&findButton.accesskey;"
>-              dlgtype="accept"/>
>+              dlgtype="accept" icon="find"/>
>+#ifdef XP_UNIX
>+      <button label="&closeButton.label;" icon="close" dlgtype="cancel"/>
>+#else
>       <button label="&cancelButton.label;" dlgtype="cancel"/>
>+#endif

please add a cancel icon attr to the cancel button.  caillon's right that this
should be generic wherever possible.

r=me, no SR is needed since cookieAcceptDialog.js is moving to
mozilla/toolkit/components/cookie/content/cookieAcceptDialog.js as part of bug
#277097, please hook up with IanN on when that bug will get resolved so you can
land ASAP.  (I'd rather avoid touching the xpfe tine of the fork if possible.)

Comment 53

[Marco Pesenti Gritti]

I checked in all but the cookie change, will check in when it's moved.

Checking in base/content/openLocation.xul;
/cvsroot/mozilla/browser/base/content/openLocation.xul,v  <--  openLocation.xul
new revision: 1.9; previous revision: 1.8
done
Checking in base/content/pageInfo.xul;
/cvsroot/mozilla/browser/base/content/pageInfo.xul,v  <--  pageInfo.xul
new revision: 1.19; previous revision: 1.18
done
Checking in base/content/pageReport.xul;
/cvsroot/mozilla/browser/base/content/pageReport.xul,v  <--  pageReport.xul
new revision: 1.11; previous revision: 1.10
done
Checking in components/bookmarks/content/addBookmark.xul;
/cvsroot/mozilla/browser/components/bookmarks/content/addBookmark.xul,v  <-- 
addBookmark.xul
new revision: 1.5; previous revision: 1.4
done
Checking in components/bookmarks/content/addBookmark2.xul;
/cvsroot/mozilla/browser/components/bookmarks/content/addBookmark2.xul,v  <-- 
addBookmark2.xul
new revision: 1.19; previous revision: 1.18
done
Checking in components/filepicker/content/filepicker.js;
/cvsroot/mozilla/toolkit/components/filepicker/content/filepicker.js,v  <-- 
filepicker.js
new revision: 1.13; previous revision: 1.12
done
Checking in components/filepicker/content/filepicker.xul;
/cvsroot/mozilla/toolkit/components/filepicker/content/filepicker.xul,v  <-- 
filepicker.xul
new revision: 1.8; previous revision: 1.7
done
Checking in components/printing/content/printPreviewBindings.xml;
/cvsroot/mozilla/toolkit/components/printing/content/printPreviewBindings.xml,v
 <--  printPreviewBindings.xml
new revision: 1.16; previous revision: 1.15
done
Checking in components/printing/content/printProgress.xul;
/cvsroot/mozilla/toolkit/components/printing/content/printProgress.xul,v  <-- 
printProgress.xul
new revision: 1.2; previous revision: 1.1
done
Checking in components/printing/content/printdialog.xul;
/cvsroot/mozilla/toolkit/components/printing/content/printdialog.xul,v  <-- 
printdialog.xul
new revision: 1.5; previous revision: 1.4
done
Checking in content/customizeCharset.xul;
/cvsroot/mozilla/toolkit/content/customizeCharset.xul,v  <--  customizeCharset.xul
new revision: 1.3; previous revision: 1.2
done
Checking in content/customizeToolbar.xul;
/cvsroot/mozilla/toolkit/content/customizeToolbar.xul,v  <--  customizeToolbar.xul
new revision: 1.13; previous revision: 1.12
done
Checking in content/finddialog.xul;
/cvsroot/mozilla/toolkit/content/finddialog.xul,v  <--  finddialog.xul
new revision: 1.14; previous revision: 1.13
done
Checking in content/widgets/button.xml;
/cvsroot/mozilla/toolkit/content/widgets/button.xml,v  <--  button.xml
new revision: 1.7; previous revision: 1.6
done
Checking in content/widgets/dialog.xml;
/cvsroot/mozilla/toolkit/content/widgets/dialog.xml,v  <--  dialog.xml
new revision: 1.19; previous revision: 1.18
done
Checking in content/widgets/expander.xml;
/cvsroot/mozilla/toolkit/content/widgets/expander.xml,v  <--  expander.xml
new revision: 1.5; previous revision: 1.4
done
Checking in content/widgets/wizard.xml;
/cvsroot/mozilla/toolkit/content/widgets/wizard.xml,v  <--  wizard.xml
new revision: 1.18; previous revision: 1.17
done
Checking in locales/en-US/chrome/global/finddialog.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/finddialog.dtd,v  <-- 
finddialog.dtd
new revision: 1.3; previous revision: 1.2
done
Checking in locales/en-US/chrome/global/wizard.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/wizard.properties,v  <-- 
wizard.properties
new revision: 1.3; previous revision: 1.2
done
Checking in mozapps/downloads/content/pref-downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/pref-downloads.xul,v  <-- 
pref-downloads.xul
new revision: 1.13; previous revision: 1.12
done
Checking in themes/gnomestripe/global/button.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/button.css,v  <--  button.css
new revision: 1.9; previous revision: 1.8
done

Comment 54

[Marco Pesenti Gritti]

Attachment: Add icons to the new preferences dialog

Comment 55

[Mike Connor [:mconnor]]

Comment on attachment 176451 [details] [diff] [review]
Add icons to the new preferences dialog

r=me, just please also add icon attributes to the OK/Cancel/Help buttons in
preferences.xml at
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/preferences.xml#4
10 to match the dialog.xml changes.  Yes it'll never get used by GNOME, but
these icon attributes will be useful to theme authors.

Comment 56

[Marco Pesenti Gritti]

Comment on attachment 176451 [details] [diff] [review]
Add icons to the new preferences dialog

Checked in:

Checking in components/passwordmgr/resources/content/passwordManager.xul;
/cvsroot/mozilla/toolkit/components/passwordmgr/resources/content/passwordManag
er.xul,v  <--  passwordManager.xul
new revision: 1.7; previous revision: 1.6
done
Checking in content/widgets/preferences.xml;
/cvsroot/mozilla/toolkit/content/widgets/preferences.xml,v  <-- 
preferences.xml
new revision: 1.8; previous revision: 1.7
done
Checking in themes/gnomestripe/global/button.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/button.css,v  <-- 
button.css
new revision: 1.10; previous revision: 1.9
done
Checking in components/preferences/connection.xul;
/cvsroot/mozilla/browser/components/preferences/connection.xul,v  <-- 
connection.xul
new revision: 1.3; previous revision: 1.2
done
Checking in components/preferences/content.xul;
/cvsroot/mozilla/browser/components/preferences/content.xul,v  <--  content.xul
new revision: 1.5; previous revision: 1.4
done
Checking in components/preferences/cookies.xul;
/cvsroot/mozilla/browser/components/preferences/cookies.xul,v  <--  cookies.xul
new revision: 1.3; previous revision: 1.2
done
Checking in components/preferences/downloadactions.xul;
/cvsroot/mozilla/browser/components/preferences/downloadactions.xul,v  <-- 
downloadactions.xul
new revision: 1.3; previous revision: 1.2
done
Checking in components/preferences/general.xul;
/cvsroot/mozilla/browser/components/preferences/general.xul,v  <--  general.xul
new revision: 1.3; previous revision: 1.2
done
Checking in components/preferences/permissions.xul;
/cvsroot/mozilla/browser/components/preferences/permissions.xul,v  <-- 
permissions.xul
new revision: 1.3; previous revision: 1.2
done
Checking in components/preferences/privacy.xul;
/cvsroot/mozilla/browser/components/preferences/privacy.xul,v  <--  privacy.xul
new revision: 1.4; previous revision: 1.3
done

Comment 57

[Marco Pesenti Gritti]

The only thing we lack to mark this FIXED is to land the changes that depends on
bug 277097.

I split the theme part to bug 285098.

Comment 58

[Marco Pesenti Gritti]

Checking in cookieAcceptDialog.js;
/cvsroot/mozilla/toolkit/components/cookie/content/cookieAcceptDialog.js,v  <--
 cookieAcceptDialog.js
new revision: 1.16; previous revision: 1.15
done