Refactor modal dialog to lower count of test failures

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
--
major
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lib][MozmillTestday])

Attachments

(6 attachments, 7 obsolete attachments)

9.50 KB, patch
geo
: review+
aaronmt
: feedback+
Details | Diff | Splinter Review
71.53 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
5.63 KB, patch
geo
: review+
Details | Diff | Splinter Review
12.77 KB, patch
geo
: review+
aaronmt
: feedback+
Details | Diff | Splinter Review
87.15 KB, patch
geo
: review+
aaronmt
: feedback+
Details | Diff | Splinter Review
89.49 KB, patch
geo
: review+
aaronmt
: feedback+
Details | Diff | Splinter Review
There are a couple of issues with the current implementation. All of them will be added to the dependency list.
Depends on: 560821

Comment 1

8 years ago
Thanks for compiling the list, Henrik.  Will add to my queue of things to look at.
Thanks Clint. If time permits I will also have a look again but I suspect it for this quarter.
Component: Mozmill → Mozmill Tests
QA Contact: mozmill → mozmilltests
Summary: [shard module] Refactor modal dialog to lower count of test failures → Refactor modal dialog to lower count of test failures
Whiteboard: [shared module]
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.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
Updating dependencies here to reflect that the refactoring should block remaining issues.
No longer depends on: 512475, 519714, 560821
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Created attachment 492025 [details] [diff] [review]
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.
Attachment #492025 - Flags: review?(ctalbert)
Created attachment 492026 [details] [diff] [review]
Patch v1.1

Missed an additional hg qrefresh.
Attachment #492025 - Attachment is obsolete: true
Attachment #492025 - Flags: review?(ctalbert)
Attachment #492026 - Flags: review?(ctalbert)
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.
Attachment #492026 - Flags: review?(ctalbert) → feedback?(fayearthur+bugs)
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
Attachment #492026 - Flags: feedback?(fayearthur+bugs) → feedback-
(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!
Created attachment 492843 [details] [diff] [review]
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.
Attachment #492026 - Attachment is obsolete: true
Attachment #492843 - Flags: feedback?(fayearthur+bugs)
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.
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.
Created attachment 493050 [details] [diff] [review]
Patch v2.1

Everything from above has been addressed. We are able to check for modal dialogs opened by content now.
Attachment #492843 - Attachment is obsolete: true
Attachment #493050 - Flags: feedback?(fayearthur+bugs)
Attachment #492843 - Flags: feedback?(fayearthur+bugs)
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?
Attachment #493050 - Flags: feedback?(fayearthur+bugs) → feedback+
Created attachment 493142 [details] [diff] [review]
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
Attachment #493050 - Attachment is obsolete: true
Attachment #493142 - Flags: feedback?(fayearthur+bugs)
Created attachment 493147 [details] [diff] [review]
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
Created attachment 493223 [details] [diff] [review]
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
Attachment #493142 - Attachment is obsolete: true
Attachment #493223 - Flags: feedback?(aaron.train)
Attachment #493142 - Flags: feedback?(fayearthur+bugs)
Created attachment 493224 [details] [diff] [review]
Patch modules/tests v1

Updated patch for modules/tests
Attachment #493147 - Attachment is obsolete: true
Attachment #493224 - Flags: feedback?(aaron.train)

Updated

8 years ago
Attachment #493223 - Flags: feedback?(aaron.train) → feedback+

Updated

8 years ago
Attachment #493224 - Flags: feedback?(aaron.train) → feedback+
Attachment #493223 - Flags: review?(gmealer)
Attachment #493224 - Flags: review?(gmealer)
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.
Attachment #493223 - Flags: review?(gmealer) → review+
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.
Attachment #493224 - Flags: review?(gmealer) → review+
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
(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.
Whiteboard: [shared module] → [shared module][MozmillTestday]
Blocks: 587819
Created attachment 493440 [details] [diff] [review]
Patch modules/tests v2

Small update to fix the modal dialog handling in the DOMWalker.
Attachment #493224 - Attachment is obsolete: true
Attachment #493440 - Flags: review?(gmealer)
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 on attachment 493440 [details] [diff] [review]
Patch modules/tests v2

r+ as given by comment 25.
Attachment #493440 - Flags: review?(gmealer) → review+
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.
Created attachment 493602 [details] [diff] [review]
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.
Attachment #493602 - Flags: review?(gmealer)
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.
Attachment #493602 - Flags: review?(gmealer) → review+
(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 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
Attachment #493602 - Attachment description: Patch modal dialog - follow-up → Patch modal dialog - follow-up [checked-in]
Created attachment 493771 [details] [diff] [review]
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.
Attachment #493771 - Flags: review?(gmealer)
Attachment #493771 - Flags: feedback?(aaron.train)
Created attachment 493774 [details] [diff] [review]
Modules/Tests - backport (1.9.2) [checked-in]

Backport of module and tests changes for mozilla1.9.2
Attachment #493774 - Flags: review?(gmealer)
Attachment #493774 - Flags: feedback?(aaron.train)
Created attachment 493775 [details] [diff] [review]
Modules/Tests - backport (1.9.1) [checked-in]

Backport of module and tests changes for mozilla1.9.1
Attachment #493775 - Flags: review?(gmealer)
Attachment #493775 - Flags: feedback?(aaron.train)

Updated

8 years ago
Attachment #493771 - Flags: feedback?(aaron.train) → feedback+

Updated

8 years ago
Attachment #493774 - Flags: feedback?(aaron.train) → feedback+

Updated

8 years ago
Attachment #493775 - Flags: feedback?(aaron.train) → feedback+
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+
Attachment #493771 - Flags: review?(gmealer) → review+
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.
Attachment #493774 - Flags: review?(gmealer) → review+
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 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.
Attachment #493775 - Flags: review?(gmealer) → review+
(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 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)
Attachment #493771 - Attachment description: Modal dialog - combined back-port to 1.9.2/1.9.1 → Modal dialog - combined back-port to 1.9.2/1.9.1 [checked-in]
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
Attachment #493774 - Attachment description: Modules/Tests - backport (1.9.2) → Modules/Tests - backport (1.9.2) [checked-in]
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
Attachment #493775 - Attachment description: Modules/Tests - backport (1.9.1) → Modules/Tests - backport (1.9.1) [checked-in]
Depends on: 615493
(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!
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 615656
Component: Mozmill Tests → Mozmill Shared Modules
Component: Mozmill Shared Modules → Mozmill Tests
Whiteboard: [shared module][MozmillTestday] → [lib]
Whiteboard: [lib] → [lib][MozmillTestday]
You need to log in before you can comment on or make changes to this bug.