712322 - Combine awful customize window hackery in test-tabmail-customize.js and test-header-toolbar.js

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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]

So the customization dialog code should be moved to the shared-modules folder?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Joachim:

Yes, that's what I had in mind.

-Mike

Comment 3

[Joachim Herb]

Attachment: 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]

(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) (:⚙️)]

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]

Attachment: cleanup of the patch, still contains sleep in customization dialog opening for MAC (or more precisely if toolbar.customization.usesheet is true)

Comment 7

[Joachim Herb]

These are the corresponding try server builds:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=0812c861e7fe
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=5416e07474a9

Comment 8

[Rain [:rain1] (inactive)]

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]

Attachment: 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]

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

[Rain [:rain1] (inactive)]

So I take it this patch doesn't work on OS X x64 then? Or is it a random failure?

Comment 12

[Joachim Herb]

Attachment: 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

[Rain [:rain1] (inactive)]

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]

Attachment: 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]

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

[Rain [:rain1] (inactive)]

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]

(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

[Rain [:rain1] (inactive)]

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]

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) (:⚙️)]

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) (:⚙️)]

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) (:⚙️)]

(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]

Attachment: Changes requested by reviewer

Comment 24

[Joachim Herb]

I cannot add the keyword "checkin-needed" so somebody else please add it.

Comment 25

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

(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]]

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