Last Comment Bug 705284 - Mozmill endurance test for bookmarking all tabs and opening and closing them
: Mozmill endurance test for bookmarking all tabs and opening and closing them
Status: RESOLVED FIXED
[mozmill-endurance][bookmarks]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
Mentors:
Depends on: 780107
Blocks: 629065
  Show dependency treegraph
 
Reported: 2011-11-25 07:41 PST by Remus Pop (:RemusPop)
Modified: 2012-08-21 02:12 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
[initial patch] patch v1.0 (7.70 KB, patch)
2012-07-24 02:27 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v2.0 (6.57 KB, patch)
2012-07-25 06:08 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
dave.hunt: review-
Details | Diff | Splinter Review
patch v2.1 (6.46 KB, patch)
2012-07-25 23:30 PDT, Maniac Vlad Florin (:vladmaniac)
no flags Details | Diff | Splinter Review
patch v3.0 (6.10 KB, patch)
2012-07-27 02:21 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v4.0 (6.00 KB, patch)
2012-07-27 08:00 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v4.1 (5.95 KB, patch)
2012-07-30 00:08 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v4.2 (5.98 KB, patch)
2012-07-30 06:52 PDT, Maniac Vlad Florin (:vladmaniac)
no flags Details | Diff | Splinter Review
patch v4.3 (5.90 KB, patch)
2012-07-30 07:43 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v4.4 (5.98 KB, patch)
2012-07-31 01:09 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
cmtalbert: feedback+
Details | Diff | Splinter Review
patch v4.5 [checked-in] (6.04 KB, patch)
2012-08-03 04:05 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review+
Details | Diff | Splinter Review
[mozilla-esr10] WIP patch (5.82 KB, patch)
2012-08-08 01:02 PDT, Maniac Vlad Florin (:vladmaniac)
no flags Details | Diff | Splinter Review
[mozilla-esr10] patch v1.0 [checked-in] (5.87 KB, patch)
2012-08-16 04:29 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
dave.hunt: review+
Details | Diff | Splinter Review

Description Remus Pop (:RemusPop) 2011-11-25 07:41:06 PST
Tracking creation of a mozmill endurance test for bookmarking all the open tabs and then closing and opening them.

The test should be like this:
Setup Module
  Open X pages
  Save X pages as bookmarks
Main Test Checkpoints
  Open all bookmarks one by one
  Close all tabs
Teardown Module
  Force close tabs

Dave, please let me know if this is the behaviour we want.
Comment 1 Dave Hunt (:davehunt) 2011-11-28 04:16:04 PST
I'm not familiar with this test. I did suggest a similar one that added bookmarks to a single folder and then opened all bookmarks using 'Open All in Tabs'.
Comment 2 Remus Pop (:RemusPop) 2011-11-28 05:08:12 PST
There is one test in the MozMill test coverage spreadsheet. Look into the Data sheet on line 456.
On line 455 is the test you mentioned.
Comment 3 Dave Hunt (:davehunt) 2011-11-28 08:31:26 PST
Can you link to the spreadsheet?
Comment 4 Remus Pop (:RemusPop) 2011-11-28 08:32:42 PST
Sure, here it is: https://docs.google.com/spreadsheet/ccc?key=0AlroormSQgiRcEFQNVk1QUgzLVRsLXdSb05nQnVqVVE&hl=en_US&pli=1#gid=0

Sorry for assuming you already have it.
Comment 5 Dave Hunt (:davehunt) 2011-11-29 02:32:08 PST
I found the original reference to this in the etherpad [1]: "Save bookmarks from all open tabs and reopen those over and over again". This looks like one of Henrik's suggestions, however there's not a lot of detail there.

My guess is that the following is how the test would look:

Name: testBookmarks_OpenBookmark

Setup Module
  Open X pages
  Save X pages as bookmarks
  Close all tabs
Main Test Checkpoints
  Open X bookmarks one by one
  Close all tabs
Teardown Module
  Force close tabs

One problem I foresee with this test (and other bookmark endurance tests using entities) is that we would need unique pages for each bookmark.

[1] https://etherpad.mozilla.org/mLWlTg7DwK
Comment 6 Remus Pop (:RemusPop) 2011-11-29 02:38:06 PST
I was thinking of using all pages in http://mozqa.com/data/firefox/layout/ . So they are unique.
Comment 7 Dave Hunt (:davehunt) 2011-11-29 02:49:24 PST
The problem is that the number of entities is set on the command line. What if someone wants to run with 100 entities, or 1000?
Comment 8 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-29 08:31:11 PST
(In reply to Dave Hunt (:davehunt) from comment #7)
> The problem is that the number of entities is set on the command line. What
> if someone wants to run with 100 entities, or 1000?

That seems like a fundamental limitation. The only way I see around it is if we could somehow dynamically and randomly generate X number of temporary test pages which are created on start and destroyed on exit.
Comment 9 Maniac Vlad Florin (:vladmaniac) 2011-11-29 08:44:12 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #8)
> (In reply to Dave Hunt (:davehunt) from comment #7)
> > The problem is that the number of entities is set on the command line. What
> > if someone wants to run with 100 entities, or 1000?
> 
> That seems like a fundamental limitation. The only way I see around it is if
> we could somehow dynamically and randomly generate X number of temporary
> test pages which are created on start and destroyed on exit.

Pretty science for just a mozmill test i would say - but I don't have a better idea. 

If we serve a fixed number of testpages and use only iterations in the endurance test, well, there is not much value in it in the end. 

If we consider having a testpage opened as part of an entity, then we won't have separate testpages. 

Initially I was thinking of having an array of urls and randomly take one page from there for a current entity to handle - bad idea because eventually they will repeat and we won't bookmark one page twice there is no sense in that.
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-29 09:03:47 PST
Do they have to be completely unique though? As an experiment:

1) Bookmark this page
2) Bookmarks > Show all Bookmarks > Unsorted Bookmarks
3) Right click bookmark for this page and select Copy
4) Left click and select Paste 3 times
5) Create a new folder on the bookmarks toolbar called "temp"
6) Move the newly created bookmarks into this folder
7) Close the Library and select Bookmarks > temp > Open all in tabs

Result:
All of the same site opens in multiple tabs (one for each bookmark).

