Comment on attachment 152909 [details] [review]
Update the Javascript TrayManager code

>+      <schema>
>+        <key>/schemas/desktop/gnome/shell/traymanager/system_icons/gnome-volume-applet</key>
>+	<applyto>/desktop/gnome/shell/traymanager/system_icons/gnome-volume-applet</applyto>
>+	<owner>gnome-shell</owner>
>+	<type>string</type>
>+	<default>volume</default>

This is backwards. The key should be ".../volume" and the default value "gnome-volume-applet". (Yes, this makes the code more complicated; you'll have to read in all the keys corresponding to the values of system_icon_list first, and then build a map from that.)

Though really, I'm not sure putting this in gconf makes sense anyway. IMHO, the applets ought to be setting some property on themselves saying what type of icon they are ("volume", "network", etc), and we should be positioning them based on that. STANDARD_TRAY_ICON_IMPLEMENTATIONS would just be a temporary kludge until the applets were fixed to do that.

>+#trayBox:oldgtk {

kill that. People should have a new gtk now. If they don't, they can suffer the broken icon backgrounds.

>+const TRAY_BACKGROUND_COLOR = new Clutter.Color();
>+TRAY_BACKGROUND_COLOR.from_pixel(0x0b0b0bff);

likewise

>+        // gtk+ < 2.16 doesn't have fully-working icon transparency,
>+        // so we want trayBox to be opaque in that case (the icons
>+        // will at least pick up its background color).
>+        if (Gtk.MAJOR_VERSION == 2 && Gtk.MINOR_VERSION < 16)
>+            this.actor.set_style_pseudo_class('oldgtk');

and here

>+    // By default, tray icons have a spacing of TRAY_SPACING.  However this
>+    // starts to fail if we have too many as can sadly happen; just jump down
>+    // to a spacing of 8 if we're over 6.
>+    // http://bugzilla.gnome.org/show_bug.cgi?id=590495

The reference to TRAY_SPACING is outdated and the reference to "8" is not necessarily correct. And the bug reference isn't really needed. Just rename the function to "_adjustTraySpacing" and then you don't need the comment anyway.

>+        this._traymanager = new Shell.TrayManager({ bg_color: TRAY_BACKGROUND_COLOR });

don't need to set bg_color here since that's only used in the now-dead non-transparency case. (We should eventually clean up shell-tray-manager.c as well.)

>+    _onTrayIconAdded: function(o, icon, wmClass) {

please change "o" to "manager" here and in _onTrayIconRemoved. (Remnants of an older code style.)

ok, while we do want to split up system vs non-system trayicons as per 608869, we don't want to do it in the way it's done there, with a distinct trayicon are in the message tray. So splitting out TrayBox is not going to be useful for how we actually want 608869 to work, so between that and the gconf problems, this patch is not going to be useful. Thanks anyway though.