Last Comment Bug 712322 - Combine awful customize window hackery in test-tabmail-customize.js and test-header-toolbar.js
: Combine awful customize window hackery in test-tabmail-customize.js and test-...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Testing Infrastructure (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Joachim Herb
:
Mentors:
Depends on: 717439 tb-tabsontop 681376
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-20 08:44 PST by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-04-05 15:23 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Move customization code into a shared module (23.31 KB, patch)
2012-01-01 05:00 PST, Joachim Herb
no flags Details | Diff | Splinter Review
cleanup of the patch, still contains sleep in customization dialog opening for MAC (or more precisely if toolbar.customization.usesheet is true) (37.57 KB, patch)
2012-01-05 11:36 PST, Joachim Herb
sid.bugzilla: review-
Details | Diff | Splinter Review
Mozmill tests using the helper constructor (34.69 KB, patch)
2012-01-09 16:03 PST, Joachim Herb
no flags Details | Diff | Splinter Review
Another version of the patch which moves the dialog out of the way (36.61 KB, patch)
2012-01-16 14:07 PST, Joachim Herb
no flags Details | Diff | Splinter Review
New version without ewait which works on all OS (34.26 KB, patch)
2012-02-08 14:21 PST, Joachim Herb
mconley: review+
Details | Diff | Splinter Review
Changes requested by reviewer (34.40 KB, patch)
2012-04-05 14:32 PDT, Joachim Herb
joachim.herb: review+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2011-12-20 08:44:12 PST
There's some pretty awful hackery and duplication going on in tabmail/test-tabmail-customize.js and message-header/test-header-toolbar.js with respect to the customization dialog.

We should combine this hackery so that it's not duplicated across the two tests.
Comment 1 Joachim Herb 2011-12-22 14:39:05 PST
So the customization dialog code should be moved to the shared-modules folder?
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2011-12-29 08:38:55 PST
Joachim:

Yes, that's what I had in mind.

-Mike
Comment 3 Joachim Herb 2012-01-01 05:00:21 PST
Created attachment 585190 [details] [diff] [review]
Move customization code into a shared module

This moves the code commonly used in test-tabmail-customize.js and test-header-toolbar.js into a shared module. It was tested in the following try server builds:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=5416e07474a9
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=e0ee142a2556
Comment 4 Joachim Herb 2012-01-01 05:18:46 PST
(In reply to Mike Conley (:mconley) from comment #2)
> Joachim:
> 
> Yes, that's what I had in mind.
> 
> -Mike

Mike: What is the reason that you use the following code for the drag'n'drop in test-tabmail-customize.js?
    let dt = synthesize_drag_start(ctw.window, button, ctw.window);
    assert_true(dt, "Drag target was undefined");

    synthesize_drag_over(mc.window, tabbar, dt);
    synthesize_drop(mc.window, tabbar, dt);

Using drag_n_drop_element defined in test-mouse-event-helpers.js this would become a oneliner:
    drag_n_drop_element(button, ctw.window, tabbar, mc.window, 0.5, 0.5, ctw.window);

and the second test:
    drag_n_drop_element(button, mc.window, tabbar, mc.window, 0.5, 0.5, mc.window);

Should I also add this change to the patch?
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-01-04 10:51:09 PST
Joachim:

Wow, I wasn't aware of drag_n_drop_element in test-mouse-event-helpers.js.  Whoops.

Yes, if the tests still pass, please go ahead and replace those lines with drag_n_drop_element.  There was no special reason for their use.

Thanks,

-Mike
Comment 6 Joachim Herb 2012-01-05 11:36:33 PST
Created attachment 586159 [details] [diff] [review]
cleanup of the patch, still contains sleep in customization dialog opening for MAC (or more precisely if toolbar.customization.usesheet is true)
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2012-01-09 14:44:31 PST
Comment on attachment 586159 [details] [diff] [review]
cleanup of the patch, still contains sleep in customization dialog opening for MAC (or more precisely if toolbar.customization.usesheet is true)

Review of attachment 586159 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! r- because I've suggested one architectural change.

I'd normally ask you to revert the whitespace changes and handle them in a separate patch, but since it's test code I don't mind it too much. Blame does work better if whitespace and formatting is tackled in a separate patch, though.

::: mail/test/mozmill/shared-modules/test-customization-helpers.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + *   Version: MPL 1.1/GPL 2.0/LGPL 2.1

We've switched to MPL 2.0! The new header's 10x simpler.

http://www.mozilla.org/MPL/headers/

@@ +79,5 @@
> + * @param {} aDialogId
> + *   the ID of the window containing the dialog to be opened
> + * @returns a controller for the customization dialog
> + */
> +function open_customization_dialog(aController, aOpenElementId, aDialogId)

So I think writing CustomizeHeaderToolbar etc over and over again is too unwieldy. Instead, here's what I propose:

1. Make the module export a constructor called (e.g.) CustomizeDialogHelper, which takes parameters (aToolbarId, aOpenElementId, aDialogId). While it would also make sense to include the controller here, the same ids are used with a variety of controllers, so I guess it wouldn't make as much sense.

2. Make the constructor return an object with these three functions which use those three variables. Something like:

function CustomizeDialogHelper(aToolbarId, aOpenElementId, aDialogId) {
  this._toolbarId = aToolbarId;
  this._openElementId = aOpenElementId;
  this._dialogId = aDialogId;
}

CustomizeDialogHelper.prototype = {
  open: function CustomizeDialogHelper_open(aController) {
    ... // use this._openElementId and this._dialogId as required
  },

  close: function ... { ... },

  restoreAndCheckDefault: function ... { ... },
};

3. Then, in callers, simply create objects in setupModule with new CustomizeDialogHelper(...), and call methods on them.

@@ +141,5 @@
> +  ctc.click(new elib.Elem(restoreButton));
> +  close_customization_dialog(ctc);
> +
> +  let hdrToolbar = aController.e(aToolbarId);
> +  let hdrBarDefaultSet = hdrToolbar.getAttribute("defaultset");

let's just call this |toolbar| and |defaultSet|.
Comment 9 Joachim Herb 2012-01-09 16:03:03 PST
Created attachment 587184 [details] [diff] [review]
Mozmill tests using the helper constructor

Ok. This this the new patch with the requested changes (removal of whitespaces still included).

A try server build using it is just building:
http://build.mozillamessaging.com/tinderboxpushlog?tree=ThunderbirdTry&rev=f04ab4fb0e22
Comment 10 Joachim Herb 2012-01-11 14:28:19 PST
For whatever reasons the try server build (http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=a98de84f389f) worked on Mac OS X but not on Mac OS X64 (http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1326275234.1326276104.2548.gz&fulltext=1).

As described in https://bugzilla.mozilla.org/show_bug.cgi?id=681376#c14 and following comments this might be explainable by the positioning of the customization dialog if it is hiding the corresponding toolbar.
Comment 11 Siddharth Agarwal [:sid0] (inactive) 2012-01-16 11:23:06 PST
So I take it this patch doesn't work on OS X x64 then? Or is it a random failure?
Comment 12 Joachim Herb 2012-01-16 14:07:51 PST
Created attachment 589001 [details] [diff] [review]
Another version of the patch which moves the dialog out of the way

This version of the patch makes sure, that the dialog is not in front of the toolbar. In the try server log the message of the dump does not appear, so perhaps this never happened:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=1db24c9e56f2
But the mozmill tests did work (on both Mac OS).

How should this bug proceed? My guess is that the sporadic problems are related to bug 717439.
Comment 13 Siddharth Agarwal [:sid0] (inactive) 2012-01-31 14:32:15 PST
So I've been looking at this failure over the past week or so. I don't have a fix yet though, since a proper one seems to involve digging into layout code (fixing bug 720993 and bug 721101), and I'm not sure it's worth the effort.

I'm wondering if we can skip the effort by moving the window to a suitable position for the purposes of the test.
Comment 14 Joachim Herb 2012-02-08 14:21:00 PST
Created attachment 595533 [details] [diff] [review]
New version without ewait which works on all OS

This patch worked for all OS:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=1356c0184c20
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=01429f388494

It uses wait_for_frame_load instead of the ewait("donebutton") and/or sleep. It looks like replacing the dialog is not necessary to get the tests working.
Comment 15 Joachim Herb 2012-02-21 02:20:48 PST
Another tryserver build including the latest patch of this bug:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=2e83c1e54744
The mozmill tests (related to this bug) worked on all OS.
Comment 16 Siddharth Agarwal [:sid0] (inactive) 2012-02-21 02:32:36 PST
Hi Joachim,

I'm kinda sorta in the middle of reviewing this patch, but I'm not sure I'll be able to take time out for it before the end of the week.
Comment 17 Joachim Herb 2012-03-20 13:33:36 PDT
(In reply to Siddharth Agarwal [:sid0] from comment #16)
> Hi Joachim,
> 
> I'm kinda sorta in the middle of reviewing this patch, but I'm not sure I'll
> be able to take time out for it before the end of the week.

Hi Siddharth,
any progress with the review?
Comment 18 Siddharth Agarwal [:sid0] (inactive) 2012-03-20 15:24:30 PDT
Comment on attachment 595533 [details] [diff] [review]
New version without ewait which works on all OS

No, sorry, Joachim, real life has taken its toll. I don't expect to have the time to do any Thunderbird reviewing until the beginning of June. Feel free to ask someone else to review it, and sorry once again for keeping this hanging around.
Comment 19 Joachim Herb 2012-03-21 15:11:17 PDT
Comment on attachment 595533 [details] [diff] [review]
New version without ewait which works on all OS

Blake,
could you review the patch?
Comment 20 Mike Conley (:mconley) - (Needinfo me!) 2012-04-05 12:31:34 PDT
Comment on attachment 595533 [details] [diff] [review]
New version without ewait which works on all OS

Stealing this review request.
Comment 21 Mike Conley (:mconley) - (Needinfo me!) 2012-04-05 13:25:19 PDT
Comment on attachment 595533 [details] [diff] [review]
New version without ewait which works on all OS

Review of attachment 595533 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Joachim,

Thanks for tackling this, and sorry for the long turn-around time on the review request!

This looks pretty good.  I found a few nits, see below.

With those fixed, r=me.

-Mike

::: mail/test/mozmill/message-header/test-header-toolbar.js
@@ +44,4 @@
>  
>  var RELATIVE_ROOT = '../shared-modules';
>  var MODULE_REQUIRES = ['folder-display-helpers', 'window-helpers',
> +                       'address-book-helpers', 'mouse-event-helpers', 'customization-helpers'];

This line is a bit long - could you put 'customization-helpers' on the next line?

::: mail/test/mozmill/shared-modules/test-customization-helpers.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var Ci = Components.interfaces;

I think we can get in trouble redefining these (Ci, Cc, Cu).  I'm pretty sure they come included "for free", so I *think* you can omit them. Let me know if I'm wrong.

@@ +66,5 @@
> +    if (Services.prefs.getBoolPref(USE_SHEET_PREF, true)) {
> +      ctc = wh.wait_for_frame_load(aController.e("customizeToolbarSheetIFrame"),
> +        "chrome://global/content/customizeToolbar.xul");
> +    }
> +    else {

nit: this one-liner else block doesn't need braces.

@@ +94,5 @@
> +   * @param {} aController
> +   *   the controller object of the window for which the customization
> +   *   dialog should be opened
> +   */
> +  restoreAndCheckDefault: function CustomizeDialogHelper_restoreAndCheckDefault(aController) {

I think this should be named restoreDefaultButtons.

@@ +97,5 @@
> +   */
> +  restoreAndCheckDefault: function CustomizeDialogHelper_restoreAndCheckDefault(aController) {
> +    let ctc = this.open(aController);
> +    let restoreButton = ctc.window.document.getElementById("main-box").
> +      querySelector("[oncommand*='overlayRestoreDefaultSet();']");

I think this should be broken up like:

let restoreButton = cwc.window
                       .document
                       .getElementById("main-box")
                       .querySelector("[oncommand*='overlayRestoreDefaultSet();']");

::: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js
@@ +137,5 @@
>    let tab1 = mc.tabmail.tabContainer.childNodes[1];
>    let tab3 = mc.tabmail.tabContainer.childNodes[3];
>  
> +  drag_n_drop_element(tab1, mc.window, tab3, mc.window, 0.75, 0.0,
> +    mc.tabmail.tabContainer);

nit: since we're inside parentheses at this point, I'd prefer this line were indented enough spaces such that the "m" in "mc" were directly beneath the "t" in "tab1".

@@ +219,5 @@
>    let tabB = mc2.tabmail.tabContainer.childNodes[0];
>    assert_true(tabB, "No movable Tab");
>  
> +  drag_n_drop_element(tabA, mc.window, tabB, mc2.window, 0.75, 0.0,
> +      mc.tabmail.tabContainer);

Same as above.

@@ +395,1 @@
>          "Tab Title does not match Menu item");

While you're here, please indent this enough spaces to put the first quote directly beneath the "t" in "tabTitles".

@@ +412,1 @@
>          "Tab Title does not match Menu item");

Same as above.
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-04-05 13:30:13 PDT
(In reply to Mike Conley (:mconley) from comment #21)
> Comment on attachment 595533 [details] [diff] [review]
> New version without ewait which works on all OS
> 
> Review of attachment 595533 [details] [diff] [review]:
> -----------------------------------------------------------------
@@ +66,5 @@
> +    if (Services.prefs.getBoolPref(USE_SHEET_PREF, true)) {
> +      ctc = wh.wait_for_frame_load(aController.e("customizeToolbarSheetIFrame"),
> +        "chrome://global/content/customizeToolbar.xul");
> +    }
> +    else {
>
> nit: this one-liner else block doesn't need braces.

Ignore this part - bwinton just pointed out that our coding standards state that one-liner else blocks that are part of if blocks with braces also need braces.

Today I learned.
Comment 23 Joachim Herb 2012-04-05 14:32:23 PDT
Created attachment 612687 [details] [diff] [review]
Changes requested by reviewer
Comment 24 Joachim Herb 2012-04-05 14:39:59 PDT
I cannot add the keyword "checkin-needed" so somebody else please add it.
Comment 25 Blake Winton (:bwinton) (:☕️) 2012-04-05 14:51:21 PDT
(In reply to Joachim Herb from comment #24)
> I cannot add the keyword "checkin-needed" so somebody else please add it.

That seems like a problem.  You should read http://www-archive.mozilla.org/quality/help/bugzilla-privilege-guide.html and ask for that to be fixed.  :)

(In the meantime, I've got this one for you.)

Thanks,
Blake.
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-04-05 15:23:41 PDT
Thanks for the patch! For future patches, please follow the directions below. It makes it easier to check patches in that way.
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

http://hg.mozilla.org/comm-central/rev/d80a66dcea14

Note You need to log in before you can comment on or make changes to this bug.