Last Comment Bug 1076741 - Refactor toolbars.js with a NavBar class that contains the locationBar and the searchBar
: Refactor toolbars.js with a NavBar class that contains the locationBar and th...
Status: RESOLVED FIXED
[lang=js]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Barbara Miller (:galgeek)
: Henrik Skupin (:whimboo) [away 02/18 - 02/27]
:
Mentors: Andrei Eftimie
https://developer.mozilla.org/en-US/d...
Depends on: 1067411 1069325
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-02 02:23 PDT by Andrei Eftimie
Modified: 2014-11-25 06:43 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed


Attachments
bug-1076741-navBar-skeleton.txt (1.94 KB, patch)
2014-10-06 10:52 PDT, Barbara Miller (:galgeek)
andrei: review-
Details | Diff | Splinter Review
git diff, adding updates per comment 8 (8.98 KB, patch)
2014-10-14 17:28 PDT, Barbara Miller (:galgeek)
andrei: feedback-
Details | Diff | Splinter Review
NOT YET WORKING updates per comment 12 (17.49 KB, patch)
2014-10-15 17:24 PDT, Barbara Miller (:galgeek)
no flags Details | Diff | Splinter Review
bug1076741 toolbars.js edits and all testAwesomeBar test edits, plus a few others (53.65 KB, patch)
2014-10-16 17:26 PDT, Barbara Miller (:galgeek)
andrei: feedback+
Details | Diff | Splinter Review
toolbars.js updates and all test updates (115.35 KB, patch)
2014-10-19 23:36 PDT, Barbara Miller (:galgeek)
no flags Details | Diff | Splinter Review
toolbars.js and test updates, rebased on 141026 master (115.86 KB, patch)
2014-10-26 14:54 PDT, Barbara Miller (:galgeek)
andrei: feedback+
Details | Diff | Splinter Review
bug1076741 patch rewriting toolbars.js constructors to use BrowserWindow (150.34 KB, patch)
2014-10-29 23:28 PDT, Barbara Miller (:galgeek)
andrei: review-
Details | Diff | Splinter Review
bug1076741-141031.patch (151.46 KB, patch)
2014-10-31 18:10 PDT, Barbara Miller (:galgeek)
no flags Details | Diff | Splinter Review
patch restoring master’s short object names (110.04 KB, patch)
2014-11-02 12:24 PST, Barbara Miller (:galgeek)
no flags Details | Diff | Splinter Review
patch restoring master’s short object names, and excluding git markup (110.03 KB, patch)
2014-11-02 12:39 PST, Barbara Miller (:galgeek)
andrei: review-
Details | Diff | Splinter Review
a still smaller patch, restoring short, though not alway shortest, object names (100.05 KB, patch)
2014-11-04 16:02 PST, Barbara Miller (:galgeek)
andrei: review+
hskupin: review-
Details | Diff | Splinter Review
updates per comments 31-37 (80.61 KB, patch)
2014-11-07 01:19 PST, Barbara Miller (:galgeek)
no flags Details | Diff | Splinter Review
toolbars.js, browser.js, and related test updates (78.03 KB, patch)
2014-11-10 22:30 PST, Barbara Miller (:galgeek)
andrei: review+
Details | Diff | Splinter Review
a complete patch? now with updated test per comment 40 (79.23 KB, patch)
2014-11-13 13:32 PST, Barbara Miller (:galgeek)
andrei: review+
hskupin: review-
Details | Diff | Splinter Review
toolbars.js updates per comment 42 (80.13 KB, patch)
2014-11-20 11:26 PST, Barbara Miller (:galgeek)
hskupin: review+
Details | Diff | Splinter Review
_browserWindow to browserWindow in toolbars.js (80.12 KB, patch)
2014-11-21 10:30 PST, Barbara Miller (:galgeek)
hskupin: review+
Details | Diff | Splinter Review

Description User image Andrei Eftimie 2014-10-02 02:23:30 PDT
+++ This bug was initially created as a clone of Bug #1069325 +++

As per bug 1069325 comment 8:
* Create a new ui class (similar to LocationBar) called NavBar
* This created class should contain instances of the location bar and search bar class
* For the combined bookmarks button a separate class might be helpful
** It should contain the getElements() method, which can return the nodes for both buttons
** It contains the handling for the editBookmarksPanel, maybe even some helper methods e.g. 'starred', 'bookmark', 'edit', 'remove'.
* The Australis menu button would be optional here, and we should do it in another bug
Comment 1 User image Andrei Eftimie 2014-10-02 02:24:23 PDT
Barbara, if you want to tackle this issue once bug 1069325 gets landed, I'd be happy to help.
Comment 2 User image Barbara Miller (:galgeek) 2014-10-02 10:14:40 PDT
(In reply to Andrei Eftimie from comment #1)
> Barbara, if you want to tackle this issue once bug 1069325 gets landed, I'd
> be happy to help.

Thank you! I'm happy to give it a try.

I'll upload an updated patch to 1069325 shortly.
Comment 3 User image Barbara Miller (:galgeek) 2014-10-03 17:00:30 PDT
I’ve started looking at this and will do more beginning Monday morning, Pacific Daylight Time.

I have the skeleton of a new navBar class, but I'm not sure what wants to live there beyond the existing locationBar and searchBar objects.
Comment 4 User image Barbara Miller (:galgeek) 2014-10-06 10:52:55 PDT
Created attachment 8500560 [details] [diff] [review]
bug-1076741-navBar-skeleton.txt

Uploading the skeleton for a new navBar class. Am I on the right track?

What is good background on this refactoring? I feel like I came across something earlier that I failed to bookmark and haven't yet managed to find again.
Comment 5 User image Barbara Miller (:galgeek) 2014-10-13 13:37:17 PDT
I'm ready to work more on this and wondering what's next.
Comment 6 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-10-13 13:57:18 PDT
Andreea and Andrei, this patch is lingering around for a week. Can one of you please make sure to review it soon? This is part of Ascend work and I would like to see those reviews done. Thanks.
Comment 7 User image Andrei Eftimie 2014-10-14 00:10:40 PDT
It will. Sorry for the delay.
A bit overwhelmed in review requests in the last 2 weeks. (>10)
Comment 8 User image Andrei Eftimie 2014-10-14 01:47:11 PDT
Comment on attachment 8500560 [details] [diff] [review]
bug-1076741-navBar-skeleton.txt

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

The start is good, but this is only a start.

We also need the getElement and getElements methods (similar as they are declared in the locationBar)
And several elements from the locationBar should be moved out into the new navBar class (basically all elements which are not directly related to the / outside of the locationBar):
bookmarksMenuButton, starButton, feedButton?

I think a custom method to take a button (any button) based on subtypes should come in handy.
Since all seem to have the same context menu entries) some methods similar to the locationBar context menu items might also be handy.

