User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110121 Thunderbird/3.3a3pre Following the example of bug 516033: For add-ons modifying the message pane it would make life easier if the box messagepanebox would be within a named hbox. Then the add-on could add stuff to the left or right of messagepanebox, e.g. toolbars. A patch will follow soon (tonight). Reproducible: Always

Have to patch two files.

I'd call the box something other than "messagepanehbox", since it took me a while to realize that the boxes had different names. I'd go with something more like "messagepanewrapper".

Comment on attachment 505933 [details] [diff] [review] Patch for two files (messenger.xul and messageWindow.xul) bwinton has his head better wrapped around this code than I do these days, redirecting the review his way...

Unfortunately things are more complicated than thought before: messagepanebox is moved around by UpdateMailPaneConfig(), referenced through GetMessagePane(). Perhaps there would also be another possibility: Change messagepanebox into a hbox and add a vbox inside it. Could you please give some advise/help?

This patch tries to move all handling for collapsing the message pane to the messagepaneboxwrapper. Also the mozmill test helper functions related to the messagepanebox collapsing are adopted. Now the patch does not break any mozmill tests (anymore).

Comment on attachment 511510 [details] [diff] [review] Patch including fixing all mozmill tests initially broken by the original patch I'm mostly convinced that this does the right thing, but I wonder if it will impact SeaMonkey at all, so r=me, but I think you should get one of the SeaMonkey people to make sure it's okay with them before we check it in. Thanks, Blake.

