538335 - Support Lightweight Themes in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Mark Banner (:standard8)]

Firefox 3.6 has introduced LightWeight themes, it should be relatively simple to port the changes.

The general list of things to do is:

- Set "lightweightthemes" attribute to true on the window element in messenger.xul, work out if we need the option for the bottom box as well.
- Copy & modify the LightWeightThemeWebInstaller code from browser/ to an appropriate location for Thunderbird (moving some of it into toolkit and sharing it there may be an option).
- Modify Thunderbird's css to ensure proper application of the LightWeight themes of the display (e.g. use of the -moz-lwtheme* attributes).

Comment 1

[Mark Banner (:standard8)]

If anyone wants to help with this, please feel free to take a look. I can provide more info over irc if necessary, but most of it should be there.

Additional notes:

(In reply to comment #0)
> - Set "lightweightthemes" attribute to true on the window element in
> messenger.xul, work out if we need the option for the bottom box as well.

Once this bit is done, selecting themes in the menu options of Personas v1.5 add-on should work (you'll have to tweak install.rdf / disable compatibility checking).

> - Copy & modify the LightWeightThemeWebInstaller code from browser/ to an
> appropriate location for Thunderbird (moving some of it into toolkit and
> sharing it there may be an option).

This bit is required to make web pages work - hovering on themes and clicking on them.
 
> - Modify Thunderbird's css to ensure proper application of the LightWeight
> themes of the display (e.g. use of the -moz-lwtheme* attributes).

This bit is just tidying up our display so that we show things correctly on all platforms.

Comment 2

[Andreas Nilsson (:andreasn)]

investigating this one

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I believe Yaoquan did some work relating to porting Personas to Thunderbird last year as part of a student project. CC'ing him.

ref http://solfu.net/tech/category/personas-for-thunderbird/

Comment 4

[Andreas Nilsson (:andreasn)]

Attachment: step 1 and 3

but step2 is the hard one (these theme fixes only apply to Linux for now, but Windows fixes coming up)

Comment 5

[David :Bienvenu]

I don't think this is blocking, but definitely wanted (please correct me if I'm wrong).

Comment 6

[Andreas Nilsson (:andreasn)]

Attachment: windows and linux styling

using this as a temporary storing to get the mac part ready

Comment 7

[Andreas Nilsson (:andreasn)]

Attachment: and pinstripe part

Still some issues to iron out (such as the lack of tab divider when using personas).

Comment 9

[Andreas Nilsson (:andreasn)]

Attachment: qute, gnomestripe and pinstripe patch

Comment 10

[Andreas Nilsson (:andreasn)]

Standard8: If you want to take over with the javascript part, as we discussed on irc, things are in a ok state to do that now.

Comment 11

[Mark Banner (:standard8)]

Attachment: The fix

Most of this is copied and adapted from Firefox's version (browser.js). There's possibly some mileage later in looking at the class being shared into toolkit, but that isn't going to happen for 3.1. Therefore we'll just have our own version.

This needs one of the theme patches that Andreas has done applying as well for it to work because they set lightweightthemes="true" on the window.

Note that this patch makes installing from the web and previewing work correctly. The best way to do this is try it on 3.1 and have the personas extension installed (hack install.rdf if necessary) and open a tab by going into one of personas' theme menus and selecting "xxx more from yyy..." at the bottom of the menu.

When an lightweight theme is installed, there will be two notification bars - one at the top, one at the bottom. The bottom one is the personas extension that we'll need to fix separately, the top one is the built-in one (so that it'll work on any site).

If you want to test allowing on non-whitelisted sites, remove the xpinstall.whitelist.add.36 pref.

Finally there's probably a way to get tests for this, but I'm looking to get this in before 3.1b2 so I'll look at those as soon as I can.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Okay, I haven't checked the code yet, but I'm seeing some errors.

http://www.getpersonas.com/en-US/persona/44201 doesn't seem to show the image.
http://www.getpersonas.com/en-US/persona/238 shows the image, but it's really stretched, and doesn't look right.