Some methods will also need to be taken out of the locationbar and move to the navBar, mainly:
bookmarkWithAnimation

We should also save the navBar root element on initialisation under this._root (id: "nav-bar")
And I think the method toggleBookmarksToolbar can live underneath the new class as well

::: firefox/lib/toolbars.js
@@ +841,5 @@
> +/**
> + *  * Nav Bar class
> + *   */
> +navBar.prototype = {
> +	  /**

Keep the indentation at 2 spaces please.

@@ +846,5 @@
> +	   *    * Get the controller of the associated browser window
> +	   *       *
> +	   *          * @returns Controller of the browser window
> +	   *             * @type MozMillController
> +	   *                */

Something's wrong with these comments, please keep the same style as the others. 

Comment blocks should be formatted:
> /**
>  * text
>  */

See the style guide: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Mozmill_Style_Guide#Commenting

@@ +959,4 @@
>  }
>  
>  // Export of classes
> +exports.navBar = navBar;

Please sort alphabetically
Comment 9 User image Andrei Eftimie 2014-10-14 01:48:39 PDT
Henrik, do you want to keep references to all methods, or should we only export navBar and get them all like: navBar.locationBar?

> // Export of classes
> exports.navBar = navBar;
> exports.locationBar = locationBar;
> exports.editBookmarksPanel = editBookmarksPanel;
> exports.autoCompleteResults = autoCompleteResults;
Comment 10 User image Barbara Miller (:galgeek) 2014-10-14 17:28:04 PDT
Created attachment 8505144 [details] [diff] [review]
git diff, adding updates per comment 8

Thanks for your feedback, Andrei!

