879382 - Create a mozmill test for finding text in a page

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andrei Eftimie]

Cross referencing bug 831941

Write a mozmill test for Firefox Metro, to test the search feature.

Comment 1

[Andrei Eftimie]

Attachment: patch v1

Basic implementation of testing the UI for Find in Page in metro.

This is still a work in progress.

We still need to:
- use touch events // bug 880426
- reference UI elements abstracted in a lib // bug 880417
- some cleaning up

Comment 2

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

Comment on attachment 759389 [details] [diff] [review]
patch v1

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

Generally it looks fine. But a lot will have to be changed, once the ui map exists.

::: tests/metro/testFindInPage.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I would put this file into a subfolder which serves as category of the test. So we can group them similarly to the other testruns.

@@ +21,5 @@
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  // Open search UI
> +  win.keypress("F", { accelKey: true });

For the final test we need the localized key.

@@ +23,5 @@
> +
> +  // Open search UI
> +  win.keypress("F", { accelKey: true });
> +  var searchContainer = new elementslib.ID(controller.window.document, "content-navigator");
> +  searchContainer.waitForElement();

This is chrome, so it usually be present all the time. Why do we need a waitForElement() call here?

@@ +26,5 @@
> +  var searchContainer = new elementslib.ID(controller.window.document, "content-navigator");
> +  searchContainer.waitForElement();
> +
> +  // Get references to all the buttons
> +  var collector = new nodeCollector(searchContainer.getNode());

Finally once the ui map is up, this can be done with a double call to nodeCollector. First get the XBL widget and then set the new root.

@@ +39,5 @@
> +  // Check that the correct word has been highlighted
> +  var selectedText;
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;

I would wait for the offset here to change and check the text later via expect.equal().

@@ +51,5 @@
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    var secondOffset = selectedText.getRangeAt(0).startOffset;
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT &&
> +           secondOffset !== firstOffset;

Same here for the offset. Given that the text should be the same, lets do a sanity check after the loop.

Comment 3

[Andrei Eftimie]

Attachment: patch v2

Moved into a folder, added manifest files, changed checks according to previous comment.

We can't have a localized shortcut yet ( bug 882074 )

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Landed a patch on inbound that incorrectly references this bug:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/14503e9ec043

That patch should reference bug 879392

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/14503e9ec043

Comment 6

[Andrei Eftimie]

This has been incorrectly closed.
See comment 4 for reference.

Comment 7

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

(In reply to Andrei Eftimie from comment #6)
> This has been incorrectly closed.
> See comment 4 for reference.

When you make such a claim, you should at least check to cc the person. I have now marked the other bug as fixed given the changeset from comment 5.

Comment 8

[Ed Morley [:emorley]]

(In reply to Andrei Eftimie from comment #6)
> This has been incorrectly closed.
> See comment 4 for reference.

The bug marking tool uses the bug number from the commit message; sorry for the churn.

Comment 9

[Andrei Eftimie]

Attachment: 3.patch

We might still want to have this use the UI map.
But besides that it should be ready to go.

Comment 10

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

I would like to see at least one of the metro tests landed, so we can start with the metro testrun implementation.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 795478 [details] [diff] [review]
3.patch

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

Removing the flag until we get the UI library finished, as discussed in Ask an expert meeting.

Comment 12

[Andrei Eftimie]

Attachment: 4.patch

Updated paths.

Lets get this in to be able to run a metro functional testrun.
I was able to run a functional metro testrun with this patch.

It failed when trying to send the report, but that looks like a dashboard issue.

Comment 13

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

Comment on attachment 8337675 [details] [diff] [review]
4.patch

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

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +33,5 @@
> +
> +  // Get references to all the buttons
> +  var previous = new findElement.ID(controller.window.document, "findbar-previous-button");
> +  var next = new findElement.ID(controller.window.document, "findbar-next-button");
> +  var close = new findElement.ID(controller.window.document, "findbar-close-button");

All this should really end-up in the library! We don't want to define any of the elements in our test. If that is dependent on Andreeas work, we should get it updated with her patch.

@@ +47,5 @@
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;
> +  }, "Text has been selected");
> +
> +  // Remember position of the first selection
> +  var firstOffset = selectedText.getRangeAt(0).startOffset;

