Closed Bug 538335 Opened 11 years ago Closed 11 years ago

Support Lightweight Themes in Thunderbird

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird3.1 beta2-fixed)

RESOLVED FIXED
Thunderbird 3.1b2
Tracking Status
thunderbird3.1 --- beta2-fixed

People

(Reporter: standard8, Assigned: andreasn)

References

()

Details

Attachments

(3 files, 12 obsolete files)

18.97 KB, patch
bwinton
: review+
Details | Diff | Splinter Review
12.61 KB, patch
bwinton
: review+
Details | Diff | Splinter Review
6.64 KB, patch
andreasn
: review+
clarkbw
: ui-review+
Details | Diff | Splinter Review
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).
Flags: blocking-thunderbird3.1?
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.
Keywords: helpwanted
investigating this one
Assignee: nobody → nisses.mail
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/
Attached patch step 1 and 3 (obsolete) — Splinter Review
but step2 is the hard one (these theme fixes only apply to Linux for now, but Windows fixes coming up)
I don't think this is blocking, but definitely wanted (please correct me if I'm wrong).
Flags: blocking-thunderbird3.1? → blocking-thunderbird3.1-
Attached patch windows and linux styling (obsolete) — Splinter Review
using this as a temporary storing to get the mac part ready
Attachment #422824 - Attachment is obsolete: true
Attached patch and pinstripe part (obsolete) — Splinter Review
Still some issues to iron out (such as the lack of tab divider when using personas).
Duplicate of this bug: 546855
Attachment #427136 - Attachment is obsolete: true
Attachment #427100 - Flags: ui-review?(clarkbw)
Attachment #437530 - Flags: ui-review?(clarkbw)
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.
Attached patch The fix (obsolete) — Splinter Review
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.
Attachment #438097 - Flags: review?(bwinton)
Keywords: helpwanted
Attachment #427100 - Attachment is obsolete: true
Attachment #427100 - Flags: ui-review?(clarkbw)
Attachment #437530 - Attachment description: updated pinstripe patch → qute, gnomestripe and pinstripe patch
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 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.
Attachment #438097 - Flags: review?(bwinton) → review+
Updated patch to make sure the image ends up in the top right corner where a lot of the themes put the interesting stuff.
Attachment #437530 - Attachment is obsolete: true
Attachment #438713 - Flags: ui-review?(clarkbw)
Attachment #438713 - Flags: review?(bugzilla)
Attachment #437530 - Flags: ui-review?(clarkbw)
(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 ;-)
Attached patch The fix v2 (obsolete) — Splinter Review
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.
Attachment #438097 - Attachment is obsolete: true
Attachment #438737 - Flags: review?(bwinton)
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?
Summary: Support LightWeight Themes in Thunderbird → Support Lightweight Themes in Thunderbird
*Facepalm* and we need to do it for qute as well of course.
Attachment #438713 - Attachment is obsolete: true
Attachment #438713 - Flags: ui-review?(clarkbw)
Attachment #438713 - Flags: review?(bugzilla)
Attachment #438754 - Flags: ui-review?(clarkbw)
Attachment #438754 - Flags: review?(bugzilla)
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.
Attachment #438737 - Flags: review?(bwinton) → review-
(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.
As it happens, I checked into the code this afternoon, and yeah, the host part was the only bit that was used.  :)

Later,
Blake.
spinning off some issues as new bugs, but I don't think we need to block on
them
bug 559097
bug 559099
bug 559101
Attached patch MozMill TestSplinter Review
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 ;-)
Attachment #438981 - Flags: review?(bwinton)
Attached patch The fix v3Splinter Review
Ok, this ones uses trim, and as you made me do that, I've also tidied up the code in permissions.js.
Attachment #438737 - Attachment is obsolete: true
Attachment #438987 - Flags: review?(bwinton)
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.
Attachment #438754 - Flags: review?(bugzilla) → review-
This patch should address the points raised by Mark in Comment 25
Attachment #438754 - Attachment is obsolete: true
Attachment #439240 - Flags: ui-review?(clarkbw)
Attachment #439240 - Flags: review?(bugzilla)
Attachment #438754 - Flags: ui-review?(clarkbw)
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.
Attachment #439240 - Flags: review?(bugzilla) → review+
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.
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
Attached patch qute and pinstripe jumping fixed (obsolete) — Splinter Review
Bryan: Does this patch prevent the jumping you're experienced with the previous patch?
Attachment #439885 - Attachment is obsolete: true
Comment on attachment 438987 [details] [diff] [review]
The fix v3

I like it.

Later,
Blake.
Attachment #438987 - Flags: review?(bwinton) → review+
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.
Attachment #438981 - Flags: review?(bwinton) → review+
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.
Attachment #439240 - Attachment is obsolete: true
Attachment #439901 - Attachment is obsolete: true
Attachment #440072 - Flags: review+
Attachment #439240 - Flags: ui-review?(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.
This should take care of the jumping on Pinstripe as well.
Attachment #440072 - Attachment is obsolete: true
Attachment #440180 - Flags: ui-review?(clarkbw)
Attachment #440180 - Flags: review+
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+
Attachment #440180 - Flags: ui-review?(clarkbw) → ui-review+
(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.
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b2
Duplicate of this bug: 566301
You need to log in before you can comment on or make changes to this bug.