Any reason we can't preload a bookmark folder with X number of bookmarks for the same URL for an Endurance test?
Comment 11 Maniac Vlad Florin (:vladmaniac) 2011-11-29 09:31:51 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #10)
> Do they have to be completely unique though? As an experiment:
> 
> 1) Bookmark this page
> 2) Bookmarks > Show all Bookmarks > Unsorted Bookmarks
> 3) Right click bookmark for this page and select Copy
> 4) Left click and select Paste 3 times
> 5) Create a new folder on the bookmarks toolbar called "temp"
> 6) Move the newly created bookmarks into this folder
> 7) Close the Library and select Bookmarks > temp > Open all in tabs
> 
> Result:
> All of the same site opens in multiple tabs (one for each bookmark).
> 
> Any reason we can't preload a bookmark folder with X number of bookmarks for
> the same URL for an Endurance test?
Yes - your idea solves duplicating bookmarks but..

Without a strong API this would take a long time and would be not robust. 
Imagine the amount of code there..

Anyway, its doable but with costs
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-29 09:42:08 PST
Dave, any feedback on this?
Comment 13 Dave Hunt (:davehunt) 2011-11-29 10:39:17 PST
As discussed on IRC, we should try adding a URL parameter with a unique value (use the current entity number). The parameter should be ignored but be a unique URL for a bookmark.
Comment 15 Dave Hunt (:davehunt) 2011-11-29 11:07:41 PST
Assuming that works, it looks good to me.
Comment 16 Remus Pop (:RemusPop) 2011-12-02 07:08:36 PST
Dave's idea is fine with using a parameter.

From this point on we have some options from which we need to choose:

1. How to bookmark
  option a: bookmark each page when visited
  option b: bookmark all pages at once () (Dave opted for this one)

2. Where to put the folder with the bookmarks (bookmarks will reside in a folder to keep them grouped)
  option a: Bookmarks Menu
  option b: Bookmarks Toolbar
  option c: Unsorted Bookmarks
  option d: Existing folder

Each option from 2 implies a different degree of difficulty and steps to perform in order to open the bookmarked page.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-02 08:07:39 PST
Here is what I propose:

1) Open each page in a new tab
2) Right click tab bar > Bookmark All Tabs
3) Give the folder a name (ie. "Mozmill Bookmarks"), save to the Bookmarks Toolbar
4) Close all tabs
> Start iteration
5) Bookmarks > "Mozmill Bookmarks" > Open all in tabs
6) Close all tabs
> End iteration

Dave, what do you think?
Comment 18 Dave Hunt (:davehunt) 2011-12-05 05:27:50 PST
I'm in agreements with all but item 5. This test is to open bookmarks one-by-one. We have another test that uses 'Open all in tabs'.

We would have to open a specific bookmark by reference. If we save it with a predictable name (probably easiest to use the current entity number) then opening it should be relatively simple.
Comment 19 Henrik Skupin (:whimboo) 2011-12-05 06:56:41 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> http://mozqa.com/data/firefox/layout/mozilla.html?entity=0