I would call it firstSelection.

@@ +73,5 @@
> +  // Close the find toolbar and check it has closed
> +  close.tap();
> +  assert.waitFor(function() {
> +    return !findBar.getNode().isActive;
> +  }, "Find in Page toolbar has been closed");

I don't see that we force this in a teardown method.

@@ +74,5 @@
> +  close.tap();
> +  assert.waitFor(function() {
> +    return !findBar.getNode().isActive;
> +  }, "Find in Page toolbar has been closed");
> +};

I miss a reference to a moztrap test. Lets not get this in without the meta info if one exist.

Comment 14

[Daniel Gherasim]

Attachment: 5.patch

I have updated the patch according the last review. Some other changes needed to be done as the metro shared module changed lately.

Regarding the moztrap test, I didn't find ATM any related test on moztrap that does the same steps.

Comment 15

[Andrei Eftimie]

Comment on attachment 8369464 [details] [diff] [review]
5.patch

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

We can still remove some unused stuff from this test, and make some small improvements.

Otherwise it looks good.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +var { assert } = require("../../../../lib/assertions");
> +var { nodeCollector } = require("../../../../lib/dom-utils");

This is not used anymore.
We can remove this line.

@@ +6,5 @@
> +
> +var { assert } = require("../../../../lib/assertions");
> +var { nodeCollector } = require("../../../../lib/dom-utils");
> +var toolbars = require("../../../lib/ui/toolbars");
> +var utils = require("../../../../firefox/lib/utils");

Same here. I don't see we're using utils anymore.

@@ +14,5 @@
> +const SEARCH_TEXT = "community";
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.win = new findElement.MozMillElement("Elem", aModule.controller.window);

This is in the library, so its not getting used here anymore.

@@ +15,5 @@
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.win = new findElement.MozMillElement("Elem", aModule.controller.window);
> +  aModule.comparator = Ci.nsIDOMRange.START_TO_START;

This is not used.

@@ +17,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.win = new findElement.MozMillElement("Elem", aModule.controller.window);
> +  aModule.comparator = Ci.nsIDOMRange.START_TO_START;
> +
> +  aModule.toolbar = new toolbars.ToolBar(aModule.controller);

nit: we should be consistent here and name it toolBar

@@ +37,5 @@
> +  // Open the findbar
> +  findBar.open("shortcut");
> +
> +  // Get references to all the buttons
> +  var previous = findBar.getElement({type: "previousButton"});

We could name this previousButton, it will make code reading better:
eg. previousButton.tap();

Also applies to the other buttons.

@@ +59,5 @@
> +  // Check that the next occurence has been highlighted
> +  next.tap();
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +   var secondSelection = selectedText.getRangeAt(0).startOffset;

nit: alignment

Comment 16

[Daniel Gherasim]

Attachment: 5.1.patch

Thanks a lot,

Updated the patch with the latest modifications.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8371325 [details] [diff] [review]
5.1.patch

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

Small updates, otherwise looks good.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +8,5 @@
> +var toolbars = require("../../../lib/ui/toolbars");
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_DATA = BASE_URL + "layout/mozilla_community.html";
> +const SEARCH_TEXT = "community";

Add a blank line between the [BASE_URL, TEST_DATA] block and SEARCH_TEXT.

