996530 - Add new PlacesOrganizer UI library

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

As discussed in bug 908649, we should have a new library as :
> firefox/lib/ui/places-organizer.js
> ** PlacesOrganizer class
that will handle the UI elements of the Library Window.

Let's call the class PlacesOrganizer.

Along with this we have to create 
> firefox/lib/browser.js
> ** Browser class
file with a Browser class. One of the method would be to open the library, and it should return an instance of the PlacesOrganizer class.

We can file new bugs to create tests for this. One or two of them can be to open & close the library using the localized shortcuts.

Comment 1

[Daniel Gherasim]

We already almost created this in bug 908649 so I'll take this bug.

Comment 2

[Daniel Gherasim]

Attachment: WIP1.patch

This is a WIP patch with this library and a test to open it.
I also moved widgets.js to it's correct location under lib/.
May need some feedback on this before continue working.

Thanks,

Comment 3

[Andrei Eftimie]

Comment on attachment 8408252 [details] [diff] [review]
WIP1.patch

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

I'm not sure about having browser.js which has a method for opening the places organizer window. I feel such a method should be in the places-organizer lib...

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +7,5 @@
> +// Include required modules
> +var browser = require("../ui/browser");
> +var placesOrganizer = require("../ui/places-organizer");
> +
> +var setupModule = function (aModule) {

Use function setupModule[...] (applies in multiple locations)

@@ +16,5 @@
> +var teardownModule = function (aModule) {
> +
> +}
> +
> +const XP_MACOSX = (mozmill.isMac || (mozmill.isWindows && (version < "6.0")));

Name this WINXP_OR_MACOSX (or maybe simpler XP_OR_OSX)

@@ +25,5 @@
> +  {type: "viewMenu", localName: ((XP_MACOSX) ? "toolbarbutton" : "menu")},
> +  {type: "maintenanceButton", localName: ((XP_MACOSX) ? "toolbarbutton" : "menu")},
> +  {type: "searchFilter", localName: "textbox"},
> +  {type: "tree", localName: "tree"}
> +];

These should be declared before setupModule