Instead of this page you probably want to use the following page or create a new one: http://mozqa.com/data/firefox/tabbedbrowsing/openinnewtab_target.html?id=1
Comment 20 Remus Pop (:RemusPop) 2011-12-14 01:12:26 PST
We will need the last tab to be an empty one in the case the test is run with 1 entity because if 1 tab is opened, bookmark all tabs option is not available.
Comment 21 Dave Hunt (:davehunt) 2011-12-14 03:37:52 PST
It sounds like we have a case for minimum entities. I would suggest setting checking if entities < 2 and if true set entities to 2. This would need a suitable comment.
Comment 22 Henrik Skupin (:whimboo) 2012-07-19 23:11:46 PDT
Vlad, mind taking this test so we can finish it up?
Comment 23 Maniac Vlad Florin (:vladmaniac) 2012-07-24 00:05:41 PDT
(In reply to Dave Hunt (:davehunt) from comment #21)
> It sounds like we have a case for minimum entities. I would suggest setting
> checking if entities < 2 and if true set entities to 2. This would need a
> suitable comment.

How would you set entities to 2? afaik we set the entities number from the command line, can we set them at runtime?
Comment 24 Henrik Skupin (:whimboo) 2012-07-24 00:18:09 PDT
In setupModule() where the test is getting prepared, and we have to restore the value in teardownModule(). Otherwise we can let tests define a minimum number of entities and if set the endurance manager makes use of it. That's probably the better way and we do not mess around with the persisted entries.
Comment 25 Maniac Vlad Florin (:vladmaniac) 2012-07-24 02:18:08 PDT
(In reply to Remus Pop (:RemusPop) from comment #20)
> We will need the last tab to be an empty one in the case the test is run
> with 1 entity because if 1 tab is opened, bookmark all tabs option is not
> available.

There will always be at least 2 tabs because tabBrowser.closeAllTabs() closes the tabs and leaves and empty one. Then we filter our bookmarked pages by title and open and close them, so no need to treat the case for minimum entities
Comment 26 Maniac Vlad Florin (:vladmaniac) 2012-07-24 02:27:24 PDT
Created attachment 645232 [details] [diff] [review]
[initial patch] patch v1.0

Proposed solution

You will probably wonder why there are three methods in this test (except setup and teardown): 

* the test method 
* setBookmarks method because we need to have lots of prerequisites for this test, meaning:
     1. open pages
     2. bookmark all pages
     and I would not want to place lots of code in setupModule(), IMHO
* handle bookmark prefs modal dialog method
     1. need to handle the modal dialog
        we cannot put this into the API because start() method from modal-dialog.js has a callback which has only the controller as parameter and I need to setup some test specific data such as the bookmarks folder name, which means that in the API this method will need more parameters. Other tests are developed in the same manner, like functional/restartTests/testMasterPassword

In terms of functionality, the test behaves normally 
http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b06cd3e
Comment 27 Henrik Skupin (:whimboo) 2012-07-24 03:43:30 PDT
Comment on attachment 645232 [details] [diff] [review]
[initial patch] patch v1.0

>+      case "tabsStrip_bookmarkAllTabs":
>+        elem = new elementslib.ID(this._controller.window.document, "context_bookmarkAllTabs");
>+      break;

This is a context menu entry and has nothing to do with the tabbrowser module. Remove that please.

>+var domUtils = require("../../../lib/dom-utils"); 
>+var endurance = require("../../../lib/endurance");
>+var modalDialog = require("../../../lib/modal-dialog");
>+var places = require("../../../lib/places");
>+var prefs = require("../../../lib/prefs");
>+var tabs = require("../../../lib/tabs");
>+var toolbars = require("../../../lib/toolbars");
>+var utils = require("../../../lib/utils");



>+const TIMEOUT_DIALOG = 25000;

This doesn't tell me anything. Which dialog? Please be more specific with the name of the variable.

>+function testOpenAndCloseAllBookmarks() {


>+  enduranceManager.run(function () {
>+    tabBrowser.closeAllTabs();

I don't think we need this call at this position.

>+      controller.rightClick(page);
>+
>+      var openInNewTab = new elementslib.ID(controller.window.document, 
>+                                            "placesContext_open:newtab");
>+      controller.click(openInNewTab);

You want to use the Menu API here which also handles context menus.

>+      controller.waitForPageLoad();

You first have to wait for the new tab been opened and selected.

>+      // Dismiss the context menu
>+      controller.keypress(null , 'VK_ESCAPE', {});

This is not necessary when used via the Menu API.


>+function setupBookmarks() {
>+  // Enable bookmarks toolbar 
>+  var navbar = new elementslib.ID(controller.window.document, "nav-bar");
>+  var tabStrip = tabBrowser.getElement({type: "tabs_strip"});
>+
>+  controller.rightClick(navbar, navbar.getNode().boxObject.width / 2, 2);
>+
>+  var toggle = new elementslib.ID(controller.window.document,
>+                                  "toggle_PersonalToolbar");
>+  controller.mouseDown(toggle);
>+  controller.mouseUp(toggle);

Now we have this code twice. In defaultbookmarks and here. I would say lets move it to a library module.

>+  for (var i = 1; i <= enduranceManager.entities; i++) {
>+    controller.open(LOCAL_TEST_PAGE + i); 
>+    controller.waitForPageLoad();
>+
>+    tabBrowser.openTab(); 
>+  } 
>+  controller.rightClick(tabStrip);
>+  
>+  // Bookmark all tabs
>+  var bookmarkAllTabs = tabBrowser.getElement({type: "tabsStrip_bookmarkAllTabs"});

If we do not perform any endurance related tasks I would say we do not go through the ui here but always make use of back-end API calls to setup the list of new bookmarks.


>+function handleBookmarkPreferencesDialog(aController) {

I would like to kill that.
Comment 28 Maniac Vlad Florin (:vladmaniac) 2012-07-24 03:52:19 PDT
> 
> If we do not perform any endurance related tasks I would say we do not go
> through the ui here but always make use of back-end API calls to setup the
> list of new bookmarks.
> 
> 
> >+function handleBookmarkPreferencesDialog(aController) {
> 
> I would like to kill that.

Perfectly agree but please see comment 17 and ealier. It was an explicit decision to use bookmark all tabs by right click...
Comment 29 Maniac Vlad Florin (:vladmaniac) 2012-07-24 03:54:14 PDT
> 
> Now we have this code twice. In defaultbookmarks and here. I would say lets
> move it to a library module.
> 

Just to make sure, are you suggesting we should create a new shared module called library.js and include a helper method to enable the bookmarks toolbar?
Comment 30 Henrik Skupin (:whimboo) 2012-07-24 03:55:15 PDT
This test covers opening and closing of bookmarks but not the addition of those. Really, I do not see why we have to go through the UI here. It's a chance of getting additional failures.
Comment 31 Henrik Skupin (:whimboo) 2012-07-24 03:56:15 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #29)
> Just to make sure, are you suggesting we should create a new shared module
> called library.js and include a helper method to enable the bookmarks
> toolbar?

No, this was meant like: a module under lib. I haven't proposed a name.
Comment 32 Maniac Vlad Florin (:vladmaniac) 2012-07-24 04:01:07 PDT
(In reply to Henrik Skupin (:whimboo) from comment #30)
> This test covers opening and closing of bookmarks but not the addition of
> those. Really, I do not see why we have to go through the UI here. It's a
> chance of getting additional failures.

I could not agree more. So I will move on and make the changes
Comment 33 Maniac Vlad Florin (:vladmaniac) 2012-07-24 04:01:45 PDT
(In reply to Henrik Skupin (:whimboo) from comment #31)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #29)
> > Just to make sure, are you suggesting we should create a new shared module
> > called library.js and include a helper method to enable the bookmarks
> > toolbar?
> 
> No, this was meant like: a module under lib. I haven't proposed a name.

In this case will be toolbars.js since we are enabling the bookmarks toolbar
Comment 34 Henrik Skupin (:whimboo) 2012-07-24 04:03:44 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #32)
> I could not agree more. So I will move on and make the changes

Please wait for Dave what he thinks about.
Comment 35 Maniac Vlad Florin (:vladmaniac) 2012-07-24 04:28:44 PDT
(In reply to Henrik Skupin (:whimboo) from comment #34)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #32)
> > I could not agree more. So I will move on and make the changes
> 
> Please wait for Dave what he thinks about.

Alright, Dave, can you please give your input re comment 32?
Comment 36 Maniac Vlad Florin (:vladmaniac) 2012-07-24 06:00:25 PDT
> 
> >+      controller.rightClick(page);
> >+
> >+      var openInNewTab = new elementslib.ID(controller.window.document, 
> >+                                            "placesContext_open:newtab");
> >+      controller.click(openInNewTab);
> 
> You want to use the Menu API here which also handles context menus.
> 
Is this applicable here? I was browsing into the menu API in Mozmill and seen that the API works for the firefox main menu

MozMillController.prototype.__defineGetter__("mainMenu", function () {
  return this.getMenu("menubar");
});

In this case we are talking about the bookmarks toolbar folder menu 
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#451

My code is 

var page = new elementslib.Selector(controller.window.document, 
                                    "*[label='Test Bookmark " + 
                                    enduranceManager.currentEntity + "']");

controller.mainMenu.select("#placesContext_open:newtab", page);

Can you help?
Comment 37 Henrik Skupin (:whimboo) 2012-07-24 06:42:47 PDT
You should check at least testTabbedBrowsing_PinAndUnpinAppTab which is already making use of the Menu API.
Comment 38 Dave Hunt (:davehunt) 2012-07-24 07:54:55 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #35)
> (In reply to Henrik Skupin (:whimboo) from comment #34)
> > (In reply to Maniac Vlad Florin (:vladmaniac) from comment #32)
> > > I could not agree more. So I will move on and make the changes
> > 
> > Please wait for Dave what he thinks about.
> 
> Alright, Dave, can you please give your input re comment 32?

I absolutely agree that the setup should use backed API calls and not the UI.
Comment 39 Dave Hunt (:davehunt) 2012-07-24 07:59:28 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #25)
> (In reply to Remus Pop (:RemusPop) from comment #20)
> > We will need the last tab to be an empty one in the case the test is run
> > with 1 entity because if 1 tab is opened, bookmark all tabs option is not
> > available.
> 
> There will always be at least 2 tabs because tabBrowser.closeAllTabs()
> closes the tabs and leaves and empty one. Then we filter our bookmarked
> pages by title and open and close them, so no need to treat the case for
> minimum entities

This is still relevant if we use the UI to bookmark all tabs as the option is not available for < 2 tabs. If we use a backend API to add the bookmarks then this is not relevant.
Comment 40 Maniac Vlad Florin (:vladmaniac) 2012-07-25 06:08:32 PDT
Created attachment 645738 [details] [diff] [review]
patch v2.0

Fixed and all changes addressed 

Note: For using the menu API, needed to add escape char '\' because of the pseudo selector in the dropdown menu, please see this example for clarifications

http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/places/places.css#197 

the problem was the ":" char in the id 

patch works as expected 
http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b16e91e
Comment 41 Henrik Skupin (:whimboo) 2012-07-25 09:46:53 PDT
Comment on attachment 645738 [details] [diff] [review]
patch v2.0

>+var domUtils = require("../../../lib/dom-utils"); 
>+var modalDialog = require("../../../lib/modal-dialog");

I think both modules can now be removed.

>+    var testFolder = new elementslib.Selector(controller.window.document, 
>+                                              ".bookmark-item[label='testFolder']");

Can we make 'testFolder' a global constant? It's getting used on multiple places across functions.

>+    enduranceManager.loop(function () {
>+      var contextMenu = controller.getMenu("#placesContext");
>+      var page = new elementslib.Selector(controller.window.document, 
>+                                          "*[label='Test Bookmark " + 
>+                                          enduranceManager.currentEntity + "']");  
>+     
>+      contextMenu.select("#placesContext_open\\\:newtab", page);

Hm, on OS X you actually don't have the change to open the context menu of a bookmark's toolbar sub folder. So that test would not that be accurate on this platform. Please x-check if I'm wrong here. 

>+      enduranceManager.addCheckpoint("Bookmarked page no: " +  
>+                                     enduranceManager.currentEntity + 
>+                                     " was opened");
>+    });
>+    // Close all tabs

nit: emptly line please to separate blocks.

>+function setupBookmarks(controller) {

nit: aController

Otherwise looks way better now! I haven't tested this patch. I would leave this up for Dave as owner of the testrun.
Comment 42 Dave Hunt (:davehunt) 2012-07-25 14:44:11 PDT
Comment on attachment 645738 [details] [diff] [review]
patch v2.0

>Bug 705284 - Mozmill endurance test for bookmarking all tabs and opening and closing them. r=hskupin, r=dhunt

We no longer bookmark all tabs, but seed a folder with bookmarks. A better commit message would be 'Mozmill endurance test opening and closing bookmarks'

>
>diff --git a/lib/toolbars.js b/lib/toolbars.js
>--- a/lib/toolbars.js
>+++ b/lib/toolbars.js
>@@ -564,13 +564,32 @@ locationBar.prototype = {
>    *        Text to enter into the location bar
>    */
>   type : function locationBar_type(text) {
>     this._controller.type(this.urlbar, text);
>     this.contains(text);
>   }
> }
> 
>+/**
>+ * Enable bookmarks toolbar
>+ *
>+ * @param {MozmillController} aController
>+ *        MozMillController of the window to operate on
>+ */
>+function enableBookmarksToolbar(aController) { 
>+  var navbar = new elementslib.ID(aController.window.document, "nav-bar");
>+
>+  aController.rightClick(navbar, navbar.getNode().boxObject.width / 2, 2);
>+
>+  var toggle = new elementslib.ID(aController.window.document,
>+                                  "toggle_PersonalToolbar");
>+  aController.mouseDown(toggle);
>+  aController.mouseUp(toggle);
>+}
>+
> // Export of classes
> exports.locationBar = locationBar;
> exports.editBookmarksPanel = editBookmarksPanel;
> exports.autoCompleteResults = autoCompleteResults;
> 
>+// Export of functions
>+exports.enableBookmarksToolbar = enableBookmarksToolbar;
>diff --git a/tests/endurance/manifest.ini b/tests/endurance/manifest.ini

>+[include:testBookmarks_OpenAndCloseAllTabs/manifest.ini]

Rename this to simply testBookmarks_OpenAndClose

>+var prefs = require("../../../lib/prefs");

>+const PREF_TAB_NUMBER_WARNING = "browser.tabs.maxOpenBeforeWarn";

>+  // Do not warn about max opened tab number
>+  prefs.preferences.setPref(PREF_TAB_NUMBER_WARNING, 
>+                            enduranceManager.entities + 1);

We shouldn't need these as we're opening the bookmarks one by one.

>+/* 
>+ * Test open all bookmarks one by one and close all  
>+ */
>+function testOpenAndCloseAllBookmarks() {
>+  enduranceManager.run(function () {
>+    var testFolder = new elementslib.Selector(controller.window.document, 
>+                                              ".bookmark-item[label='testFolder']");
>+
>+    controller.click(testFolder);      

Remove the trailing whitespace.

>+
>+    enduranceManager.loop(function () {
>+      var contextMenu = controller.getMenu("#placesContext");                 

Remove the trailing whitespace.

>+      var page = new elementslib.Selector(controller.window.document, 
>+                                          "*[label='Test Bookmark " + 
>+                                          enduranceManager.currentEntity + "']");  
>+     
>+      contextMenu.select("#placesContext_open\\\:newtab", page);
>+      controller.waitForPageLoad();
>+      enduranceManager.addCheckpoint("Bookmarked page no: " +  
>+                                     enduranceManager.currentEntity + 
>+                                     " was opened");

The checkpoint will already contain the iteration and entity, so simply 'Bookmark opened' would be enough here.

>+    });
>+    // Close all tabs
>+    tabBrowser.closeAllTabs();

Again, this comment is redundant.

>+    enduranceManager.addCheckpoint("All tabs are closed");
>+  }); 
>+}
>+
>+/*
>+ * Insert bookmarks in a custom folder under Bookmarks Toolbar  
>+ */
>+function setupBookmarks(controller) {
>+  // Enable bookmarks toolbar 
>+  toolbars.enableBookmarksToolbar(controller);

This comment seems unnecessary.

Also, this test is failing for me locally with just a single entity/iteration. The error message is "could not find element Selector: .bookmark-item[label='testFolder']"
Comment 43 Dave Hunt (:davehunt) 2012-07-25 14:45:12 PDT
Sorry for the noise in the above comment. I submitted before trimming.
Comment 44 Henrik Skupin (:whimboo) 2012-07-25 15:16:31 PDT
(In reply to Dave Hunt (:davehunt) from comment #42)
> Also, this test is failing for me locally with just a single
> entity/iteration. The error message is "could not find element Selector:
> .bookmark-item[label='testFolder']"

That could be related to what I have mentioned in comment 41. I think we can't run this test on OS X.
Comment 45 Maniac Vlad Florin (:vladmaniac) 2012-07-25 23:30:03 PDT
Created attachment 646043 [details] [diff] [review]
patch v2.1

* fixed nits and change requests
* skipping the test if OS = mac, with confirmation of Dave's error on Mac OS X systems - we can't make usage of this test on mac platform
Comment 46 Henrik Skupin (:whimboo) 2012-07-25 23:31:03 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #45)
> * skipping the test if OS = mac, with confirmation of Dave's error on Mac OS
> X systems - we can't make usage of this test on mac platform

Have you tried to reproduce this issue on your own on OS X?
Comment 47 Maniac Vlad Florin (:vladmaniac) 2012-07-25 23:32:09 PDT
(In reply to Henrik Skupin (:whimboo) from comment #46)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #45)
> > * skipping the test if OS = mac, with confirmation of Dave's error on Mac OS
> > X systems - we can't make usage of this test on mac platform
> 
> Have you tried to reproduce this issue on your own on OS X?

Yes, got the exact same error
Comment 48 Henrik Skupin (:whimboo) 2012-07-26 04:01:52 PDT
Hm, that failure isn't caused by trying to access a context menu entry. In which iteration does it happen?
Comment 49 Maniac Vlad Florin (:vladmaniac) 2012-07-26 04:03:36 PDT
(In reply to Henrik Skupin (:whimboo) from comment #48)
> Hm, that failure isn't caused by trying to access a context menu entry. In
> which iteration does it happen?

in the first iteration
Comment 50 Maniac Vlad Florin (:vladmaniac) 2012-07-26 04:09:34 PDT
@resource://mozmill/modules/frame.js:687\n@resource://jsbridge/modules/server.js:184\n@resource://jsbridge/modules/server.js:188\n@resource://jsbridge/modules/server.js:288\n", "message": ": could not find element Selector: .bookmark-item[label='testFolder']"
Comment 51 Henrik Skupin (:whimboo) 2012-07-26 04:25:19 PDT
In that case it's not the problem I was talking about and it needs further investigation. Just skipping the test for OS X is not the right solution at this point.
Comment 52 Maniac Vlad Florin (:vladmaniac) 2012-07-26 06:09:37 PDT
(In reply to Henrik Skupin (:whimboo) from comment #51)
> In that case it's not the problem I was talking about and it needs further
> investigation. Just skipping the test for OS X is not the right solution at
> this point.

    var testFolder = new elementslib.Selector(controller.window.document, 
                                              ".bookmark-item[label='" + 
                                              BOOKMARK_FOLDER_NAME + "']");

    controller.click(testFolder);

so replaced click with waitThenClick and does not fail anymore for me. 
http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b23c67b
Comment 53 Maniac Vlad Florin (:vladmaniac) 2012-07-26 06:10:09 PDT
(In reply to Henrik Skupin (:whimboo) from comment #51)
> In that case it's not the problem I was talking about and it needs further
> investigation. Just skipping the test for OS X is not the right solution at
> this point.

And yes, we do not need to skip the test on OS X. Yey!
Comment 54 Henrik Skupin (:whimboo) 2012-07-26 06:28:33 PDT
In that case I think it's because the popup gets populated during onshow. And in some cases it can take a bit longer. So yeah, would make sense to me.

Would you mind testing on a heavily loaded system too?
Comment 55 Maniac Vlad Florin (:vladmaniac) 2012-07-27 02:16:39 PDT
Nightly: http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b2c46fc
Aurora: http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b23fa00 

Its a pass, I think we did it, so I'm going to just upload the patch and we'll not going to skip it on mac os x, which is great news
Comment 56 Maniac Vlad Florin (:vladmaniac) 2012-07-27 02:21:25 PDT
Created attachment 646494 [details] [diff] [review]
patch v3.0

* fixed change requests
* not skipping for mac os x
* changed click with waitThenClick to fix intermittent failure
Comment 57 Dave Hunt (:davehunt) 2012-07-27 06:41:13 PDT
Comment on attachment 646494 [details] [diff] [review]
patch v3.0

>+var prefs = require("../../../lib/prefs");

This is not used.

>+      contextMenu.select("#placesContext_open\\\:newtab", page);

You should open in the current tab, and if this is not the first entity then open a new tab before opening the bookmark. This means we use all the tabs, and you can see examples of this in other endurance tests.

>+    // Bookmark page and save in a custom folder 
>+    places.bookmarksService.insertBookmark(customFolder, URI, defaultIndex,  
>+                                           "Test Bookmark " + 
>+                                           enduranceManager.currentEntity);

You're not using enduranceManager.loop, so currentEntity will not be incremented. Use i instead.

Have you tried this with a large number of bookmarks? I would be interested to know what happens when there are enough to cause scrolling in the bookmarks menu. With the code as it currently is, you're always opening the first bookmark.
Comment 58 Maniac Vlad Florin (:vladmaniac) 2012-07-27 06:47:34 PDT
(In reply to Dave Hunt (:davehunt) from comment #57)
> Comment on attachment 646494 [details] [diff] [review]
> patch v3.0
> 
> >+var prefs = require("../../../lib/prefs");
> 
> This is not used.

Ops, forgot to kill that

> 
> >+      contextMenu.select("#placesContext_open\\\:newtab", page);
> 
> You should open in the current tab, and if this is not the first entity then
> open a new tab before opening the bookmark. This means we use all the tabs,
> and you can see examples of this in other endurance tests.
> 

Not sure if I get the picture here, but the test requires we open a new tab from the context menu. 

> >+    // Bookmark page and save in a custom folder 
> >+    places.bookmarksService.insertBookmark(customFolder, URI, defaultIndex,  
> >+                                           "Test Bookmark " + 
> >+                                           enduranceManager.currentEntity);
> 
> You're not using enduranceManager.loop, so currentEntity will not be
> incremented. Use i instead.
> 
> Have you tried this with a large number of bookmarks? I would be interested
> to know what happens when there are enough to cause scrolling in the
> bookmarks menu. With the code as it currently is, you're always opening the
> first bookmark.

Yes this is a bad one and was forgotten due to the lots of refactoring I did here
Comment 59 Dave Hunt (:davehunt) 2012-07-27 06:50:45 PDT
> Not sure if I get the picture here, but the test requires we open a new tab
> from the context menu. 

Where are you getting that requirement from?
Comment 60 Maniac Vlad Florin (:vladmaniac) 2012-07-27 07:10:29 PDT
(In reply to Dave Hunt (:davehunt) from comment #59)
> > Not sure if I get the picture here, but the test requires we open a new tab
> > from the context menu. 
> 
> Where are you getting that requirement from?

The test is about opening bookmarks one by one from the bookmarks toolbar, and from a folder created there, please see previous comments in this bug. this implies using the context menu, simulating a user-action.  If that changed, I am not aware of atm
Comment 61 Dave Hunt (:davehunt) 2012-07-27 07:27:28 PDT
The test is simply about opening bookmarks one by one in separate tabs. The bookmarks toolbar and folder is an implementation choice (I personally expected this to use the main menu, but that isn't likely to make much difference). I don't see how a context menu was ever implied, and it certainly shouldn't be necessary.
Comment 62 Maniac Vlad Florin (:vladmaniac) 2012-07-27 07:58:11 PDT
(In reply to Dave Hunt (:davehunt) from comment #61)
> The test is simply about opening bookmarks one by one in separate tabs. The
> bookmarks toolbar and folder is an implementation choice (I personally
> expected this to use the main menu, but that isn't likely to make much
> difference). I don't see how a context menu was ever implied, and it
> certainly shouldn't be necessary.

Glad we settled it on good old iRC aka "problem fixer" 
In the meantime, it seems like the scrollable bookmarks toolbar menu is not a problem for the test 
http://mozmill-crowd.blargon7.com/#/endurance/report/a6839eac670d27cd915461a1da0150dd

Ran with 2 iterations and 40 entities, meaning 40 bookmarks, just enough to trigger the scrollbars
Comment 63 Maniac Vlad Florin (:vladmaniac) 2012-07-27 08:00:42 PDT
Created attachment 646587 [details] [diff] [review]
patch v4.0

Changes addressed 
See previous comment for test results
We do not use context menu anymore, but open a new tab via tabBrowser.openTab() helper method
Comment 64 Dave Hunt (:davehunt) 2012-07-27 08:27:59 PDT
Comment on attachment 646587 [details] [diff] [review]
patch v4.0

This is looking good. Just a few minor things left.

>+    controller.waitThenClick(testFolder);

I feel this should be inside the loop (sorry for not picking this up before).

>+    enduranceManager.loop(function () {              

There's still trailing whitespace here.

>+      var page = new elementslib.Selector(controller.window.document, 
>+                                          "*[label='Test Bookmark " + 
>+                                          enduranceManager.currentEntity + "']");

I think we should call this bookmark rather than page.

>+    // Dismiss the context menu
>+    controller.keypress(null , 'VK_ESCAPE', {});

I'm surprised that the click doesn't dismiss the menu... Can we also move this into the loop so the menu is closed between entities? Also, this isn't a context menu, so simply drop that from the comment.

>+    tabBrowser.closeAllTabs();
>+    enduranceManager.addCheckpoint("All tabs are closed");

Let's drop this checkpoint, as it will be immediately followed by an iteration end checkpoint anyway.

>+function setupBookmarks(aController) { 
>+  toolbars.enableBookmarksToolbar(aController);
>+  

Remove this whitespace. Blank lines should be empty.
Comment 65 Maniac Vlad Florin (:vladmaniac) 2012-07-30 00:08:43 PDT
Created attachment 647086 [details] [diff] [review]
patch v4.1

There is no one available at this hour to review my patch internally but judging there were simple changes to address, moving on with r from Dave 

This was tested on the default branch since we are going to land it there first
http://mozmill-crowd.blargon7.com/#/endurance/report/a6839eac670d27cd915461a1da19ed24
Comment 66 Dave Hunt (:davehunt) 2012-07-30 05:08:15 PDT
Comment on attachment 647086 [details] [diff] [review]
patch v4.1

>+      controller.click(bookmark);
>+      controller.waitForPageLoad();
>+
>+      tabBrowser.openTab();

Sorry for not spotting this before, but we only want to open a new tab ahead of using it. As it is, we will open a new tab that is not used. See other endurance tests that open a new tab only in this is not the first entity:

  if (enduranceManager.currentEntity > 1) {
    tabBrowser.openTab();
  }

Otherwise, this looks good to me. I would like Henrik to take a look over this patch though, as I would expect the click on the menu would be necessary to click the menu item (it seems it is not), and the click on the menu item should really dismiss the menu.

Please update the patch and request final review from Henrik. Also, please check your patch for unnecessary whitespace.
Comment 67 Maniac Vlad Florin (:vladmaniac) 2012-07-30 06:52:33 PDT
Created attachment 647142 [details] [diff] [review]
patch v4.2

* change addressed 
* no more trailing spaces (hope so...) 

local testrun report
http://mozmill-crowd.blargon7.com/#/endurance/report/1cdc75cbd62095d2665556bbf400ecb1
Comment 68 Maniac Vlad Florin (:vladmaniac) 2012-07-30 07:43:48 PDT
Created attachment 647164 [details] [diff] [review]
patch v4.3

Yes, the 'ESC' is not necessary any more. 
Killed that code in this new patch
Comment 69 Dave Hunt (:davehunt) 2012-07-30 08:32:28 PDT
Comment on attachment 647164 [details] [diff] [review]
patch v4.3

I think you've misunderstood me. The escape key should not be necessary because the bookmark folder menu should be hidden after a click on an individual bookmark. Can you please confirm that this is not the case? We may have an issue with clicking menu items where it is not effectively simulating a user click. I was only expecting the open tab fix and whitespace fixes, and hoping for feedback from Henrik on the menu click concerns.
Comment 70 Maniac Vlad Florin (:vladmaniac) 2012-07-31 01:06:17 PDT
(In reply to Dave Hunt (:davehunt) from comment #69)
> Comment on attachment 647164 [details] [diff] [review]
> patch v4.3
> 
> I think you've misunderstood me. The escape key should not be necessary
> because the bookmark folder menu should be hidden after a click on an
> individual bookmark. Can you please confirm that this is not the case? We
> may have an issue with clicking menu items where it is not effectively
> simulating a user click. I was only expecting the open tab fix and
> whitespace fixes, and hoping for feedback from Henrik on the menu click
> concerns.

This is happening in all mozmill tests which use menus, not only in this one, and afair, it has always worked this way. Henrik can provide further clarifications to you, I'm sure.
Comment 71 Maniac Vlad Florin (:vladmaniac) 2012-07-31 01:09:19 PDT
Created attachment 647465 [details] [diff] [review]
patch v4.4

* using esc key to dismiss the menu is back
Comment 72 Dave Hunt (:davehunt) 2012-07-31 04:19:08 PDT
Comment on attachment 647465 [details] [diff] [review]
patch v4.4

>+      controller.waitThenClick(testFolder);
>+
>+      var bookmark = new elementslib.Selector(controller.window.document,
>+                                              "*[label='Test Bookmark " +
>+                                              enduranceManager.currentEntity + "']");
>+
>+      controller.click(bookmark);
>+      controller.waitForPageLoad();
>+
>+      // Dismiss the menu
>+      controller.keypress(null , 'VK_ESCAPE', {});

Clint: Is it expected that a click on a menu item (this is from a bookmark folder from the bookmarks toolbar) would not dismiss the menu as it would when an actual user clicks the item? It also appears that the menu doesn't even need to be open in order for the child item to be clicked. If this is a Mozmill limitation then I'm happy with this new test.
Comment 73 cmtalbert 2012-08-02 13:28:58 PDT
Comment on attachment 647465 [details] [diff] [review]
patch v4.4

(In reply to Dave Hunt (:davehunt) from comment #72)
> Comment on attachment 647465 [details] [diff] [review]
> patch v4.4
> 
> >+      controller.waitThenClick(testFolder);
> >+
> >+      var bookmark = new elementslib.Selector(controller.window.document,
> >+                                              "*[label='Test Bookmark " +
> >+                                              enduranceManager.currentEntity + "']");
> >+
> >+      controller.click(bookmark);
> >+      controller.waitForPageLoad();
> >+
> >+      // Dismiss the menu
> >+      controller.keypress(null , 'VK_ESCAPE', {});
> 
> Clint: Is it expected that a click on a menu item (this is from a bookmark
> folder from the bookmarks toolbar) would not dismiss the menu as it would
> when an actual user clicks the item? It also appears that the menu doesn't
> even need to be open in order for the child item to be clicked. If this is a
> Mozmill limitation then I'm happy with this new test.

I think this test is fine. I think that mozmill should close the menu after simulating the click on it. If that isn't happening then it's a bug and should be opened separately. But I don't see why we'd block this test on that mozmill bug when simply simulating the ESC keypress is a simple, logical work around.  I'd just file the bug and add the bug number in a comment before the ESC press and get this test checked in.
Comment 74 Dave Hunt (:davehunt) 2012-08-03 03:49:55 PDT
Sounds good to me. Vlad: can you raise the bug as Clint suggests (preferably with a minimal failing testcase), and add an appropriate comment to your patch. Thanks.
Comment 75 Maniac Vlad Florin (:vladmaniac) 2012-08-03 04:05:56 PDT
Created attachment 648661 [details] [diff] [review]
patch v4.5 [checked-in]

* Comment dismissing of menus with ESC key since Mozmill does not do that atm
Comment 76 Dave Hunt (:davehunt) 2012-08-03 05:01:47 PDT
Comment on attachment 648661 [details] [diff] [review]
patch v4.5 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/6f4ed9189c7e (default)
Comment 77 Maniac Vlad Florin (:vladmaniac) 2012-08-06 00:00:41 PDT
I see no failures on default branch so I think we can check this test in for all branches 

http://mozmill-ci.blargon7.com/#/endurance/report/29fc09ba0c8360d637617903a01aeb83
Comment 78 Dave Hunt (:davehunt) 2012-08-06 01:37:53 PDT
Transplanted as:
http://hg.mozilla.org/qa/mozmill-tests/rev/55f1cae88f3c (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/f8a36a23422d (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/d29c59049754 (release)

Please provide a patch for mozilla-esr10 as this patch does not apply cleanly.
Comment 79 Henrik Skupin (:whimboo) 2012-08-06 01:54:41 PDT
(In reply to Dave Hunt (:davehunt) from comment #64)
> >+    // Dismiss the context menu
> >+    controller.keypress(null , 'VK_ESCAPE', {});
> 
> I'm surprised that the click doesn't dismiss the menu... Can we also move
> this into the loop so the menu is closed between entities? Also, this isn't
> a context menu, so simply drop that from the comment.

Items on the bookmarks toolbar are implemented differently. Means those react on mousedown/mouseup and not click. We have had to make some workarounds already. But there was no need yet to make use of the Menu API together. So I'm not surprised that we do not close the popup. As Clint suggested lets get this fixed in bug
Comment 80 Maniac Vlad Florin (:vladmaniac) 2012-08-08 01:02:00 PDT
(In reply to Dave Hunt (:davehunt) from comment #78)
 
> Please provide a patch for mozilla-esr10 as this patch does not apply
> cleanly.

This might sound stupid but at the moment the test is failing on mozilla-esr10 branch only, with the error

"message": "Timeout exceeded for waitForElement Selector: *[label='Test Bookmark 1']", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 447}}]

This means, and can be easily seen, that the test does not click the bookmarks folder created under bookmarks toolbar. The weird thing is that the locator are the same for all branches so I cannot say what is wrong here. 
I've tested the WIP patch on many machines and all systems and its the same situation with Firefox 10.0.6
Comment 81 Maniac Vlad Florin (:vladmaniac) 2012-08-08 01:02:50 PDT
Created attachment 649986 [details] [diff] [review]
[mozilla-esr10] WIP patch

Attaching a WIP patch for mozilla-esr10
Comment 82 Henrik Skupin (:whimboo) 2012-08-08 01:33:27 PDT
Comment on attachment 649986 [details] [diff] [review]
[mozilla-esr10] WIP patch

There is really no need to request feedback on a patch which hasn't been changed according to the other branches. If it doesn't work, you should investigate the problem, and figure out why we do not click the element.
Comment 83 Maniac Vlad Florin (:vladmaniac) 2012-08-09 02:44:53 PDT
(In reply to Henrik Skupin (:whimboo) from comment #82)
> Comment on attachment 649986 [details] [diff] [review]
> [mozilla-esr10] WIP patch
> 
> There is really no need to request feedback on a patch which hasn't been
> changed according to the other branches. If it doesn't work, you should
> investigate the problem, and figure out why we do not click the element.

At the moment I have concluded to no real reason why esr behaves this way. 
What I did is: 

* Check the locators see if they somehow changed - not the case (used DOM Inspector and Inspect Context to check that) 
* Check with previous versions of Mozmill until 1.5.14 - same error
* Check if we can click other elements in the toolbar - we can 
* Minimize the test to the bare minimum in which we click a folder under the Bookmarks Toolbar - the test is a pass but the actual click does not happen 

It remains to check with previous versions of esr branch and find a possible regression range. This is my P1 after lunch.
Comment 84 Henrik Skupin (:whimboo) 2012-08-09 02:57:31 PDT
I would propose then to use controller.click() and add the expectedEvent as parameter. See: https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/controller/expected_events.js
Comment 85 Maniac Vlad Florin (:vladmaniac) 2012-08-09 06:00:44 PDT
(In reply to Henrik Skupin (:whimboo) from comment #84)
> I would propose then to use controller.click() and add the expectedEvent as
> parameter. See:
> https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/controller/
> expected_events.js

Thanks for the tip. 
I've used controller.click(testFolder, {type: "click"});
but had no luck identifying other error than the one mentioned
Comment 86 Henrik Skupin (:whimboo) 2012-08-09 06:14:19 PDT
By using expected events you should definitely get another error, directly from the click() method. One thing to say is that it doesn't have to be the click event. It could also be mousedown or whatever. Better you check the source of the bookmarks toolbar how folders behave, or ask in #places.
Comment 87 Maniac Vlad Florin (:vladmaniac) 2012-08-09 08:28:46 PDT
(In reply to Henrik Skupin (:whimboo) from comment #86)
> By using expected events you should definitely get another error, directly
> from the click() method. One thing to say is that it doesn't have to be the
> click event. It could also be mousedown or whatever. Better you check the
> source of the bookmarks toolbar how folders behave, or ask in #places.

So the event is click, no doubt in that anymore. 
But after click, the folder does not expand - this can be easily verified by checking the 
"open" attribute

testFolder.getNode().hasAttribute("open") and this equals false all the time. 
It is very strange..
Comment 88 Dave Hunt (:davehunt) 2012-08-14 05:57:52 PDT
This sounds similar to bug 780107 but for the initial folder click. I'm not sure how much effort we should spend trying to get this working on ESR. Have you tried using mouseDown and mouseUp rather than click? Could you try previous versions of Firefox in case this is a regression?
Comment 89 Maniac Vlad Florin (:vladmaniac) 2012-08-16 04:27:39 PDT
(In reply to Dave Hunt (:davehunt) from comment #88)
> This sounds similar to bug 780107 but for the initial folder click. I'm not
> sure how much effort we should spend trying to get this working on ESR. Have
> you tried using mouseDown and mouseUp rather than click? Could you try
> previous versions of Firefox in case this is a regression?

works Dave! 

http://mozmill-crowd.blargon7.com/#/endurance/report/d87d47fd1034f072b9bece6ee6452a7e
Comment 90 Maniac Vlad Florin (:vladmaniac) 2012-08-16 04:29:01 PDT
Created attachment 652394 [details] [diff] [review]
[mozilla-esr10] patch v1.0 [checked-in]

* fixed
Comment 91 Maniac Vlad Florin (:vladmaniac) 2012-08-16 04:30:42 PDT
Still I have no clue why click() won't work so what I did was just 'magic' code - see Dave's suggestion in comment 88.
Comment 92 Henrik Skupin (:whimboo) 2012-08-17 07:05:36 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #91)
> Still I have no clue why click() won't work so what I did was just 'magic'
> code - see Dave's suggestion in comment 88.

Which I have already mentioned in comment 79. So there could have been a change in how the bookmarks toolbar react on mouse clicks since Firefox 11.
Comment 93 Henrik Skupin (:whimboo) 2012-08-20 12:51:05 PDT
Comment on attachment 652394 [details] [diff] [review]
[mozilla-esr10] patch v1.0 [checked-in]

I think it's fine but I will leave it up to Dave for the final decision.
Comment 94 Dave Hunt (:davehunt) 2012-08-21 02:10:53 PDT
Comment on attachment 652394 [details] [diff] [review]
[mozilla-esr10] patch v1.0 [checked-in]

Looks fine. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/4380ab2e050a

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