@@ +23,5 @@
> +  catch(e) {
> +  }
> +}
> +
> +var testFindInPage = function() {

js doc missing.

Comment 18

[Daniel Gherasim]

Attachment: 5.2.patch

Thanks for the tips,
Updated the patch.

Comment 19

[Andrei Eftimie]

Comment on attachment 8371351 [details] [diff] [review]
5.2.patch

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

Looks good to me.

Adding the r? flag for Henrik since this is a new test.

Comment 20

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

Comment on attachment 8371351 [details] [diff] [review]
5.2.patch

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

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +11,5 @@
> +const TEST_DATA = BASE_URL + "layout/mozilla_community.html";
> +
> +const SEARCH_TEXT = "community";
> +
> +var setupModule = function(aModule) {

No var declarations please.

@@ +21,5 @@
> +  try {
> +    toolBar.findBar.close();
> +  }
> +  catch(e) {
> +  }

Why that? In a teardown module we never want to call methods which exercise ui paths. Otherwise we might need a force flag.

@@ +43,5 @@
> +  findBarTextbox.sendKeys(SEARCH_TEXT);
> +
> +  // Check that the correct word has been highlighted
> +  var selectedText;
> +  assert.waitFor(function() {

nit: blank between function and opening bracket

@@ +45,5 @@
> +  // Check that the correct word has been highlighted
> +  var selectedText;
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;

Why toLowerCase? Have you made sure that the 'Match case' setting is disabled?

@@ +57,5 @@
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    var secondSelection = selectedText.getRangeAt(0).startOffset;
> +    return secondSelection !== firstSelection;
> +  }, "Next occurence of the text has been selected");

I think we should add two methods to the library which let us search for the next and the previous selection. Otherwise this code will be repeated too often.

Comment 21

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #20)
@@ +45,5 @@
> @@ +45,5 @@
> > +  // Check that the correct word has been highlighted
> > +  var selectedText;
> > +  assert.waitFor(function() {
> > +    selectedText = controller.tabs.activeTabWindow.getSelection();
> > +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;
> 
> Why toLowerCase? Have you made sure that the 'Match case' setting is disabled?

I think this comes from the homologue test for regular firefox on which this is based:
http://hg.mozilla.org/qa/mozmill-tests/file/fc717c0f116b/firefox/tests/functional/testFindInPage/testFindInPage.js#l80

Comment 22

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

If we want to get better in our tests we should not simply copy and paste, but think about what is better.

Comment 23

[Daniel Gherasim]

Looks like the preference for case sensitive exists but doesn't work on metro.
I logged bug 970875 for this.

Comment 24

[Daniel Gherasim]

Attachment: 6.patch

Details about the patch:
1. Created 2 new methods in toolbars.js::FindBar ( findNext & findPrevious ).
3. Updated test to use them.
2. Updated FindBar.close() method to use a force flag.

Comment 25

[Andrei Eftimie]

Comment on attachment 8374093 [details] [diff] [review]
6.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +190,5 @@
>     *
>     * @param {string} aMethod
>     *        Specifies a method for closing the find bar
>     */
> +  close: function FindBar_close(aMethod, force) {

nit: please preserve the style guide rule we have to named arguments: aForce

@@ +199,5 @@
>  
> +    if(this.isOpen) {
> +      var findBar = this.getElement({type: "findbar"});
> +      if(force) {
> +        findBar.getNode().dismiss();

Move this code at the beginning of the function and instead of wrapping the rest in an else clause just return early. You can also use a single if clause to make both checks.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +12,5 @@
> +const TEST_DATA = BASE_URL + "layout/mozilla_community.html";
> +
> +const SEARCH_TEXT = "community";
> +
> +const FIND_CASESENSITIVE_PROP = "accessibility.typeaheadfind.casesensitive";

For preferences use PREF_[...] for a constant name.

@@ +46,5 @@
> +    position: null
> +  };
> +  assert.waitFor(function () {
> +    firstSelection.text = controller.tabs.activeTabWindow.getSelection();
> +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;

Add a todo comment to remove the lowercase transform once bug 968739 lands.

Comment 26

[Daniel Gherasim]

Attachment: 6.1.patch

(In reply to Andrei Eftimie from comment #25)
> Comment on attachment 8374093 [details] [diff] [review]
> 6.patch
> 
> Review of attachment 8374093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +46,5 @@
> > +    position: null
> > +  };
> > +  assert.waitFor(function () {
> > +    firstSelection.text = controller.tabs.activeTabWindow.getSelection();
> > +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;
> 
> Add a todo comment to remove the lowercase transform once bug 968739 lands.

I think the bug you are referring is bug 970875. Should we set the CASESENSITIVE preference to 1 in setup ? In this case we will need this TO DO once that is landed, otherwise the toLowerCase is needed here.

Comment 27

[Andrei Eftimie]

(In reply to daniel.gherasim from comment #26)
> I think the bug you are referring is bug 970875. Should we set the
> CASESENSITIVE preference to 1 in setup ? In this case we will need this TO
> DO once that is landed, otherwise the toLowerCase is needed here.
Yes, that bug. I probably miss-copied the id.

As I've said, leave toLowerCase for now, but before that we need to put a comment referencing the bug and add a short description. This comment should be inserted for every instance of toLowerCase where we would need to remove it later.

See the style guide for how to format this for a temporary workaround:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Commenting

Comment 28

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8378993 [details] [diff] [review]
6.1.patch

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

This looks good, but it misses the TODO with bug references Andrei mentioned.

Comment 29

[Daniel Gherasim]

Attachment: 6.2.patch

Added the TO DO.

Hope now it's ok!

Comment 30

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8380500 [details] [diff] [review]
6.2.patch

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

Yes, it's fine now. Asking review from Henrik as well.

Comment 31

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

The final review here should really have been gone to Dave during my absence. I will try to get to it soon.

Comment 32

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

Comment on attachment 8380500 [details] [diff] [review]
6.2.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +190,5 @@
>     *
>     * @param {string} aMethod
>     *        Specifies a method for closing the find bar
>     */
> +  close: function FindBar_close(aMethod, aForce) {

I don't see a jsdoc entry for aForce here. Also aMethod is optional.

@@ +193,5 @@
>     */
> +  close: function FindBar_close(aMethod, aForce) {
> +    var findBar = this.getElement({type: "findbar"});
> +
> +    if(!this.isOpen || aForce) {

Why do you want to dismiss the find bar when it is not visible?

@@ +203,4 @@
>      function onTransitionEnd() {
>        transitioned = true;
>      }
> +    var method = aMethod || "button";

Checks for function parameters should come first in a function. So please move this up again.

@@ +241,5 @@
> +    var selectedText = this._controller.tabs.activeTabWindow.getSelection();
> +    return {
> +      text: selectedText.toString(),
> +      position: selectedText.getRangeAt(0).startOffset
> +    }

I really would return what you get from getSelection() and not construct a custom dict.

@@ +256,5 @@
> +    var selectedText = this._controller.tabs.activeTabWindow.getSelection();
> +    return {
> +      text: selectedText.toString(),
> +      position: selectedText.getRangeAt(0).startOffset
> +    }

Same here.

::: metrofirefox/tests/functional/testFindInPage/manifest.ini
@@ +1,1 @@
> +[testFindInPage.js]

Can we call the sub folder name 'testFindBar' instead? Otherwise we have duplication which is not necessary.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +28,5 @@
> +  toolBar.findBar.close("", true);
> +}
> +
> +/**
> + * Test find in page functionality

Mind adding the bug number here?

@@ +38,5 @@
> +  toolBar.findBar.open();
> +
> +  // Do a search
> +  var findBarTextbox = toolBar.findBar.getElement({type: "textbox"});
> +  findBarTextbox.sendKeys(SEARCH_TEXT);

Don't we have findBar.search() yet? We should move that code over there.

@@ +44,5 @@
> +  // Check that the correct word has been highlighted
> +  var firstSelection = {
> +    text: "",
> +    position: null
> +  };

Why it has to be initialized that way? 'var firstSelection' should be totally enough.

@@ +46,5 @@
> +    text: "",
> +    position: null
> +  };
> +  assert.waitFor(function () {
> +    firstSelection.text = controller.tabs.activeTabWindow.getSelection();

I don't see a reason why we have to store that under '.text'. Why is that necessary?

As best I would like to see findBar.getSelection(), which would make it more obvious what we are doing here.

@@ +51,5 @@
> +
> +    // Bug 970875
> +    // The "accessibility.typeaheadfind.casesensitive" pref doesn't work
> +    // Remove toLowerCase once the preference is fixed
> +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;

I still don't think that we should call toLowerCase() here, and really search for the text with the wanted capitalization. Otherwise we do not find regressions for the default behavior of Firefox.

@@ +55,5 @@
> +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;
> +  }, "Text has been selected");
> +
> +  // Remember position of the first selection
> +  firstSelection.position = firstSelection.text.getRangeAt(0).startOffset;

Same as above. Unnecessary copy of the actual data.

Comment 33

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #32)

> @@ +51,5 @@
> > +
> > +    // Bug 970875
> > +    // The "accessibility.typeaheadfind.casesensitive" pref doesn't work
> > +    // Remove toLowerCase once the preference is fixed
> > +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;
> 
> I still don't think that we should call toLowerCase() here, and really
> search for the text with the wanted capitalization. Otherwise we do not find
> regressions for the default behavior of Firefox.
> 

The default behavior of firefox is to ignore capitalization so the _CASESENSITIVE_ preference is default false. So I think toLowerCase() is needed here unless we compare the selection with the expected word on the page, or set the preference to true initially, (but we found out it's not working properly in metro).

Comment 34

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

Oh, my bad. Looks like I was wrong here. Given that we should use the default behavior of Firefox, we should not turn on case sensitivity, but keep the toLowerCase() call. Not sure why I have suggested something else earlier, but at least we found a bug in Metro!

Comment 35

[Daniel Gherasim]

Attachment: 6.3.patch

(In reply to Henrik Skupin (:whimboo) from comment #32)
> > +    if(!this.isOpen || aForce) {
> 
> Why do you want to dismiss the find bar when it is not visible?
> 

Right. We should call dismiss and return always if the force flag is active, given that we'll use that in tearDownModules. Also, dismiss() is already checking if the findbar is visible and doesn't throw anything if it's not.

> Don't we have findBar.search() yet? We should move that code over there.

We do have findBar.type() => used that.

Comment 36

[Andrei Eftimie]

Comment on attachment 8387525 [details] [diff] [review]
6.3.patch

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

::: metrofirefox/lib/ui/toolbars.js
@@ +277,5 @@
>      var method = aMethod || "menu";
>      var transitioned = false;
>  
> +    if(this.isOpen) {
> +      assert.fail("Find bar already opened");

Do we need this?
We will fail further down waiting for the `transition` to be updated.

::: metrofirefox/tests/functional/testFindBar/testFindInPage.js
@@ +47,5 @@
> +  var firstSelection = selectedText.getRangeAt(0).startOffset;
> +
> +  // Check that the next occurence has been highlighted
> +  assert.waitFor(function () {
> +    selectedText = toolBar.findBar.findNext();

This should not be called inside the waitFor loop. We want to click on next only once.

@@ +57,5 @@
> +               "The correct text has been selected");
> +
> +  // Check that the first selection has been reselected
> +  assert.waitFor(function () {
> +    selectedText = toolBar.findBar.findPrevious();

Same here.

Comment 37

[Daniel Gherasim]

Attachment: 6.4.patch

Updated patch as requested.

Comment 38

[Andrei Eftimie]

Comment on attachment 8389130 [details] [diff] [review]
6.4.patch

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

Will need a small update since it's no longer apply cleanly.
With the mentioned changes this has r+ from me.

::: metrofirefox/lib/ui/toolbars.js
@@ +187,5 @@
>  
>    /**
>     * Close the find bar
>     *
> +   * @param {Object} aSpec

The whole aSpec object is optional

@@ +194,2 @@
>     *        Specifies a method for closing the find bar
> +   * @param {boolean} [aSpec.force]

Lets also mark the default value here.

Comment 39

[Daniel Gherasim]

Attachment: 7.patch

Updated patch and asked Henrik for a final check.

Comment 40

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

Comment on attachment 8390516 [details] [diff] [review]
7.patch

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

Looks fine. Just remove the comment and we can get this landed.

::: metrofirefox/tests/functional/testFindBar/testFindInPage.js
@@ +20,5 @@
> +  aModule.toolBar = new toolbars.ToolBar(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +  // Force closing findBar

nit: comment is not necessary.

Comment 41

[Daniel Gherasim]

Attachment: 8.patch

Final patch.

Comment 42

[Andrei Eftimie]

Comment on attachment 8391052 [details] [diff] [review]
8.patch

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/421d1073361e (default)