::: firefox/lib/ui/browser.js
@@ +50,5 @@
> +   *        Which pane to open to
> +   *
> +   * @returns {PlacesOrganizerWindow} Instance of an PlacesOrganizerWindow
> +   */
> +  openPlacesOrganizerWindow : function Browser_openPlacesOrganizerWindow(aSpec) {

Not sure. Shouldn't this be located in places-organizer.js ?

@@ +90,5 @@
> +
> +    switch (type) {
> +      case "shortcut":
> +        this._controller.keypress(null, spec.cmdKey, {accelKey: true,
> +                                                      shiftKey: true});

Use elem.keypress. We might need to reference the window for that.

@@ +101,5 @@
> +    }
> +
> +    var windowLoaded = false;
> +
> +    utils.handleWindow("type", "Places:Organizer", (aController) => {

This should work without parentheses.

::: firefox/lib/ui/places-organizer.js
@@ +191,5 @@
> +      return;
> +    }
> +    else {
> +      var cmdKey = utils.getEntity(this.dtds, "closeCmd.key");
> +      this._controller.keypress(null, cmdKey, {accelKey: true});

elem.keypress

@@ +195,5 @@
> +      this._controller.keypress(null, cmdKey, {accelKey: true});
> +    }
> +    // TODO: Rely on an observer of DOM change or some event
> +
> +    assert.waitFor(function () {

Fat arrow syntax.

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var browser = require("../../../lib/ui/browser");
> +var browser = require("../../../lib/ui/browser");

Duplicate line.

@@ +7,5 @@
> +// Include required modules
> +var browser = require("../../../lib/ui/browser");
> +var browser = require("../../../lib/ui/browser");
> +var utils = require("../../../lib/utils");
> +var widgets = require("../../../lib/widgets");

This currently fails. I assume the path is wrong.

@@ +74,5 @@
> +  var treeNode = libraryWindow.getElement({type: "tree"}).getNode();
> +  assert.equal(widgets.getSelectedCell(libraryWindow.controller, treeNode).title,
> +               "Today",
> +               "Library has been opened and focused on correct location");
> +  libraryWindow.close();

On OSX one of these tests fails. I'm not sure which one, but instead of the PlacesOrganizer window it opens the BrowserConsole. With en-US.

::: lib/widgets.js
@@ +50,5 @@
> + *        MozMillController of the browser window to operate on
> + * @param {tree} aTree
> + *        Tree to operate on
> + */
> +function getSelectedCell(aController, aTree) {

I see you don't need the controller at all.

Comment 4

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #3)
> Comment on attachment 8408252 [details] [diff] [review]
> WIP1.patch
> 
> Review of attachment 8408252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure about having browser.js which has a method for opening the
> places organizer window. I feel such a method should be in the
> places-organizer lib...

This is what Henrik suggested in :
https://bugzilla.mozilla.org/show_bug.cgi?id=908649#c9 (the 14th reply).

Comment 5

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

(In reply to daniel.gherasim from comment #4)
> > I'm not sure about having browser.js which has a method for opening the
> > places organizer window. I feel such a method should be in the
> > places-organizer lib...
> 
> This is what Henrik suggested in :
> https://bugzilla.mozilla.org/show_bug.cgi?id=908649#c9 (the 14th reply).

That's correct. Why should the places ui library know how to open itself? Keep in mind that only the browser window knows the proper shortcuts, menus, or whatever to do so.

Comment 6

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> (In reply to daniel.gherasim from comment #4)
> > > I'm not sure about having browser.js which has a method for opening the
> > > places organizer window. I feel such a method should be in the
> > > places-organizer lib...
> > 
> > This is what Henrik suggested in :
> > https://bugzilla.mozilla.org/show_bug.cgi?id=908649#c9 (the 14th reply).
> 
> That's correct. Why should the places ui library know how to open itself?
> Keep in mind that only the browser window knows the proper shortcuts, menus,
> or whatever to do so.

That makes the most sense to me. Architecturally we might end up with an overblown browser because _everything_ can be opened from the browser. And I don't see how this helps us manage code in the long run... I'm arguing for modularity and encapsulation.

Comment 7

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

Not everything. We will make sure to limit any methods and properties to only add those, which are really bound to the browser. Anything else will live somewhere else.

Comment 8

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

Comment on attachment 8408252 [details] [diff] [review]
WIP1.patch

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

Looks way better now and it goes into the right direction.

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +20,5 @@
> +const XP_MACOSX = (mozmill.isMac || (mozmill.isWindows && (version < "6.0")));
> +const ELEMENTS = [
> +  {type: "backButton", localName: "toolbarbutton"},
> +  {type: "forwardButton", localName: "toolbarbutton"},
> +  {type: "organizeButton", localName: ((XP_MACOSX) ? "toolbarbutton" : "menu")},

Why is that for a specific Windows version too? Is that related to how our theme for Aero was implemented?

::: firefox/lib/ui/browser.js
@@ +15,5 @@
> + * @constructor
> + * @param {MozMillController} controller
> + *        MozMillController of the Browser
> + */
> +function Browser(aController) {

BrowserWindow please.

@@ +16,5 @@
> + * @param {MozMillController} controller
> + *        MozMillController of the Browser
> + */
> +function Browser(aController) {
> +  this._controller = aController || mozmill.getBrowserController();

I like that. We could simplify our tests later and wrap the browser controller entirely.

@@ +46,5 @@
> +   *        Information for opening the window
> +   * @param {string} [aSpec.type="shortcut"]
> +   *        How to open the Library Window
> +   * @param {location} [aSpec.location="bookmarks"]
> +   *        Which pane to open to

Best would be a callback like for other open methods, so the calling code can decide what to use here. But then we might need improvements in handling dtd entities and properties first. We can do that later.

@@ +55,5 @@
> +    var spec = aSpec || {};
> +    var type = spec.type || "shortcut";
> +    var location = spec.location || "bookmarks";
> +
> +    var placesOrganizerWindow = null;

Please declare it when needed.

@@ +90,5 @@
> +
> +    switch (type) {
> +      case "shortcut":
> +        this._controller.keypress(null, spec.cmdKey, {accelKey: true,
> +                                                      shiftKey: true});

Or we can route through the controller.mainMenu instance?

@@ +106,5 @@
> +      placesOrganizerWindow = new placesOrganizer.PlacesOrganizerWindow(aController);
> +      windowLoaded = true;
> +    }, false);
> +
> +    assert.waitFor(() => windowLoaded, "Library window has been opened");

This is not necessary. The loaded state is handled internally by handleWindow.

::: firefox/lib/ui/places-organizer.js
@@ +21,5 @@
> +function PlacesOrganizerWindow(aController) {
> +  assert.ok(aController, "A valid controller has to be specified");
> +
> +  this._controller = aController;
> +  this._tree = this.getElement({type: "tree"}).getNode();

This is not used yet, so why do we need it here?

@@ +95,5 @@
> +   * @returns {ElemBase} Element which has been found
> +   */
> +  getElements : function libraryManager_getElements(aSpec) {
> +    var spec = aSpec || {};
> +    var parent = spec.parent || this.controller;

Parent should be a node but not the controller.

@@ +114,5 @@
> +        elems = [new findElement.ID(this.controller.window.document,
> +                                    "downloadsRichListBox")];
> +        break;
> +      case "download":
> +        assert.ok(spec.value, "Item's value has been specified");

You want to be more specific with the message. It's unclear what it expects.

@@ +185,5 @@
> +
> +    // TODO: assert if window is not opened
> +
> +    // Check if we should force the closing of the window
> +    if (aForce && this._controller.window) {

The second part is not a valid check. It will always be true. Check for .closed instead.

@@ +193,5 @@
> +    else {
> +      var cmdKey = utils.getEntity(this.dtds, "closeCmd.key");
> +      this._controller.keypress(null, cmdKey, {accelKey: true});
> +    }
> +    // TODO: Rely on an observer of DOM change or some event

Correct. That's why we wanted to have a method to close a window in the windows.js module. That way we can share the code given that we might need this more often.

@@ +197,5 @@
> +    // TODO: Rely on an observer of DOM change or some event
> +
> +    assert.waitFor(function () {
> +      return mozmill.utils.getWindows().length === (windowCount - 1);
> +    }, "Library Window has been closed");

Better to check the .closed property of the window.

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +17,5 @@
> +  aModule.libraryWindow = null;
> +}
> +
> +var teardownModule = function(aModule) {
> +  libraryWindow.close(true);

I would still like to see a closeAllWindows() method.

@@ +43,5 @@
> +  assert.equal(widgets.getSelectedCell(libraryWindow.controller, treeNode).title,
> +               "Downloads",
> +               "Library has been opened and focused on correct location");
> +  libraryWindow.close();
> +

Instead of duplicating all this code you might want to add a test data driven test.

::: lib/widgets.js
@@ +48,5 @@
> + *
> + * @param {MozMillController} aController
> + *        MozMillController of the browser window to operate on
> + * @param {tree} aTree
> + *        Tree to operate on

This should be of MozElement type.

Comment 9

[Daniel Gherasim]

This will replace functional/testDownloading/ tests with a new ones for PlacesOrganizer so marking it as P2.

Comment 10

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 8408252 [details] [diff] [review]
> WIP1.patch
> 
> Review of attachment 8408252 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: firefox/lib/tests/testPlacesOrganizer.js
> @@ +20,5 @@
> > +const XP_MACOSX = (mozmill.isMac || (mozmill.isWindows && (version < "6.0")));
> > +const ELEMENTS = [
> > +  {type: "backButton", localName: "toolbarbutton"},
> > +  {type: "forwardButton", localName: "toolbarbutton"},
> > +  {type: "organizeButton", localName: ((XP_MACOSX) ? "toolbarbutton" : "menu")},
> 
> Why is that for a specific Windows version too? Is that related to how our
> theme for Aero was implemented?
> 

Yes. Different elements were defined for the places organizer on different platforms.
e.g.: http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.xul#156 

> 
> @@ +90,5 @@
> > +
> > +    switch (type) {
> > +      case "shortcut":
> > +        this._controller.keypress(null, spec.cmdKey, {accelKey: true,
> > +                                                      shiftKey: true});
> 
> Or we can route through the controller.mainMenu instance?
> 

Not sure what you're reffering here, we have the option to click on controller.mainMenu item in the next case statement.

> :::
> firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
> @@ +17,5 @@
> > +  aModule.libraryWindow = null;
> > +}
> > +
> > +var teardownModule = function(aModule) {
> > +  libraryWindow.close(true);
> 
> I would still like to see a closeAllWindows() method.
> 

Wouldn't be better to file a bug to implement this method, where it should live ?

Comment 11

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

(In reply to daniel.gherasim from comment #10)
> > > +        this._controller.keypress(null, spec.cmdKey, {accelKey: true,
> > > +                                                      shiftKey: true});
> > 
> > Or we can route through the controller.mainMenu instance?
> > 
> 
> Not sure what you're reffering here, we have the option to click on
> controller.mainMenu item in the next case statement.

We cannot use controller.keypress anymore. Instead this event has to be issued against the window. So you would have to create an element instance of the window first. To avoid that I was wondering if we could use the mainMenu instead for issuing the keypress against.

> > firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
> > @@ +17,5 @@
> > > +  aModule.libraryWindow = null;
> > > +}
> > > +
> > > +var teardownModule = function(aModule) {
> > > +  libraryWindow.close(true);
> > 
> > I would still like to see a closeAllWindows() method.
> > 
> 
> Wouldn't be better to file a bug to implement this method, where it should
> live ?

Haven't we already created a bug for a windows library module?

Comment 12

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Haven't we already created a bug for a windows library module?

We had one some time ago, but we decided there the scope was different, and we didn't ship any window lib. I'm not sure if we have filed another bug to get a window lib implemented

Comment 13

[Daniel Gherasim]

Attachment: fix_1.patch

Filed bug 1006996 for the windows lib.
Patch with the latest requested changes by Henrik & Andrei is up!

Comment 14

[Andrei Eftimie]

Comment on attachment 8418578 [details] [diff] [review]
fix_1.patch

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

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +7,5 @@
> +// Include required modules
> +var browser = require("../ui/browser");
> +var placesOrganizer = require("../ui/places-organizer");
> +
> +const XP_OR_OSX = (mozmill.isMac || (mozmill.isWindows && (version < "6.0")));

nit: you can remove the outermost parentheses.

@@ +11,5 @@
> +const XP_OR_OSX = (mozmill.isMac || (mozmill.isWindows && (version < "6.0")));
> +const ELEMENTS = [
> +  {type: "backButton", localName: "toolbarbutton"},
> +  {type: "forwardButton", localName: "toolbarbutton"},
> +  {type: "organizeButton", localName: ((XP_OR_OSX) ? "toolbarbutton" : "menu")},

Same here, you don't need to wrap every js expression with parentheses, they will be evaluated nonetheless.

@@ +33,5 @@
> +function testPlacesOrganizerWindow() {
> +  library = browserWindow.openPlacesOrganizerWindow();
> +
> +  // Test all available elements
> +  ELEMENTS.forEach(function (aElement) {

Fat arrow syntax please.

::: firefox/lib/ui/browser.js
@@ +33,5 @@
> +
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns {[String]} Array of external DTD urls

string[]

::: firefox/lib/ui/places-organizer.js
@@ +11,5 @@
> +var domUtils = require("../../../lib/dom-utils");
> +var utils = require("../utils");
> +
> +var observerService = Cc["@mozilla.org/observer-service;1"].
> +                      getService(Ci.nsIObserverService);

Please update the indentation as per the recent changes in the style guide: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Mozmill_Style_Guide#Components

@@ +41,5 @@
> +
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns {[String]} Array of external DTD urls

string[]

@@ +56,5 @@
> +   * @param {ElemBase} aDownload
> +   *        Download which state should be checked
> +   */
> +  getDownloadState : function downloadManager_getDownloadState(aDownload) {
> +    return aDownload.getAttribute('state');

nit: double quotes please

@@ +62,5 @@
> +
> +  /**
> +   * Retrieve an UI element based on the given specification
> +   *
> +   * @param {Object} aSpec

nit: please use lowercase for native types

@@ +84,5 @@
> +
> +  /**
> +   * Retrieve an UI element based on the given specification
> +   *
> +   * @param {Object} aSpec

Same here

@@ +172,5 @@
> +   *
> +   * @param {string} aProperty
> +   *        Requested propriety name
> +   *
> +   * @returns

Missing description

@@ +182,5 @@
> +
> +  /**
> +   * Close the Places Organizer Window
> +   *
> +   * @param {Boolean} aForce

lowercase type

@@ +187,5 @@
> +   *         Force closing the window
> +   *
> +   */
> +  close : function placesOrganizerWindow_close(aForce) {
> +    if(this._controller.window.closed) {

nit: missing space after `if`

@@ +193,5 @@
> +    }
> +
> +    var windowClosed = false;
> +    var observer = {
> +      observe: function(aSubject, aTopic, aData) {

nit: missing space after `function`

@@ +210,5 @@
> +        this.getElement({type: "window"}).keypress(cmdKey, {accelKey: true});
> +      }
> +
> +      // Wait for it to be finished
> +      assert.waitFor(function () {

Fat arrow syntax please.

@@ +215,5 @@
> +        return windowClosed;
> +      }, "Places Organizer window has been closed");
> +    }
> +    catch (ex) {
> +        assert.fail("Places Organizer Window has been closed.");

nit: indentation

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +19,5 @@
> +  },{
> +    location: "history",
> +    type: ["shortcut","menu"],
> +    title: "Today"
> +  }];

I think some shortcuts might have changed. Instead of the PlacesOrganizer window I get the Browser Console opened when running this test on OSX.

@@ +38,5 @@
> + * Test opening the places organizer
> + */
> +var testOpenClosePlacesOrganizer = function() {
> +  METHODS.forEach((aValue, aIndex) => {
> +    aValue.type.forEach( aType => {

nit: there is an extra space before aType

@@ +44,5 @@
> +                                                               type: aType});
> +      var treeNode = libraryWindow.getElement({type: "tree"});
> +      assert.equal(widgets.getSelectedCell(treeNode).title,
> +               aValue.title,
> +               "Library has been opened and focused on correct location");

Indentation.

::: lib/widgets.js
@@ +19,2 @@
>   *        Tree to operate on
> + * @param {number } aRowIndex

nit: extra space

@@ +38,1 @@
>                                             x, y, width, height);

nit: indent

Comment 15

[Daniel Gherasim]

Attachment: fix_2.patch

Shortcuts differ from a platform to another so on mac & xp we don't have shortcuts for opening library to specific locations like downloads & history anymore, but shortcuts for opening Developer Tools are implemented now.

I fixed the patch to work on all platforms now.

Comment 16

[Daniel Gherasim]

Attachment: fix_2.1.patch

Fixed some nits.

Comment 17

[Andrei Eftimie]

Comment on attachment 8420944 [details] [diff] [review]
fix_2.1.patch

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

Just a few changes. Looks really good otherwise.

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +10,5 @@
> +var browser = require("../ui/browser");
> +var placesOrganizer = require("../ui/places-organizer");
> +
> +const VERSION = Services.sysinfo.getProperty("version");
> +const XP_OR_OSX = mozmill.isMac || (mozmill.isWindows && (parseFloat(version) < "6.0"));

You can remove both parentheses and since we're comparing Numbers we should also supply the comparator as a Number, not as a string.
Also it should be VERSION.
And I would move the transform to float in the above assignment.
> mozmill.isMac || mozmill.isWindows && VERSION < 6.0;

@@ +30,5 @@
> +  aModule.library = null;
> +}
> +
> +function teardownModule(aModule) {
> +  library.close(true);

aModule.library

::: firefox/lib/ui/browser.js
@@ +1,1 @@
> +  /* This Source Code Form is subject to the terms of the Mozilla Public

nit: extra space

::: firefox/lib/ui/places-organizer.js
@@ +41,5 @@
> +
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns {string[]]} Array of external DTD urls

nit: there is an extra `]` character

@@ +193,5 @@
> +    }
> +
> +    var windowClosed = false;
> +    var observer = {
> +      observe: function (aSubject, aTopic, aData) {

Fat arrow syntax. Do we also need to declare all arguments even if we don't use them?

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +10,5 @@
> +var browser = require("../../../lib/ui/browser");
> +var widgets = require("../../../../lib/widgets");
> +
> +const VERSION = Services.sysinfo.getProperty("version");
> +const XP_OR_OSX = mozmill.isMac || (mozmill.isWindows && (parseFloat(VERSION) < "6.0"));

Same comments for the version as the other test.

@@ +14,5 @@
> +const XP_OR_OSX = mozmill.isMac || (mozmill.isWindows && (parseFloat(VERSION) < "6.0"));
> +
> +const METHODS = [{
> +    location: "downloads",
> +    type:  (!mozmill.isLinux) ? ["menu"] : ["shortcut", "menu"],

nit: spacing. you can also remove the parentheses around the ternary condition
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Conditional_Operator

@@ +22,5 @@
> +    type: ["shortcut","menu"],
> +    title: "All Bookmarks"
> +  },{
> +    location: "history",
> +    type: (XP_OR_OSX) ? ["menu"] : ["shortcut", "menu"],

Same here

@@ +34,5 @@
> +  aModule.libraryWindow = null;
> +}
> +
> +var teardownModule = function(aModule) {
> +  libraryWindow.close(true);

aModule.libraryWindow

Comment 18

[Daniel Gherasim]

Attachment: fix_2.2.patch

Patch updated based on previous review.

Comment 19

[Andrei Eftimie]

Comment on attachment 8424782 [details] [diff] [review]
fix_2.2.patch

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

Looks good to me.

Henrik, please have a look at this patch.

Comment 20

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

Comment on attachment 8424782 [details] [diff] [review]
fix_2.2.patch

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

As you can see in the amount of review comments here, there are way too many things remaining. I don't understand why most of them haven't been already caught earlier by a peer review.

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +23,5 @@
> +  {type: "tree", localName: "tree"}
> +];
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();

You may want to remove this line. It's not needed.

@@ +26,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.browserWindow = new browser.BrowserWindow();
> +
> +  aModule.library = null;

Better name here?

@@ +30,5 @@
> +  aModule.library = null;
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.library.close(true);

You want to do a null check first.

@@ +34,5 @@
> +  aModule.library.close(true);
> +}
> +
> +function testPlacesOrganizerWindow() {
> +  library = browserWindow.openPlacesOrganizerWindow();

Lets add tests to ensure we open the right pane.

::: firefox/lib/ui/browser.js
@@ +37,5 @@
> +   */
> +  get dtds() {
> +    return ["chrome://browser/locale/browser.dtd"];
> +  },
> +  /**

nit: missing empty line

@@ +45,5 @@
> +   *        Information for opening the window
> +   * @param {string} [aSpec.type="shortcut"]
> +   *        How to open the Library Window
> +   * @param {string} [aSpec.location="bookmarks"]
> +   *        Which pane to open to

nit: Which pane to be selected after opening the window

@@ +64,5 @@
> +          spec.cmdKey = utils.getEntity(this.dtds, "bookmarksCmd.commandkey");
> +        }
> +        spec.menuItem = "#bookmarksShowAll";
> +        break;
> +      case "downloads":

Please add all the options to the jsdoc parameter description

@@ +78,5 @@
> +        spec.cmdKey = utils.getEntity(this.dtds, "showAllHistoryCmd.commandkey");
> +        spec.menuItem = "#menu_showAllHistory";
> +        break;
> +      case "tags":
> +        assert.fail("Tags not supported yet directly");

nit: Opening the pane 'Tags' directly is not supported yet.

@@ +87,5 @@
> +
> +    switch (type) {
> +      case "shortcut":
> +        this._controller.mainMenu.keypress(spec.cmdKey, {accelKey: true,
> +                                                         shiftKey: true});

Lets revert that for now and really raise the keypress against the window by this._controller.keypress(0, ...). We have to figure out a better solution when using MozMillElement directly.

@@ +105,5 @@
> +    return placesOrganizerWindow;
> +  }
> +}
> +
> +// Export of methods

This is not a method but a class.

::: firefox/lib/ui/places-organizer.js
@@ +13,5 @@
> +
> +var observerService = Cc["@mozilla.org/observer-service;1"]
> +                      .getService(Ci.nsIObserverService);
> +
> +const DOM_WINDOW_DESTROYED_TOPIC = "dom-window-destroyed";

So what about bug 1006996 here? It would be great to directly get all this code in that other module first. Otherwise we will keep copying it over and over again. It shouldn't be that much work. It should also define the Window class, both BrowserWindow and PlacesOrganizerWindow would be inherited from.

@@ +95,5 @@
> +   *        Value of the attribute to filter
> +   * @param {string} [aSpec.parent=document]
> +   *        Parent of the to find element
> +   *
> +   * @returns {ElemBase[]]} Elements which have been found

nit: extra closing bracket. Please re-use the lines we have in the other modules.

@@ +107,5 @@
> +
> +    switch (spec.type) {
> +      case "backButton":
> +        elems = [new findElement.ID(this.controller.window.document,
> +                                    "back-button")];

findElement.ID() is a method and does not need the new operator. Same for all the other cases.

@@ +123,5 @@
> +        elems = [new findElement.ID(this._controller.window.document,
> +                                    "downloadsItem_id:" + spec.value)];
> +        break;
> +      case "downloadButton":
> +        assert.ok(spec.subtype, "Type of download button has been specified");

I don't see that you force a parent to be set for this method. That's absolutely necessary. So the nodecollector instantiation can be completely moved in here. No other code makes use of it.

@@ +183,5 @@
> +  /**
> +   * Close the Places Organizer Window
> +   *
> +   * @param {boolean} aForce
> +   *         Force closing the window

I don't think this parameter is mandatory. Also I don't see it used in that method.

@@ +186,5 @@
> +   * @param {boolean} aForce
> +   *         Force closing the window
> +   *
> +   */
> +  close : function placesOrganizerWindow_close(aForce) {

As mentioned at the beginning of the file, better to have this in the general window class.

@@ +220,5 @@
> +    finally {
> +      observerService.removeObserver(observer, DOM_WINDOW_DESTROYED_TOPIC);
> +    }
> +  }
> +}

nit: semicolon please.

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +10,5 @@
> +var browser = require("../../../lib/ui/browser");
> +var widgets = require("../../../../lib/widgets");
> +
> +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;

Code like this needs to be documented!

@@ +14,5 @@
> +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;
> +
> +const METHODS = [{
> +    location: "downloads",
> +    type: !mozmill.isLinux ? ["menu"] : ["shortcut", "menu"],

I don't understand this distinction. Why is that necessary. I see shortcuts on all platforms. Also you really want to work with comments in your patches.

@@ +18,5 @@
> +    type: !mozmill.isLinux ? ["menu"] : ["shortcut", "menu"],
> +    title: "Downloads"
> +  },{
> +    location: "bookmarks",
> +    type: ["shortcut","menu"],

Given that the value is always an array you want to use the plural. Also type is not that clear to me. 'action' or 'event' may be better.

@@ +26,5 @@
> +    type: XP_OR_OSX ? ["menu"] : ["shortcut", "menu"],
> +    title: "Today"
> +  }];
> +
> +var setupModule = function(aModule) {

No var declarations please. That's known for a long time now.

@@ +27,5 @@
> +    title: "Today"
> +  }];
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();

Why this line? I don't see that it is in use.

@@ +46,5 @@
> +  METHODS.forEach((aValue, aIndex) => {
> +    aValue.type.forEach(aType => {
> +      libraryWindow = browserWindow.openPlacesOrganizerWindow({location: aValue.location,
> +                                                               type: aType});
> +      var treeNode = libraryWindow.getElement({type: "tree"});

I think 'tree' should become a property on the class, given that we will use it a lot.

@@ +50,5 @@
> +      var treeNode = libraryWindow.getElement({type: "tree"});
> +      assert.equal(widgets.getSelectedCell(treeNode).title,
> +                   aValue.title,
> +                   "Library has been opened and focused on correct location");
> +      libraryWindow.close();

Keep in mind that when the above assert fails, you will not close the window! Also for the above I would use 'expect'. There is no need to force that the correct pane is shown. We don't have to issue any ui event on the node.

::: lib/widgets.js
@@ +41,3 @@
>    EventUtils.synthesizeMouse(tree.body, x.value + 4, y.value + 4,
> +                             aEventDetails, tree.ownerDocument.defaultView);
> +  aController.sleep(0);

We shouldn't need this sleep() call here anymore, right?

@@ +55,1 @@
>  }

Lets file a new bug so we can turn these functions into a class.

Comment 21

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Comment on attachment 8424782 [details] [diff] [review]
> fix_2.2.patch
>
> Review of attachment 8424782 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +34,5 @@
> > +  aModule.library.close(true);
> > +}
> > +
> > +function testPlacesOrganizerWindow() {
> > +  library = browserWindow.openPlacesOrganizerWindow();
>
> Lets add tests to ensure we open the right pane.
>

You mean the right window ? Doesn't 'handleWindow()' already do that ?

> @@ +183,5 @@
> > +  /**
> > +   * Close the Places Organizer Window
> > +   *
> > +   * @param {boolean} aForce
> > +   *         Force closing the window
>
> I don't think this parameter is mandatory. Also I don't see it used in that
> method.
>

It is used below and if is true we close the window by directly by calling close() on the element.

> @@ +14,5 @@
> > +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;
> > +
> > +const METHODS = [{
> > +    location: "downloads",
> > +    type: !mozmill.isLinux ? ["menu"] : ["shortcut", "menu"],
>
> I don't understand this distinction. Why is that necessary. I see shortcuts
> on all platforms. Also you really want to work with comments in your patches.
>

As I can see in the code, the shortcut get's duplicated :
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#207
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#228
so downloads.accesskey get's replaced by browserConsoleCmd.accesskey.
On unix we still have downloadsUnix.commandkey which we can use.

> @@ +50,5 @@
> > +      var treeNode = libraryWindow.getElement({type: "tree"});
> > +      assert.equal(widgets.getSelectedCell(treeNode).title,
> > +                   aValue.title,
> > +                   "Library has been opened and focused on correct location");
> > +      libraryWindow.close();
>
> Keep in mind that when the above assert fails, you will not close the
> window! Also for the above I would use 'expect'. There is no need to force
> that the correct pane is shown. We don't have to issue any ui event on the
> node.
>

We will close the window in the teardownModule. But I'll use expect, np!

> We shouldn't need this sleep() call here anymore, right?
>
> @@ +55,1 @@
> >  }
>
> Lets file a new bug so we can turn these functions into a class.

Should we have a class for handling trees in a file like tree.js ? Or we can have widgets.js with multiple classes (Tree, ...) ?

Comment 22

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

(In reply to daniel.gherasim from comment #21)
> > > +function testPlacesOrganizerWindow() {
> > > +  library = browserWindow.openPlacesOrganizerWindow();
> >
> > Lets add tests to ensure we open the right pane.
> 
> You mean the right window ? Doesn't 'handleWindow()' already do that ?

No, I mean pane. There is no code in this test which checks that we really open e.g. bookmarks.

> > > +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;
> > > +
> > > +const METHODS = [{
> > > +    location: "downloads",
> > > +    type: !mozmill.isLinux ? ["menu"] : ["shortcut", "menu"],
> >
> > I don't understand this distinction. Why is that necessary. I see shortcuts
> > on all platforms. Also you really want to work with comments in your patches.
> 
> As I can see in the code, the shortcut get's duplicated :
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#207
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#228
> so downloads.accesskey get's replaced by browserConsoleCmd.accesskey.
> On unix we still have downloadsUnix.commandkey which we can use.

Sorry but those links are not referring anything related to this bug. What I wanted to say is, why don't you test shortcuts on Windows and OS X?

> > We shouldn't need this sleep() call here anymore, right?
> >
> > @@ +55,1 @@
> > >  }
> >
> > Lets file a new bug so we can turn these functions into a class.
> 
> Should we have a class for handling trees in a file like tree.js ? Or we can
> have widgets.js with multiple classes (Tree, ...) ?

That's why widgets.js exists, and is named with the plural form. :)

Comment 23

[Daniel Gherasim]

This blocks a P1 bug & depends on the windows.js library.

Comment 24

[Daniel Gherasim]

Attachment: v4.patch

We are unblocked here so I finished the patch.

Comment 25

[Andrei Eftimie]

Comment on attachment 8494530 [details] [diff] [review]
v4.patch

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

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +35,5 @@
> +
> +function testPlacesOrganizerWindow() {
> +  var placesOrganizerWindow = browserWindow.openPlacesOrganizer();
> +  expect.equal(widgets.getSelectedCell(placesOrganizerWindow.tree).title,
> +               "All Bookmarks",

The title is localized. Need to get it from a DTD.

@@ +36,5 @@
> +function testPlacesOrganizerWindow() {
> +  var placesOrganizerWindow = browserWindow.openPlacesOrganizer();
> +  expect.equal(widgets.getSelectedCell(placesOrganizerWindow.tree).title,
> +               "All Bookmarks",
> +               "Library has been opened and focused on correct location");

"... opened on the correct location" should be enough.

::: firefox/lib/ui/browser.js
@@ +107,5 @@
> + *        Which pane to be selected after opening the window
> + *
> + * @returns {PlacesOrganizerWindow} Instance of an PlacesOrganizerWindow
> + */
> +BrowserWindow.prototype.openPlacesOrganizer = function BrowserWindow_openPlacesOrganizer(aSpec) {

Ugh, I still feel this it backwards.
Looked up old comments and I got the same feeling 6 months ago. See comment 6 and comment 7.

Each UI class should know how to open itself. (It will use BrowserWindow in the background sure, but API wise it makes so much more sense to:
a) instantiate the library window
b) tell it to open itself to a specific page
c) do stuff with it
d) tell it to close itself

Than to:
a) get a browser object
b) tell it to open the library window to a specific page, this returns the library object
c) use the library object
d) tell the library object to close itself

@@ +138,5 @@
> +        spec.cmdKey = utils.getEntity(this.dtds, "showAllHistoryCmd.commandkey");
> +        spec.menuItem = "#menu_showAllHistory";
> +        break;
> +      case "tags":
> +        assert.fail("Opening the pane 'Tags' directly is not supported yet");

This was the state about 1 year ago.
Please check whether we have a shortcut or menu entry for tags now.

@@ +147,5 @@
> +
> +    var controller = windows.waitForWindowState(() => {
> +      switch (type) {
> +        case "shortcut":
> +          this._controller.keypress(0, spec.cmdKey, {accelKey: accelKey,

Why `0`?

::: firefox/lib/ui/places-organizer.js
@@ +93,5 @@
> +    var elems = null;
> +
> +    switch (spec.type) {
> +      case "backButton":
> +        elems = [findElement.ID(this.controller.window.document,

mark:
> var root = spec.parent || this.controller.window.document;

And use `root` throughout this method.

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

This test fails on OSX for me with
> ERROR | Test Failure | {
>   "exception": {
>     "message": "Expected window state has been reached - open (Window has been opened)", 
>     "lineNumber": 27, 
>     "name": "TimeoutError", 
>     "fileName": "resource://mozmill/modules/errors.js"
>   }
> }

@@ +17,5 @@
> +
> +const METHODS = [
> +  {
> +    location: "downloads",
> +    action: ["shortcut", "menu"],

Please name it `actions` since it's an array.

@@ +18,5 @@
> +const METHODS = [
> +  {
> +    location: "downloads",
> +    action: ["shortcut", "menu"],
> +    title: "Downloads"

The titles will need to be localised.

Comment 26

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

(In reply to Andrei Eftimie from comment #25)
> Each UI class should know how to open itself. (It will use BrowserWindow in
> the background sure, but API wise it makes so much more sense to:
> a) instantiate the library window
> b) tell it to open itself to a specific page
> c) do stuff with it
> d) tell it to close itself

Keep in mind that not only the browser window can open the places organizer. It could be implemented in any kind of window, which have their own menu items, shortcuts, or whatever. All those properties the target window is not aware of.

Comment 27

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #26)
> (In reply to Andrei Eftimie from comment #25)
> > Each UI class should know how to open itself. (It will use BrowserWindow in
> > the background sure, but API wise it makes so much more sense to:
> > a) instantiate the library window
> > b) tell it to open itself to a specific page
> > c) do stuff with it
> > d) tell it to close itself
> 
> Keep in mind that not only the browser window can open the places organizer.
> It could be implemented in any kind of window, which have their own menu
> items, shortcuts, or whatever. All those properties the target window is not
> aware of.

Exactly! This only reinforces my point that the open method should live in PlacesOrganizer. And based on the arguments it receives it will open with the specific method / from the specific place.

Comment 28

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

So we would still need this method in BrowserWindow, but it should only specify the method via a callback. The underlying logic to find and handle the PlacesOrganizer should be in the other module, so this code exists only once.

Comment 29

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #25)
> Comment on attachment 8494530 [details] [diff] [review]
> v4.patch
>
> Review of attachment 8494530 [details] [diff] [review]:a
> -----------------------------------------------------------------

> @@ +138,5 @@
> > +        spec.cmdKey = utils.getEntity(this.dtds, "showAllHistoryCmd.commandkey");
> > +        spec.menuItem = "#menu_showAllHistory";
> > +        break;
> > +      case "tags":
> > +        assert.fail("Opening the pane 'Tags' directly is not supported yet");
>
> This was the state about 1 year ago.
> Please check whether we have a shortcut or menu entry for tags now.
>

Couldn't find any.

> :::
> firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
>
> This test fails on OSX for me with
> > ERROR | Test Failure | {
> >   "exception": {
> >     "message": "Expected window state has been reached - open (Window has been opened)",
> >     "lineNumber": 27,
> >     "name": "TimeoutError",
> >     "fileName": "resource://mozmill/modules/errors.js"
> >   }
> > }
>

Yeah sorry, simply replaced accelKey with shiftKey by mistake.

Comment 30

[Daniel Gherasim]

Attachment: v5.patch

Updated based on review.

Comment 31

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8500944 [details] [diff] [review]
v5.patch

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

Few items left.

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +11,5 @@
> +var placesOrganizer = require("../ui/places-organizer");
> +var utils = require("../../../lib/utils");
> +var windows = require("../../../lib/windows");
> +
> +var widgets = require("../../../lib/ui/widgets");

This should be above windows or did we establish somewhere to keep ui separate?

@@ +38,5 @@
> +function testPlacesOrganizerWindow() {
> +  var placesOrganizerWindow = browserWindow.openPlacesOrganizer();
> +  expect.equal(widgets.getSelectedCell(placesOrganizerWindow.tree).title,
> +               utils.getProperty("chrome://browser/locale/places/places.properties",
> +                                 "OrganizerQueryAllBookmarks"),

I'd call this outside the expect

::: firefox/lib/ui/places-organizer.js
@@ +66,5 @@
> + *        Parent of the element
> + *
> + * @returns {ElemBase} Element which has been found
> + */
> +PlacesOrganizerWindow.prototype.getElement = function PlacesOrganizerWindow_getElement(aSpec) {

We should use POW everywhere as above.

@@ +252,5 @@
> +      case "shortcut":
> +        aSpec.controller.keypress(null, cmdKey, {accelKey: true,
> +                                                 shiftKey: shiftKey});
> +        break;
> +      case "menu":

menu comes before shortcut.

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +12,5 @@
> +var widgets = require("../../../../lib/ui/widgets");
> +var windows = require("../../../../lib/windows");
> +
> +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> +// Check if we are on XP or an MAC system

nit: 'or on a MAC'

@@ +60,5 @@
> +      var placesOrganizerWindow = browserWindow.openPlacesOrganizer({location: aValue.location,
> +                                                                     type: aType});
> +      expect.equal(widgets.getSelectedCell(placesOrganizerWindow.tree).title,
> +                   aValue.title(),
> +                   "Library has been opened and focused on correct location");

nit: 'on the correct'

Comment 32

[Daniel Gherasim]

(In reply to Andreea Matei [:AndreeaMatei] from comment #31)
> Comment on attachment 8500944 [details] [diff] [review]
> v5.patch
> 
> Review of attachment 8500944 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Few items left.
> 
> ::: firefox/lib/tests/testPlacesOrganizer.js
> @@ +11,5 @@
> > +var placesOrganizer = require("../ui/places-organizer");
> > +var utils = require("../../../lib/utils");
> > +var windows = require("../../../lib/windows");
> > +
> > +var widgets = require("../../../lib/ui/widgets");
> 
> This should be above windows or did we establish somewhere to keep ui
> separate?
> 

Henrik requested in a bug somewhere to keep the ui libraries separated.

Comment 33

[Daniel Gherasim]

Attachment: v6.patch

Thanks!
Patch updated with your requests and following what we discussed about the structure in bug 1005811.

Comment 34

[Daniel Gherasim]

Attachment: v6.1.patch

Moved the modification we need to widgets in bug 1081083.
Updated this patch to not include that.
To test you need patch from there applied first.

Comment 35

[Andrei Eftimie]

Comment on attachment 8503106 [details] [diff] [review]
v6.1.patch

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

Deps have landed (ATM on relevant branches).

There's some bitrot:
> Hunk #1 FAILED at 2
> 1 out of 2 hunks FAILED -- saving rejects to file firefox/lib/ui/browser.js.rej

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +18,5 @@
> +
> +const METHODS = [
> +  {
> +    location: "downloads",
> +    actions: ["shortcut", "menu"],

Sort them alphabetically please.

@@ +21,5 @@
> +    location: "downloads",
> +    actions: ["shortcut", "menu"],
> +    getTitle: () => {
> +      return utils.getProperty("chrome://browser/locale/places/places.properties",
> +                               "OrganizerQueryDownloads");

This doesn't need to be a function:
> title: utils.getProperty(

@@ +55,5 @@
> + * Bug 996530
> + * Test open/close the places organizer
> + */
> +function testOpenClosePlacesOrganizer() {
> +  METHODS.forEach((aValue, aIndex) => {

Please rename aValue in something like aItem.

Comment 36

[Daniel Gherasim]

Attachment: v7.patch

Comment 37

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

Comment on attachment 8513440 [details] [diff] [review]
v7.patch

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

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +35,5 @@
> +  windows.closeAllWindows(aModule.browserWindow);
> +}
> +
> +function testPlacesOrganizerWindow() {
> +  var placesOrganizerWindow = browserWindow.openPlacesOrganizer();

just win please.

@@ +37,5 @@
> +
> +function testPlacesOrganizerWindow() {
> +  var placesOrganizerWindow = browserWindow.openPlacesOrganizer();
> +  var bookmarksTitle = utils.getProperty("chrome://browser/locale/places/places.properties",
> +                                         "OrganizerQueryAllBookmarks");

Lets get this updated with win.getProperty("OrganizerQueryAllBookmarks")

::: firefox/lib/ui/browser.js
@@ +111,5 @@
> + * Open the places organizer window
> + *
> + * @param {object} [aSpec]
> + *        Information for opening the window
> + * @param {string} [aSpec.type="shortcut"|"menu"]

The different types should be part of the description. Here the default value should only be listed. Also please sort alphabetically

@@ +114,5 @@
> + *        Information for opening the window
> + * @param {string} [aSpec.type="shortcut"|"menu"]
> + *        How to open the Library Window
> + * @param {string} [aSpec.location="bookmarks"|"downloads"|"history"|"tags"]
> + *        Which pane to be selected after opening the window

Same here as above.

::: firefox/lib/ui/places-organizer.js
@@ +24,5 @@
> +  baseWindow.BaseWindow.call(this, aController);
> +
> +  this._dtds = ["chrome://browser/locale/browser.dtd",
> +                "chrome://mozapps/locale/downloads/downloads.dtd",
> +                "chrome://browser/locale/places/places.dtd"];

Please sort by url.

@@ +35,5 @@
> + * Get the main tree element from the Places Organizer window
> + *
> + * @returns {ElemBase} The tree element from window
> + */
> +PlacesOrganizerWindow.prototype.__defineGetter__("tree", function() {

nit: a blank after function please in all cases.

@@ +36,5 @@
> + *
> + * @returns {ElemBase} The tree element from window
> + */
> +PlacesOrganizerWindow.prototype.__defineGetter__("tree", function() {
> +  return this.getElement({type: "tree"});

Given that we need it everywhere, we should better cache it.

@@ +91,5 @@
> +
> +      var nodeCollector = new domUtils.nodeCollector(root);
> +      switch (spec.subtype) {
> +        case "cancel":
> +          nodeCollector.queryAnonymousNode("class", "downloadButton downloadCancel");

I would create the nodeCollector instance at the top, and have elements like download_cancelButton etc. Btw. don't those need the parent to be defined?

@@ +137,5 @@
> + *
> + * @param {object} [aSpec={}]
> + *        Information about closing the window
> + * @param {string} [aSpec.method="shortcut"]
> + *        Method to use for closing ("shortcut", or "callback")

nit: sorting

@@ +139,5 @@
> + *        Information about closing the window
> + * @param {string} [aSpec.method="shortcut"]
> + *        Method to use for closing ("shortcut", or "callback")
> + * @param {function} [aSpec.callback]
> + *        Define new function that closes the window

Please try to stay with the descriptions as we have for other classes.

@@ +154,5 @@
> +  }
> +
> +  switch (method) {
> +    case "shortcut":
> +       var closeWindowKey = utils.getEntity(this.dtds, "closeCmd.key");

this.getEntity() when we have that landed by the other bug, which will happen soonish.

@@ +159,5 @@
> +       callback = () => {
> +         this.controller.keypress(null, closeWindowKey, {accelKey: true});
> +       }
> +      break;
> +    case "callback":

before shortcut

@@ +161,5 @@
> +       }
> +      break;
> +    case "callback":
> +      assert.equal(typeof aSpec.callback, "function",
> +                   "Callback function has been defined");

Please remove function.

@@ +174,5 @@
> +
> +/**
> + * Open the places organizer window
> + *
> + * @param {function} aCalback

nit: aCallback

@@ +175,5 @@
> +/**
> + * Open the places organizer window
> + *
> + * @param {function} aCalback
> + *        Callback function that opens the Place:Organizer Window

Same as above.

@@ +181,5 @@
> + * @returns {PlacesOrganizerWindow} New instance of PlacesOrganizerWindow
> + */
> +function open(aCallback) {
> +  assert.equal(typeof aCallback, "function",
> +               "Callback function has been defined");

Again.

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +13,5 @@
> +var windows = require("../../../../lib/windows");
> +
> +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> +// Check if we are on XP or an a MAC system
> +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;

This should be hidden and not visible to tests. Also it needs a description why we have to do this check.

@@ +20,5 @@
> +  {
> +    actions: ["menu", "shortcut"],
> +    location: "downloads",
> +    title: utils.getProperty("chrome://browser/locale/places/places.properties",
> +                             "OrganizerQueryDownloads")

Similar here to getEntity. Please use the browser window to get the property.

Comment 38

[Teodor Druta]

Attachment: 996530v8.patch

> 
> ::: firefox/lib/tests/testPlacesOrganizer.js
> @@ +35,5 @@
> > +  windows.closeAllWindows(aModule.browserWindow);
> > +}
> > +
> > +function testPlacesOrganizerWindow() {
> > +  var placesOrganizerWindow = browserWindow.openPlacesOrganizer();
> 
> just win please.
> 

Renamed to "win".

> @@ +37,5 @@
> > +
> > +function testPlacesOrganizerWindow() {
> > +  var placesOrganizerWindow = browserWindow.openPlacesOrganizer();
> > +  var bookmarksTitle = utils.getProperty("chrome://browser/locale/places/places.properties",
> > +                                         "OrganizerQueryAllBookmarks");
> 
> Lets get this updated with win.getProperty("OrganizerQueryAllBookmarks")
> 
> @@ +154,5 @@
> > +  }
> > +
> > +  switch (method) {
> > +    case "shortcut":
> > +       var closeWindowKey = utils.getEntity(this.dtds, "closeCmd.key");
> 
> this.getEntity() when we have that landed by the other bug, which will
> happen soonish.
> 
> @@ +20,5 @@
> > +  {
> > +    actions: ["menu", "shortcut"],
> > +    location: "downloads",
> > +    title: utils.getProperty("chrome://browser/locale/places/places.properties",
> > +                             "OrganizerQueryDownloads")
> 
> Similar here to getEntity. Please use the browser window to get the property.

Updated to "win"/this.getProperty and *.getEntity everywhere it applied both in test and in places-organizer and browser libs.

> ::: firefox/lib/ui/browser.js
> @@ +111,5 @@
> > + * Open the places organizer window
> > + *
> > + * @param {object} [aSpec]
> > + *        Information for opening the window
> > + * @param {string} [aSpec.type="shortcut"|"menu"]
> 
> The different types should be part of the description. Here the default
> value should only be listed. Also please sort alphabetically
> 
> @@ +114,5 @@
> > + *        Information for opening the window
> > + * @param {string} [aSpec.type="shortcut"|"menu"]
> > + *        How to open the Library Window
> > + * @param {string} [aSpec.location="bookmarks"|"downloads"|"history"|"tags"]
> > + *        Which pane to be selected after opening the window
> 
> Same here as above.
> 

Sorted alphabetically everywhere it applied

> ::: firefox/lib/ui/places-organizer.js
> @@ +24,5 @@
> > +  baseWindow.BaseWindow.call(this, aController);
> > +
> > +  this._dtds = ["chrome://browser/locale/browser.dtd",
> > +                "chrome://mozapps/locale/downloads/downloads.dtd",
> > +                "chrome://browser/locale/places/places.dtd"];
> 
> Please sort by url.
> 

Sorted alphabetically.

> @@ +35,5 @@
> > + * Get the main tree element from the Places Organizer window
> > + *
> > + * @returns {ElemBase} The tree element from window
> > + */
> > +PlacesOrganizerWindow.prototype.__defineGetter__("tree", function() {
> 
> nit: a blank after function please in all cases.
> 

Fixed.

> @@ +36,5 @@
> > + *
> > + * @returns {ElemBase} The tree element from window
> > + */
> > +PlacesOrganizerWindow.prototype.__defineGetter__("tree", function() {
> > +  return this.getElement({type: "tree"});
> 
> Given that we need it everywhere, we should better cache it.
> 

Moved to private ._tree property, returning the property in the getter.

> @@ +91,5 @@
> > +
> > +      var nodeCollector = new domUtils.nodeCollector(root);
> > +      switch (spec.subtype) {
> > +        case "cancel":
> > +          nodeCollector.queryAnonymousNode("class", "downloadButton downloadCancel");
> 
> I would create the nodeCollector instance at the top, and have elements like
> download_cancelButton etc. Btw. don't those need the parent to be defined?
> 

Moved the nodeCollector to the top renamed elements type to download_*.
Added a new element type "downloadItem" to use as root for download buttons.

> @@ +137,5 @@
> > + *
> > + * @param {object} [aSpec={}]
> > + *        Information about closing the window
> > + * @param {string} [aSpec.method="shortcut"]
> > + *        Method to use for closing ("shortcut", or "callback")
> 
> nit: sorting
> 
> @@ +139,5 @@
> > + *        Information about closing the window
> > + * @param {string} [aSpec.method="shortcut"]
> > + *        Method to use for closing ("shortcut", or "callback")
> > + * @param {function} [aSpec.callback]
> > + *        Define new function that closes the window
> 
> Please try to stay with the descriptions as we have for other classes.
> 
> @@ +159,5 @@
> > +       callback = () => {
> > +         this.controller.keypress(null, closeWindowKey, {accelKey: true});
> > +       }
> > +      break;
> > +    case "callback":
> 
> before shortcut
> 
> @@ +161,5 @@
> > +       }
> > +      break;
> > +    case "callback":
> > +      assert.equal(typeof aSpec.callback, "function",
> > +                   "Callback function has been defined");
> 
> Please remove function.
> 
> @@ +174,5 @@
> > +
> > +/**
> > + * Open the places organizer window
> > + *
> > + * @param {function} aCalback
> 
> nit: aCallback
> 
> @@ +175,5 @@
> > +/**
> > + * Open the places organizer window
> > + *
> > + * @param {function} aCalback
> > + *        Callback function that opens the Place:Organizer Window
> 
> Same as above.
> 
> @@ +181,5 @@
> > + * @returns {PlacesOrganizerWindow} New instance of PlacesOrganizerWindow
> > + */
> > +function open(aCallback) {
> > +  assert.equal(typeof aCallback, "function",
> > +               "Callback function has been defined");
> 
> Again.
> 

Fixed.

> :::
> firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
> @@ +13,5 @@
> > +var windows = require("../../../../lib/windows");
> > +
> > +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> > +// Check if we are on XP or an a MAC system
> > +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;
> 
> This should be hidden and not visible to tests. Also it needs a description
> why we have to do this check.
> 

Moved these to the places-organizer lib, added a comment.

Comment 39

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8528968 [details] [diff] [review]
996530v8.patch

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

Patch not applying anymore. Few updates:

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +7,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// Include required modules
> +var browser = require("../ui/browser");
> +var placesOrganizer = require("../ui/places-organizer");

Being ui, should be before widgets in the block below.

::: firefox/lib/ui/places-organizer.js
@@ +114,5 @@
> +    case "download_showButton":
> +      nodeCollector.root = this.getElement({type: "downloadItem", value: spec.value});
> +      nodeCollector.queryAnonymousNode("class", "downloadButton downloadShow");
> +      elems = nodeCollector.elements;
> +      break;

Please sort these download* items alphabetically.

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +13,5 @@
> +var windows = require("../../../../lib/windows");
> +
> +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> +// Check if we are on XP or an a MAC system
> +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;

Henrik had a comment here too about a description on why we need this. Please add that too.

Comment 40

[Teodor Druta]

Attachment: 996530v9.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #39)
> Comment on attachment 8528968 [details] [diff] [review]
> 996530v8.patch
> 
> Review of attachment 8528968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch not applying anymore.

Fixed rejections.

> Few updates:
> 
> ::: firefox/lib/tests/testPlacesOrganizer.js
> @@ +7,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +// Include required modules
> > +var browser = require("../ui/browser");
> > +var placesOrganizer = require("../ui/places-organizer");
> 
> Being ui, should be before widgets in the block below.
> 

Fixed.

> ::: firefox/lib/ui/places-organizer.js
> @@ +114,5 @@
> > +    case "download_showButton":
> > +      nodeCollector.root = this.getElement({type: "downloadItem", value: spec.value});
> > +      nodeCollector.queryAnonymousNode("class", "downloadButton downloadShow");
> > +      elems = nodeCollector.elements;
> > +      break;
> 
> Please sort these download* items alphabetically.
> 

Sorted alphabetically

> :::
> firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
> @@ +13,5 @@
> > +var windows = require("../../../../lib/windows");
> > +
> > +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> > +// Check if we are on XP or an a MAC system
> > +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;
> 
> Henrik had a comment here too about a description on why we need this.
> Please add that too.

Moved the XP_OR_OSX constant to the lib added explanatory comments.

Comment 41

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8531238 [details] [diff] [review]
996530v9.patch

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

Please test your patches before uploading.

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +16,5 @@
> +
> +const ELEMENTS = [
> +  {type: "backButton", localName: "toolbarbutton"},
> +  {type: "forwardButton", localName: "toolbarbutton"},
> +  {type: "organizeButton", localName: placesOrganizer.XP_OR_OSX ? "toolbarbutton" : "menu"},

This is failing, not being able to find the XP_OR_OSX constant.

::: firefox/lib/ui/places-organizer.js
@@ +14,5 @@
> +var baseWindow = require("../../../lib/ui/base-window");
> +
> +// Cache the OS type for handling the lack of shortcut for history access
> +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;

You need to export this in order to be able to use it in the tests.

Comment 42

[Teodor Druta]

Attachment: 996530v9.1.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #41)
> Comment on attachment 8531238 [details] [diff] [review]
> 996530v9.patch
> 
> Review of attachment 8531238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please test your patches before uploading.
> 
> ::: firefox/lib/tests/testPlacesOrganizer.js
> @@ +16,5 @@
> > +
> > +const ELEMENTS = [
> > +  {type: "backButton", localName: "toolbarbutton"},
> > +  {type: "forwardButton", localName: "toolbarbutton"},
> > +  {type: "organizeButton", localName: placesOrganizer.XP_OR_OSX ? "toolbarbutton" : "menu"},
> 
> This is failing, not being able to find the XP_OR_OSX constant.
> 
> ::: firefox/lib/ui/places-organizer.js
> @@ +14,5 @@
> > +var baseWindow = require("../../../lib/ui/base-window");
> > +
> > +// Cache the OS type for handling the lack of shortcut for history access
> > +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> > +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;
> 
> You need to export this in order to be able to use it in the tests.

Indeed it was failing on OSX and Win XP.
Added XP_OR_OSX constant to exports.

Comment 43

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8531579 [details] [diff] [review]
996530v9.1.patch

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

Works well now, thanks. Moving review to Henrik.

Comment 44

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8531579 [details] [diff] [review]
996530v9.1.patch

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

Works well now, thanks. Moving review to Henrik.

::: firefox/lib/ui/places-organizer.js
@@ +204,5 @@
> +}
> +
> +// Export of classes
> +exports.PlacesOrganizerWindow = PlacesOrganizerWindow;
> +exports.XP_OR_OSX = XP_OR_OSX;

Actually, missed to see this is under "Export of classes". Please make it separate in "Export of constants". With that is r+.

Comment 45

[Teodor Druta]

Attachment: 996530v9.2.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #44)
> Comment on attachment 8531579 [details] [diff] [review]
> 996530v9.1.patch
> 
> Review of attachment 8531579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works well now, thanks. Moving review to Henrik.
> 
> ::: firefox/lib/ui/places-organizer.js
> @@ +204,5 @@
> > +}
> > +
> > +// Export of classes
> > +exports.PlacesOrganizerWindow = PlacesOrganizerWindow;
> > +exports.XP_OR_OSX = XP_OR_OSX;
> 
> Actually, missed to see this is under "Export of classes". Please make it
> separate in "Export of constants". With that is r+.

Fixed.

Comment 46

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

Comment on attachment 8532053 [details] [diff] [review]
996530v9.2.patch

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

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +24,5 @@
> +  {type: "tree", localName: "tree"}
> +];
> +
> +const TEST_DATA = "ftp://ftp.mozilla.org/pub/firefox/releases/3.6/mac/en-US/" +
> +                  "Firefox%203.6.dmg";

That is unused.

@@ +43,5 @@
> +
> +  // Test all available elements
> +  ELEMENTS.forEach(aElement => {
> +    var element = win.getElement({type: aElement.type});
> +    assert.ok(element.getNode().localName === aElement.localName,

expect.equal() please, so that we see all the failures.

::: firefox/lib/ui/places-organizer.js
@@ +14,5 @@
> +var baseWindow = require("../../../lib/ui/base-window");
> +
> +// Cache the OS type for handling the lack of shortcut for history access
> +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;

Usually we do not want to export those constants. When is that necessary? Only for the library test I assume? If yes, it can do the same on its own.

@@ +28,5 @@
> +  baseWindow.BaseWindow.call(this, aController);
> +
> +  this._dtds = ["chrome://browser/locale/browser.dtd",
> +                "chrome://browser/locale/places/places.dtd",
> +                "chrome://mozapps/locale/downloads/downloads.dtd"];

Nit: Please switch those two lines.

@@ +54,5 @@
> + * @param {ElemBase} aDownload
> + *        Download which state should be checked
> + */
> +PlacesOrganizerWindow.prototype.getDownloadState = function POW_getDownloadState(aDownload) {
> +  return aDownload.getAttribute("state");

I do not see that we check that aDownload has been specified.

@@ +135,5 @@
> +    case "viewMenu":
> +      elems = [findElement.ID(root, "viewMenu")];
> +      break;
> +    case "window":
> +      elems = [findElement.ID(root, "places")];

Why do you need that? We don't have such an element in any other class, but get it with controller.window.document.documentElement.

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +24,5 @@
> +    location: "bookmarks",
> +    title: utils.getProperty("chrome://browser/locale/places/places.properties",
> +                             "OrganizerQueryAllBookmarks")
> +  },{
> +    actions: [],

This is a no-op, right? If this is wanted please add a comment with the reason.

@@ +51,5 @@
> +                                                                     type: aType});
> +
> +      // On xp & osx we don't have shortcut for history acccess
> +      // http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#352
> +      placesOrganizerWindow.action = placesOrganizerWindow.XP_OR_OSX ? ["menu"] : ["menu", "shortcut"];

I don't see why we have to exec this code for each iteration. Isn't that a setup step? Also this is augmenting an action property to the class instance. Please don't do that.

Comment 47

[Teodor Druta]

Attachment: placesfinal.patch

(In reply to Henrik Skupin (:whimboo) from comment #46)
> Comment on attachment 8532053 [details] [diff] [review]
> 996530v9.2.patch
> 
> Review of attachment 8532053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tests/testPlacesOrganizer.js
> @@ +24,5 @@
> > +  {type: "tree", localName: "tree"}
> > +];
> > +
> > +const TEST_DATA = "ftp://ftp.mozilla.org/pub/firefox/releases/3.6/mac/en-US/" +
> > +                  "Firefox%203.6.dmg";
> 
> That is unused.
> 

Removed.

> @@ +43,5 @@
> > +
> > +  // Test all available elements
> > +  ELEMENTS.forEach(aElement => {
> > +    var element = win.getElement({type: aElement.type});
> > +    assert.ok(element.getNode().localName === aElement.localName,
> 
> expect.equal() please, so that we see all the failures.
> 

Fixed.

> ::: firefox/lib/ui/places-organizer.js
> @@ +14,5 @@
> > +var baseWindow = require("../../../lib/ui/base-window");
> > +
> > +// Cache the OS type for handling the lack of shortcut for history access
> > +const VERSION = parseFloat(Services.sysinfo.getProperty("version"));
> > +const XP_OR_OSX = mozmill.isMac || mozmill.isWindows && VERSION < 6.0;
> 
> Usually we do not want to export those constants. When is that necessary?
> Only for the library test I assume? If yes, it can do the same on its own.
> 

Actually it seems that this was fixed on windows xp. Removed both constants and simply using mozmill.isMac for local names differences of the elements between OSX and rest of the platforms.

> @@ +54,5 @@
> > + * @param {ElemBase} aDownload
> > + *        Download which state should be checked
> > + */
> > +PlacesOrganizerWindow.prototype.getDownloadState = function POW_getDownloadState(aDownload) {
> > +  return aDownload.getAttribute("state");
> 
> I do not see that we check that aDownload has been specified.
> 

Added an assertion to check if aDownload is an object.

> @@ +135,5 @@
> > +    case "viewMenu":
> > +      elems = [findElement.ID(root, "viewMenu")];
> > +      break;
> > +    case "window":
> > +      elems = [findElement.ID(root, "places")];
> 
> Why do you need that? We don't have such an element in any other class, but
> get it with controller.window.document.documentElement.
> 

Removed it.

> :::
> firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
> @@ +24,5 @@
> > +    location: "bookmarks",
> > +    title: utils.getProperty("chrome://browser/locale/places/places.properties",
> > +                             "OrganizerQueryAllBookmarks")
> > +  },{
> > +    actions: [],
> 
> This is a no-op, right? If this is wanted please add a comment with the
> reason.
> 

Added "menu" to actions.

> @@ +51,5 @@
> > +                                                                     type: aType});
> > +
> > +      // On xp & osx we don't have shortcut for history acccess
> > +      // http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#352
> > +      placesOrganizerWindow.action = placesOrganizerWindow.XP_OR_OSX ? ["menu"] : ["menu", "shortcut"];
> 
> I don't see why we have to exec this code for each iteration. Isn't that a
> setup step? Also this is augmenting an action property to the class
> instance. Please don't do that.

Removed this line, it wasn't really used. 
Added a new XP constant to this test to handle differences between XP and rest of the platforms in the History tree.

Comment 48

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8534342 [details] [diff] [review]
placesfinal.patch

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

Just one thing.

Also I'm curious about the difference on XP regarding the History default selected option. Maybe we can get an answer from the Desktop team on the expected option.

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +42,5 @@
> +  // Test all available elements
> +  ELEMENTS.forEach(aElement => {
> +    var element = win.getElement({type: aElement.type});
> +    expect.equal(element.getNode().localName, aElement.localName,
> +              "Element '" + aElement.type + "' has been found");

Indentation is off.

Comment 49

[Teodor Druta]

Attachment: placesv10.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #48)
> Comment on attachment 8534342 [details] [diff] [review]
> placesfinal.patch
> 
> Review of attachment 8534342 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just one thing.
> 
> Also I'm curious about the difference on XP regarding the History default
> selected option. Maybe we can get an answer from the Desktop team on the
> expected option.
> 

I was actually mislead by this, the time wasn't set correctly (in the past) on my local xp machine, because of this firefox forbids opening of web pages behind the 'https' protocol, mainly I think because of security issues. Firefox Nightly "first run" page is located under such a protocol with a secure certificate, so actually when I was running the test the history was empty that's why the Places Organizer History tree, had only its root element "History", selected by default, and it was missing "Today" child node, which should contain history for the "first run" web page, which is open everytime with a clean profile when running the test.

Removed the XP specific part from testOpenPlacesOrganizer.js and fixed indentation for testPlacesOrganizer.js

Comment 50

[Teodor Druta]

Comment on attachment 8534970 [details] [diff] [review]
placesv10.patch

Actually Andreea already gave r+. Henrik, can you please review this patch ?

Comment 51

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

Comment on attachment 8534970 [details] [diff] [review]
placesv10.patch

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

r+ with the inclusion that you get back the shortcut test for the history test. Otherwise nits. Thanks for working on it!

::: firefox/lib/tests/testPlacesOrganizer.js
@@ +19,5 @@
> +  {type: "forwardButton", localName: "toolbarbutton"},
> +  // Handle differences in elements local names between OSX and rest of the platforms
> +  {type: "organizeButton", localName: mozmill.isMac ? "toolbarbutton" : "menu"},
> +  {type: "viewMenu", localName: mozmill.isMac ? "toolbarbutton" : "menu"},
> +  {type: "maintenanceButton", localName: mozmill.isMac ? "toolbarbutton" : "menu"},

So the distinction between Win XP and other versions is not necessary anymore?

::: firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
@@ +24,5 @@
> +    location: "bookmarks",
> +    title: utils.getProperty("chrome://browser/locale/places/places.properties",
> +                             "OrganizerQueryAllBookmarks")
> +  },{
> +    actions: ["menu"],

What happened with the shortcut test for platforms other than OS X and > Win XP?

@@ +27,5 @@
> +  },{
> +    actions: ["menu"],
> +    location: "history",
> +    title: utils.getProperty("chrome://places/locale/places.properties",
> +                             "finduri-AgeInDays-is-0")

I would propose we retrieve the title below via the getProperty() call of the places window.

Comment 52

[Teodor Druta]

Attachment: placesv11.patch

(In reply to Henrik Skupin (:whimboo) from comment #51)
> Comment on attachment 8534970 [details] [diff] [review]
> placesv10.patch
> 
> Review of attachment 8534970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the inclusion that you get back the shortcut test for the history
> test. Otherwise nits. Thanks for working on it!
> 
> ::: firefox/lib/tests/testPlacesOrganizer.js
> @@ +19,5 @@
> > +  {type: "forwardButton", localName: "toolbarbutton"},
> > +  // Handle differences in elements local names between OSX and rest of the platforms
> > +  {type: "organizeButton", localName: mozmill.isMac ? "toolbarbutton" : "menu"},
> > +  {type: "viewMenu", localName: mozmill.isMac ? "toolbarbutton" : "menu"},
> > +  {type: "maintenanceButton", localName: mozmill.isMac ? "toolbarbutton" : "menu"},
> 
> So the distinction between Win XP and other versions is not necessary
> anymore?
> 

Yes, in my investigations I encountered no differences between win xp and other platforms, which would affect our current library and tests.

> :::
> firefox/tests/functional/testPlacesOrganizer/testOpenClosePlacesOrganizer.js
> @@ +24,5 @@
> > +    location: "bookmarks",
> > +    title: utils.getProperty("chrome://browser/locale/places/places.properties",
> > +                             "OrganizerQueryAllBookmarks")
> > +  },{
> > +    actions: ["menu"],
> 
> What happened with the shortcut test for platforms other than OS X and > Win
> XP?
> 

Mac OS X doesn't have a shortcut for the places history window, added the "shortcut" test back, and handle this operating system accordingly.

> @@ +27,5 @@
> > +  },{
> > +    actions: ["menu"],
> > +    location: "history",
> > +    title: utils.getProperty("chrome://places/locale/places.properties",
> > +                             "finduri-AgeInDays-is-0")
> 
> I would propose we retrieve the title below via the getProperty() call of
> the places window.

Replaced "title" property from the "METHODS" object to "titleProperty", and now they are retrieved below in the test using placesOrganizerWindow.getProperty(aItem.titleProperty).

Comment 53

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536562 [details] [diff] [review]
placesv11.patch

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

Needs updating after landing the page-infi patch.

Comment 54

[Teodor Druta]

Attachment: placesv11.patch

Fixed rejections.

Comment 55

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536586 [details] [diff] [review]
placesv11.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/530536be4b4e (default)

Comment 56

[Teodor Druta]

Attachment: places-aurora-beta.patch

This is the patch for mozilla-beta, and mozilla-aurora branches.

Comment 57

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536610 [details] [diff] [review]
places-aurora-beta.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/6ade312fcd4d (aurora)

Beta I'll land tomorrow, I want to avoid it today when we do the runs.

Comment 58

[Teodor Druta]

Attachment: places-beta.patch

Patch no longer applied on mozilla-beta, updated.

Comment 59

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8545208 [details] [diff] [review]
places-beta.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/130577258a8c (beta)