(In reply to comment #7) > I'm mostly convinced that this does the right thing, but I wonder if it will > impact SeaMonkey at all, so r=me, but I think you should get one of the > SeaMonkey people to make sure it's okay with them before we check it in. Won't Seamonkey be unaffected since everything is in mail/? Though I suppose we should probably ping them to see if they want to port this so as to maximize addon cross-compatibility.

(In reply to comment #7) > Comment on attachment 511510 [details] [diff] [review] > I'm mostly convinced that this does the right thing, but I wonder if it will > impact SeaMonkey at all, so r=me, but I think you should get one of the > SeaMonkey people to make sure it's okay with them before we check it in. Sorry, but I have no idea who the SeaMonkey people are and how to contact them. Could somebody please explain this?

I'm pretty sure this has no affect on SeaMonkey code Pinging Niel for confirmation.

Comment on attachment 511510 [details] [diff] [review] Patch including fixing all mozmill tests initially broken by the original patch > <splitter id="threadpane-splitter" > collapse="after" > collapsed="true" > onmouseup="OnMouseUpThreadAndMessagePaneSplitter()"/> > >- <vbox id="messagepanebox" flex="2" minheight="100" height="200" >- minwidth="100" width="200" persist="height width"> ... >+ <!-- a convenience box for ease of extension overlaying --> >+ <hbox id="messagepaneboxwrapper" flex="1" persist="collapsed"> >+ <vbox id="messagepanebox" flex="2" minheight="100" height="200" >+ minwidth="100" width="200" persist="height width"> Unfortunately this change is going to break your persistence. You might be able to put a messagepanewrapper box inside the messagepanebox though. (We have a <hbox id="messagepanewrapper"> although things work slightly differently for us since we don't have to deal with a multimessage browser.)

(In reply to comment #11) > Comment on attachment 511510 [details] [diff] [review] > Patch including fixing all mozmill tests initially broken by the original patch > > > <splitter id="threadpane-splitter" > > collapse="after" > > collapsed="true" > > onmouseup="OnMouseUpThreadAndMessagePaneSplitter()"/> > > > >- <vbox id="messagepanebox" flex="2" minheight="100" height="200" > >- minwidth="100" width="200" persist="height width"> > ... > >+ <!-- a convenience box for ease of extension overlaying --> > >+ <hbox id="messagepaneboxwrapper" flex="1" persist="collapsed"> > >+ <vbox id="messagepanebox" flex="2" minheight="100" height="200" > >+ minwidth="100" width="200" persist="height width"> > Unfortunately this change is going to break your persistence. You might be able > to put a messagepanewrapper box inside the messagepanebox though. (We have a > <hbox id="messagepanewrapper"> although things work slightly differently for us > since we don't have to deal with a multimessage browser.) Could you please explain, what the problem is? Up to now the collapsed state is saved for messagepanebox. With this patch it is saved for messagepaneboxwrapper. This might break the collapsed state at the time of the update, but after this it should be saved again. I think this is also tested in one of the mozmill tests. Here is the patch for removing the persist state form messagepanebox: +++ a/mail/base/content/messageWindow.xul @@ -159,35 +159,38 @@ <!-- msg header view --> - <vbox id="messagepanebox" flex="3" persist="collapsed" - ondragover="nsDragAndDrop.dragOver(event, messagepaneObserver);" - ondragdrop="nsDragAndDrop.drop(event, messagepaneObserver);" - ondragexit="nsDragAndDrop.dragExit(event, messagepaneObserver);"> + <!-- a convenience box for ease of extension overlaying --> + <hbox id="messagepaneboxwrapper" flex="1"> + <vbox id="messagepanebox" flex="3" + ondragover="nsDragAndDrop.dragOver(event, messagepaneObserver);" + ondragdrop="nsDragAndDrop.drop(event, messagepaneObserver);" + ondragexit="nsDragAndDrop.dragExit(event, messagepaneObserver);">

How to proceed with the patch? Is there a chance to get it into Thunderbird or not? The only other way I can think of achieving the proposed behavior would be the change the type of the existing messagepanebox to hbox and add an additional vbox inside it. Any comments? (Also regarding the questions in comment #12)

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 511510 [details] [diff] [review]) > > > <splitter id="threadpane-splitter" > > > collapse="after" > > > collapsed="true" > > > onmouseup="OnMouseUpThreadAndMessagePaneSplitter()"/> > > > > > >- <vbox id="messagepanebox" flex="2" minheight="100" height="200" > > >- minwidth="100" width="200" persist="height width"> > > ... > > >+ <!-- a convenience box for ease of extension overlaying --> > > >+ <hbox id="messagepaneboxwrapper" flex="1" persist="collapsed"> > > >+ <vbox id="messagepanebox" flex="2" minheight="100" height="200" > > >+ minwidth="100" width="200" persist="height width"> > > Unfortunately this change is going to break your persistence. You might be able > > to put a messagepanewrapper box inside the messagepanebox though. (We have a > > <hbox id="messagepanewrapper"> although things work slightly differently for us > > since we don't have to deal with a multimessage browser.) > Could you please explain, what the problem is? Up to now the collapsed state is > saved for messagepanebox. With this patch it is saved for > messagepaneboxwrapper. This might break the collapsed state at the time of the > update, but after this it should be saved again. I think this is also tested in > one of the mozmill tests. On second thoughts your main problem is that your new box is missing height and width persistence. I thought there might be a problem with the old persistence data applying to the wrong box, but you weren't persisting the collapsed state so that might actually work after all.

This patch moves the height and width persistence for the existing messagepanebox to the new messagepaneboxwrapper. It also adds a mozmill test for the persistence of the height of the messagepaneboxwrapper. At the moment I cannot see how the width attribute of the messagepanebox is used (in Thunderbird without the patch): If the folderpane_splitter is moved the value does not change. So why make it persistent?

Sorry for noise, but now I understand when the width is used: If the vertical mail pane layout is used. So I added another mozmill test for checking the persistence of the width. For this the mail pane layout has to be changed to vertical view at the begin of the test (and back to the classical view at the end). Implicitly this test also checks the persistence of the mail pane layout.

I could repeat my question from comment #13. I still have not understand how to proceed. I think the mozmill tests show that collapsed state as well as the width and height of the message pane are now persistent (again). This information is only lost when updating from an older version of Thunderbird. Is this acceptable?

xref bug 634831 A lot of folks are depending on this extension. Pinging Blake for review.

Comment on attachment 516960 [details] [diff] [review] Now with addtional mozmill test for width persistence >+++ a/mail/test/mozmill/session-store/test-session-store.js >@@ -288,6 +289,75 @@ >+function test_message_pane_width_persistence() { >+ be_in_folder(folderA); >+ assert_message_pane_visible(); >+ >+ /* at the beginning we are in classic layout and will >+ * set reset to it at the end of this test after switching >+ * to vertical layout because width is only relevant then >+ */ I would rephrase this as: /* At the beginning we are in classic layout. We will switch to * vertical layout to test the width, and then back to classic layout. */ and as a more general comment, our comments should be full sentences, with spaces after the "//", and capital letters at the start, and periods at the end. >+ let oldWidth = mc.e("messagepaneboxwrapper").clientWidth; >+ let minWidth = parseInt(mc.e("messagepaneboxwrapper").getAttribute("minwidth")); >+ let newWidth = parseInt((minWidth + oldWidth) / 2); I think "Math.floor" would be better than "parseInt" here… I might also assert that oldWidth != newWidth, just to make sure we're testing the behaviour. >+ mc = open3PaneWindow(windowWatcher); Is this synchronous, or do we wait for it to open in one of the following calls? >+ // Reset to classical mail layout >+ set_pane_layout(kClassicMailLayout); >+ assert_pane_layout(kClassicMailLayout); >+ >+ // We don't need the address book window any more. >+ plan_for_window_close(abwc); >+ abwc.window.close(); >+ wait_for_window_close(); I wonder if these should go in a teardown function, so that they put Thunderbird back in a reasonable state even if the test fails? Either teardownTest or teardownModule seem like reasonable candidates. Aside from those fairly minor changes, I like this, so r=me. Later, Blake.

(In reply to comment #19) > I would rephrase this as: > /* At the beginning we are in classic layout. We will switch to > * vertical layout to test the width, and then back to classic layout. > */ > > and as a more general comment, our comments should be full sentences, with > spaces after the "//", and capital letters at the start, and periods at > the end. I will change all new comments. > >+ let oldWidth = mc.e("messagepaneboxwrapper").clientWidth; > >+ let minWidth = parseInt(mc.e("messagepaneboxwrapper").getAttribute("minwidth")); > >+ let newWidth = parseInt((minWidth + oldWidth) / 2); > > I think "Math.floor" would be better than "parseInt" here… I changed it. > I might also assert that oldWidth != newWidth, just to make sure we're > testing the behaviour. I added new asserts for this. > > >+ mc = open3PaneWindow(windowWatcher); > > Is this synchronous, or do we wait for it to open in one of the following > calls? open3PaneWindow is defined in the same file: https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/session-store/test-session-store.js#99 At the end of the function there is a call to windowHelper.wait_for_new_window("mail:addressbook"). So this is a synchronous call as the waiting is done in the function. > >+ // Reset to classical mail layout > >+ set_pane_layout(kClassicMailLayout); > >+ assert_pane_layout(kClassicMailLayout); > >+ > >+ // We don't need the address book window any more. > >+ plan_for_window_close(abwc); > >+ abwc.window.close(); > >+ wait_for_window_close(); > > I wonder if these should go in a teardown function, so that they put > Thunderbird back in a reasonable state even if the test fails? Either > teardownTest or teardownModule seem like reasonable candidates. As discussed in the linked thread, teardownTest will be removed in future versions of mozmill. As other tests in the same module (file) might rely on these states I think it has to be done/checked in the test itself. http://groups.google.com/group/mozmill-dev/browse_thread/thread/e9c88e4be37e1579?hl=en New version of patch will follow soon...

Please set flag for this patch to be checked into the tree.

Since you are the assignee of this bug, you should be able to set the "checkin-needed" in the "Keywords" field yourself, otherwise someone can do it for you (usually I also add [c-n: comm-central] to the white-board to indicate that it's supposed to go on trunk, but that's the default anyway).

(In reply to comment #22) > Since you are the assignee of this bug, you should be able to set the > "checkin-needed" in the "Keywords" field yourself, otherwise someone can do > it for you (usually I also add [c-n: comm-central] to the white-board to > indicate that it's supposed to go on trunk, but that's the default anyway). Thank you for these information. I didn't know. I also set the in-testsuite, because there are the (new) mozmill tests.

Checked in: http://hg.mozilla.org/comm-central/rev/e2efee5a9956

Unfortunately this broke both single message in a tab and viewing a message in the standalone message window. Therefore I've backed it out: http://hg.mozilla.org/comm-central/rev/57eebd88c533

The problem could be fixed by adding a flex attribute with value 1 to the messagepanewrapperbox. But then there is a problem with the mozmill tests (for height and width persistence): They change the height (or width) by setAttribute commands. But if the messagepanewrapperbox has a flex attribute the actual size is different from the value set to the attribute. In "normal use" the attributes are newly set if the threadpane-splitter (or the folderpane_splitter) is moved with the mouse. But I do not know how to simulate this in mozmill (mc.mouseDown, .mouseUp do not seem to work). One "fix" would be to set the flex attribute of the displayDeck and threadContentArea to a very high value (999). Then the values set to the height and width attribute of messagepanewrapperbox result in the correct height/width and the same clientHeight/clientWidth values. It would also be possible to change to large flex attributes just in the mozmill tests to simulate the correct behavior. But then still the values of the height/width attributes of displayDeck (or threadContentArea) are set to wrong values. Is there a way to move the splitters in mozmill? If not, what would be an acceptable way to go? Thank you for your help!

I suspect some combination of mouseDown/mouseUp might work. Also, asking in #mozmill on irc.mozilla.org might get you a better answer. Or you could dig into the source, and find https://github.com/mikeal/mozmill/blob/1.4.2/mozmill/mozmill/extension/resource/modules/controller.js#L922 which, while it's not exactly what you want to call, should give you the code you want to copy to do what you want. ;) Later, Blake.

Just an FYI, the mozmill repo has moved to https://github.com/mozautomation/mozmill. Mikeal's branch is over about a year out of date at this point. If you want to see code that isn't the latest development version, just select the appropriate branch on github.

The following code works for moving the splitters: _move_splitter(mc.e("threadpane-splitter"), 0, diffHeight); _move_splitter(mc.e("folderpane_splitter"), diffWidth, 0); function _move_splitter(aSplitter, aDiffX, aDiffY) { EventUtils.synthesizeMouse(aSplitter, 0, 0, {type:"mousedown"}, mc.window); EventUtils.synthesizeMouse(aSplitter, 2*aDiffX, aDiffY, {type:"mousemove"}, mc.window); EventUtils.synthesizeMouse(aSplitter, 0, 0, {type:"mouseup"}, mc.window); wait_for_message_display_completion(mc); } But why do I have to multiply the X offset by 2?

I have no idea. Where did you get the code? I can say that there's some drag and drop code at https://gist.github.com/731332 that looks like it might work. (Specifically, using "0, 0" for the x and y of the mousedown and mouseup doesn't seem like the right thing to do…)

I created that code based on tabmail/test-tabmail-dragndrop.js: https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/tabmail/test-tabmail-dragndrop.js#501 The x and y are offsets relative to the node clicked on (see https://mxr.mozilla.org/comm-central/source/mozilla/testing/mozmill/mozmill/mozmill/extension/resource/stdlib/EventUtils.js#224). So the mouse button is pressed over the splitter, then the mouse moves left/right or up/down and with it the splitter. Then the mouse button is released so the the splitter stays where it was moved to. For the y direction everything works as expected. But why do I have to multiply the relative x coordinate with 2? I tried it with different X offsets and the factor 2 seems to be "universal". I got the hint abut tabmail/test-tabmail-dragndrop.js from #mozmill on irc but I haven't received some answers about the factor 2 problem.

This patch adds a flex attribute to the messagepaneboxwrapper that fixes the problem reported in bug 645327 (and the original patch). Also adds checks to test_open_single_message_in_tab() and test_open_message_in_new_window() to verify the fix. This patch replaces the former patch (which were in/excluded from the tree). (The problem reported in comment #29 was caused by using the wrong splitter in the mozmill test: When moving folderpane_splitter by two pixels in horizontal direction threadpane-splitter is moved by one)

Comment on attachment 522810 [details] [diff] [review] New patch which add flex attribute to messagepanewrapperbox So, I did a search for "messagepanebox[^w]" to get all the places you didn't change "messagepanebox" to "messagepaneboxwrapper", so that we can double-check that you caught them all. And speaking of catching things, http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1303238678.1303239465.14128.gz shows two failures: TEST-UNEXPECTED-FAIL | test-opening-messages.js | test_open_message_in_new_window EXCEPTION: Messagepaneboxwrapper height is not correct. at: test-opening-messages.js line 176 test_open_message_in_new_window() test-opening-messages.js 176 frame.js 474 frame.js 530 and TEST-UNEXPECTED-FAIL | test-session-store.js | test_message_pane_width_persistence EXCEPTION: The message pane width should be 141, but is actually 142 at: test-session-store.js line 337 test_message_pane_width_persistence() test-session-store.js 337 frame.js 474 frame.js 530 frame.js 573 frame.js 417 frame.js 579 server.js 164 server.js 168 Both of those seem like they could be caused by your patch, so I think you should look into them. >+++ b/mail/base/content/folderDisplay.js >@@ -1689,17 +1689,17 @@ FolderDisplayWidget.prototype = { > // save the message pane's state only when it is potentially visible > this.messagePaneCollapsed = >- document.getElementById("messagepanebox").collapsed; >+ document.getElementById("messagepaneboxwrapper").collapsed; What if the messagepanebox was collapsed, but the wrapper wasn't? For instance, if someone added a splitter in an extension… >+++ b/mail/test/mozmill/folder-display/test-opening-messages.js >@@ -87,19 +87,27 @@ function test_open_single_message_in_tab >+ // Throw error if messagepaneboxwrapper height is not correct here, >+ // because it has to be done in the first tab. >+ if (!checkMessagePaneFullHeight) There are some trailing spaces on these lines (and others) that should be removed. >+++ b/mail/test/mozmill/session-store/test-session-store.js >@@ -228,16 +228,162 @@ function test_restore_single_3pane_persi >+ // Get the state object. This assumes there is one and only one >+ // 3pane window. Heh. That's going to be a bad assumption in the future, but we can burn that bridge when we get to it. :) >+ let diffHeight = oldHeight - newHeight; >+ if (oldHeight == newHeight) >+ throw new Error("To really perform a test the new message pane height should be " >+ + "should be different from the old one but they are the same: " >+ + newHeight); Please wrap these to be less than 80 characters. And we like to put the + on the end of the previous line, instead of the start of the next line. >+function test_message_pane_width_persistence() { [snip…] >+ if (newWidth == oldWidth) >+ throw new Error("To really perform a test the new message pane width should be " >+ + "should be different from the old one but they are the same: " >+ + newWidth); Ditto here. So, I think you're close, but I can't give an r+ to a patch which makes tests fail, so I'm going to have to r- it. But I suspect the next version will get an r+. Thanks, Blake.

(In reply to comment #33) > And speaking of catching things, > http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1303238678.1303239465.14128.gz > shows two failures: > TEST-UNEXPECTED-FAIL | test-opening-messages.js | > test_open_message_in_new_window > EXCEPTION: Messagepaneboxwrapper height is not correct. > at: test-opening-messages.js line 176 > test_open_message_in_new_window() test-opening-messages.js 176 > frame.js 474 > frame.js 530 > > and > > TEST-UNEXPECTED-FAIL | test-session-store.js | > test_message_pane_width_persistence > EXCEPTION: The message pane width should be 141, but is actually 142 > at: test-session-store.js line 337 > test_message_pane_width_persistence() test-session-store.js 337 > frame.js 474 > frame.js 530 > frame.js 573 > frame.js 417 > frame.js 579 > server.js 164 > server.js 168 > > Both of those seem like they could be caused by your patch, so I think you > should look into them. > > >+++ b/mail/base/content/folderDisplay.js > >@@ -1689,17 +1689,17 @@ FolderDisplayWidget.prototype = { > > // save the message pane's state only when it is potentially visible > > this.messagePaneCollapsed = > >- document.getElementById("messagepanebox").collapsed; > >+ document.getElementById("messagepaneboxwrapper").collapsed; > I cannot reproduce this. What was your setup? This is the state of the branch (comm-central) I use: ./client.py checkout --mozilla-repo=http://hg.mozilla.org/releases/mozilla-2.0 $ hg summary parent: 7585:c673f486cb43 tip fix bug 650750, perf slowdown with NFS local mailboxes, r=bienvenu branch: default commit: 8 modified, 15 unknown update: (current) In mozilla: $ hg summary parent: 63347:c5e4090374e6 tip Add GECKO_2_0_BASE tag. a=NPOTB on a CLOSED TREE branch: default commit: 16 deleted (clean) update: (current) >>+++ b/mail/base/content/folderDisplay.js >>@@ -1689,17 +1689,17 @@ FolderDisplayWidget.prototype = { >> // save the message pane's state only when it is potentially visible >> this.messagePaneCollapsed = >>- document.getElementById("messagepanebox").collapsed; >>+ document.getElementById("messagepaneboxwrapper").collapsed; > What if the messagepanebox was collapsed, but the wrapper wasn't? For > instance, if someone added a splitter in an extension… I am not sure for what this code is used at all. Where is messagePaneCollapsed used? Also I guess it would be the responsibility of the addon to handle this because the persistence of the messagepane is saved in the messagepaneboxwrapper and if the addon collapses the messagepanebox it should save this.

(In reply to comment #34) > This is the state of the branch (comm-central) I use: > ./client.py checkout --mozilla-repo=http://hg.mozilla.org/releases/mozilla-2.0 As per the tb-planning thread (http://groups.google.com/group/tb-planning/browse_thread/thread/d6e78da21ae1d511), we've now moved onto http://hg.mozilla.org/mozilla-aurora as the mozilla repo for Miramar builds (3.3).

(In reply to comment #34) > > And speaking of catching things, > > http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1303238678.1303239465.14128.gz > > shows two failures: > > TEST-UNEXPECTED-FAIL | test-opening-messages.js | > > test_open_message_in_new_window > > EXCEPTION: Messagepaneboxwrapper height is not correct. > > at: test-opening-messages.js line 176 > > test_open_message_in_new_window() test-opening-messages.js 176 > > frame.js 474 > > frame.js 530 > > > > and > > > > TEST-UNEXPECTED-FAIL | test-session-store.js | > > test_message_pane_width_persistence > > EXCEPTION: The message pane width should be 141, but is actually 142 > > at: test-session-store.js line 337 > > test_message_pane_width_persistence() test-session-store.js 337 > > frame.js 474 > > frame.js 530 > > frame.js 573 > > frame.js 417 > > frame.js 579 > > server.js 164 > > server.js 168 > > > > Both of those seem like they could be caused by your patch, so I think you > > should look into them. > I cannot reproduce this. What was your setup? http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=621 shows that I was on http://hg.mozilla.org/try-comm-central/pushloghtml?changeset=a3830df7c0f6 and http://hg.mozilla.org/mozilla-central/rev/9b8188993c1a and that it's failing on Windows and Mac OS X. Hopefully the information on that page will help you figure out what's going wrong. Thanks, Blake.

> http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=621 shows that I was on > http://hg.mozilla.org/try-comm-central/pushloghtml?changeset=a3830df7c0f6 and > http://hg.mozilla.org/mozilla-central/rev/9b8188993c1a and that it's failing on > Windows and Mac OS X. Hopefully the information on that page will help you > figure out what's going wrong. I tried it on Windows with Thunderbird from comm-central and mozilla from http://hg.mozilla.org/mozilla-aurora and on Linux with Thunderbird from comm-central and mozilla from http://hg.mozilla.org/mozilla-central/ both tip of the branch. On both systems all tests pass. What I cannot find is the revision 9b8188993c1a of mozilla-central. The above link results in an error. Just a guess: Would it be possible that the screen resolution of the test computer (1280x800 in my case) could influence the test results?

(In reply to comment #37) > > http://arbpl.visophyte.org/?tree=ThunderbirdTry&pushid=621 shows that I was on > > http://hg.mozilla.org/try-comm-central/pushloghtml?changeset=a3830df7c0f6 and > > http://hg.mozilla.org/mozilla-central/rev/9b8188993c1a and that it's failing on > > Windows and Mac OS X. Hopefully the information on that page will help you > > figure out what's going wrong. > > I tried it on Windows with Thunderbird from comm-central and mozilla from > http://hg.mozilla.org/mozilla-aurora and on Linux with Thunderbird from > comm-central and mozilla from http://hg.mozilla.org/mozilla-central/ both tip > of the branch. On both systems all tests pass. > > What I cannot find is the revision 9b8188993c1a of mozilla-central. The above > link results in an error. > > Just a guess: Would it be possible that the screen resolution of the test > computer (1280x800 in my case) could influence the test results? Could be… I'm really not sure what's going wrong there. :( Do you have access to push to the try servers, so that you can attempt to debug it? Thanks, Blake.

If not i can push , just let me know if you need pushing :-)

(In reply to comment #38) > Could be… I'm really not sure what's going wrong there. :( Do you have > access to push to the try servers, so that you can attempt to debug it? > > Thanks, > Blake. I do not have try server access but I would be willing to become a (level 1) committer. Anybody here willing to vouch for me?

I have created builds on the try server including the patch: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=28ef85adaf3a There are fails of unit tests on Windows and Linux. I downloaded the build and tests following the description on https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing/Running_Thunderbird_MozMill_tests_from_packaged_tests but I could not reproduce the errors locally. Could please somebody check this. Unfortunately there are no details in the mozmill logs. There are different failures in the OS X versions. I cannot check them because I do not have access to such a system. Could please also somebody try to reproduce them? Is there a way to add more detailed output from mozmill tests to the try server logs?

I've tried this again on try server, and it failed, likewise locally it passed. Unfortunately I think the errors we're seeing are an artefact of MozMill error reporting currently being broken and we need to upgrade MozMill. I'll try and get onto that soon.

I tried it again on the try server and this time the windows build fails, even if the patch is not applied: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=9d31da7237c6 http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=267e2c7d1f62 Also there are no mozmill tests for Linux? But I haven't found a bug report about all this problems.

The windows build failed in the same way that it was on the trunk builders at the time you pushed. You'll need to repush from a changeset based on the latest version of comm-central. The Linux try builders had got stuck, so the tests (mozmill & xpcshell) haven't been running. I've cleared the blockage, so hopefully they will run and the results will show up. If they get blocked again, then it'll be late today before I can unblock.

> >+++ b/mail/base/content/folderDisplay.js > >@@ -1689,17 +1689,17 @@ FolderDisplayWidget.prototype = { > > // save the message pane's state only when it is potentially visible > > this.messagePaneCollapsed = > >- document.getElementById("messagepanebox").collapsed; > >+ document.getElementById("messagepaneboxwrapper").collapsed; > > What if the messagepanebox was collapsed, but the wrapper wasn't? For > instance, if someone added a splitter in an extension… Sorry, I do not understand, what you mean. With the new version of mozmill on the tryservers there are meaningful outputs (again): http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=5c5fb754f4b6 The problem on Mac OS X seems to be that the persistant message pane width is one pixel to small. I have no idea why this happens. (See failure in above tryserver build). Also when opening a message in a new window the message window seems to be one pixel higher than the DOM children within it (see https://hg.mozilla.org/try-comm-central/rev/5c5fb754f4b6#l6.104) So how to proceed with this patch?

With this patch there are no (related) mozmill test failures: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=062b1859bf4b The problem with the failing tests on Mac OS X was caused by the vertical movement of the splitter "threadpane-splitter". At the end of the movement the width of the message pane was off by one pixel (the try server run http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1316650956.1316651669.4588.gz&fulltext=1#err0 proved, that there was actually a movement). As the tests for this patch should check the persistence of the width (and height) of the message pane I ignore this one pixel offset (see comment in patch). What this patch does not address is the issue I asked for in comment #45. (Sorry for the double post.)

Comment on attachment 562042 [details] [diff] [review] Patch with working mozmill tests Review of attachment 562042 [details] [diff] [review]: ----------------------------------------------------------------- Okay, working around the weirdness on Mac isn't great, but I don't think there's much else you could do here. r=me, assuming the tests still pass! Thanks, and I apologize for the long wait on the review, Blake.

I will mark the patch for check in as soon as the tryserver for Mac OS X are working again so I can check that the mozmill tests still work.

Ok. Finally try server is working again (more or less). All mozmill tests related to this patch seem to pass: Linux: http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319580729.1319581380.2404.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319581693.1319582356.4024.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319586328.1319587324.13185.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319590623.1319591392.20386.gz&fulltext=1 Mac OS X: http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319657082.1319657835.1519.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319639169.1319639925.27185.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319639946.1319640696.28688.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319651844.1319652595.23737.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319656737.1319657492.811.gz&fulltext=1 Windows: http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319619270.1319620205.10433.gz&fulltext=1 http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1319619313.1319620505.10822.gz&fulltext=1

http://hg.mozilla.org/comm-central/rev/8ec73dd248a3 Thanks!