560820 - Refactor modal dialog to lower count of test failures

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There are a couple of issues with the current implementation. All of them will be added to the dependency list.

Comment 1

[cmtalbert]

Thanks for compiling the list, Henrik.  Will add to my queue of things to look at.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Clint. If time permits I will also have a look again but I suspect it for this quarter.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Updating dependencies here to reflect that the refactoring should block remaining issues.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It will not be a complete refactoring but I will try to get most of the dependencies fixed. Being able to reliable working with modal dialogs is one of the highest priorities now.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This is the patch to fix a couple of issues in the modal dialog:

* When finding the window rely on existing code and simply check the opener
* Refactored constructor and start functions for re-use of the instance with different callbacks
* Wait function until the modal dialog has been closed (important for delayed dialogs)
* Don't leak a dozen of nsITimer instances
* Pass thrown exceptions to the callee (works even when called from a modal dialog)

To run this patch against our tests another patch is necessary, which I will try to upload tomorrow. It requires changes to the interface of the handleWindow and openPreferencesDialog function. So this is for code review.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1.1

Missed an additional hg qrefresh.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 492026 [details] [diff] [review]
Patch v1.1

Heather, it would be great to get the first feedback for my patch. You cannot run it right now without modified tests.

Comment 9

[Heather Arthur [:harth]]

Comment on attachment 492026 [details] [diff] [review]
Patch v1.1

> /**
>  * Observer object to find the modal dialog
>  */
> var mdObserver = {


Why is mdObserver an object literal instead of an instance object? this way you can't create more than one modalDialog() object because they will share the same mdObserver and it looks like that wouldn't work out too well given the instance vars it has.


>+  start : function modalDialog_start(aCallback) {
>+    this.observer.callback = aCallback;
> 
>-  var modalDialogTimer = Cc["@mozilla.org/timer;1"].
>-                         createInstance(Ci.nsITimer);
>+    this.observer.exception = null;
>+    this.observer.finished = false;
>+    this.observer.timer.init(this.observer, 100, Ci.nsITimer.TYPE_ONE_SHOT);
>+  },

pass the callback into the constructor for the mdObserver object, likewise for the other options.


>-}
> 
> // Export of classes
> exports.modalDialog = modalDialog;