I could use more tips regarding the button method. I think this patch addresses your other comments.
Comment 11 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-10-14 20:14:10 PDT
(In reply to Andrei Eftimie from comment #9)
> Henrik, do you want to keep references to all methods, or should we only
> export navBar and get them all like: navBar.locationBar?

navBar should be an object (class) and contain those references for easy access. So exporting only the navbar sounds right to me.
Comment 12 User image Andrei Eftimie 2014-10-15 06:32:11 PDT
Comment on attachment 8505144 [details] [diff] [review]
git diff, adding updates per comment 8

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

Thanks Barbara for the update.

We'll have to do some changes still, and they will also require to change all tests that use these methods since we're changing how they can be called.

::: firefox/lib/toolbars.js
@@ +835,5 @@
> +   *    * Get the controller of the associated browser window
> +   *       *
> +   *          * @returns Controller of the browser window
> +   *             * @type MozMillController
> +   *                */

Please format the comment like:
> /**
>  * Get the controller of the associated browser window
>  * 
>  * @returns Controller of the browser window
>  * @type MozMillController
>  */

There are a few instances like this.

@@ +902,5 @@
> +       * subtype: subtype to match
> +       * value: value to match
> +       */
> +      case "bookmarksMenuButton":
> +        return [new elementslib.ID(root, "bookmarks-menu-button")];

elementslib is deprecated right now, we should make use of findElement. Their API's are compatible.
> new elementslib.ID(root, "bookmarks-menu-button")

should be translated into:
> findElement.ID(root, "bookmarks-menu-button")
(notice the lack of the new keyword, it is not needed with the new implementation)

Please do update all instances in the code you are changing.

@@ +906,5 @@
> +        return [new elementslib.ID(root, "bookmarks-menu-button")];
> +      case "feedButton":
> +        return [new elementslib.ID(root, "feed-button")];
> +      case "starButton":
> +        if (utils.australis.isAustralis()) {

Australis is now enabled on all Firefox branches we support, so we can also remove this check and the code required for non-australis.

@@ +956,5 @@
> +    finally {
> +      mutationObserver.disconnect();
> +    }
> +  },
> +  

nit: there's an empty space here

@@ +961,5 @@
> +  /**
> +   * Toogle bookmarks toolbar
> +   *
> +   * @param {MozmillController} aController
> +   *        MozMillController of the window to operate on

When you remove the argument, make sure to also keep the jsdoc comments in sync.

@@ +965,5 @@
> +   *        MozMillController of the window to operate on
> +   * @param {Boolean} aState
> +   *        Expected state of the BookmarksToolbar
> +   */
> +  toggleBookmarksToolbar : function navBar_toggleBookmarksToolbar(aController, aState) {

Since this is part of the navBar class now, we don't need to pass in the controller as an argument, this method should only receive aState, and use `this.controller` for the controller.

In tests, you'll pass in the controller when instantiating navBar.

@@ +966,5 @@
> +   * @param {Boolean} aState
> +   *        Expected state of the BookmarksToolbar
> +   */
> +  toggleBookmarksToolbar : function navBar_toggleBookmarksToolbar(aController, aState) {
> +    var navbar = new elementslib.ID(aController.window.document, "nav-bar");

We have already access to this via this._root now, no need to requery for it.

@@ +968,5 @@
> +   */
> +  toggleBookmarksToolbar : function navBar_toggleBookmarksToolbar(aController, aState) {
> +    var navbar = new elementslib.ID(aController.window.document, "nav-bar");
> +
> +    aController.rightClick(navbar, navbar.getNode().boxObject.width / 2, 2);

`controller.method(element)` is also deprecated, please also update these to `element.method()`
> navbar.rightClick(navbar.getNode().boxObject.width / 2, 2)

@@ +971,5 @@
> +
> +    aController.rightClick(navbar, navbar.getNode().boxObject.width / 2, 2);
> +
> +    var toggle = new elementslib.ID(aController.window.document,
> +                                    "toggle_PersonalToolbar");

For elements used in this method, please move this to the `getElements` method, and retrieve them with a call to this.getElements()

@@ +973,5 @@
> +
> +    var toggle = new elementslib.ID(aController.window.document,
> +                                    "toggle_PersonalToolbar");
> +    aController.mouseDown(toggle);
> +    aController.mouseUp(toggle);

Same here: toggle.mouseDown() and toggle.mouseUp() should work.

@@ +978,5 @@
> +
> +    // Check that the Bookmark toolbar is in the correct state
> +    var state = !!aState;
> +    var bookmakrsToolbar = new elementslib.ID(aController.window.document,
> +                                              "PersonalToolbar");

Same here. Move to getElements.

@@ +1048,4 @@
>  exports.autoCompleteResults = autoCompleteResults;
> +exports.editBookmarksPanel = editBookmarksPanel;
> +exports.locationBar = locationBar;
> +exports.navBar = navBar;

Henrik's reply came a bit late in regards to this. We'll only export navBar, and we'll have to update all tests that make use of these methods.

Instead of using `locationBar.method()` we'll use `navBar.locationBar.method()`
We'll have to update all tests that make use of these methods.

@@ +1052,3 @@
>  
>  // Export of functions
> +exports.toggleBookmarksToolbar = navBar.toggleBookmarksToolbar;

We won't export this directly. Clients will have to instantiate navBar and call navBar.toggleBookmarksToolbar()
You will have to update the tests that make use of it, fortunately there's only one that makes use of it: firefox/tests/endurance/testBookmarks_OpenAllInTabs/
Comment 13 User image Barbara Miller (:galgeek) 2014-10-15 17:24:49 PDT
Created attachment 8505863 [details] [diff] [review]
NOT YET WORKING updates per comment 12

I will work on this more tomorrow, but in the meantime:

tests failing in setupModule...

$ mozmill -t "firefox/lib/tests/testIdentity.js" -b "/Applications/FirefoxNightly.app"
TEST-START | /Users/bara/Dev/qa-mozmill-tests/firefox/lib/tests/testIdentity.js | setupModule
ERROR | Test Failure | {
  "exception": {
    "message": "Unknown element type - undefined", 
    "lineNumber": 881, 
    "name": "Error", 
    "fileName": "file:///Users/bara/Dev/qa-mozmill-tests/firefox/lib/toolbars.js"
  }
}
TEST-UNEXPECTED-FAIL | /Users/bara/Dev/qa-mozmill-tests/firefox/lib/tests/testIdentity.js | setupModule
TEST-START | /Users/bara/Dev/qa-mozmill-tests/firefox/lib/tests/testIdentity.js | testIdentity
TEST-SKIPPED | testIdentity | setupModule failed
TEST-START | /Users/bara/Dev/qa-mozmill-tests/firefox/lib/tests/testIdentity.js | teardownModule
TEST-END | /Users/bara/Dev/qa-mozmill-tests/firefox/lib/tests/testIdentity.js | finished in 6ms

Line 881 is in the new navBar class's getElements method
Comment 14 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-10-15 19:00:01 PDT
Comment on attachment 8505863 [details] [diff] [review]
NOT YET WORKING updates per comment 12

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

::: firefox/lib/toolbars.js
@@ +784,5 @@
> +function navBar(aController) {
> +  assert.ok(aController, "A controller has to be specified");
> +
> +  this._controller = aController;
> +  this._root = this.getElement({id: "nav-bar"});

The problem you have lays here. There is no element of this type listed in getElements. also it should be type: '' and not id. So simply add this item to the method.
Comment 15 User image Barbara Miller (:galgeek) 2014-10-16 17:26:28 PDT
Created attachment 8506529 [details] [diff] [review]
bug1076741 toolbars.js edits and all testAwesomeBar test edits, plus a few others

toolbar.js edits complete? along with all testAwesomeBar tests and a few others.

after applying patch, you could test with 

mozmill -t "firefox/tests/functional/testAwesomeBar/" -b "/Applications/FirefoxNightly.app"
Comment 16 User image Andrei Eftimie 2014-10-17 02:51:43 PDT
Comment on attachment 8506529 [details] [diff] [review]
bug1076741 toolbars.js edits and all testAwesomeBar test edits, plus a few others

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

Really great work Barbara!

There are still some tests that need attention tough. See below for a list from running a full testrun.

I've run some testruns, here's an example on how to run a full testrun:
> testrun_functional --repository=. --report=http://mozmill-crowd.blargon7.com/db/ /Applications/FirefoxNightly.app/

Here are the results, we'll have to have green testruns:
http://mozmill-crowd.blargon7.com/#/functional/report/ad8dad905f3281cb521a23ddf3462bdb
http://mozmill-crowd.blargon7.com/#/remote/report/ad8dad905f3281cb521a23ddf3464e27
http://mozmill-crowd.blargon7.com/#/endurance/report/ad8dad905f3281cb521a23ddf34695d4

::: firefox/lib/toolbars.js
@@ +200,1 @@
>                                          getNode().getItemAtIndex(spec.value));

nit: when changing multi-line statements please make sure you also update the indentation of all lines.
In this case the second line should also have 4 leading spaces removed.

@@ +1009,1 @@
>  exports.editBookmarksPanel = editBookmarksPanel;

Based on comment 11 I think this should also live under navBar (similar to the locationBar)
Comment 17 User image Barbara Miller (:galgeek) 2014-10-19 23:36:56 PDT
Created attachment 8507668 [details] [diff] [review]
toolbars.js updates and all test updates

This should include updates for all dependent tests. 

I've also moved editBookmarksPanel under navBar.

I see a couple of failures on the automated runs, but I don't believe they're related to these updates. I'm not sure how to produce the linked report you included in your last comment, Andrei. Should I paste here links to the raw report?
Comment 18 User image Andrei Eftimie 2014-10-20 02:14:02 PDT
I'll check you patch soon.

For the reports, when you run a testrun you can specify if you want to sent it to the dashboard, just add the report option to the testrun command: `--report=http://mozmill-crowd.blargon7.com/db/` and the report will be visible on http://mozmill-crowd.blargon7.com/
Comment 19 User image Barbara Miller (:galgeek) 2014-10-20 10:43:39 PDT
Thanks! Here are links to reports from my last testruns:

functional: http://mozmill-crowd.blargon7.com/#/functional/report/ad8dad905f3281cb521a23ddf38470ed

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/ad8dad905f3281cb521a23ddf3847f25

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/ad8dad905f3281cb521a23ddf384819b

I've been using --report=http://mozmill-crowd.blargon7.com/db/ for all testruns. The first time I tried to create one of the pretty report URLs from the raw report URL, I guess that I flubbed something.
Comment 20 User image Barbara Miller (:galgeek) 2014-10-26 14:54:10 PDT
Created attachment 8511708 [details] [diff] [review]
toolbars.js and test updates, rebased on 141026 master

Here's one more update, rebased on today's master that introduces new "MenuPanel" object in toolbars.js. I get one test failure each in remote and endurance--exactly the same results, in exactly the same tests, as master.

functional: http://mozmill-crowd.blargon7.com/#/functional/report/db1e72a89500c1dbca9e703f711e4a04
remote:     http://mozmill-crowd.blargon7.com/#/remote/report/db1e72a89500c1dbca9e703f711e4f12
endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/db1e72a89500c1dbca9e703f711e5c8f

Here are my results from master:

functional: http://mozmill-crowd.blargon7.com/#/functional/report/db1e72a89500c1dbca9e703f711e6ab4
remote:     http://mozmill-crowd.blargon7.com/#/remote/report/db1e72a89500c1dbca9e703f711e789f
endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/db1e72a89500c1dbca9e703f711e7c4c

Henrik, should the MenuPanel object be part of navBar, too?
Comment 21 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-10-27 03:36:45 PDT
(In reply to Barbara Miller (:galgeek) from comment #20)
> Henrik, should the MenuPanel object be part of navBar, too?

Indeed. It is located at the very end of the navbar. Please keep in mind that we shortly also land the patch on bug 1081024 for the download panel.
Comment 22 User image Andrei Eftimie 2014-10-27 08:25:52 PDT
Comment on attachment 8511708 [details] [diff] [review]
toolbars.js and test updates, rebased on 141026 master

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

A lot of work went into this. Thanks Barbara!

You could have saved some typing by leaving the shortcut for used in some tests `aModule.identityPopup = aModule.navBar.locationBar.identityPopup;` then most of the small changes could have been averted.
You also didn't need to change all elementsLib calls, those could have been done separately (I'm not complaining, thanks!).

You might have missed a couple tests:
> functional: testDefaultBookmarks/test1.js
> endurance: testBookmarks_OpenAndClose/test1.js

::: firefox/tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +70,3 @@
>                   "The second item in the drop down list should be selected");
>  
> +  navBarlocationBar.contains("mission");

You missed a dot here.

::: firefox/tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +66,3 @@
>    }, "Autocomplete popup has been opened - expected 'true'");
>  
>    // Wait for the autocomplete to be populated

There's a line underneath this one that references `locationBar` directly.

::: firefox/tests/functional/testGeolocation/testShareLocation.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Ohhh, this file actually had Window line endings, and now has Unix type line endings.
The rest of the files are also on Unix type endings.

That's why it appears as the whole file is changed.
Comment 23 User image Barbara Miller (:galgeek) 2014-10-28 21:24:00 PDT
The new MenuPanel and DownloadsPanel objects use BrowserWindow instead of controller. 

Should navBar, too?

What about other objects defined in toolbars.js?
Comment 24 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-10-28 23:47:54 PDT
Yes, that would be the best. Other objects should also use BrowserWindow for reference. If you want you can patch that first, or we leave this for now and pass browserWindow.controller into their constructors, which is fine too.
Comment 25 User image Barbara Miller (:galgeek) 2014-10-29 23:28:52 PDT
Created attachment 8514047 [details] [diff] [review]
bug1076741 patch rewriting toolbars.js constructors to use BrowserWindow

Thank you for the feedback! I believe this patch addresses everyone's comments.

The test results are nearly but not entirely clean. I'm puzzled by the functional test failure; I haven't seen that one before, and it's not one of the files patched. The remote test failure looks like bug 1085286 and bug 1018161

functional: http://mozmill-crowd.blargon7.com/#/functional/report/24f9dc1b051c99c8247bed4e780b03ad

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/24f9dc1b051c99c8247bed4e780b0642

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/24f9dc1b051c99c8247bed4e780b0704
Comment 26 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-10-30 00:04:52 PDT
(In reply to Barbara Miller (:galgeek) from comment #25)
> functional:
> http://mozmill-crowd.blargon7.com/#/functional/report/
> 24f9dc1b051c99c8247bed4e780b03ad

That is actually a known issue and bug 942737.

> remote:    
> http://mozmill-crowd.blargon7.com/#/remote/report/
> 24f9dc1b051c99c8247bed4e780b0642

Is that issue in testSafeBrowsing_initialDownload reproducible for you? If yes how often? So a comment on bug 1018161 might be very helpful for us! If you can reproduce can you please test to increase the timeouts in that test? Maybe the files are present about 1min later.

If this patch is ready please set the review flag for Andrei.
Comment 27 User image Andrei Eftimie 2014-10-31 04:36:11 PDT
Comment on attachment 8514047 [details] [diff] [review]
bug1076741 patch rewriting toolbars.js constructors to use BrowserWindow

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

There's a few issues as mentioned inline.

::: firefox/lib/toolbars.js
@@ +24,3 @@
>   */
> +function autoCompleteResults(aBrowserWindow) {
> +  assert.ok(aBrowserWindow, "A browser window has been defined");f

Theres's an random `f` character at the end of the line which fail:
> "message": "f is not defined",

@@ +1010,5 @@
> +   * @returns {object} menuPanel
> +   */
> +  get menuPanel() {
> +    return this._menuPanel;
> +  },

Please leave an empty newline between all getters/properties/methods/

@@ +1054,5 @@
> +   */
> +  getElements : function navBar_getElements(aSpec) {
> +    var spec = aSpec || {};
> +
> +    var root = spec.parent ? spec.parent.getNode() 

nit: extra trailing whitepsace

@@ +1205,4 @@
>  }
>  
>  // Export of classes
> +exports.NavBar = NavBar;

You'll need a small update here as the DownloadsPanel landed yesterday on default (bug 1081024) so this doesn't apply cleanly anymore.

::: firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js
@@ +139,4 @@
>   *        MozMillController of the window to operate on
>   */
>  function confirmHandler(aController) {
> +  var dialogButton = new elementsLib.Lookup(aController.window.document,

This fails:
> "message": "elementsLib is not defined",

The class is not camelcased.
Comment 28 User image Barbara Miller (:galgeek) 2014-10-31 18:10:51 PDT
Created attachment 8515403 [details] [diff] [review]
bug1076741-141031.patch

Thank you for the feedback, and sorry I missed those typos! I looked hard at the entire diff this time, but I also wonder if you're using other tools to do your review, Andrei.

This may at last be everything, including adding DownloadPanel to NavBar and clean test runs, except for the same remote test that's been failing.

functional: http://mozmill-crowd.blargon7.com/#/functional/report/46dbc8f622b855f9ec017c8a6049214f

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/46dbc8f622b855f9ec017c8a60490ba6

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/46dbc8f622b855f9ec017c8a604917bf
Comment 29 User image Barbara Miller (:galgeek) 2014-11-02 12:24:38 PST
Created attachment 8515668 [details] [diff] [review]
patch restoring master’s short object names

This is a substantially smaller patch, restoring current master's short object names.

Test results:

functional: http://mozmill-crowd.blargon7.com/#/functional/report/46dbc8f622b855f9ec017c8a6064ce70

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/46dbc8f622b855f9ec017c8a6064dd02

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/46dbc8f622b855f9ec017c8a6064e327
Comment 30 User image Barbara Miller (:galgeek) 2014-11-02 12:39:13 PST
Created attachment 8515671 [details] [diff] [review]
patch restoring master’s short object names, and excluding git markup

In Bugzilla's diff, I noticed a line of ======= in toolbars.js, a remnant of git conflict markup, that I've now removed. I made no other changes, and I haven't retested.
Comment 31 User image Andrei Eftimie 2014-11-04 03:44:06 PST
Comment on attachment 8515671 [details] [diff] [review]
patch restoring master’s short object names, and excluding git markup

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

Indeed, this patch is smaller.

Barbara, how are you running a testrun? You need to pass in the mercurial repository with the patch applied (either completely or in a hg queue) for mozmill-automation to take it into consideration. I think that's why you're seeing green builds.

Not sure if this works with the git clone, I think it will probably fail.
> hg clone http://hg.mozilla.org/qa/mozmill-tests
> cd mozmill-tests
> hg qimport > %path/to/your/exported/patch%
> hg qpush
> testrun_functional --repository=. --report=http://mozmill-crowd.blargon7.com/db/ %path/to/firefox/binary

Fortunately that's the only error I still see.

In regard to how I check the patches, for something this size, running full testruns is the best way to see nothing regressed.
I do check them in bugzilla's diff tool (click on `review` once a patch is uploaded) and from the command line with `hg qdiff` or `git show`.

Other than the failure, there are a few tests which make use of autoCompleteResults, where if you just reference the `locationBar` the rest of the test can remain intact. I have made comment in some of them, not all, but it might be good to have them all work in a similar way. This will further reduce the size of the patch.

===

With this changes I think we should be close to landing the patch. Thanks

::: firefox/lib/tests/testMenuPanel.js
@@ +8,2 @@
>  var browser = require("../ui/browser");
> +var toolbars = require("../toolbars");

Please leave this where it was, there's a separation between backend/api and ui classes.

::: firefox/tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +23,5 @@
> +  aModule.controller = aModule.browserWindow.controller;
> +
> +  aModule.navBar = new toolbars.NavBar(aModule.browserWindow);
> +  aModule.locationBar = aModule.navBar.locationBar;
> +  aModule.autoCompleteResults = aModule.locationBar.autoCompleteResults;

You can remove this assignment, and leave the rest of the test as it was: `locationBar.autoCompleteResults...`

::: firefox/tests/functional/testAwesomeBar/testSuggestHistory.js
@@ +24,5 @@
> +  aModule.controller = aModule.browserWindow.controller;
> +
> +  aModule.navBar = new toolbars.NavBar(aModule.browserWindow);
> +  aModule.locationBar = aModule.navBar.locationBar;
> +  aModule.autoCompleteResults = aModule.locationBar.autoCompleteResults;

Same here, if you remove this assignment, you can probably leave most of the test itself intact.

::: firefox/tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +27,4 @@
>    aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);
>  
> +  aModule.locationBar = aModule.navBar.locationBar;
> +  aModule.autoCompleteResults = aModule.locationBar.autoCompleteResults;

I think you could leave this directly under locationBar, the rest of the test would require then close to no other changes.

::: firefox/tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +68,4 @@
>    }, "Location bar contains the typed data - expected '" + testString + "'");
>  
>    // Get the visible results from the autocomplete list. Verify it is equal to maxrows
> +  var popup = autoCompleteResults.getElement({type:"popup"});

This currently fails:
> ERROR | Test Failure | {
>   "exception": {
>     "message": "autoCompleteResults is not defined", 
>     "lineNumber": 71, 
>     "name": "ReferenceError", 
>     "fileName": "resource://mozmill/modules/frame.js -> file:///var/folders/9l/sn_p3bw914s360j602z20jsc0000gq/T/tmpUbJscS.workspace/mozmill-tests/firefox/tests/functional/testAwesomeBar/testVisibleItemsMax.js"
>   }
> }

You should leave `locationbar.autoCompleteResults` in this  test as they were, and it should work fine :)
Comment 32 User image Barbara Miller (:galgeek) 2014-11-04 16:02:58 PST
Created attachment 8517054 [details] [diff] [review]
a still smaller patch, restoring short, though not alway shortest, object names

Thank you again for your comments, and your patience!

I'd been doing testruns without specifying a repository; I now see the error of my ways.

Regarding autoCompleteResults, I suspect I edited tests using identityPopup first, which gets defined separately from locationBar. I've reverted my autoCompleteResults changes.

Regarding specifying browser requirement, I've generally inserted it in alphabetical order among the other requirements. Is this a problem? I've corrected all the files I've edited today.

Thanks again!
Comment 33 User image Barbara Miller (:galgeek) 2014-11-04 16:12:38 PST
oops! forgot the latest test results, run against the patched hg repo...
still one remote fail, probably related to bug 1018161

functional:  http://mozmill-crowd.blargon7.com/#/functional/report/1e5e44bea8f3e17708194a929a8b828e

remote:      http://mozmill-crowd.blargon7.com/#/remote/report/1e5e44bea8f3e17708194a929a8b8715

endurance:   http://mozmill-crowd.blargon7.com/#/endurance/report/1e5e44bea8f3e17708194a929a8b898a
Comment 34 User image Andrei Eftimie 2014-11-05 01:14:18 PST
Comment on attachment 8517054 [details] [diff] [review]
a still smaller patch, restoring short, though not alway shortest, object names

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

All tests are green for me now.
Comment 35 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-05 05:04:44 PST
Comment on attachment 8517054 [details] [diff] [review]
a still smaller patch, restoring short, though not alway shortest, object names

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

A lovely and huge patch! Thank you Barbara. It looks really good, given that you started recently. As you will notice it is not ready yet. You will see a couple of comments inline. Some are general and apply to a lot of other files. So you may want to look for those closely. Let me know if you have remaining questions.

::: firefox/lib/tests/testDownloadsPanel.js
@@ +17,5 @@
>  
>  function setupModule(aModule) {
>    aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.navBar = new toolbars.NavBar(aModule.browserWindow);
> +  aModule.downloadsPanel = aModule.navBar.downloadsPanel;

While checking this code I wonder if we should directly add the navBar property to the browser window class. That would save us a lot of code, and we could access the downloadsPanel via browserWindow.navBar.downloadsPanel. We should only make sure it is a lazily loaded property, means only create an instance of the NavBar class when necessary.

::: firefox/lib/toolbars.js
@@ +183,5 @@
>    getElements : function autoCompleteResults_getElements(aSpec) {
>      var spec = aSpec || {};
>  
> +    var root = spec.parent ? spec.parent.getNode()
> +                           : this._browserWindow.controller.window.document;

lets use the browserWindow property here.

@@ +231,4 @@
>          this._popup.getNode().hidePopup();
>        }
>        else {
> +        this._browserWindow.controller.keypress(locationBar.urlbar,

Same here and for all the other cases. Maybe we could also change it to locationBar.urlbar.keypress()?

@@ +467,4 @@
>  
>  /**
>   * Constructor
> + * Edit Bookmarks Panel class

nit: Please remove the constructor while you are on it.

@@ +1189,4 @@
>  }
>  
>  /**
> + * Nav Bar class

nit: Navigation bar class and please add @constructor

@@ +1201,5 @@
> +  this._downloadsPanel = new DownloadsPanel(aBrowserWindow);
> +  this._editBookmarksPanel = new editBookmarksPanel(aBrowserWindow);
> +  this._locationBar = new locationBar(aBrowserWindow);
> +  this._menuPanel = new MenuPanel(aBrowserWindow);
> +  this._searchBar = new search.searchBar(aBrowserWindow.controller);

Please lets not create instances for all of those classes initially. We have to do it via a getter. So initialize the internal properties to null, and inside the getter you will check for null, and create an instance if necessary.

@@ +1202,5 @@
> +  this._editBookmarksPanel = new editBookmarksPanel(aBrowserWindow);
> +  this._locationBar = new locationBar(aBrowserWindow);
> +  this._menuPanel = new MenuPanel(aBrowserWindow);
> +  this._searchBar = new search.searchBar(aBrowserWindow.controller);
> +  this._root = this.getElement({type: "nav-bar"});

I would put root and browserWindow in a separate block.

@@ +1352,5 @@
> +          self.ended = !aMutation.target.hasAttribute("notification");
> +        }
> +      });
> +    });
> +    try {

nit: Please add an empty line above.

::: firefox/tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +80,4 @@
>  
>    // Finally - Check that the mozilla page was loaded by verifying the
>    // Mozilla logo exists
> +  var mozillaLogo = findElement.ID(controller.tabs.activeTab, "mozilla_logo");

I would say we should leave those changes alone, and really only do that for the toolbar classes.
Comment 36 User image Barbara Miller (:galgeek) 2014-11-06 00:05:34 PST
Thank you for the feedback!

I have a couple of questions. First, do you really want to think about adding NavBar to BrowserWindow as part of this bug?

In your comments regarding toolbars.js lines beginning at 183, I am not sure what you mean by "lets use the browserWindow property here."

And I'm not sure about your final comment; we should update "new elementslib.ID" to findElement.ID only for toolbars.js?

I am done with the other changes and should be able to upload a new patch later Thursday.
Comment 37 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-06 03:12:01 PST
(In reply to Barbara Miller (:galgeek) from comment #36)
> I have a couple of questions. First, do you really want to think about
> adding NavBar to BrowserWindow as part of this bug?

Yes, otherwise we would have to go over the same code again soon, and remove all the newly added lines. I don't see that as optimal, especially when checking for changes via hg blame. 

> In your comments regarding toolbars.js lines beginning at 183, I am not sure
> what you mean by "lets use the browserWindow property here."

You should not use the "private" _browserWindow property when there is a getter for browserWindow. In any case it can happen that this getter has to be modified, so you would also have to update all the other occurrences where you access the private property. By using the getter instead, everything will automatically get the new behavior. 

> And I'm not sure about your final comment; we should update "new
> elementslib.ID" to findElement.ID only for toolbars.js?

As Andrei mentioned earlier, we should limit changes of code to what should actually be covered in the bug. Changing those lines is not necessary but only makes the patch way larger. In case of toolbar.js it is fine given that the code blocks are moved around anyway. But leaving out the changes on different tests, will make it easier for the reviewer to go through.

I hope that clarifies your questions. If not please follow-up appropriately. Thanks!

> 
> I am done with the other changes and should be able to upload a new patch
> later Thursday.
Comment 38 User image Barbara Miller (:galgeek) 2014-11-07 01:19:36 PST
Created attachment 8518752 [details] [diff] [review]
updates per comments 31-37

Thanks for your comments! The latest patch includes all the lib changes everyone has requested, but as for the tests, so far, I've updated just one test, firefox/lib/tests/testToolbars.js, to use browserWindow.navBar, and it passes.

functional: http://mozmill-crowd.blargon7.com/#/functional/report/d5154521c56af709cdb6989810276692

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/d5154521c56af709cdb6989810277787

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/d5154521c56af709cdb698981027a0cc
Comment 39 User image Barbara Miller (:galgeek) 2014-11-10 22:30:02 PST
Created attachment 8520421 [details] [diff] [review]
toolbars.js, browser.js, and related test updates

This patch should address everyone's comments to date. Thanks for your feedback and for your patience!

Regarding this line in toolbars.js (line 234 of patched toolbars.js)...

    this._browserWindow.controller.keypress(locationBar.urlbar,

... switching to locationBar.urlbar.keypress(... results in an error,
"locationBar.urlbar is undefined." 

I'm not sure why ...keypress(locationBar.urlbar,...) succeeds.

Here are the test results, against the patched hg repo, all green for me except for the remote test that's been failing:

functional: http://mozmill-crowd.blargon7.com/#/functional/report/43b0604030afed1afa48590287b0ce39

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/43b0604030afed1afa48590287b0d7c5

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/43b0604030afed1afa48590287b0db44
Comment 40 User image Andrei Eftimie 2014-11-13 00:30:47 PST
Comment on attachment 8520421 [details] [diff] [review]
toolbars.js, browser.js, and related test updates

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

Looks really great.

Unfortunately there's a new test which landed a couple days ago which will need a small update, as it currently fails:
> firefox/tests/remote/testGeolocation/testShareLocation.js
This gets an r+ from me with that fixed so we're all green.

To shorten review time, asking a final review from Henrik as well.
Comment 41 User image Barbara Miller (:galgeek) 2014-11-13 13:32:20 PST
Created attachment 8522493 [details] [diff] [review]
a complete patch? now with updated test per comment 40

Thanks, Andrei!

Here's a patch including an update of the test you pointed out.

Latest test results, with patched hg repo from today 
(still 2 remote tests failing, otherwise green):

functional: http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c43fcdf8

remote:     http://mozmill-crowd.blargon7.com/#/remote/report/b67428b6e502c230ff85b6a4c43fdcba

endurance:  http://mozmill-crowd.blargon7.com/#/endurance/report/b67428b6e502c230ff85b6a4c43fdd6d
Comment 42 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-20 07:21:01 PST
Comment on attachment 8522493 [details] [diff] [review]
a complete patch? now with updated test per comment 40

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

Those are wonderful changes. I still see that some lines will need updates, so please see my inline comments for those. We are close in getting it landed. Thanks Barbara.

::: firefox/lib/toolbars.js
@@ +969,4 @@
>     *        Text to enter into the location bar
>     */
>    type : function locationBar_type(aText) {
> +    this.browserWindow._controller.type(this.urlbar, aText);

nit: this.browserWindow.controller.

@@ +1222,5 @@
> +   * @returns {object} menuPanel
> +   */
> +  get downloadsPanel() {
> +    return this._downloadsPanel = this._downloadsPanel ||
> +	                                new DownloadsPanel(this._browserWindow);

Please separate this into multiple lines with an if condition as check. It may be a bit more code but way better readable. This applies to any of those properties, and to the other classes you modify here.

@@ +1270,5 @@
> +   * Retrieve an UI element based on the given spec
> +   *
> +   * @param {object} aSpec
> +   *        Information of the UI element which should be retrieved
> +   * @config {string}  type      - General type information

This is all @param and not @config. Please also do not add extra indentation, just a single blank between type, name, dash, and description.

@@ +1276,5 @@
> +   * @config {string}  [value]   - Value of the element or property
> +   * @config {element} [parent]  - Parent of the to find element
> +   *
> +   * @returns Element which has been found
> +   * @type {ElemBase}

Please combine those two. You may want to check the other classes.

@@ +1296,5 @@
> +   * @config {element} [parent]  - Parent of the to find element
> +   *
> +   * @returns Elements which have been found
> +   * @type {array of ElemBase}
> +   */

Same here as mentioned above.
Comment 43 User image Barbara Miller (:galgeek) 2014-11-20 11:26:55 PST
Created attachment 8526212 [details] [diff] [review]
toolbars.js updates per comment 42

Thank you for the feedback!

This patch includes updates to toolbars.js per comment 42.

testruns (that one remote test continues to fail)...

functional: http://mozmill-crowd.blargon7.com/#/functional/report/635fcffb06b246be47416301bd748588

remote: http://mozmill-crowd.blargon7.com/#/remote/report/635fcffb06b246be47416301bd74b1ee

endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/635fcffb06b246be47416301bd74c1dc
Comment 44 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-21 05:55:06 PST
Comment on attachment 8526212 [details] [diff] [review]
toolbars.js updates per comment 42

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

That looks great now. Just some minor changes left. Once those are fixed we should be good to get this large patch landed!

::: firefox/lib/toolbars.js
@@ +683,4 @@
>     */
>    get autoCompleteResults() {
> +    if (!this._autoCompleteResults) {
> +      this._autoCompleteResults = new autoCompleteResults(this._browserWindow);

Please use the browserWindow property here, and in all the other cases in this file.
Comment 45 User image Barbara Miller (:galgeek) 2014-11-21 10:30:44 PST
Created attachment 8526888 [details] [diff] [review]
_browserWindow to browserWindow in toolbars.js

a new, last? patch updating toolbars.js per comment 44 -- thank you!

remote test testInstallAddonWithEULA is failing for me today on master repo as well as my patched repo, but all else is clean.

functional: http://mozmill-crowd.blargon7.com/#/functional/report/635fcffb06b246be47416301bdeca0d9

remote: http://mozmill-crowd.blargon7.com/#/remote/report/635fcffb06b246be47416301bdecc72e

endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/635fcffb06b246be47416301bdece1e3

I am not clear on why it's important to use browserWindow rather than _browserWindow in these getters. I'll see if I can figure that out.
Comment 46 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-24 06:23:43 PST
(In reply to Barbara Miller (:galgeek) from comment #45)
> I am not clear on why it's important to use browserWindow rather than
> _browserWindow in these getters. I'll see if I can figure that out.

The property starting with an underscore is virtually private and should not directly be used. It's implementation could change at any time. The public browserWindow property hides that. So you would not have to change so many other code lines, when such a situation happens.
Comment 47 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-24 06:33:21 PST
Comment on attachment 8526888 [details] [diff] [review]
_browserWindow to browserWindow in toolbars.js

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

This looks great! I will get this landed in a couple of minutes.
Comment 48 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-24 06:41:28 PST
Barbara, for further hg patches please ensure to have your hg username set. Also we require a proper commit message.

Landed on default as:
https://hg.mozilla.org/qa/mozmill-tests/rev/7c8f24debef4

We might also wanna have that for the aurora branch. Barbara, can you please check if that patch applies and runs cleanly? Thanks.
Comment 49 User image Barbara Miller (:galgeek) 2014-11-24 13:09:21 PST
Thank you, Henrik!

Andrei has been very tolerant of my git patches... I'll submit an hg-compatible patch for review next time.

Aurora test results:

functional: http://mozmill-crowd.blargon7.com/#/functional/report/4b004f9a9a3a5bc3aa0f3a07c130e960
clean!

remote: http://mozmill-crowd.blargon7.com/#/remote/report/4b004f9a9a3a5bc3aa0f3a07c1310391
continues to fail: /restartTests/testSafeBrowsing_initialDownload/test2.js

endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/4b004f9a9a3a5bc3aa0f3a07c131d093
new failure? /restartTests/testSafeBrowsing_initialDownload/test2.js but NOT affected by this patch

Also, I'm seeing lots of the warnings noted in Bug 1078134, for each of these test runs. I don't believe this is a problem with the patch.
Comment 50 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2014-11-25 06:43:09 PST
Yeah, those failures are not related to your changes. I would still like to see that we can get them investigated. So if you have the time, I would appreciate your help regarding the safe browsing test.

I did those runs locally and all is fine too. So I transplanted the changeset to the mozill-aurora branch as https://hg.mozilla.org/qa/mozmill-tests/rev/1e274b74b0c0.

All what we can do here is done. Thanks a lot Barbara for the time you spent on this patch!

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