When I click "Wear this persona", the bar along the top pops down, and if I hit "Manage Themes…", it takes me to the "Get Add-ons" pane in the Addons dialog instead of the "Themes" pane.

Code review coming later, but I wanted to provide this bit of feedback, in case it pointed at larger problems.

Thanks,
Blake.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 438097 [details] [diff] [review]
The fix

>+++ b/mail/app/profile/all-thunderbird.js
>@@ -174,6 +174,7 @@ pref("extensions.getAddons.search.browse
> pref("xpinstall.whitelist.add", "update.mozilla.org");
>+pref("xpinstall.whitelist.add.36", "getpersonas.com");

When grepping for xpinstall.whitelist.add, I notice that we have:
mail/components/preferences/permissions.js
373:      var prefList = [["xpinstall.whitelist.add", nsIPermissionManager.ALLOW_ACTION],
374:                      ["xpinstall.whitelist.add.103", nsIPermissionManager.ALLOW_ACTION],

but Firefox has:
mozilla/browser/components/preferences/permissions.js
373:      var prefList = [["xpinstall.whitelist.add", nsIPermissionManager.ALLOW_ACTION],
374:                      ["xpinstall.whitelist.add.36", nsIPermissionManager.ALLOW_ACTION],

Should we also add .36 to our permissions.js?  (Or why not?)

>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -1231,3 +1242,201 @@ function ThreadPaneOnDrop(aEvent) {
>+  handleEvent: function (event) {
>+    switch (event.type) {
>+      case "InstallBrowserTheme":
>+      case "PreviewBrowserTheme":
>+      case "ResetBrowserThemePreview":
>+        // ignore requests from background tabs

But we don't ignore pagehide requests from background tabs, cause they don't send any, I'm guessing.

>+  onTabTitleChanged: function (aTab) {
>+  },
>+  onTabSwitched: function (aTab, aOldTab) {
>+    this._resetPreview();
>+  },

We could probably have a blank line between these two functions.

>+
>+  get _manager () {
>+    let temp = {};
>+    Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
>+    delete this._manager;
>+    return this._manager = temp.LightweightThemeManager;
>+  },

Do we really want to do that every time we get the manager?  And if so, why are we even bothering to store it in this._manager?

>+  _installRequest: function (event) {
[snip…]
>+    let messengerBundle = document.getElementById("bundle_messenger");
>+
>+    let allowButtonText =
>+      messengerBundle.getString("lwthemeInstallRequest.allowButton");
>+    let allowButtonAccesskey =
>+      messengerBundle.getString("lwthemeInstallRequest.allowButton.accesskey");
>+    let message =
>+      messengerBundle.getFormattedString("lwthemeInstallRequest.message",
>+                                          [node.ownerDocument.location.host]);

The previous line should be indented by one less character.
And you only appear to use these variables once, so you could inline them.

>+  _postInstallNotification: function (newTheme, previousTheme) {
>+    let buttons = [{
[snip…]
>+      label: text("manageButton"),
>+      accessKey: text("manageButton.accesskey"),
>+      callback: function () {
>+        openAddonsMgr("themes");

So, that really doesn't work, but it looks like we could crib some code from
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#5794
and get it working fairly easily.

>+  _removePreviousNotifications: function () {
>+    let box = this._getNotificationBox();
>+
>+    ["lwtheme-install-request",
>+     "lwtheme-install-notification"].forEach(function (value) {
>+        var notification = box.getNotificationWithValue(value);

I like what you've done there!

>+  _isAllowed: function (node) {
>+    let pm = Components.classes["@mozilla.org/permissionmanager;1"]
>+      .getService(Components.interfaces.nsIPermissionManager);
>+
>+    let prefs = [["xpinstall.whitelist.add", pm.ALLOW_ACTION],
>+                 ["xpinstall.whitelist.add.36", pm.ALLOW_ACTION],
>+                 ["xpinstall.blacklist.add", pm.DENY_ACTION]];
>+
>+    prefs.forEach(function ([pref, permission]) {
>+      let hosts = Application.prefs.getValue(pref, "");
>+      if (hosts) {
>+        hosts.split(",").forEach(function (host) {
>+          pm.add(makeURI("http://" + host), "install", permission);
>+        });
>+
>+        Application.prefs.setValue(pref, "");

That seems like a slightly odd thing to do.
Won't this mess us up the second time we call _isAllowed, or will the pm we get on the first line of this function always be the same pm, and so the prefs will be saved in it?

>+  _getNotificationBox: function () {

I wonder if this should be a property getter instead.

>+++ b/mail/locales/en-US/chrome/messenger/messenger.properties
>@@ -554,3 +554,15 @@ mailServerLoginFailedEnterNewPasswordBut
>+# LOCALIZATION NOTE (lwthemeInstallRequest.message): %S will be replaced with
>+# the host name of the site.

I think this should all be on the same line, even though it blows through the 80-character limit.

>+lwthemeInstallRequest.message=This site (%S) attempted to install a theme.
>+lwthemeInstallRequest.allowButton=Allow
>+lwthemeInstallRequest.allowButton.accesskey=a
>+ 

There's an extra space on this line.

>+lwthemePostInstallNotification.message=A new theme has been installed.
>+lwthemePostInstallNotification.undoButton=Undo
>+lwthemePostInstallNotification.undoButton.accesskey=U
>+lwthemePostInstallNotification.manageButton=Manage Themes…
>+lwthemePostInstallNotification.manageButton.accesskey=M

Finally, I've run into some problems when downloading a theme in one profile, then trying to switch to it in another profile.  Now, it could be that I've got different versions of the addon installed, but I thought it might be something you should give a quick test to see if it WFY.

So, I'm giving this an r+ based on the assumption that the multi-profile stuff works for you.  If you wanted a review of the code to fix the call to "openAddonsMgr("themes");" please feel free to attach another patch, and re-ask me for a review.

Thanks,
Blake.

Comment 14

[Andreas Nilsson (:andreasn)]

Attachment: updated qute, gnomestripe and pinstripe patch

Updated patch to make sure the image ends up in the top right corner where a lot of the themes put the interesting stuff.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #13)
> >@@ -1231,3 +1242,201 @@ function ThreadPaneOnDrop(aEvent) {
> >+  handleEvent: function (event) {
> >+    switch (event.type) {
> >+      case "InstallBrowserTheme":
> >+      case "PreviewBrowserTheme":
> >+      case "ResetBrowserThemePreview":
> >+        // ignore requests from background tabs
> 
> But we don't ignore pagehide requests from background tabs, cause they don't
> send any, I'm guessing.

Yeah, I think that's probably true. In any case, this is the same as what FF is doing.

> >+
> >+  get _manager () {
> >+    let temp = {};
> >+    Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
> >+    delete this._manager;
> >+    return this._manager = temp.LightweightThemeManager;
> >+  },
> 
> Do we really want to do that every time we get the manager?  And if so, why are
> we even bothering to store it in this._manager?

We're not doing that every time. We're doing it the first time - see the delete this._manager - it deletes the function and then replaces it with an attribute.

> >+  _isAllowed: function (node) {
> >+    let pm = Components.classes["@mozilla.org/permissionmanager;1"]
> >+      .getService(Components.interfaces.nsIPermissionManager);
...

> That seems like a slightly odd thing to do.
> Won't this mess us up the second time we call _isAllowed, or will the pm we get
> on the first line of this function always be the same pm, and so the prefs will
> be saved in it?

We're using .getService, therefore we'll always get the same pm (and yes, I find it weird as well).

> >+  _getNotificationBox: function () {
> 
> I wonder if this should be a property getter instead.

Hmm. I'm not sure its right to make it a getter in this instance. "notificationbox" doesn't belong to LightWeightThemeWebInstaller and it is more just a utility function.

> >+++ b/mail/locales/en-US/chrome/messenger/messenger.properties
> >@@ -554,3 +554,15 @@ mailServerLoginFailedEnterNewPasswordBut
> >+# LOCALIZATION NOTE (lwthemeInstallRequest.message): %S will be replaced with
> >+# the host name of the site.
> 
> I think this should all be on the same line, even though it blows through the
> 80-character limit.

If you look at our existing localisation notes, I believe we don't need to do that.

> Finally, I've run into some problems when downloading a theme in one profile,
> then trying to switch to it in another profile.  Now, it could be that I've got
> different versions of the addon installed, but I thought it might be something
> you should give a quick test to see if it WFY.

If you're swapping between old-personas extension only and newer lightweight themes + personas. I think there are a few issues, that are to do with the personas extension itself rather than the core code we put into Thunderbird.

> So, I'm giving this an r+ based on the assumption that the multi-profile stuff
> works for you.  If you wanted a review of the code to fix the call to
> "openAddonsMgr("themes");" please feel free to attach another patch, and re-ask
> me for a review.

Coming up ;-)

Comment 16

[Mark Banner (:standard8)]

Attachment: The fix v2

With corrected switching to the right pane in the extension manager.

Looking at the possibilities for testing, I believe from the personas wiki pages that we can probably do some mozmill tests if we can access the required APIs for a non-personas site.

This is going to take a bit of working out, and given that I'm not exactly sure if notificationbar testing is the problem in bug 550844, I'm going to suggest that I file a follow up bug for the tests, and get this patch (including the strings) landed for b2 and try and drive the tests home for 3.1.

Comment 17

[Reed Loden [:reed]]

Comment on attachment 438737 [details] [diff] [review]
The fix v2

> pref("xpinstall.whitelist.add", "update.mozilla.org");

update.mozilla.org is long gone... would be good to remove it.

>+    prefs.forEach(function ([pref, permission]) {
>+      let hosts = Application.prefs.getValue(pref, "");
>+      if (hosts) {
>+        hosts.split(",").forEach(function (host) {
>+          pm.add(makeURI("http://" + host), "install", permission);
>+        });

The "http://" in that caught my attention. Why is that specifically http://? What is that used for?

Comment 18

[Andreas Nilsson (:andreasn)]

Attachment: updated qute, gnomestripe and pinstripe patch (v2)

*Facepalm* and we need to do it for qute as well of course.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 438737 [details] [diff] [review]
The fix v2

I agree that we should get this in, and I like the changes, but…

1) Sometimes, when I attempt to show the details of an installed persona I get the following error:
JavaScript strict warning: chrome://personas/content/personas.js, line 979: reference to undefined property PersonaService.currentPersona.detailURL

2) When I hover over an image, the preview doesn't show, and also when I click the "Wear this Persona" button, I see:
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: chrome://global/content/contentAreaUtils.js :: makeURI :: line 749" data: no]

As discussed over IRC, that second error looks like it's from my xpinstall.whitelist.add preference having the value "update.mozilla.org, www.thunderbrowse.com". Presumably because I installed thunderbrowse sometime in the past.  So it looks like we need to handle spaces around the hostnames in the preferences.

So, I think that we should have one more go-around with (at least) that second bug fixed, and then it'll be ready to land.  :)

Thanks,
Blake.

Comment 20

[Mark Banner (:standard8)]

(In reply to comment #17)
> (From update of attachment 438737 [details] [diff] [review])
> > pref("xpinstall.whitelist.add", "update.mozilla.org");
> 
> update.mozilla.org is long gone... would be good to remove it.

That's a separate bug (boy I wished FF folks could at least cc us on things like that).
> 
> >+    prefs.forEach(function ([pref, permission]) {
> >+      let hosts = Application.prefs.getValue(pref, "");
> >+      if (hosts) {
> >+        hosts.split(",").forEach(function (host) {
> >+          pm.add(makeURI("http://" + host), "install", permission);
> >+        });
> 
> The "http://" in that caught my attention. Why is that specifically http://?
> What is that used for?

I'm guessing so it has a valid scheme for the ioService not to be upset. You might want to try asking the people in browser/ who wrote the original code... I wouldn't be surprised if on the "install" side of things, the host part of the uri is the only bit that is cared about and the scheme is ignored.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

As it happens, I checked into the code this afternoon, and yeah, the host part was the only bit that was used.  :)

Later,
Blake.

Comment 22

[Andreas Nilsson (:andreasn)]

spinning off some issues as new bugs, but I don't think we need to block on
them
bug 559097
bug 559099
bug 559101

Comment 23

[Mark Banner (:standard8)]

Attachment: MozMill Test

Mozmill unit test. Based on the work in bug 550844. I'm a little concerned about the sleeps, but I'm pretty sure they really are just to make MozMill sleep and let Thunderbird deal with itself.

In any case, I'll submit this in a separate changeset in case it is really bad ;-)

Comment 24

[Mark Banner (:standard8)]

Attachment: The fix v3

Ok, this ones uses trim, and as you made me do that, I've also tidied up the code in permissions.js.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 438754 [details] [diff] [review]
updated qute, gnomestripe and pinstripe patch (v2)

>-        windowtype="mail:3pane">
>+        windowtype="mail:3pane"
>+	lightweightthemes="true">

nit: these should be aligned.

>+#messengerWindow:-moz-lwtheme {
>+  background-position: top right;
>+}

As mentioned on irc, this should include background-repeat: no-repeat; (like FF) and be moved to a content css file (don't forget to remove both of them).

>diff --git a/mail/themes/pinstripe/mail/primaryToolbar.css b/mail/themes/pinstripe/mail/primaryToolbar.css
>+}
>\ No newline at end of file

Please fix.

>diff --git a/mail/themes/pinstripe/mail/searchBox.css b/mail/themes/pinstripe/mail/searchBox.css
>+}
>\ No newline at end of file

Please fix.

I've not tested these yet, we'll get some try server builds going with both of our patches once you've updated, which should help Bryan with his ui review.

Comment 26

[Andreas Nilsson (:andreasn)]

Attachment: updated qute, gnomestripe and pinstripe patch (v3)

This patch should address the points raised by Mark in Comment 25

Comment 27

[Mark Banner (:standard8)]

Try server builds of Andreas' v3 patch and my v3 patch available here (you'll probably also want to get the personas extension and hack the install.rdf for the version number):

http://s3.mozillamessaging.com/build/try-server/2010-04-15_09:[e-mail]-st8-538335/[e-mail]2
http://s3.mozillamessaging.com/build/try-server/2010-04-15_09:[e-mail]-st8-538335/[e-mail]
http://s3.mozillamessaging.com/build/try-server/2010-04-15_09:[e-mail]-st8-538335/[e-mail]

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 439240 [details] [diff] [review]
updated qute, gnomestripe and pinstripe patch (v3)

Ok, as long as Bryan's happy, this looks fine to me.

Comment 29

[Andreas Nilsson (:andreasn)]

Bryan: do you want screenshots for these as well?
I feel it's several parts at work here, so it might be best to try it out in action.

Comment 30

[Bryan Clark (DevTools PM) [:clarkbw]]

Right now the only thing I'm noticing wrong is that there is a vertical shift happening when I switch from the default theme to another theme.  This happens on both Windows and the Mac

Comment 31

[Andreas Nilsson (:andreasn)]

Attachment: updated patch to make pinstripe statusbar not jump

Comment 32

[Andreas Nilsson (:andreasn)]

Attachment: qute and pinstripe jumping fixed

Bryan: Does this patch prevent the jumping you're experienced with the previous patch?

Comment 33

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 438987 [details] [diff] [review]
The fix v3

I like it.

Later,
Blake.

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 438981 [details] [diff] [review]
MozMill Test

>+++ b/mail/test/mozmill/content-tabs/html/test-lwthemes.html
>@@ -0,0 +1,46 @@
>+<html><head>
>+<title>test lightweight themes</title>
>+</head><body>
>+<script type="text/javascript">
[snip…]
>+const INSTALL = "InstallBrowserTheme";
>+const PREVIEW = "PreviewBrowserTheme";
>+const RESET_PREVIEW = "ResetBrowserThemePreview";
>+
>+function setTheme(node, theme, action) {
>+  node.setAttribute("data-browsertheme", JSON.stringify(themes[theme]));
>+  var event = document.createEvent("Events");
>+  dump("dispatching " + action + "\n");
>+  event.initEvent(action, true, false);
>+  node.dispatchEvent(event);
>+}
>+</script>

So, this looks like it should be kept in sync with the personas page, somehow, and I'm a little nervous that webdev might make a change which doesn't get propagated to this page.  But then our installations out in the field could break, so I guess it's not that bad…

>+++ b/mail/test/mozmill/content-tabs/test-lwthemes.js
>@@ -0,0 +1,225 @@
>+/**
>+ * The purpose of this test is to check that lightweight theme installation
>+ * works correctly.
>+ */
>+
>+//

Uh, was that a typo?

>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

I thought the license should go before the comment.

>+var MODULE_NAME = 'test-lightweight themes';

Should that be "test-lightweight-themes" instead?
(And double-quotes, please.)

>+var RELATIVE_ROOT = '../shared-modules';
>+var MODULE_REQUIRES = ['window-helpers'];
>+
>+var controller = {};
>+Components.utils.import('resource://mozmill/modules/controller.js', controller)
>+;

I think this could go on the previous line.  ;)

>+var mozmill = {};
>+Components.utils.import('resource://mozmill/modules/mozmill.js', mozmill);
>+var elib = {};
>+Components.utils.import('resource://mozmill/modules/elementslib.js', elib);
>+
>+var windowHelper;
>+var mc;
>+
>+// RELATIVE_ROOT messes with the collector, so we have to bring the path back
>+// so we get the right path for the resources.
>+var url = collector.addHttpResource('../content-tabs/html', 'content');
>+
>+var setupModule = function (module) {
>+  windowHelper = collector.getModule('window-helpers');
>+  mc = windowHelper.wait_for_existing_window("mail:3pane");
>+  windowHelper.installInto(module);
>+  windowHelper.augment_controller(mc);
>+};

I think this is done in the folder-display-helpers, so maybe we should just use it.  (I know, but sid0 suggested I used it for my migration assistant, too, and it seemed to help.)

>+const ALERT_TIMEOUT = 10000;
>+
>+let AlertWatcher = {
>+  planForAlert: function(aController) {

We already have plan_for_modal_dialog, plan_for_new_window, plan_for_window_close, etc, so we should probably change these to plan_for_alert.

And this seems useful to more than just the lightweight themes.  What do you think about moving it into a utility class?

>+function install_theme(themeNo, previousThemeNo) {
[snip…]
>+  // Clicking the button will bring up a notification box requesting to allow
>+  // installation of the theme
>+  dump("Selecting to install\n");

I don't think we need the dump statements in our tests.

>+    if (currentLwTheme().id != ("test-0" + previousThemeNo))
>+      throw new Error("After undo expected: test-0" + previousThemeNo + " but got " + currentLwTheme().id);

I think we could break this line to keep it under 80 characters.

So, I ran it, and it worked, and it looks pretty good to me, so you'll get my r+, but I think the code could probably also benefit from a once-over from sid0 or asuth, if you wanted to make it as good as possible.

Thanks,
Blake.

Comment 35

[Andreas Nilsson (:andreasn)]

Attachment: gnomestripe, pinstripe, qute patch

This patch avoids the jumping on qute entirely by adding padding. I don't have access to the Mac right now, so there might be some jumping going on in the tab bar there still, but the statusbar jumping of pinstripe is gone.
Putting it up for UI review in about 8 hours or so when I'm back at the office and can try it out on the mac as well.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #35)
> Created an attachment (id=440072) [details]
> gnomestripe, pinstripe, qute patch
> 
> This patch avoids the jumping on qute entirely by adding padding. I don't have
> access to the Mac right now, so there might be some jumping going on in the tab
> bar there still, but the statusbar jumping of pinstripe is gone.
> Putting it up for UI review in about 8 hours or so when I'm back at the office
> and can try it out on the mac as well.

Tried this patch but I'm still seeing the jumping on the Mac.  It appears to be above and below the tabs as well as the status bar.

Comment 37

[Andreas Nilsson (:andreasn)]

Attachment: patch for gnomestripe, pinstripe, qute v4

This should take care of the jumping on Pinstripe as well.

Comment 38

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 440180 [details] [diff] [review]
patch for gnomestripe, pinstripe, qute v4

This works great for me on the Mac.  I'm going to take a quick look at it on Windows today but I expect that it will work like it did before.  Unless I holler in an hour consider this a ui-r+

Comment 39

[Mark Banner (:standard8)]

(In reply to comment #34)
> So, this looks like it should be kept in sync with the personas page, somehow,
> and I'm a little nervous that webdev might make a change which doesn't get
> propagated to this page.  But then our installations out in the field could
> break, so I guess it's not that bad…

Well if they change the notification system, we're going to break badly anyway. About the only thing we can do is back this up with a litms test.
> >+var mozmill = {};
> >+Components.utils.import('resource://mozmill/modules/mozmill.js', mozmill);
> >+var elib = {};
> >+Components.utils.import('resource://mozmill/modules/elementslib.js', elib);
> >+
> >+var windowHelper;
> >+var mc;
> >+
> >+// RELATIVE_ROOT messes with the collector, so we have to bring the path back
> >+// so we get the right path for the resources.
> >+var url = collector.addHttpResource('../content-tabs/html', 'content');
> >+
> >+var setupModule = function (module) {
> >+  windowHelper = collector.getModule('window-helpers');
> >+  mc = windowHelper.wait_for_existing_window("mail:3pane");
> >+  windowHelper.installInto(module);
> >+  windowHelper.augment_controller(mc);
> >+};
> 
> I think this is done in the folder-display-helpers, so maybe we should just use
> it.  (I know, but sid0 suggested I used it for my migration assistant, too, and
> it seemed to help.)

Whilst that may be true, I don't want to bring folder stuff into non-folder tests, we should probably look at a more generic test.

> >+const ALERT_TIMEOUT = 10000;
> >+
> >+let AlertWatcher = {
> >+  planForAlert: function(aController) {
> 
> We already have plan_for_modal_dialog, plan_for_new_window,
> plan_for_window_close, etc, so we should probably change these to
> plan_for_alert.
> 
> And this seems useful to more than just the lightweight themes.  What do you
> think about moving it into a utility class?

We've got another bug that is going to use this object, I'll look into unifying the object there.

> I don't think we need the dump statements in our tests.

I find them useful especially if the test does fail as it quickly allows you to remember where they are. However, I've removed them for now.

> So, I ran it, and it worked, and it looks pretty good to me, so you'll get my
> r+, but I think the code could probably also benefit from a once-over from sid0
> or asuth, if you wanted to make it as good as possible.

They are both pretty busy right now, but given this is largely based on another test that asuth has looked at, I'll land it.

Comment 40

[Mark Banner (:standard8)]

Checked in:

http://hg.mozilla.org/comm-central/rev/45651089a8da (themes part)
http://hg.mozilla.org/comm-central/rev/5aad9657aef4 (UI part)

We need to get the Personas extension updated, but I'll file a bug on them to do that once we get 3.1b2 added to AMO.

Any follow-up issues in new bugs please.