That comment doesn't seem necessary

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> > var mdObserver = {
> 
> Why is mdObserver an object literal instead of an instance object? this way you
> can't create more than one modalDialog() object because they will share the
> same mdObserver and it looks like that wouldn't work out too well given the
> instance vars it has.

That's the way it has been used before. Nothing which I have changed. Everything works well and we haven't noticed any issue in the past. While doing the refactoring I played around to work with an instance of an observer object but that didn't work with the timer. Let me check again.

> >+  start : function modalDialog_start(aCallback) {
> >+    this.observer.callback = aCallback;
> > 
> >-  var modalDialogTimer = Cc["@mozilla.org/timer;1"].
> >-                         createInstance(Ci.nsITimer);
> >+    this.observer.exception = null;
> >+    this.observer.finished = false;
> >+    this.observer.timer.init(this.observer, 100, Ci.nsITimer.TYPE_ONE_SHOT);
> >+  },
> 
> pass the callback into the constructor for the mdObserver object, likewise for
> the other options.

If it is possible to use an instance of an observer in the modal dialog class, we can go for it. Otherwise it can't be done the proposed way.

> > // Export of classes
> > exports.modalDialog = modalDialog;
> 
> That comment doesn't seem necessary

We use such a comment across our shared modules to make it easier to understand for testers. Probably it should become a jsdoc comment, so we can add that stuff into our auto-generated documentation.

Thanks for your feedback!

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

This takes the last review comments into account. We now have an observer class for modal dialogs. That allows modalDialog instances to create their own observer instance. It looks much cleaner now.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As a note to the last patch, it will not work for windows spawned by content. The wrapped object has to be handled correctly. Also the waitForClosed method needs a timeout argument. Both will be addressed in the next patch.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Blake, for our Mozmill tests we have a shared module which handles modal dialogs. Right now I'm rewriting that module and trying to access the window, which is returned by getMostRecentWindow from the WindowMediator. Sadly this window object is wrapped in a XrayWrapper. Is there a way to get access to the real object, so I can use window.opener? Right now I can find a way. Would be great if you could give me a hint. Thanks.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2.1

Everything from above has been addressed. We are able to check for modal dialogs opened by content now.

Comment 15

[Heather Arthur [:harth]]

Comment on attachment 493050 [details] [diff] [review]
Patch v2.1

This looks good to me, though I haven't tried it on any tests or examples. Just a small note, nbd:

>+  get finished() {
>+    return this._finished;
>+  },

why not just always set this.finished instead of setting this._finished and having a getter?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP modal dialog v3

Actually, those patches should be WIP's at the moment.

This version fixes the following issues:
* Handle windows which do not have an opener (null)
* Don't pass in the controller but the window (should help us later after the refactoring)
* Removes the unnecessary properties as given by the last feedback

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP modules/tests v1

Updates modules and tests to the introduced API changes. It's a bit larger as expected. Aaron, would be great if you could do some test-runs on Windows and Linux with both patches applied.

So what's remaining for the modal dialog:
* adding a stop function which will remove the timer and observer
* adding tests

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch modal dialog v1

This patch now includes:

* A property to check if the dialog has been closed / finished processing
* A stop function if you don't want to use waitForDialog()
* Move numbers to consts

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch modules/tests v1

Updated patch for modules/tests

Comment 20

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 493223 [details] [diff] [review]
Patch modal dialog v1

Looks fine. 

The catch/store/rethrow of exceptions from observer to modal dialog class will have the unfortunate effect of losing the call stack info, I think, but I don't see a good way to avoid that. r+, pending Aaron's clean test runs.

Comment 21

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 493224 [details] [diff] [review]
Patch modules/tests v1

>-  } finally {
>-    // If a failure happened make sure we close the window if wanted
>-    if (dontClose != true & window != null) {
>-      window.close();
...
>+  } catch (ex) {
>+    if (window)
>+      window.close();
>+
>+    throw ex;
>   }

Kinda the same issue as I mentioned above. These look equivalent at a shallow level, but try..finally will preserve the call stack and try..catch..throw won't.  

Is there a particular reason you replaced finally with a catch? If not, you should go back to try..finally.

Other than that, looks fine to me. With that change (and Aaron's clean test runs), r=me.

Comment 22

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

BTW, above issue was in:

diff --git a/shared-modules/utils.js b/shared-modules/utils.js
--- a/shared-modules/utils.js
+++ b/shared-modules/utils.js

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> The catch/store/rethrow of exceptions from observer to modal dialog class will
> have the unfortunate effect of losing the call stack info, I think, but I don't
> see a good way to avoid that. r+, pending Aaron's clean test runs.

Why should we lose the callstack info? The complete exception object is stored and not only the message. So everything will be forwarded. Nothing is lost.

(In reply to comment #21)
> Is there a particular reason you replaced finally with a catch? If not, you
> should go back to try..finally.

If there is an error raised inside the callback handler we have to make sure to hardly close the open window. We don't have that information in the finally handler. And I don't see another way ATM. Other ideas?

Beside that I have to come up with an updated patch which also includes the DOMWalker, which has been landed today.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch modules/tests v2

Small update to fix the modal dialog handling in the DOMWalker.

Comment 25

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Talked to you quickly offline and you explained some of the flow to me more closely. I see now that my try..finally concern was bogus, and that you only wanted to close the window on error.

The DOMWalker stuff looks fine too.

r+ the whole thing.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 493440 [details] [diff] [review]
Patch modules/tests v2

r+ as given by comment 25.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Patches have been landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/68728dcb82d4
http://hg.mozilla.org/qa/mozmill-tests/rev/5d12d0119684

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

On 1.9.2 and 1.9.1 I got some problems with XPCNativeWrapper.unwrap, which is surprisingly not defined, even MDN ensures that:

https://developer.mozilla.org/en/XPCNativeWrapper#Unwrapping_an_object

I will have to check what's the best way for backports.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch modal dialog - follow-up [checked-in]

As mentioned in the last comment, there was a problem with unwrapping nodes. The reason for that was a broken documentation on MDN. A feature which had been checked-in got backed-out, but the docs haven't been updated. Given that I have updated the code to make sure, that we are using the unwrap method if it is available in XPCNativeWrapper. Otherwise the less secure wrappedJSObject method is called, which is the only way to unwrap an object in such a case.

Further while updating the tests for 1.9.2 I have noticed that we fail with the modal dialog, if another window with no opener is already open. That means we really have to check first, if the window under test is really a modal one. A patch for that is also included in this patch.

For both cases no tests have been updated.

Comment 30

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 493602 [details] [diff] [review]
Patch modal dialog - follow-up [checked-in]

>+function unwrapNode(aNode) {
>+  // If aNode doesn't exist do nothing
>+  var node = aNode;

Comment doesn't seem to match the code? Perhaps you meant to do an if !aNode return?

Other than that, looks fine. Clarify or correct, and r=me.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #30)
> >+function unwrapNode(aNode) {
> >+  // If aNode doesn't exist do nothing
> >+  var node = aNode;
> 
> Comment doesn't seem to match the code? Perhaps you meant to do an if !aNode
> return?

Ah, that comment was from former code. Simply forgot to remove it. I will take care of.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 493602 [details] [diff] [review]
Patch modal dialog - follow-up [checked-in]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/dc48e071eb06

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Modal dialog - combined back-port to 1.9.2/1.9.1 [checked-in]

Combined back-port of the module changes to both older branches.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Modules/Tests - backport (1.9.2) [checked-in]

Backport of module and tests changes for mozilla1.9.2

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Modules/Tests - backport (1.9.1) [checked-in]

Backport of module and tests changes for mozilla1.9.1

Comment 36

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 493771 [details] [diff] [review]
Modal dialog - combined back-port to 1.9.2/1.9.1 [checked-in]

Looks good to me, r+

Comment 37

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 493774 [details] [diff] [review]
Modules/Tests - backport (1.9.2) [checked-in]

Realizing these issues may have existed in the trunk version and I missed them (yay 90k code reviews), here you go:

>--- a/shared-modules/prefs.js
>+++ b/shared-modules/prefs.js
...
>-function openPreferencesDialog(callback, launcher) {
>-  if(!callback)
>+function openPreferencesDialog(controller, callback, launcher) {
>+  if(!controller)
>+    throw new Error("No controller given for Preferences Dialog");
>+  if(typeof callback != "function")
>     throw new Error("No callback given for Preferences Dialog");

We should put a space between if and (). Keeps it from looking like a function when you scan the code quickly, and I think standards mention it.

>   }
> }
> 
> // Export of variables
> exports.preferences = preferences;
> 
> // Export of functions
>diff --git a/shared-modules/private-browsing.js b/shared-modules/private-browsing.js
>--- a/shared-modules/private-browsing.js
>+++ b/shared-modules/private-browsing.js
>@@ -54,17 +54,18 @@ const gTimeout = 5000;
> /**
>  * Create a new privateBrowsing instance.
>  *
>  * @class This class adds support for the Private Browsing mode
>  * @param {MozMillController} controller
>  *        MozMillController to use for the modal entry dialog
>  */
> function privateBrowsing(controller) {
>-    this._controller = controller;
>+  this._controller = controller;
>+  this._handler = null;
> 
>   /**
>    * Menu item in the main menu to enter/leave Private Browsing mode
>    * @private
>    */
>   this._pbMenuItem = new elementslib.Elem(this._controller.menus['tools-menu'].privateBrowsingItem);
>   this._pbTransitionItem = new elementslib.ID(this._controller.window.document, "Tools:PrivateBrowsing");
> 
>@@ -75,20 +76,24 @@ function privateBrowsing(controller) {
>   });
> }
> 
> /**
>  * Prototype definition of the privateBrowsing class
>  */
> privateBrowsing.prototype = {
>   /**
>-   * Callback function for the modal enter dialog
>-   * @private
>+   * Returns the controller of the current window
>+   *
>+   * @returns Mozmill Controller
>+   * @type {MozMillController}
>    */
>-  _handler: null,
>+  get controller() {
>+    return this._controller;
>+  },
> 
>   /**
>    * Checks the state of the Private Browsing mode
>    *
>    * @returns Enabled state
>    * @type {boolean}
>    */
>   get enabled() {
>@@ -163,37 +168,37 @@ privateBrowsing.prototype = {
>   },
> 
>   /**
>    * Start the Private Browsing mode
>    *
>    * @param {boolean} useShortcut
>    *        Use the keyboard shortcut if true otherwise the menu entry is used
>    */
>-  start: function privateBrowsing_start(useShortcut)
>-  {
>+  start: function privateBrowsing_start(useShortcut) {
>+    var dialog = null;
>+
>     if (this.enabled)
>       return;
> 
>     if (this.showPrompt) {
>-      // Check if handler is set to prevent a hang when the modal dialog is opened
>-      if (!this._handler)
>-        throw new Error("Private Browsing mode not enabled due to missing callback handler");
>-
>-      dialog = new modalDialog.modalDialog(this._handler);
>-      dialog.start();
>+      dialog = new modalDialog.modalDialog(this._controller.window);
>+      dialog.start(this._handler);
>     }
> 
>     if (useShortcut) {
>       var cmdKey = utils.getEntity(this.getDtds(), "privateBrowsingCmd.commandkey");
>       this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
>     } else {
>       this._controller.click(this._pbMenuItem);
>     }
> 
>+    if (dialog) {
>+      dialog.waitForDialog();
>+    }
>     this.waitForTransistionComplete(true);
>   },
> 
>   /**
>    * Stop the Private Browsing mode
>    *
>    * @param {boolean} useShortcut
>    *        Use the keyboard shortcut if true otherwise the menu entry is used
>diff --git a/shared-modules/search.js b/shared-modules/search.js
>--- a/shared-modules/search.js
>+++ b/shared-modules/search.js
>@@ -207,32 +207,26 @@ engineManager.prototype = {
>    *
>    * @param {string} name
>    *        Name of the engine to remove
>    * @param {function} handler
>    *        Callback function for Engine Manager
>    */
>   editKeyword : function engineManager_editKeyword(name, handler)
>   {
>-    if (!handler)
>-      throw new Error(arguments.callee.name + ": No callback handler specified.");
>-
>     // Select the search engine
>     this.selectedEngine = name;
> 
>     // Setup the modal dialog handler
>-    md = new modalDialog.modalDialog(handler);
>-    md.start(200);
>+    md = new modalDialog.modalDialog(this._controller.window);
>+    md.start(handler);
> 
>     var button = this.getElement({type: "engine_button", subtype: "edit"});
>     this._controller.click(button);
>-
>-    // XXX: We have to wait a bit more, so the modal dialog handler can kick in. Otherwise
>-    // we continue executing remaining tests too early.
>-    this._controller.sleep(400);
>+    md.waitForDialog();
>   },
> 
>   /**
>    * Gets all the needed external DTD urls as an array
>    *
>    * @returns Array of external DTD urls
>    * @type [string]
>    */
>@@ -244,17 +238,17 @@ engineManager.prototype = {
>   /**
>    * Retrieve an UI element based on the given spec
>    *
>    * @param {object} spec
>    *        Information of the UI element which should be retrieved
>    *        type: General type information
>    *        subtype: Specific element or property
>    *        value: Value of the element or property
>-   * @returns Element which has been created  
>+   * @returns Element which has been created
>    * @type {ElemBase}
>    */
>   getElement : function engineManager_getElement(spec) {
>     var elem = null;
> 
>     switch(spec.type) {
>       /**
>        * subtype: subtype to match
>@@ -591,17 +585,17 @@ searchBar.prototype = {
>   /**
>    * Retrieve an UI element based on the given spec
>    *
>    * @param {object} spec
>    *        Information of the UI element which should be retrieved
>    *        type: General type information
>    *        subtype: Specific element or property
>    *        value: Value of the element or property
>-   * @returns Element which has been created  
>+   * @returns Element which has been created
>    * @type {ElemBase}
>    */
>   getElement : function searchBar_getElement(spec) {
>     var elem = null;
> 
>     switch(spec.type) {
>       /**
>        * subtype: subtype to match
>@@ -715,37 +709,31 @@ searchBar.prototype = {
>   /**
>    * Open the Engine Manager
>    *
>    * @param {function} handler
>    *        Callback function for Engine Manager
>    */
>   openEngineManager : function searchBar_openEngineManager(handler)
>   {
>-    if (!handler)
>-      throw new Error(arguments.callee.name + ": No callback handler specified.");
>-
>     this.enginesDropDownOpen = true;
>     var engineManager = this.getElement({type: "engine_manager"});
> 
>     // Setup the modal dialog handler
>-    md = new modalDialog.modalDialog(handler);
>-    md.start();
>+    md = new modalDialog.modalDialog(this._controller.window);
>+    md.start(handler);
> 
>     // XXX: Bug 555347 - Process any outstanding events before clicking the entry
>     this._controller.sleep(0);
>     this._controller.click(engineManager);
>+    md.waitForDialog();
> 
>-    // Wait until the drop down has been closed
>-    this._controller.waitForEval("subject.search.enginesDropDownOpen == false", gTimeout, 100,
>-                                 {search: this});
>-
>-    // XXX: We have to wait a bit more, so the modal dialog handler can kick in. Otherwise
>-    // we continue executing remaining tests too early.
>-    this._controller.sleep(200);
>+    this._controller.assert(function () {
>+      return this.enginesDropDownOpen == false;
>+    }, "The search engine drop down menu has been closed", this);
>   },
> 
>   /**
>    * Remove the search engine with the given name (API call)
>    *
>    * @param {string} name
>    *        Name of the search engine to remove
>    */
>diff --git a/shared-modules/utils.js b/shared-modules/utils.js
>--- a/shared-modules/utils.js
>+++ b/shared-modules/utils.js
...
> function handleWindow(type, text, callback, dontClose) {
...
>+    dontClose = dontClose || false;

foo OR false always equals foo, this line is a no-op. If you really want to assign false as a default, you need to go back to the if === undefined syntax...

>+    if (dontClose == false & window != null) {

...but pretty sure you don't need to do it at all, since I believe undefined == false.

Rest looks fine. I'll let you decide whether to make the changes, since I know keeping the code the same across branches is a value too and they're minor style issues.

Comment 38

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

wow, that was a lot more quoted than I meant to. Reposting it here, cut down.

Realizing these issues may have existed in the trunk version and I missed them (yay 90k code reviews), here you go:

>--- a/shared-modules/prefs.js
>+++ b/shared-modules/prefs.js
...
>-function openPreferencesDialog(callback, launcher) {
>-  if(!callback)
>+function openPreferencesDialog(controller, callback, launcher) {
>+  if(!controller)
>+    throw new Error("No controller given for Preferences Dialog");
>+  if(typeof callback != "function")
>     throw new Error("No callback given for Preferences Dialog");

We should put a space between if and (). Keeps it from looking like a function when you scan the code quickly, and I think standards mention it.

>diff --git a/shared-modules/utils.js b/shared-modules/utils.js
>--- a/shared-modules/utils.js
>+++ b/shared-modules/utils.js
...
> function handleWindow(type, text, callback, dontClose) {
...
>+    dontClose = dontClose || false;

foo OR false always equals foo, this line is a no-op. If you really want to assign false as a default, you need to go back to the if === undefined syntax...

>+    if (dontClose == false & window != null) {

...but pretty sure you don't need to do it at all, since I believe undefined == false.

Rest looks fine. I'll let you decide whether to make the changes, since I know keeping the code the same across branches is a value too and they're minor style issues.

Comment 39

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 493775 [details] [diff] [review]
Modules/Tests - backport (1.9.1) [checked-in]

Same issues/comments as above, if() and foo = foo || false. r+, your choice whether to fix or keep code consistency w/ trunk.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #39)
> Same issues/comments as above, if() and foo = foo || false. r+, your choice
> whether to fix or keep code consistency w/ trunk.

Thanks for spotting those issues. Lets do that with a follow-up. I don't want you to check the large interdiffs again, and they don't cause any harm when correctly called.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 493771 [details] [diff] [review]
Modal dialog - combined back-port to 1.9.2/1.9.1 [checked-in]

Landed on older branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/5205e39591c7 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/60e6453b5dc7 (1.9.1)

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 493774 [details] [diff] [review]
Modules/Tests - backport (1.9.2) [checked-in]

Landed on 1.9.1 as:
http://hg.mozilla.org/qa/mozmill-tests/rev/78d46d288f04

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 493775 [details] [diff] [review]
Modules/Tests - backport (1.9.1) [checked-in]

Landed on 1.9.1 as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1a38af8d6437

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #40)
> Thanks for spotting those issues. Lets do that with a follow-up. I don't want
> you to check the large interdiffs again, and they don't cause any harm when
> correctly called.

Well, lets do it on a separate bug. Filed bug 615493 for it.

Marking the refactoring as done. Yay!