880417 - Create user interface shared module for metro

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

We need a UI shared module for metro in order to work with all the elements and functionalities.
This will be in a lib/ui/metro folder.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Here are the main libraries for the metro UI. After the discussion today in Ask an expert, I've marked the methods I've made for 'Pin from tab' as deprecated and commented out the code I was using in the test, in case you want to check it and see that dialog.

I'll create another method that can pin/unpin from the about:start page, after selecting an item from the sections in there.

For downloading, I still get the switch to Desktop, even after editing the rdf Jim helped me with in bug 885714. I'll be able to check that further when we can stop the switching.

Wanted to add the patch in order to better understand what I was talking about in the emails, regarding the attribute situation. I'll start to file bugs in order to get the main attributes like "disable", "hidden", "visible" to show the state of an element.
Hope everything will still work fine when you can look at it :)

Comment 2

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

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

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

Please keep in mind that it is not only me who is doing reviews and feedback. I will be away soon so most likely Dave has to do it.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Oh, sorry, I thought only you will do on Metro, will keep that in mind.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

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

This is looking really good. I've added a few comments and nits.

::: lib/ui/metro/downloads.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Missing 'use strict'

@@ +50,5 @@
> +        break;
> +      case "openButton":
> +        elem = nodeCollector.queryAnonymousNode("anonid", "open-button");
> +        break;
> +      case "panelClose":

Inconsistent use of 'button' in the type string. Is it necessary to include it in some cases but not others?

@@ +70,5 @@
> +  /**
> +   * Close the downloads pane (goes back to the previous page)
> +   *
> +   */
> +  close: function close_Pane() {

Should this function be called downloadManager_closePane for consistency? Similar for other functions.

@@ +80,5 @@
> +   * Get the downloads pane
> +   *
> +   * @returns {Boolean} True if the correct page has been loaded
> +   */
> +  downloadPane : function downloadPane() {

Should this be named downloadManager_openPane?

::: lib/ui/metro/findBar.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";

Nit: new line before 'use strict'

@@ +37,5 @@
> +   *        value: Value of the element or property
> +   *
> +   * @returns Element which has been created
> +   */
> +  getElement : function FindBar_getElement(aSpec) {

findBar_getElement (applies elsewhere)

@@ +96,5 @@
> +        findInPage.click();
> +        break;
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", aController.window);
> +        win.keypress("F", {accelKey: true});

Rather than hard-coding, is this available in a DTD?

@@ +98,5 @@
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", aController.window);
> +        win.keypress("F", {accelKey: true});
> +        break;
> +      case "touchEvent":

I think simply 'touch' would be suitable here.

::: lib/ui/metro/metroMode.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Missing 'use strict'

::: lib/ui/metro/tabs.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Missing 'use strict'

@@ +107,5 @@
> +        throw new Error(arguments.callee.name + ": Unknown opening method - " + method);
> +    }
> +
> +    var self = this;
> +    var tabs = this.getElement({type: "tabsContainer"});

could we just call isVisible here, rather than duplicate the code?

@@ +145,5 @@
> +
> +    switch (type) {
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        win.keypress("t", {accelKey:true});

Use a DTD instead of hard-coding?

::: lib/ui/metro/tests/testDownloads.js
@@ +9,5 @@
> +// Include the required modules
> +var { expect } = require("../../../assertions");
> +var downloads = require("../downloads");
> +
> +const DOWNLOAD_URL = "http://mozqa.com/mozmill-env/1.5.21-windows.zip";

Can we use a local resource instead of depending on mozqa.com? Also, should this be named TEST_DATA?

::: lib/ui/metro/tests/testFindBar.js
@@ +32,5 @@
> +  toolbar.open();
> +
> +  findbar.open(controller);
> +
> +  var findContent = findbar.getElement({type: "findContent"});

This is not currently used.

::: lib/ui/metro/tests/testTabs.js
@@ +41,5 @@
> +  expect.ok(!newTabButton.getNode().hidden, "New tab button is visible");
> +
> +  tabs.closeTab();
> +  expect.waitFor(function () {
> +    return tabs.length === 2;

You could use TEST_DATA.length -1 here so we're not hard-coding the expected tab count

@@ +46,5 @@
> +  }, "Tab has been closed");
> +
> +  tabs.openTab();
> +  expect.waitFor(function () {
> +    return tabs.length === 3;

As above, we could make this relative to the length of TEST_DATA

@@ +50,5 @@
> +    return tabs.length === 3;
> +  }, "Another tab has been opened");
> +
> +  tabs.selectTab(1);
> +  expect.contain(tabs.activeTabUrl, "mozilla_mission",

This would break if we changed the contents of TEST_DATA. We should store this value in TEST_DATA to make it less fragile.

@@ +56,5 @@
> +
> +  // Close the first tab
> +  tabs.closeTab("shortcut", 0);
> +  expect.waitFor(function() {
> +    return tabs.activeTabUrl.indexOf("mozilla_community") == -1;

As above.

Comment 5

[Andreea Matei [:AndreeaMatei]]

Thanks for the quick reply!

(In reply to Dave Hunt (:davehunt) from comment #4)
> Comment on attachment 768337 [details] [diff] [review]
> @@ +96,5 @@
> > +        findInPage.click();
> > +        break;
> > +      case "shortcut":
> > +        var win = new findElement.MozMillElement("Elem", aController.window);
> > +        win.keypress("F", {accelKey: true});
> 
> Rather than hard-coding, is this available in a DTD?

At the moment we're blocked here by bug 882055, but I will add a TODO comment to be replaced when we have the keys available.

At the next feedback round, I hope I can stop that switch to Desktop after downloading and be able to check some more elements there, also get the pin methods ready.

Comment 6

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

No update here on that P1 in the last 2 months!! Please get this ready given that we need this for any other upcoming work for the metro testrun.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated the patch with the latest changes, hopefully you'll get a chance to look at it and have some feedback for the next week to finish it. There will be another update for the structure as well, when it's decided. 

We have cases like in tabs.js where some functions from the general tabs.js lib would apply on metro as well. But I'm not sure we want to import 2 tabs.js in our tests, each with part of the code.
So I transfered them in the metro tabs.js. If you feel we can import both, I'll remove them.

I tried to use as many properties/attributes as in the mochitests from MXR (transitionend, isShowing, disabled and so on). There's still a button I don't seem to find yet, the small close button from each open tab in the tabs container. I'll keep looking for it.

For downloading a file, it automatically opens it in a OS window, haven't found a way to disable that yet either so I can check the buttons: "Run, Show in Files, Cancel".

Comment 8

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #7)
> We have cases like in tabs.js where some functions from the general tabs.js
> lib would apply on metro as well. But I'm not sure we want to import 2
> tabs.js in our tests, each with part of the code.

Having those separate would make sense. Features can always change for different products. So it's better to already handle them differently.

Comment 9

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

Comment on attachment 814930 [details] [diff] [review]
patch v2

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

I think that design goes into the right direction. It looks fine and I hope we can get it landed soon. There are some things you will have to update. See my inline comments. Otherwise given that the patch is not ready yet, you should only ask for feedback. You get my f+ on it.

::: lib/ui/metro/downloads.js
@@ +7,5 @@
> +/**
> + * Constructor
> + */
> +function DownloadManager(aController) {
> +  this._controller = aController || null;

How is the download manager displayed in metro? Is it a new window? We might have to talk about the best handling of the controller.

@@ +33,5 @@
> +   *        value: Value of the element or property
> +   * @returns Element which has been created
> +   * @type ElemBase
> +   */
> +  getElement : function downloadManager_getElement(spec) {

getElement is only a wrapper method. All this here has to be added to getElements().

@@ +36,5 @@
> +   */
> +  getElement : function downloadManager_getElement(spec) {
> +    var elem = null;
> +
> +    var nodeCollector = new domUtils.nodeCollector(this._controller.window.document);

I don't see it being used here.

@@ +39,5 @@
> +
> +    var nodeCollector = new domUtils.nodeCollector(this._controller.window.document);
> +    switch(spec.type) {
> +      case "downloadsButton":
> +        elem = new elementslib.ID(this._controller.window.document, 'download-progress');

If we keep using the ID() method, we should do it in the new MozElement way.

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

This file is not related to any work of shared modules on this bug.

::: lib/ui/metro/metroMode.js
@@ +4,5 @@
> +
> +/**
> + * Constructor
> + */
> +function MetroMode(aController) {

Not sure for what this module is for? Is it for the BrowserWindow?

@@ +30,5 @@
> +   *        subtype: Specific element or property
> +   *        value: Value of the element or property
> +   * @returns Element which has been created
> +   */
> +  getElement : function metroMode_getElement(aSpec) {

Same here for getElements().

@@ +35,5 @@
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "flyoutBack":
> +        elem =  new elementslib.ID(this._controller.window.document, 'cmd_flyout_back');

MozElement() style declarations please.

::: lib/ui/metro/tabs.js
@@ +65,5 @@
> +   *        subtype: Specific element or property
> +   *        value: Value of the element or property
> +   * @returns Element which has been created
> +   */
> +  getElement : function metroMode_getElement(aSpec) {

getElements()

@@ +70,5 @@
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "newTabButton":
> +        elem = new elementslib.ID(this._controller.window.document, 'newtab-button');

MozElement() style.

::: lib/ui/metro/tests/manifest.ini
@@ +1,1 @@
> +[testDownloads.js]

Wrong location of all the test files. Those should be part of ´/lib/tests´ and not below the metro ui folder. If we want to refactor this we should do that later for all different kinds of tests.

::: lib/ui/metro/toolbar.js
@@ +36,5 @@
> +   *        subtype: Specific element or property
> +   *        value: Value of the element or property
> +   * @returns {Object} Element which has been created
> +   */
> +  getElement : function toolbar_getElement(aSpec) {

getElements()

@@ +114,5 @@
> +
> +    function onTransitionend() {
> +      transitionend = true;
> +      toolbar.getNode().removeEventListener("transitionend", onTransitionend, false);
> +    }

I should get to bug 808586 so we can finally finish it off. That would help us a lot for all those event handlings.

@@ +123,5 @@
> +    switch (method) {
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        var dtd = ["chrome://browser/locale/browser.dtd"];
> +        var cmdKey = utils.getEntity(dtd, "urlbar.accesskey");

We usually have a getDtd() property on a class for that.

@@ +263,5 @@
> +   *        subtype: Specific element or property
> +   *        value: Value of the element or property
> +   * @returns {Object} Element which has been created
> +   */
> +  getElement : function locationBar_getElement(aSpec) {

getElements()

Comment 10

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> > + * Constructor
> > + */
> > +function DownloadManager(aController) {
> > +  this._controller = aController || null;
> 
> How is the download manager displayed in metro? Is it a new window? We might
> have to talk about the best handling of the controller.

At the moment there is no Download Manager per say. There was at the beginning a different pane where all downloads were listed. That was removed now.

When you download something, you get a download toolbar above the urlbar asking "Do you want to download" with 3 button: Run, Save and Cancel and when the download it's done you have Run, Show in Files and Cancel buttons.

If you download several files, at the end of the second one you get:
"2 downloads have been completed" with "Show in files" button which opens the OS folder.

Should we rename the class?

> ::: lib/ui/metro/findBar.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> This file is not related to any work of shared modules on this bug.

Not sure I follow. You mean it's not related to UI or that it should not be a separate library but be inside toolbars.js?

> ::: lib/ui/metro/metroMode.js
> @@ +4,5 @@
> > +
> > +/**
> > + * Constructor
> > + */
> > +function MetroMode(aController) {
> 
> Not sure for what this module is for? Is it for the BrowserWindow?

It has the elements from about:start: the containers for history, bookmarks, topsites, the buttons from a tab with plus and back arrow (the ones in the laterals). I could move these 2 in tabs actually. I am also adding methods to pin/unpin or delete the tabs in the containers from about:start (if you select a tab you'll get a small button in the top-right corner). This is something like tabview from desktop mode.

> ::: lib/ui/metro/tests/manifest.ini
> @@ +1,1 @@
> > +[testDownloads.js]
> 
> Wrong location of all the test files. Those should be part of ´/lib/tests´
> and not below the metro ui folder. If we want to refactor this we should do
> that later for all different kinds of tests.

Correct. I will move them.

> @@ +123,5 @@
> > +    switch (method) {
> > +      case "shortcut":
> > +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> > +        var dtd = ["chrome://browser/locale/browser.dtd"];
> > +        var cmdKey = utils.getEntity(dtd, "urlbar.accesskey");
> 
> We usually have a getDtd() property on a class for that.

Right. I will create a utils.js lib as well since many methods in the current utils.js are not applying for metro so it's not worth putting it in the general lib bucket.

Comment 11

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #10)
> At the moment there is no Download Manager per say. There was at the
> beginning a different pane where all downloads were listed. That was removed
> now.
> 
> When you download something, you get a download toolbar above the urlbar
> asking "Do you want to download" with 3 button: Run, Save and Cancel and
> when the download it's done you have Run, Show in Files and Cancel buttons.
> 
> If you download several files, at the end of the second one you get:
> "2 downloads have been completed" with "Show in files" button which opens
> the OS folder.
> 
> Should we rename the class?

In that case it is a child of the toolbar too and should be handled that way, yes.

> > ::: lib/ui/metro/findBar.js
> > @@ +1,1 @@
> > > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > 
> > This file is not related to any work of shared modules on this bug.
> 
> Not sure I follow. You mean it's not related to UI or that it should not be
> a separate library but be inside toolbars.js?

Yes, it should be a class in toolbar.js

> > ::: lib/ui/metro/metroMode.js
> > > +function MetroMode(aController) {
> > 
> > Not sure for what this module is for? Is it for the BrowserWindow?
> 
> It has the elements from about:start: the containers for history, bookmarks,
> topsites, the buttons from a tab with plus and back arrow (the ones in the
> laterals). I could move these 2 in tabs actually. I am also adding methods
> to pin/unpin or delete the tabs in the containers from about:start (if you
> select a tab you'll get a small button in the top-right corner). This is
> something like tabview from desktop mode.

Then we should find a better name for it. metroMode is not that helpful, also we know that those are Metro tests.

> > We usually have a getDtd() property on a class for that.
> 
> Right. I will create a utils.js lib as well since many methods in the
> current utils.js are not applying for metro so it's not worth putting it in
> the general lib bucket.

No, you don't have to. The l10n module is part of Mozmill 2.0 and can be used directly. So no need for the helper methods from our own l10n module.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Updating the patch so that contributors can use it until we restructure the repository and we get to the final version.

I've moved downloads.js and findbar.js in toolbars.js as they are toolbars. So we have 3 libraries, but in the tests I've left them separate so we don't make toolbar test too long. And I also think downloads at least will be much more complex when we can support the actual download (see bug 885714).

Addressed all feedback comments besides the l10n module which is getting resolved in bug 928314.

Renamed metroMode into startScreen as this is the term used by the devs as well for the elements in about:start.

Comment 13

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

We might want to check for which branches we should bring in this lib. Was there any announcement about metro support for Firefox yet in an official release? That should be our lowest branch.

Comment 14

[Andrei Eftimie]

Comment on attachment 818979 [details] [diff] [review]
patch v3

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

Nice work Andreea!
I think we're really close with this one.
We should aim to have a tight feedback loop so we can get this landed as soon as possible.

Some small things I've noticed from looking over the patch are inline:

::: metrofirefox/lib/tests/testFindBar.js
@@ +32,5 @@
> +
> +  var nextButton = findbar.getElement({type: "nextButton"});
> +  var previousButton = findbar.getElement({type: "previousButton"});
> +  expect.ok(nextButton.getNode(), "Next button is present");
> +  expect.ok(previousButton.getNode(), "Previous button is present");

I'm not sure that these checks are entirely correct.
Aren't all findbar elements in the DOM from the start?
If that is the case shouldn't we also check for their visibility?

::: metrofirefox/lib/tests/testStartScreen.js
@@ +18,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Check the containers on the main page
> +  var topSitesContainer = metroMode.getElement({type: "startTopSites"});
> +  expect.ok(topSitesContainer.exists(), "Top Sites container is visible");

exists() only check for DOM availability, this will return true even if the element is not visible.
We might want to amend the error message.

::: metrofirefox/lib/tests/testToolbar.js
@@ +70,5 @@
> +
> +  var reloadButton = locationBar.getElement({type: "reloadButton"});
> +  var stopButton = locationBar.getElement({type: "stopButton"});
> +  expect.ok(reloadButton.getNode(), "Reload button is present");
> +  expect.equal(stopButton.getNode().clientWidth, 0, "Stop button is hidden");

This might break if in upcoming versions the stop button visibility will be handled in a different way.
I guess this should suffice for now since we don't have a more robust method of checking visibility.

::: metrofirefox/lib/ui/tabs.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

nit: "use strict";

@@ +80,5 @@
> +   *                             [optional - default: ""]
> +   *                  value    - Value of the attribute to filter
> +   *                             [optional - default: ""]
> +   *                  parent   - Parent of the to find element
> +   *                             [optional - default: document]

nit: shouldn't we use the new JSDOC comments for sub-elements?
http://code.google.com/p/jsdoc-toolkit/wiki/TagParam#Parameters_With_Properties

@@ +148,5 @@
> +    var self = this;
> +    var tabs = this.getElement({type: "tabsContainer"});
> +    assert.waitFor(function () {
> +      return (tabs.getNode().outerHTML.indexOf('startpage="true"') !== -1);
> +    }, "Tabs container is visible");

we could call this.isVisible() here since its the same code.

@@ +158,5 @@
> +   * @returns {Boolean} True if the tabs container is visible
> +   */
> +  isVisible: function toolbar_isVisible() {
> +    var tabs = this.getElement({type: "tabsContainer"});
> +    return (tabs.getNode().outerHTML.indexOf('startpage="true"') !== -1);

Can't we take this directly with getAttribute ?

@@ +172,5 @@
> +  openTab : function tabs_openTab(aEventType) {
> +    var type = aEventType || "shortcut";
> +
> +    // Disable tab closing animation for default behavior
> +    Services.prefs.setBoolPref(PREF_TABS_ANIMATE, false);

1. The comment is misleading. Isn't animation on by default?
2. Do we fail if we're to let the animation on? It would be great to be able to leave this on and wait for the animation to finish.
Cosmin has been working on something similar in bug 890181. I realise that that solution might be overkill right now.
But then again, this is Metro and we might be able to have a simpler fix.

If we leave it like this we might want to open a followup bug to try letting the default animations on.

@@ +181,5 @@
> +    this._controller.window.addEventListener("TabOpen", checkTabOpened, false);
> +
> +    switch (type) {
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);

We can call this before the switch since we use it in multiple cases.

@@ +249,5 @@
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        var cmdKey = utils.getEntity(this.dtds, "closeTab.key");
> +        win.keypress(cmdKey, {accelKey: true});
> +        break;

Wr can also close the tab via the tab toolbar close button, and there _should_ be a gesture as well.
We should ad least add a TODO item here for these.

::: metrofirefox/lib/ui/toolbars.js
@@ +290,5 @@
> +/**
> + * Constructor
> + */
> +function FindBar(aController) {
> +  this._controller = aController || null;

we should enforce the controller here (and in all classes of this library).

If we don't specify a controller at initiation time, we will fail further down in getElements() of other methods as this._controller will be null (or undefined in some of the other classes).
Since we don't have a method of setting a controller later on, we should require it here, and fail if there isn't one.

@@ +389,5 @@
> +    findBar.getNode().addEventListener("transitionend", onTransitionend, false);
> +
> +    switch (method) {
> +      case "menu":
> +        if (!this._toolbar.isVisible()) {

I'm wondering if we shouldn't make this check part of toolbar.open()

Comment 15

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

Comment on attachment 818979 [details] [diff] [review]
patch v3

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

Great start, I agree. But there is still some work to do. See my comments inline.

::: metrofirefox/lib/tests/testDownloads.js
@@ +19,5 @@
> +  controller.open("about:support");
> +  controller.waitForPageLoad();
> +
> +  var downloadsButton = downloads.getElement({type: "downloadsButton"});
> +  expect.ok(downloadsButton.exists(), "Downloads Button is present");

I would suggest you check for ´node.localName´ here.

@@ +22,5 @@
> +  var downloadsButton = downloads.getElement({type: "downloadsButton"});
> +  expect.ok(downloadsButton.exists(), "Downloads Button is present");
> +
> +  // Uncomment this once the deskop switch at the end of a download is stopped
> +  // and add buttons check (run download, Show in Files, cancel download)

Not sure I understand this.

::: metrofirefox/lib/tests/testFindBar.js
@@ +22,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Open findbar via menu button and check it's elements
> +  findbar.open("menu");
> +  var findBar = findbar.getElement({type: "findbar"});

You might want to check localName here too, just to be sure the right element has been found.

@@ +32,5 @@
> +
> +  var nextButton = findbar.getElement({type: "nextButton"});
> +  var previousButton = findbar.getElement({type: "previousButton"});
> +  expect.ok(nextButton.getNode(), "Next button is present");
> +  expect.ok(previousButton.getNode(), "Previous button is present");

I would always use localName and check if the right type of element has been found and we don't get undefined.

::: metrofirefox/lib/tests/testTabs.js
@@ +27,5 @@
> +  });
> +
> +  // Temporary workaround to open the tabs container
> +  // TODO: Replace with touch event
> +  var content = new findElement.ID(controller.window.document, "content");

So why is that workaround necessary? I thought we already have a touch event available?

@@ +60,5 @@
> +  }, "Another 2 tabs have been opened");
> +
> +  tabs.selectTab(1);
> +  expect.contain(tabs.activeTabUrl, "mozilla_mission",
> +                 "Second tab has been selected");

It's better to check for the index as for the contained web page.

@@ +65,5 @@
> +
> +  // Close the first tab
> +  tabs.closeTab("shortcut", 0);
> +  expect.waitFor(function() {
> +    return tabs.activeTabUrl.indexOf("mozilla_community") == -1;

Same here for index.

@@ +69,5 @@
> +    return tabs.activeTabUrl.indexOf("mozilla_community") == -1;
> +  }, "First tab has been closed");
> +
> +  tabs.closeAllTabs();
> +  expect.equal(tabs.length, 1, "All past tabs have been closed");

´All previously opened...´

::: metrofirefox/lib/ui/startScreen.js
@@ +31,5 @@
> +   *                  subtype  - Attribute of the element to filter
> +   *                             [optional - default: ""]
> +   *                  value    - Value of the attribute to filter
> +   *                             [optional - default: ""]
> +   *                  parent   - Parent of the to find element

Please make use of the correct jsdoc @param lines here, which can also include sub-properties. It will help our new community members to get started.

@@ +50,5 @@
> +   * @param {Object} aSpec
> +   *        Information of the UI element which should be retrieved
> +   *        type: General type information
> +   *        subtype: Specific element or property
> +   *        value: Value of the element or property

Same here where details are missing.

@@ +57,5 @@
> +  getElements : function startScreen_getElements(aSpec) {
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "startBookmarks":

I don't see a reason why to carry the ´start´ prefix around. Please remove it.

::: metrofirefox/lib/ui/tabs.js
@@ +105,5 @@
> +  getElements : function tabs_getElements(aSpec) {
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "overlayBack":

I would say we should give better names. We do not have to mimic what devs have used. We have to make it easier for people to work with.

@@ +125,5 @@
> +    return [elem];
> +  },
> +
> +  /**
> +   * Open the tabs container

Please explain what the tabs container is. Not sure I would understand it as of now.

@@ +138,5 @@
> +      case "shortcut":
> +        // TODO: add code for shortcuts, if any
> +        break;
> +      case "touchEvent":
> +        // TODO: add code for touch event, (swipe up)

I think both should work now, right?

@@ +147,5 @@
> +
> +    var self = this;
> +    var tabs = this.getElement({type: "tabsContainer"});
> +    assert.waitFor(function () {
> +      return (tabs.getNode().outerHTML.indexOf('startpage="true"') !== -1);

Is there no specific property or attribute we can wait for? Not sure I like because any other page could equal to true here.

@@ +172,5 @@
> +  openTab : function tabs_openTab(aEventType) {
> +    var type = aEventType || "shortcut";
> +
> +    // Disable tab closing animation for default behavior
> +    Services.prefs.setBoolPref(PREF_TABS_ANIMATE, false);

Yeah, I wonder the same. And the underlying code in Firefox shouldn't be that different I think.

@@ +181,5 @@
> +    this._controller.window.addEventListener("TabOpen", checkTabOpened, false);
> +
> +    switch (type) {
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);

No, please leave it in for each individual case statement. It makes it easier to maintain if the DOM hierarchy gets updated.

@@ +249,5 @@
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        var cmdKey = utils.getEntity(this.dtds, "closeTab.key");
> +        win.keypress(cmdKey, {accelKey: true});
> +        break;

I would better recommend that the test itself is doing that via an optional callback. But that would not apply for shortcuts for sure, or we have to get better support in getting a specific value from the class instance.

@@ +265,5 @@
> +      Services.prefs.clearUserPref(PREF_TABS_ANIMATE);
> +    }
> +  },
> +
> +  selectTab : function tabs_selectTab(aIndex) {

Please add the jsdoc comment.

::: metrofirefox/lib/ui/toolbars.js
@@ +10,5 @@
> +
> +/**
> + * Constructor
> + */
> +function Toolbar(aController) {

Please keep the alphabetical order of classes in this module.

Beside this I miss getters for instances of findbar, locationBar etc. Please check the desktop version of the toolbars.js module.

@@ +89,5 @@
> +        break;
> +      case "contextPopup":
> +        elem = new findElement.ID(this._controller.window.document, 'context-popup');
> +        break;
> +      case "keyBack":

Please also think about better names here.

@@ +170,5 @@
> +        throw new Error(arguments.callee.name + ": Unknown opening method - " + method);
> +    }
> +
> +    assert.waitFor(function () {
> +     return transitionEnd;

Don't forget to remove the event listener.

@@ +189,5 @@
> +  },
> +
> +  /**
> +   * Pin a page
> +   * Deprecated, needs Marionette due to the OS dialog

Deprecated or not supported? Please flag appropriately with a @ flag.

@@ +291,5 @@
> + * Constructor
> + */
> +function FindBar(aController) {
> +  this._controller = aController || null;
> +  this._toolbar = new Toolbar(aController);

I think the findbar is a child of the toolbar and not the other way around.

@@ +346,5 @@
> +    switch(aSpec.type) {
> +      case "findbar":
> +        elem = new findElement.ID(this._controller.window.document, 'findbar');
> +        break;
> +      case "findTextbox":

just ´textbox´?

@@ +353,5 @@
> +      case "closeButton":
> +        elem = new findElement.ID(this._controller.window.document, 'findbar-close-button');
> +        break;
> +      case "findInPage":
> +        elem = new findElement.ID(this._controller.window.document, 'context-findinpage');

This element is not clear to me.

@@ +571,5 @@
> +        break;
> +      case "identityBox":
> +        elem = new findElement.ID(this._controller.window.document, 'identity-box');
> +        break;
> +      case "locationBar":

For desktop we use ´urlBar´ or? Lets stay in sync.

Comment 16

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> We might want to check for which branches we should bring in this lib. Was
> there any announcement about metro support for Firefox yet in an official
> release? That should be our lowest branch.

From the Desktop team I understand Metro is now in default and aurora and it's supposed to be in beta next week, but there's still work to do there so we'll have to check back later.

This is the latest status I found on their page (https://wiki.mozilla.org/Firefox/Metro):
https://wiki.mozilla.org/Firefox/Planning/2013-09-18#Firefox_Metro

Comment 17

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

Ok, so we only care about 26 and 27 then. Thanks.

Comment 18

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

What's blocking us here? No update since nearly two months! It causes us to have not a single Metro test in our tree. Can we please get this P1 done ASAP?

Comment 19

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

I'm still waiting for feedback here. Please let me know about the status.

Comment 20

[Andreea Matei [:AndreeaMatei]]

I've done all the requested updates, but then I was on infrastructure duty last week and this one I was trying to test it with the latest build to make sure nothing has changed when I encountered bug 951715 so I was unable to do that. I'll work on that with Andrei to get it fixed.

Comment 21

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4

Updated the patch. Comments will come inline from the other reviews to answer some questions.

Comment 22

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> > +  // Uncomment this once the deskop switch at the end of a download is stopped
> > +  // and add buttons check (run download, Show in Files, cancel download)
> 
> Not sure I understand this.
At the moment we can't check the download or the buttons after a download has finished because we are switched on desktop and the folder with the download gets opened.

> ::: metrofirefox/lib/tests/testTabs.js
> @@ +27,5 @@
> > +  });
> > +
> > +  // Temporary workaround to open the tabs container
> > +  // TODO: Replace with touch event
> > +  var content = new findElement.ID(controller.window.document, "content");
> 
> So why is that workaround necessary? I thought we already have a touch event
> available?

No, we don't have at this point a swipe up/down event that would make the tabs container to come up. 

> @@ +138,5 @@
> > +      case "shortcut":
> > +        // TODO: add code for shortcuts, if any
> > +        break;
> > +      case "touchEvent":
> > +        // TODO: add code for touch event, (swipe up)
> 
> I think both should work now, right?

Unfortunately no, for the touch event I explained above and I haven't found any shortcut at this time.

> @@ +147,5 @@
> > +
> > +    var self = this;
> > +    var tabs = this.getElement({type: "tabsContainer"});
> > +    assert.waitFor(function () {
> > +      return (tabs.getNode().outerHTML.indexOf('startpage="true"') !== -1);
> 
> Is there no specific property or attribute we can wait for? Not sure I like
> because any other page could equal to true here.

This was the only difference I found, compared all attributes for each state.

> > +      case "findInPage":
> > +        elem = new findElement.ID(this._controller.window.document, 'context-findinpage');
> 
> This element is not clear to me.
I'm using the same name as in the context menu where it appears. In the right of the urlbar, at the menu button, we have "Find in page".

Comment 23

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #22)
> (In reply to Henrik Skupin (:whimboo) from comment #15)
> > > +  // Uncomment this once the deskop switch at the end of a download is stopped
> > > +  // and add buttons check (run download, Show in Files, cancel download)
> > 
> > Not sure I understand this.
> At the moment we can't check the download or the buttons after a download
> has finished because we are switched on desktop and the folder with the
> download gets opened.

Then please correct the comment and mention something like 'once the download folder is not opened after downloading the file'. Also I want to see a bug reference here. If not such a bug exists, or we cannot control the behavior via a preference, please file one.

> > > +  // Temporary workaround to open the tabs container
> > > +  // TODO: Replace with touch event
> > > +  var content = new findElement.ID(controller.window.document, "content");
> > 
> > So why is that workaround necessary? I thought we already have a touch event
> > available?
> 
> No, we don't have at this point a swipe up/down event that would make the
> tabs container to come up. 

Has a bug been filed here? If not do so and reference the number as usual.

> > > +      case "shortcut":
> > > +        // TODO: add code for shortcuts, if any
> > > +        break;
> > > +      case "touchEvent":
> > > +        // TODO: add code for touch event, (swipe up)
> > 
> > I think both should work now, right?
> 
> Unfortunately no, for the touch event I explained above and I haven't found
> any shortcut at this time.

Please ask a dev about the plan of a shortcut if you cannot find anything in the source files. Not sure if such one is helpful on a touch device though.

> > > +    var self = this;
> > > +    var tabs = this.getElement({type: "tabsContainer"});
> > > +    assert.waitFor(function () {
> > > +      return (tabs.getNode().outerHTML.indexOf('startpage="true"') !== -1);
> > 
> > Is there no specific property or attribute we can wait for? Not sure I like
> > because any other page could equal to true here.
> 
> This was the only difference I found, compared all attributes for each state.

Have you asked a dev? I really would like to have a better solution here.

> > > +      case "findInPage":
> > > +        elem = new findElement.ID(this._controller.window.document, 'context-findinpage');
> > 
> > This element is not clear to me.
> I'm using the same name as in the context menu where it appears. In the
> right of the urlbar, at the menu button, we have "Find in page".

If that is a context menu item it should be accessible via the Menu API but not directly via getElement.

Comment 24

[Andrei Eftimie]

> > No, we don't have at this point a swipe up/down event that would make the
> > tabs container to come up. 
>
> Has a bug been filed here? If not do so and reference the number as usual.

We have `touchDrag` (swipe) which should do what you want here.

I've started writing a new bug for more advanced touch gestures, but I've come short.
We actually have all the building blocks, what might help is to have helpers -- as in specific gestures like `swipeFromTop` which would be a shortcut to `touchDrag(50%,0,50%,100px)`

That should not block this bug.

Comment 25

[Andrei Eftimie]

I've also just filed bug 957547 to implement more advanced touch gestures.

Comment 26

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

Comment on attachment 8356591 [details] [diff] [review]
patch v4

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

::: metrofirefox/lib/tests/testDownloads.js
@@ +7,5 @@
> +// Include the required modules
> +var { expect } = require("../../../lib/assertions");
> +var toolbars = require("../ui/toolbars");
> +
> +const DOWNLOAD_URL = "http://mozqa.com/mozmill-env/1.5.21-windows.zip";

Can you please update to something else like a video file on mozqa.com? The files in this folder will not exist permanently, especially not for 1.5.x.

@@ +24,5 @@
> +               "Downloads Button is present");
> +
> +  // TODO: Test downloading a file once the switch to desktop at the end of the
> +  // download is fixed. Also check buttons: run download, Show in Files, cancel.
> +  //controller.open(DOWNLOAD_URL);

The tests you want to do here should not be part of this test module but really under metrofirefox/tests/functional. Under lib we only test the basic functionality like accessing elements.

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

Why a separate file for those tests? It should be part of the testToolBar.js module.

@@ +10,5 @@
> +
> +const BASE_URL = collector.addHttpResource("../../../data/");
> +const TEST_DATA = BASE_URL + "layout/mozilla_community.html";
> +
> +var setupModule = function(aModule) {

Please check the coding styles i.e. blanks before the opening bracket. That's happening across all the files.

@@ +13,5 @@
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.findbar = new toolbars.FindBar(aModule.controller);
> +  aModule.toolbar = new toolbars.Toolbar(aModule.controller);

The Toolbar class should be able to give you an instance of the findbar.

@@ +24,5 @@
> +  // Open findbar via menu button and check it's elements
> +  findbar.open("menu");
> +  var findBar = findbar.getElement({type: "findbar"});
> +  expect.equal(findBar.getNode().localName, "appbar", "Find bar is present");
> +  expect.ok(findBar.getNode().isShowing, "Findbar has been opened");

Don't we have a method on the FindBar class which gives us the current state and which we can compare with here?

@@ +28,5 @@
> +  expect.ok(findBar.getNode().isShowing, "Findbar has been opened");
> +
> +  var textbox = findbar.getElement({type: "textbox"});
> +  controller.type(textbox, "project");
> +  expect.equal(textbox.getNode().value, "project", "Expected text is present");

Don't we have a property on the FindBar class to retrieve the current search string?

@@ +39,5 @@
> +               "Previous button is present");
> +
> +  // Close findbar via the close button
> +  findbar.close();
> +  expect.ok(!findBar.getNode().isShowing, "Findbar has been closed");

Same as above also for the following tests in that file.

::: metrofirefox/lib/tests/testStartScreen.js
@@ +9,5 @@
> +var startScreen = require("../ui/startScreen");
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.metroMode = new startScreen.StartScreen(aModule.controller);

Why not calling the variable startscreen?

@@ +24,5 @@
> +  var bookmarksContainer = metroMode.getElement({type: "startBookmarks"});
> +  expect.equal(bookmarksContainer.getNode().localName, "vbox", "Bookmarks container is visible");
> +
> +  var historyContainer = metroMode.getElement({type: "startHistory"});
> +  expect.equal(historyContainer.getNode().localName, "vbox", "History container is visible");

Why do we need all those containers? Those are just vbox elements we never depend on in the past.

::: metrofirefox/lib/tests/testTabs.js
@@ +33,5 @@
> +  // Temporary workaround to open the tabs container
> +  // TODO: Replace with touch event
> +  var content = new findElement.ID(controller.window.document, "content");
> +  content.click();
> +  content.rightClick();

Where is a bug reference?

@@ +40,5 @@
> +    return tabs.isVisible();
> +  }, "Tabs container is visible");
> +
> +  // Check the buttons on each side
> +  var sideBack = tabs.getElement({type: "sideBack"});

I wouldn't use a 'side' prefix here for the element type. That's misleading. Also I miss the 'Button' suffix. Can we use a name like 'sidebar_backButton' instead? And similar for other elements in that area.

@@ +42,5 @@
> +
> +  // Check the buttons on each side
> +  var sideBack = tabs.getElement({type: "sideBack"});
> +  expect.ok(sideBack.getNode(),
> +            "Back button in tab is visible");

This is a library test so here you also have to check for the localName.

@@ +60,5 @@
> +  tabs.openTab();
> +  tabs.openTab("shortcut2");
> +  expect.waitFor(function () {
> +    return tabs.length === 4;
> +  }, "Another 2 tabs have been opened");

Shortcut tests I would move over to real functional tests. Nothing we should do here. We have to keep lib tests to the bare minimum and only do additional actions if elements won't be existent.

@@ +70,5 @@
> +  tabs.closeTab("button", 0);
> +  expect.waitFor(function() {
> +    return tabs.selectedIndex == 0 &&
> +           tabs.activeTabUrl.indexOf("mozilla_community") == -1;
> +  }, "First tab has been closed");

Why do we need waitFor here? Doesn't wait closeTab itself? If not, it should do.

::: metrofirefox/lib/tests/testToolbar.js
@@ +16,5 @@
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.findbar = new toolbars.FindBar(aModule.controller);
> +  aModule.locationBar = new toolbars.LocationBar(aModule.controller);

Similar to the FindBar, the Toolbar class should be able to return an instance of the LocationBar.

@@ +17,5 @@
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.findbar = new toolbars.FindBar(aModule.controller);
> +  aModule.locationBar = new toolbars.LocationBar(aModule.controller);
> +  aModule.toolbar = new toolbars.Toolbar(aModule.controller);

Why no camelcase for Toolbar?

@@ +26,5 @@
> +  expect.waitFor(function () {
> +    return locationBar.value === "url";
> +  }, "Correct text has been typed in location bar");
> +
> +  var closeSearch = toolbar.getElement({type: "closeButton"});

Not sure what kind of button this is. Please give a better name for 'closeButton'.

@@ +43,5 @@
> +  var menuButton = toolbar.getElement({type: "menuButton"});
> +  var pinButton = toolbar.getElement({type: "pinButton"});
> +  expect.ok(starButton.exists(), "Bookmark Button is present");
> +  expect.ok(menuButton.exists(), "Menu Button is present");
> +  expect.ok(pinButton.exists(), "Pin Button is present");

Why no check for localName here?

@@ +49,5 @@
> +  // Check the navigation buttons
> +  var backButton = locationBar.getElement({type: "backButton"});
> +  var forwardButton = locationBar.getElement({type: "forwardButton"});
> +  expect.ok(!backButton.getNode().disabled, "Back button in location bar is visible");
> +  expect.ok(forwardButton.getNode().disabled, "Forward button is hidden");

Why no check for localName here? The tests you are doing here should be better real functional tests.

@@ +62,5 @@
> +
> +  forwardButton.getNode().addEventListener("transitionend", onTransitionend, false);
> +
> +  backButton.tap();
> +  controller.waitForPageLoad();

Same for that. It's too complex for a unit test.

@@ +71,5 @@
> +
> +  var reloadButton = locationBar.getElement({type: "reloadButton"});
> +  var stopButton = locationBar.getElement({type: "stopButton"});
> +  expect.equal(reloadButton.getNode().localName, "toolbarbutton", "Reload button is present");
> +  expect.equal(stopButton.getNode().clientWidth, 0, "Stop button is hidden");

Test for localName and not for the visibility. That's not part of a unit test.

@@ +77,5 @@
> +  // Bookmark and Pin options
> +  toolbar.bookmarkPage();
> +  expect.waitFor(function () {
> +    return starButton.getNode().checked;
> +  }, "Page has been bookmarked");

Same here regarding a real test. I think we should better define what unit tests should contain.

::: metrofirefox/lib/ui/startScreen.js
@@ +41,5 @@
> +  getElement : function screen_getElement(aSpec) {
> +    var elements = this.getElements(aSpec);
> +
> +    return (elements.length > 0) ? elements[0] : undefined;
> +  },

Can you please file a new bug which covers the creation of a base ui class which already contains the basic methods and properties? I don't see why we have to define them over and over again.

@@ +61,5 @@
> +  getElements : function screen_getElements(aSpec) {
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "startBookmarks":

I still don't see a reason why we need the 'start' prefix. The class is already the container. Also what type are those entries?

::: metrofirefox/lib/ui/tabs.js
@@ +37,5 @@
> +   *
> +   * @returns {Number} Number of tabs opened
> +   */
> +  get length() {
> +    return this._controller.tabs.length;

There is no ui element existent we could make use of?

@@ +46,5 @@
> +   *
> +   * @returns {Number} Index of the active tab
> +   */
> +  get selectedIndex() {
> +    return this._controller.tabs.activeTabIndex;

Same here.

@@ +50,5 @@
> +    return this._controller.tabs.activeTabIndex;
> +  },
> +
> +  /**
> +   * Returns the url location of the active tab

url and location is doubled. Stick by location or URL.

@@ +55,5 @@
> +   *
> +   * @returns {String} Url of the active tab
> +   */
> +  get activeTabUrl() {
> +    return this._controller.tabs.activeTab.location.href;

Same as above regarding a real XULElement.

@@ +59,5 @@
> +    return this._controller.tabs.activeTab.location.href;
> +  },
> +
> +  /**
> +   * Gets all the needed external DTD urls as an array

URLs please

@@ +62,5 @@
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns URL's of external DTD files
> +   * @type {String[]}

In a single line please.

@@ +131,5 @@
> +          case "index":
> +            elem = this._controller.browserObject.tabs[aSpec.value];
> +            break;
> +        }
> +        break;

Why do we need this? We already have getTab().

@@ +165,5 @@
> +   * Check if the tabs container at the top of the browser is visible
> +   *
> +   * @returns {Boolean} True if the tabs container is visible
> +   */
> +  isVisible: function toolbar_isVisible() {

You missed to rename the toolbar prefix.

@@ +176,5 @@
> +   *
> +   * @param {string} aMethod
> +   *        Specifies a method for opening the tabs container
> +   */
> +  open: function tabs_open(aMethod) {

Please give it a better name. Open is a bit too vague for me.

@@ +193,5 @@
> +
> +    var self = this;
> +    var tabs = this.getElement({type: "tabsContainer"});
> +    assert.waitFor(function () {
> +      return this.isVisible();

Are there no events we have to wait for? If thats the case the above line should contain self and not this.

@@ +253,5 @@
> +    while (this.length > 1) {
> +      this.closeTab();
> +    }
> +
> +    this._controller.open("about:start");

Do we have to hard-code this value? I would prefer to read that from a pref or do it like for desktop via this._controller.window.BROWSER_NEW_TAB_URL

@@ +306,5 @@
> +   * @param {number} aIndex
> +   *        Index of the tab which should be selected
> +   */
> +  selectTab : function tabs_selectTab(aIndex) {
> +    return this._controller.tabs.selectTabIndex(aIndex);

Why not via a getter and setter like for firefox desktop? Please try to keep the API as close as possible.

::: metrofirefox/lib/ui/toolbars.js
@@ +70,5 @@
> +    switch(aSpec.type) {
> +      case "downloadsButton":
> +        elem = new findElement.ID(this._controller.window.document, 'download-progress');
> +        break;
> +      case "panelClose":

'closeButton'

@@ +86,5 @@
> + * Constructor
> + */
> +function FindBar(aController) {
> +  this._controller = aController || null;
> +  this._toolbar = new Toolbar(aController);

The Toolbar class should crate the default instance and pass in itself as param.

@@ +196,5 @@
> +        var menuButton = this._toolbar.getElement({type: "menuButton"});
> +        menuButton.tap();
> +
> +        var findInPage = this.getElement({type: "findInPage"});
> +        findInPage.tap();

This method is about opening the find bar but not focusing it. Please remove those two lines. If it is not auto-focused I wonder if we should file a bug.

@@ +238,5 @@
> +        closeButton.tap();
> +        break;
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        win.keypress("VK_ESCAPE", {accelKey: true});

Escape I wouldn't call a shortcut here. Does "find.key" not toggle the bar?

@@ +258,5 @@
> + * @param {MozmillController} controller
> + *        MozMillController of the window to operate on
> + */
> +function LocationBar(aController) {
> +  this._controller = aController;

Same as for the findbar constructor.

@@ +301,5 @@
> +  clear : function locationBar_clear() {
> +    this.focus();
> +    this._controller.keypress(this.urlbar, "VK_DELETE");
> +    assert.waitFor(function () {
> +      return this.urlbar.getNode().value === '';

I would do this with self.

@@ +312,5 @@
> +   * @param {String} text
> +   *        Text which should be checked against
> +   */
> +  contains : function locationBar_contains(aText) {
> +    return this.urlbar.getNode().value.indexOf(aText) != -1;

Why not 'this.value.indexOf'?

@@ +321,5 @@
> +   */
> +  focus : function locationBar_focus() {
> +    this.urlbar.tap();
> +    assert.waitFor(function () {
> +      return this.urlbar.getNode().getAttribute('focused') === 'true';

Or better wait for the event? I would also use self here.

@@ +375,5 @@
> +      case "identityBox":
> +        elem = new findElement.ID(this._controller.window.document, 'identity-box');
> +        break;
> +      case "locationBar":
> +        elem = new findElement.ID(this.controller.window.document, 'urlbar-edit');

Better 'input' or 'textbox'?

@@ +411,5 @@
> +  this._controller = aController || null;
> +}
> +
> +/**
> + * Prototype definition of the Tools Bar class

'tool bar class'

@@ +427,5 @@
> +  /**
> +   * Gets all the needed external DTD urls as an array
> +   *
> +   * @returns URL's of external DTD files
> +   * @type {String[]}

Single line and URLs

@@ +481,5 @@
> +      case "closeButton":
> +        elem = new findElement.ID(this._controller.window.document, 'panel-close-button');
> +        break;
> +      case "contextApp":
> +        elem = new findElement.ID(this._controller.window.document, 'contextappbar');

Isn't appbar the findbar? You should better name the element then.

@@ +484,5 @@
> +      case "contextApp":
> +        elem = new findElement.ID(this._controller.window.document, 'contextappbar');
> +        break;
> +      case "contextAction":
> +        elem = new findElement.ID(this._controller.window.document, 'contextualactions-tray');

Same here for the name. It's hard to get what it is.

@@ +505,5 @@
> +      case "starButton":
> +        elem = new findElement.ID(this._controller.window.document, 'star-button');
> +        break;
> +      case "toolbar":
> +        elem = new findElement.ID(this._controller.window.document, 'toolbar');

What is navbar vs toolbar?

@@ +508,5 @@
> +      case "toolbar":
> +        elem = new findElement.ID(this._controller.window.document, 'toolbar');
> +        break;
> +      case "toolbarClose":
> +        elem = new findElement.ID(this._controller.window.document, 'close-button');

This is already part of the Downloads class. Where does this button belong to?

@@ +586,5 @@
> +  },
> +
> +  /**
> +   * Pin a page
> +   * @deprecated - Needs Marionette due to the OS dialog

deprecated means something else. So please remove it, but keep why it is not working atm. Which kind of dialog is that and what does pin do?

Comment 27

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v5

Updated patch, comments inline to the previous review.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8356591 [details] [diff] [review]
patch v4

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

::: metrofirefox/lib/tests/testDownloads.js
@@ +7,5 @@
> +// Include the required modules
> +var { expect } = require("../../../lib/assertions");
> +var toolbars = require("../ui/toolbars");
> +
> +const DOWNLOAD_URL = "http://mozqa.com/mozmill-env/1.5.21-windows.zip";

Since we don't need to download anything here, I removed it.

::: metrofirefox/lib/tests/testStartScreen.js
@@ +24,5 @@
> +  var bookmarksContainer = metroMode.getElement({type: "startBookmarks"});
> +  expect.equal(bookmarksContainer.getNode().localName, "vbox", "Bookmarks container is visible");
> +
> +  var historyContainer = metroMode.getElement({type: "startHistory"});
> +  expect.equal(historyContainer.getNode().localName, "vbox", "History container is visible");

These are the containers that hold the tabs lists, I thought we might need them. For the tabs we have actions we could do on them (long press on one will open a toolbar to allow us to pin the item or delete it)

::: metrofirefox/lib/tests/testTabs.js
@@ +33,5 @@
> +  // Temporary workaround to open the tabs container
> +  // TODO: Replace with touch event
> +  var content = new findElement.ID(controller.window.document, "content");
> +  content.click();
> +  content.rightClick();

Used touchDrag() now.

@@ +70,5 @@
> +  tabs.closeTab("button", 0);
> +  expect.waitFor(function() {
> +    return tabs.selectedIndex == 0 &&
> +           tabs.activeTabUrl.indexOf("mozilla_community") == -1;
> +  }, "First tab has been closed");

We do wait, but I knew we currently double check anyway, in the library tests. Changed to expect.ok now.

::: metrofirefox/lib/tests/testToolbar.js
@@ +26,5 @@
> +  expect.waitFor(function () {
> +    return locationBar.value === "url";
> +  }, "Correct text has been typed in location bar");
> +
> +  var closeSearch = toolbar.getElement({type: "closeButton"});

It's the close button that dismisses the search suggestions, from when we type in the location bar. Renamed it.

::: metrofirefox/lib/ui/tabs.js
@@ +131,5 @@
> +          case "index":
> +            elem = this._controller.browserObject.tabs[aSpec.value];
> +            break;
> +        }
> +        break;

Removed it.

::: metrofirefox/lib/ui/toolbars.js
@@ +196,5 @@
> +        var menuButton = this._toolbar.getElement({type: "menuButton"});
> +        menuButton.tap();
> +
> +        var findInPage = this.getElement({type: "findInPage"});
> +        findInPage.tap();

It's not focusing it. It's only opening. We need to click menu-button in the right of the toolbar, then in the context menu click the 'Find in page' item which opens the findbar.

@@ +238,5 @@
> +        closeButton.tap();
> +        break;
> +      case "shortcut":
> +        var win = new findElement.MozMillElement("Elem", this._controller.window);
> +        win.keypress("VK_ESCAPE", {accelKey: true});

No, unfortunately does not toggle.

@@ +481,5 @@
> +      case "closeButton":
> +        elem = new findElement.ID(this._controller.window.document, 'panel-close-button');
> +        break;
> +      case "contextApp":
> +        elem = new findElement.ID(this._controller.window.document, 'contextappbar');

No. Appbar is the toolbar that opens from the start page, when you long-press over a tab. I moved all the related items to startScreen library since we will only find them there. I was not sure, since it's a toolbar, but it would have been messy here (there are other 5 elements in that toolbar) and it's only related to the start page.

@@ +505,5 @@
> +      case "starButton":
> +        elem = new findElement.ID(this._controller.window.document, 'star-button');
> +        break;
> +      case "toolbar":
> +        elem = new findElement.ID(this._controller.window.document, 'toolbar');

toolbar in wrapped in navbar.

@@ +508,5 @@
> +      case "toolbar":
> +        elem = new findElement.ID(this._controller.window.document, 'toolbar');
> +        break;
> +      case "toolbarClose":
> +        elem = new findElement.ID(this._controller.window.document, 'close-button');

It belongs here. It's the same for both, but general it's a toolbar closing button.

@@ +586,5 @@
> +  },
> +
> +  /**
> +   * Pin a page
> +   * @deprecated - Needs Marionette due to the OS dialog

Updated the comment to explain better what pin is trying to do. It regards the start mode apps from metro, outside the browser.

Comment 29

[Andrei Eftimie]

Comment on attachment 8360462 [details] [diff] [review]
patch v5

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

Some small issues and nits.

Overall looks good.
Lets aim landing this early next week.

::: metrofirefox/lib/tests/testToolbar.js
@@ +26,5 @@
> +  // Open findbar via menu button and check it's elements
> +  toolbar.findBar.open("menu");
> +  var findBar = toolbar.findBar.getElement({type: "findbar"});
> +  expect.equal(findBar.getNode().localName, "appbar", "Find bar is present");
> +  expect.ok(findBar.getNode().isShowing, "Findbar has been opened");

Since you added the method `isOpen()` in the findBar class we should probably call that directly.
Also applies to the rest of this test.

::: metrofirefox/lib/ui/tabs.js
@@ +13,5 @@
> +/**
> + * Constructor
> + */
> +function Tabs(aController) {
> +  this._controller = aController || null;

Something is not right here.
Setting this to null doesn't help us in any way. We have no other place where we can reassign the controller.

We should probably throw an error is no controller is specified when instantiating this class, as everything else will fail in this case.

Same thing in the toolbars lib and the startScreen lib

@@ +102,5 @@
> +   * @returns {Object[]} Array of elements which have been found
> +   */
> +  getElements : function tabs_getElements(aSpec) {
> +    var elem = null;
> +    var document = this._controller.window.document;

If we are already caching `document` here, we could also use it in all further findElement calls

@@ +172,5 @@
> +        // TODO: add code for shortcuts, if any
> +        break;
> +      case "touchEvent":
> +        var chromeTab = new findElement.MozMillElement("Elem", this.getTab().chromeTab);
> +        chromeTab.touchDrag();

Does this work?
This method needs coordinates for start- and end-position.

::: metrofirefox/lib/ui/toolbars.js
@@ +174,5 @@
> +
> +  /**
> +   * Returns the state of the find bar (opened/closed)
> +   */
> +  isOpen : function findbar_isOpen() {

This should be a getter.

@@ +176,5 @@
> +   * Returns the state of the find bar (opened/closed)
> +   */
> +  isOpen : function findbar_isOpen() {
> +    var findbar = this.getElement({type: "findbar"});
> +    return findbar.getNode().isShowing();

I think this shouldn't be a function call.

@@ +190,5 @@
> +    var method = aMethod || "menu";
> +
> +    var transitionend = false;
> +
> +    function onTransitionend() {

We might want to use camelCase for the whole name: `onTransitionEnd`
This will also make it on par with the regular tabs lib.

@@ +270,5 @@
> +   *        String term to search for in the find bar
> +   */
> +  type : function FindBar_type(aSearchTerm) {
> +    var textbox = this.getElement({type: "textbox"});
> +    this._controller.type(textbox, aSearchTerm);

You should use the mozelement analogue method `sendKeys()` instead of `type()`

@@ +322,5 @@
> +   * Clear the location bar
> +   */
> +  clear : function locationBar_clear() {
> +    this.focus();
> +    this._controller.keypress(this.urlbar, "VK_DELETE");

Lets not use deprecated actions directly on the controller.
> this.urlbar.keypress("VK_DELETE");

@@ +326,5 @@
> +    this._controller.keypress(this.urlbar, "VK_DELETE");
> +    var self = this;
> +    assert.waitFor(function () {
> +      return self.urlbar.getNode().value === '';
> +    }, "Location bar has been cleared.", undefined, undefined, this);

No need for the last 3 arguments.

@@ +423,5 @@
> +   *        Text to enter into the location bar
> +   */
> +  type : function locationBar_type(aText) {
> +    var location = this.getElement({type: "locationBar"});
> +    this._controller.type(location, aText);

location.sendKeys(aText);

@@ +584,5 @@
> +    var method = aMethod || "shortcut";
> +
> +    var transitionend = false;
> +
> +    function onTransitionend() {

nit: Same camelCase issue: onTransitionEnd

Comment 30

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v6

Updated as requested.

Unfortunately, touchDrag() does not work to open the tabs container, I've added the coordinates too.

Comment 31

[Andrei Eftimie]

> Unfortunately, touchDrag() does not work to open the tabs container, I've added
> the coordinates too.
That should not block us right now.

As a note on how to figure this out:
We should use monitorEvents() to capture all event's triggered on a touch device and see exactly what is being triggered when the tabs container is opened via a swipe from the top.

Or look into FF Metro code in mxr to see what events are hooked for opening the tabs container.

Comment 32

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

Comment on attachment 8361704 [details] [diff] [review]
patch v6

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

::: metrofirefox/lib/tests/testStartScreen.js
@@ +24,5 @@
> +  var bookmarksContainer = startScreen.getElement({type: "bookmarks"});
> +  expect.equal(bookmarksContainer.getNode().localName, "vbox", "Bookmarks container is visible");
> +
> +  var historyContainer = startScreen.getElement({type: "history"});
> +  expect.equal(historyContainer.getNode().localName, "vbox", "History container is visible");

I would create an array constant with the type values and do all the checks in a for loop.

::: metrofirefox/lib/tests/testTabs.js
@@ +41,5 @@
> +               "Plus button in tab is visible");
> +
> +  var newTabButton = tabs.getElement({type: "newTabButton"});
> +  expect.equal(newTabButton.getNode().localName, "toolbarbutton",
> +               "New tab button is visible");

Same here for the array constant and the loop.

@@ +62,5 @@
> +  expect.ok(tabs.selectedIndex == 0 &&
> +            controller.tabs.activeTab.location.href.indexOf("mozilla_community") == -1,
> +            "First tab has been closed");
> +
> +  tabs.closeAllTabs();

You also need a teardownModule in case of failures.

::: metrofirefox/lib/tests/testToolbar.js
@@ +107,5 @@
> +  var findInPage = toolbar.findBar.getElement({type: "findInPage"});
> +  expect.equal(viewOnDesktop.getNode().localName, "richlistitem",
> +               "View on desktop option is present");
> +  expect.equal(findInPage.getNode().localName, "richlistitem",
> +               "Find in page option is present");

I would do all this with an array constant and a for loop.

::: metrofirefox/lib/ui/startScreen.js
@@ +5,5 @@
> +/**
> + * Constructor
> + */
> +function StartScreen(aController) {
> +  this._controller = aController || null;

The start screen is the same window. So we cannot default to null here. We have to force a valid instance instead.

@@ +67,5 @@
> +        break;
> +      case "clearSelectedButton":
> +        elem = new findElement.ID(this._controller.window.document, 'clear-selected-button');
> +        break;
> +      case "contextAppbar":

I would still prefer a better name here. What is this context menu in detail?

@@ +70,5 @@
> +        break;
> +      case "contextAppbar":
> +        elem = new findElement.ID(this._controller.window.document, 'contextappbar');
> +        break;
> +      case "contextAction":

Is that the context menu for the whole page or specific elements?

@@ +73,5 @@
> +        break;
> +      case "contextAction":
> +        elem = new findElement.ID(this._controller.window.document, 'contextualactions-tray');
> +        break;
> +      case "contextPopup":

And what about this?

@@ +76,5 @@
> +        break;
> +      case "contextPopup":
> +        elem = new findElement.ID(this._controller.window.document, 'context-popup');
> +        break;
> +      case "deleteSelectedButton":

I think it should be enough to name it 'deleteButton'. We don't have and don't want to sync the name with the underlying implementation. It would force us later to also rename our elements. The same would apply to the other buttons here.

@@ +85,5 @@
> +        break;
> +      case "history":
> +        elem = new findElement.ID(this._controller.window.document, 'start-history');
> +        break;
> +      case "pinToStart":

This still misses the Button suffix. Please make sure to check all entries. I have mentioned this in my last review already.

@@ +101,5 @@
> +      case "topSites":
> +        elem = new findElement.ID(this._controller.window.document, 'start-topsites');
> +        break;
> +      case "unpinSelected":
> +        elem = new findElement.ID(this._controller.window.document, 'unpin-selected-button');

Same here with button

::: metrofirefox/lib/ui/tabs.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

Any reason why this is a separate module? Aren't tabs part of the toolbar? I wonder if we should start putting sub-ui classes also into sub folders with the parent name.

@@ +12,5 @@
> +
> +/**
> + * Constructor
> + */
> +function Tabs(aController) {

Is tabs separate or a child of the toolbars in metro? Also why not TabBrowser as in desktop?

@@ +13,5 @@
> +/**
> + * Constructor
> + */
> +function Tabs(aController) {
> +  this._controller = aController;

We should throw if null or undefined is passed in.

@@ +58,5 @@
> +   * Gets all the needed external DTD URLs as an array
> +   *
> +   * @returns {String[]} URL's of external DTD files
> +   */
> +  get dtds() {

Keep the alphabetical order.

@@ +144,5 @@
> +  getTab : function tabs_getTab(aIndex) {
> +    if (aIndex === undefined)
> +      aIndex = this.selectedIndex;
> +
> +    return this._controller.browserObject.tabs[aIndex];

This returns the underlying tab object, which should also be reachable via controller.tabs. Also it differs from the desktop implementation were it returns the ui element. We should keep it in sync.

::: metrofirefox/lib/ui/toolbars.js
@@ +11,5 @@
> +/**
> + * Constructor
> + */
> +function Downloads(aController) {
> +  this._controller = aController;

A controller has to be existent.

@@ +67,5 @@
> +  getElements : function downloads_getElements(aSpec) {
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "downloadsButton":

This is not a button.

@@ +101,5 @@
> +
> +  /**
> +   * Returns the current value in the find bar
> +   */
> +  get value() {

alphabetical order for getters please.

@@ +113,5 @@
> +  get isOpen() {
> +    var findbar = this.getElement({type: "findbar"});
> +    return findbar.getNode().isShowing;
> +  },
> +  /**

nit: missing empty line

@@ +191,5 @@
> +    var transitioned = false;
> +
> +    function onTransitionEnd() {
> +      transitioned = true;
> +      findBar.getNode().removeEventListener("transitionend", onTransitionEnd, false);

Use a try/finally in the open method. We do this everywhere else, why has it been implemented differently here?

@@ +194,5 @@
> +      transitioned = true;
> +      findBar.getNode().removeEventListener("transitionend", onTransitionEnd, false);
> +    }
> +
> +    var findBar = this.getElement({type: "findbar"});

Shall we cache this node/element?

@@ +196,5 @@
> +    }
> +
> +    var findBar = this.getElement({type: "findbar"});
> +    findBar.getNode().addEventListener("transitionend", onTransitionEnd, false);
> +    this._toolbar = new Toolbar(this._controller);

The findbar is a child of the toolbar. So you might want to pass in a parent to the constructor.

@@ +280,5 @@
> + * @param {MozmillController} controller
> + *        MozMillController of the window to operate on
> + */
> +function LocationBar(aController) {
> +  this._controller = aController;

Same as for the findbar.

@@ +304,5 @@
> +   * @type {ElemBase}
> +   */
> +  get urlbar() {
> +    return this.getElement({type: "locationBar"});
> +  },

Do we really need this that often to have a getter for?

@@ +346,5 @@
> +    var self = this;
> +    this.urlbar.tap();
> +    assert.waitFor(function () {
> +      return self.urlbar.getNode().getAttribute('focused') === 'true';
> +    }, "Location bar has been focused", undefined, undefined, this);

please kill the extra parameters which are not used anymore.

@@ +398,5 @@
> +        break;
> +      case "identityBox":
> +        elem = new findElement.ID(this._controller.window.document, 'identity-box');
> +        break;
> +      case "locationBar":

call it urlbar so we are in sync.

@@ +434,5 @@
> +function Toolbar(aController) {
> +  this._controller = aController;
> +  this._downloads = new Downloads(aController);
> +  this._findBar = new FindBar(aController);
> +  this._locationBar = new LocationBar(aController);

I don't think all those need to be private.

@@ +473,5 @@
> +   * Returns the controller of the class instance
> +   *
> +   * @returns {MozMillController} Controller of the window
> +   */
> +  get controller() {

Please keep the alphabetical order.

@@ +544,5 @@
> +      case "starButton":
> +        elem = new findElement.ID(this._controller.window.document, 'star-button');
> +        break;
> +      case "toolbar":
> +        elem = new findElement.ID(this._controller.window.document, 'toolbar');

What's the difference between navbar and toolbar?

@@ +549,5 @@
> +        break;
> +      case "toolbarClose":
> +        elem = new findElement.ID(this._controller.window.document, 'close-button');
> +        break;
> +      case "viewOnDesktop":

If this is a context menu entry, it should not be part of this method but be handled via the Menu API.

@@ +567,5 @@
> +   */
> +  isVisible: function toolbar_isVisible() {
> +    var toolbar = this.getElement({type: "navbar"});
> +    return toolbar.getNode().hasAttribute("visible") ||
> +           toolbar.getNode().hasAttribute("startpage");

I'm still not sure why we have to check for the startpage here. If it is visible why is the visible attribute not set? Is this a bug we have to fix in Firefox?

@@ +583,5 @@
> +    var transitioned = false;
> +
> +    function onTransitionEnd() {
> +      transitioned = true;
> +      toolbar.getNode().removeEventListener("transitionend", onTransitionEnd, false);

Same for try/finally.

@@ +631,5 @@
> +    var pinButton = this.getElement({type: "pinButton"});
> +    pinButton.tap();
> +    assert.waitFor(function () {
> +     return pinButton.getNode().checked;
> +    }, "Page has been pinned");

We should throw an exception or remove the method at all. Nothing we can handle as of now.

Comment 33

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8361704 [details] [diff] [review]
patch v6

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

::: metrofirefox/lib/ui/startScreen.js
@@ +67,5 @@
> +        break;
> +      case "clearSelectedButton":
> +        elem = new findElement.ID(this._controller.window.document, 'clear-selected-button');
> +        break;
> +      case "contextAppbar":

It's the appbar that appears when you select several tabs or items from the start page. "contextAction" contains the actions available for this appbar (delete, pin, unpin, hide, clear).
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#365

I'm not sure what other name to use, appbar or itemsAppbar maybe? We're already in startScreen library so we know we're talking about an appbar in that page.

::: metrofirefox/lib/ui/tabs.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

I used a different library because we do the same in firefox libs.

@@ +12,5 @@
> +
> +/**
> + * Constructor
> + */
> +function Tabs(aController) {

It's separate.
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#188

::: metrofirefox/lib/ui/toolbars.js
@@ +191,5 @@
> +    var transitioned = false;
> +
> +    function onTransitionEnd() {
> +      transitioned = true;
> +      findBar.getNode().removeEventListener("transitionend", onTransitionEnd, false);

It's been done this way as well, there are examples in the repository, I'll change it too try/finally.

@@ +304,5 @@
> +   * @type {ElemBase}
> +   */
> +  get urlbar() {
> +    return this.getElement({type: "locationBar"});
> +  },

We have it in the desktop libs, wanted to be in sync. Most likely will be used more in tests.

@@ +544,5 @@
> +      case "starButton":
> +        elem = new findElement.ID(this._controller.window.document, 'star-button');
> +        break;
> +      case "toolbar":
> +        elem = new findElement.ID(this._controller.window.document, 'toolbar');

toolbar in wrapped in navbar. But I think I can remove it, at least now it's not used.

@@ +567,5 @@
> +   */
> +  isVisible: function toolbar_isVisible() {
> +    var toolbar = this.getElement({type: "navbar"});
> +    return toolbar.getNode().hasAttribute("visible") ||
> +           toolbar.getNode().hasAttribute("startpage");

I've used this as it is in Metro:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/ContextUI.js#55

Comment 34

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

Comment on attachment 8361704 [details] [diff] [review]
patch v6

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

::: metrofirefox/lib/ui/startScreen.js
@@ +67,5 @@
> +        break;
> +      case "clearSelectedButton":
> +        elem = new findElement.ID(this._controller.window.document, 'clear-selected-button');
> +        break;
> +      case "contextAppbar":

So this should be like the findbar a separate class. It should not be part of the startscreen class.

::: metrofirefox/lib/ui/tabs.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +

Hm, we might have to fix that in Firefox desktop in the future. For now I'm ok with it.

@@ +12,5 @@
> +
> +/**
> + * Constructor
> + */
> +function Tabs(aController) {

Ok, so in this case we want to leave it as a separate module. Thanks.

::: metrofirefox/lib/ui/toolbars.js
@@ +191,5 @@
> +    var transitioned = false;
> +
> +    function onTransitionEnd() {
> +      transitioned = true;
> +      findBar.getNode().removeEventListener("transitionend", onTransitionEnd, false);

If there are still those usages we have to update them ASAP. It could cause troubles all over the place. As usual always take the latest coding styles.

@@ +304,5 @@
> +   * @type {ElemBase}
> +   */
> +  get urlbar() {
> +    return this.getElement({type: "locationBar"});
> +  },

Oh ok, we might have to re-evaluate that later. For now its fine.

@@ +544,5 @@
> +      case "starButton":
> +        elem = new findElement.ID(this._controller.window.document, 'star-button');
> +        break;
> +      case "toolbar":
> +        elem = new findElement.ID(this._controller.window.document, 'toolbar');

It's a bit weird that we have appbar vs. toolbar, which makes it hard to understand. Do we have to use both or is one enough?

@@ +567,5 @@
> +   */
> +  isVisible: function toolbar_isVisible() {
> +    var toolbar = this.getElement({type: "navbar"});
> +    return toolbar.getNode().hasAttribute("visible") ||
> +           toolbar.getNode().hasAttribute("startpage");

I would then read this property directly.

Comment 35

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v7

Updated patch, took longer to find a way to get the tab as an UI element as everywhere API calls are used :)

Comment 36

[Andrei Eftimie]

Comment on attachment 8365138 [details] [diff] [review]
patch v7

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

This is good.

I've found a few issues, but they are mainly small issues like nits and some naming conventions.

::: metrofirefox/lib/tests/testToolbar.js
@@ +14,5 @@
> +  BASE_URL + "layout/mozilla_mission.html"
> +];
> +
> +const FINDBAR_ELEMENTS = ["findbar", "nextButton", "previousButton"];
> +const LOCATIONBAR_ELEMENTS = [ "backButton", "forwardButton",  "reloadButton", "stopButton"];

nit: extra whitespace at the start of the array

::: metrofirefox/lib/ui/startScreen.js
@@ +68,5 @@
> +    var elem = null;
> +
> +    switch(aSpec.type) {
> +      case "clearButton":
> +        elem = new findElement.ID(this._controller.window.document, 'clear-selected-button');

nit: we should use double quotes for consistency

::: metrofirefox/lib/ui/tabs.js
@@ +18,5 @@
> +    assert.fail("A valid controller must be specified");
> +  }
> +
> +  this._controller = aController;
> +  this._tabs = this._controller.tabs;

This is not used anywhere.

@@ +178,5 @@
> +    var method = aMethod || "shortcut";
> +
> +    switch (method) {
> +      case "shortcut":
> +        // TODO: add code for shortcuts, if any

I'm wondering if we should file a bug now, so we don't forget about these, and mention the bug id in these comments.

@@ +183,5 @@
> +        break;
> +      case "touchEvent":
> +        // TODO: add code for swipe up
> +        break;
> +      default:

So right now we have no way of opening the Tab Container at all?

@@ +231,5 @@
> +      default:
> +        throw new Error(arguments.callee.name + ": Unknown event type - " + type);
> +    }
> +
> +    try {

In the other locations we use this construct, the switch is also part of the try.
Basically everything after we set up the observer.

@@ +287,5 @@
> +      default:
> +        throw new Error(arguments.callee.name + ": Unknown event type - " + type);
> +    }
> +
> +    try {

Same thing with the try block, it should probably encompass all logic after we set up the observer.

::: metrofirefox/lib/ui/toolbars.js
@@ +8,5 @@
> +var utils = require("../../../firefox/lib/utils");
> +var tabs = require("tabs");
> +
> +/**
> + * Constructor

nit: jsdoc

@@ +11,5 @@
> +/**
> + * Constructor
> + */
> +function Downloads(aController) {
> +  if (aController == null || aController == undefined) {

You are checking !aController in the other classes.
It should suffice here as well.

@@ +83,5 @@
> +  }
> +};
> +
> +/**
> + * Constructor

nit: jsdoc

@@ +85,5 @@
> +
> +/**
> + * Constructor
> + */
> +function FindBar(aController, aToolbar) {

We have `FindBar` and we have `Toolbar`.
I feel they should either be both camelCased or none.
Either way is fine for me.

@@ +86,5 @@
> +/**
> + * Constructor
> + */
> +function FindBar(aController, aToolbar) {
> +  if (aController == null || aController == undefined) {

!aController should be fine here as well

@@ +91,5 @@
> +    assert.fail("A valid controller must be specified");
> +  }
> +
> +  this.toolbar = aToolbar;
> +  this._controller = aController;

Since the FindBar can not be initialised outside of the Toolbar, we don't need to specify the controller.
We should use aToolbar.controller

@@ +459,5 @@
> +
> +  this._controller = aController;
> +  this.downloads = new Downloads(aController);
> +  this.findBar = new FindBar(aController, this);
> +  this.locationBar = new LocationBar(aController);

Same thing with the downloads and the locationBar.
Since all these 3 are only used as part of the toolbar, don't pass the controller around.
Instead pass a reference to the instance of the toolbar, and use the toolbars controller.

@@ +557,5 @@
> +
> +  /**
> +   * Bookmark a page
> +   *
> +   * @returns {Boolean} True if the page has been bookmarked

This comment states that we return true of the page has been bookmarked, yet we don't return anything in this method.

@@ +609,5 @@
> +      default:
> +        throw new Error(arguments.callee.name + ": Unknown opening method - " + method);
> +    }
> +
> +    try {

Same here with the try block.

Comment 37

[Andrei Eftimie]

Comment on attachment 8365138 [details] [diff] [review]
patch v7

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

::: metrofirefox/lib/ui/tabs.js
@@ +228,5 @@
> +        var sideNewTab = this.getElement({type: "sidebar_newTabButton"});
> +        sideNewTab.tap();
> +        break;
> +      default:
> +        throw new Error(arguments.callee.name + ": Unknown event type - " + type);

If we reach this code, we get the following error message:

> ERROR | Test Failure | {
>   "exception": {
>     "message": "'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them",
>     "lineNumber": 232,
>     "name": "TypeError",
>     "fileName": "resource://mozmill/stdlib/securable-module.js -> file:///c:/work/bugs/924077/mozmill-tests/metrofirefox/lib/ui/tabs.js"
>   }
> }

We might need to amend `arguments.calee.name` here.
Please also check other instances.

Comment 38

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v7.1

Addressed all requests, indeed I removed the arguments.callee.name that we no longer use.

About the quotes, I would like an item about this in the style guide. We should only use single quotes then for cases in which we also have double quotes wrapping them? Like:
"Element '" + id + "' has been found"

About tabs container, when we open a new tab, it will automatically open to show that tab. I filed bug 964704 for it.

Also changed the try/finally cases to wrap the switch as well, but maybe we should update other cases in the repo in a separate mentored bug. Just an example:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/tabs.js#l460

Comment 39

[Andrei Eftimie]

Comment on attachment 8366592 [details] [diff] [review]
patch v7.1

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

Looks good.

Henrik, would you like to have a final look?
I would like to checkin this patch as soon as possible so we can also land some Metro tests that depend on it.

Comment 40

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

Comment on attachment 8366592 [details] [diff] [review]
patch v7.1

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

Something is wrong with the patch. Bugzilla claims that it is a Windows patch. I assume we have \n as line endings. Other comments see inline. We are getting closer. So we should be able to finish it off today.

::: metrofirefox/lib/tests/testStartScreen.js
@@ +21,5 @@
> +
> +  // Check the containers on the main page
> +  ELEMENTS.forEach(function (aElement) {
> +    var element = startScreen.getElement({type: aElement});
> +    expect.equal(element.getNode().localName, "vbox", aElement + " container is visible");

If localName contains the expected value, it doesn't mean the element is visible. But it exists and has the right type.

::: metrofirefox/lib/tests/testTabs.js
@@ +33,5 @@
> +  });
> +
> +  expect.waitFor(function () {
> +    return tabBrowser.isVisible();
> +  }, "Tabs container is visible");

Does this really have to be a waitFor call? We open multiple tabs inside the above loop. So the tab container should be visible already after the first iteration, and a simple expect.ok() should be fine here.

@@ +43,5 @@
> +      expect.equal(element.getNode().localName, "toolbarbutton", aElement + " button is visible");
> +    }
> +    else {
> +      expect.equal(element.getNode().localName, "div", aElement + " button is visible");
> +    }

Not sure why we differentiate here. Lets build the correct array above, like [{name: 'foo', type: 'toolbarbutton'}, ...]

@@ +49,5 @@
> +
> +  tabBrowser.closeTab();
> +  expect.waitFor(function () {
> +    return tabBrowser.length === 2;
> +  }, "Tab has been closed");

closeTab() waits for the event to happen which indicates that the tab has been closed. So a test should not have to call waitFor().

@@ +54,5 @@
> +
> +  tabBrowser.openTab();
> +  expect.waitFor(function () {
> +    return tabBrowser.length === 3;
> +  }, "Another tab has been opened");

Same here.

@@ +61,5 @@
> +  expect.equal(tabBrowser.selectedIndex, 1, "Second tab has been selected");
> +
> +  // Close the first tab
> +  tabBrowser.closeTab("button", 0);
> +  expect.ok(tabBrowser.selectedIndex == 0 &&

=== please.

::: metrofirefox/lib/tests/testToolbar.js
@@ +15,5 @@
> +];
> +
> +const FINDBAR_ELEMENTS = ["findbar", "nextButton", "previousButton"];
> +const LOCATIONBAR_ELEMENTS = ["backButton", "forwardButton",  "reloadButton", "stopButton"];
> +const TOOLBAR_ELEMENTS = ["menuButton", "pinButton", "starButton"];

I would have created a single array with sub-elements as mentioned for the testTabs.js module.

@@ +26,5 @@
> +function testDownloads() {
> +  controller.open("about:support");
> +  controller.waitForPageLoad();
> +
> +  var downloadProgress = toolbar.downloads.getElement({type: "downloadProgress"});

I still don't see a need for the 'download' prefix of the element. Given that it is located in the downloads class I would assume that by default. Simply call it 'progressbar'.

@@ +28,5 @@
> +  controller.waitForPageLoad();
> +
> +  var downloadProgress = toolbar.downloads.getElement({type: "downloadProgress"});
> +  expect.equal(downloadProgress.getNode().localName, "circularprogressindicator",
> +               "Downloads Button is present");

It's still not a button.

@@ +36,5 @@
> +  controller.open(TEST_DATA[0]);
> +  controller.waitForPageLoad();
> +
> +  // Open findbar via menu button and check it's elements
> +  toolbar.findBar.open("menu");

Can we add a check that it is open? Also please add a blank line below.

@@ +45,5 @@
> +      expect.ok(toolbar.findBar.isOpen, "Findbar has been opened");
> +    }
> +    else {
> +      expect.equal(element.getNode().localName, "toolbarbutton", aElement + " is present");
> +    }

Please add the expected localName to the array as sub property so we don't have to do the differentiation here.

@@ +54,5 @@
> +  expect.equal(textbox.getNode().value, "project", "Expected text is present");
> +
> +  // Close findbar via the close button
> +  toolbar.findBar.close();
> +  expect.ok(!toolbar.findBar.isOpen, "Findbar has been closed");

You want to mention the close button here.

@@ +58,5 @@
> +  expect.ok(!toolbar.findBar.isOpen, "Findbar has been closed");
> +
> +  // Open and close findbar via shortcut
> +  toolbar.findBar.open("shortcut");
> +  expect.ok(toolbar.findBar.isOpen, "Findbar has been opened");

You want to mention shortcut here.

@@ +65,5 @@
> +  expect.ok(!toolbar.findBar.isOpen, "Findbar has been closed");
> +}
> +
> +function testToolbarCheck() {
> +  toolbar.open();

Shouldn't we add a check that the open call was successful?

@@ +70,5 @@
> +  toolbar.locationBar.focus();
> +  toolbar.locationBar.type("url");
> +  expect.waitFor(function () {
> +    return toolbar.locationBar.value === "url";
> +  }, "Correct text has been typed in location bar");

I wouldn't expect that a test has to wait for that the text has been entered. Once type() returns it should be clear that the given string has been set as value.

@@ +73,5 @@
> +    return toolbar.locationBar.value === "url";
> +  }, "Correct text has been typed in location bar");
> +
> +  var closeSuggestions = toolbar.getElement({type: "closeSuggestionsButton"});
> +  closeSuggestions.tap();

Any test for that action?

@@ +97,5 @@
> +  });
> +
> +  var findInPage = toolbar.findBar.getElement({type: "findInPage"});
> +  expect.equal(findInPage.getNode().localName, "richlistitem",
> +               "Find in page option is present");

Shouldn't this be part of the FINDBAR_ELEMENTS array?

::: metrofirefox/lib/ui/startScreen.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +/**
> + * Constructor

Can you add a description here so everyone knows what this class is for?

@@ +96,5 @@
> +        elem = new findElement.ID(this._controller.window.document, "unpin-selected-button");
> +        break;
> +      default:
> +        throw new Error("Unknown element type - " + aSpec.type);
> +    }

Looks way better with this separation into a new class now!

::: metrofirefox/lib/ui/tabs.js
@@ +27,5 @@
> +/**
> + * Prototype definition of the TabBrowser class
> + */
> +
> +TabBrowser.prototype = {

nit: remove the blank line

@@ +49,5 @@
> +    return dtds;
> +  },
> +
> +  /**
> +   * Returns the number of tabs opened

number of open tabs

@@ +54,5 @@
> +   *
> +   * @returns {Number} Number of tabs opened
> +   */
> +  get length() {
> +    return this._controller.tabs.length;

This is still based on a global JS object. Why can't we retrieve it via a real element? We don't have to fix this right now in this bug, so please file a follow-up and work on it next once the lib has been landed.

@@ +63,5 @@
> +   *
> +   * @returns {Number} Index of the active tab
> +   */
> +  get selectedIndex() {
> +    return this._controller.tabs.activeTabIndex;

Same here.

@@ +72,5 @@
> +   *
> +   * @returns {ElemBase} Tab at the selected index
> +   */
> +  set selectedIndex(aIndex) {
> +    return this._controller.tabs.selectTabIndex(aIndex);

Same here.

@@ +156,5 @@
> +    if (aIndex === undefined)
> +      aIndex = this.selectedIndex;
> +
> +    var tabList = this.getElement({type: "tabsList"});
> +    return tabList.getNode().strip.childNodes[aIndex];

You should be able to get the child nodes via a nodeCollector query, which will return a list of elements.

::: metrofirefox/lib/ui/toolbars.js
@@ +13,5 @@
> + *
> + * @param {Object} aToolBar
> + *        Instance of the ToolBar class
> + */
> +function Downloads(aToolBar) {

There is no check that aToolBar is not undefined/null.

@@ +89,5 @@
> + * @param {Object} aToolBar
> + *        Instance of the ToolBar class
> + */
> +function FindBar(aToolBar) {
> +  this.toolbar = aToolBar;

aToolBar has to be not undefined/null.

@@ +196,5 @@
> +   */
> +  close: function FindBar_close(aMethod) {
> +    var method = aMethod || "button";
> +
> +    var transitioned = false;

nit: remove the blank line above

@@ +249,5 @@
> +
> +    try {
> +      switch (method) {
> +        case "menu":
> +          if (!this.toolbar.isVisible()) {

Mind adding a small comment why we have to do that?

@@ +296,5 @@
> + * @param {Object} aToolBar
> + *        Instance of the ToolBar class
> + */
> +function LocationBar(aToolBar) {
> +  this.toolbar = aToolBar;

The toolbar has to be  not undefined/null.

@@ +581,5 @@
> +   */
> +  open: function toolbar_open(aMethod) {
> +    var method = aMethod || "shortcut";
> +
> +    var transitioned = false;

nit : remove the blank line above.

@@ +598,5 @@
> +          var cmdKey = utils.getEntity(this.dtds, "urlbar.accesskey");
> +          win.keypress(cmdKey, {accelKey:true});
> +          break;
> +        case "touchEvent":
> +          // TODO: add code for touch event (swipe up)

Do we have a bug here?

Comment 41

[Andrei Eftimie]

Some info from me:

(In reply to Henrik Skupin (:whimboo) from comment #40)
> @@ +65,5 @@
> > +  expect.ok(!toolbar.findBar.isOpen, "Findbar has been closed");
> > +}
> > +
> > +function testToolbarCheck() {
> > +  toolbar.open();
> 
> Shouldn't we add a check that the open call was successful?
This check is already made as part of toolbar.open()


> ::: metrofirefox/lib/ui/toolbars.js
> @@ +13,5 @@
> > + *
> > + * @param {Object} aToolBar
> > + *        Instance of the ToolBar class
> > + */
> > +function Downloads(aToolBar) {
> 
> There is no check that aToolBar is not undefined/null.

We're only calling this from inside the toolBar class as this isn't exposed to the outside world.
Adding another check here is superfluous IMHO.

> @@ +89,5 @@
> > + * @param {Object} aToolBar
> > + *        Instance of the ToolBar class
> > + */
> > +function FindBar(aToolBar) {
> > +  this.toolbar = aToolBar;
> 
> aToolBar has to be not undefined/null.

Same.

> @@ +296,5 @@
> > + * @param {Object} aToolBar
> > + *        Instance of the ToolBar class
> > + */
> > +function LocationBar(aToolBar) {
> > +  this.toolbar = aToolBar;
> 
> The toolbar has to be  not undefined/null.

Same as above.

Comment 42

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

(In reply to Andrei Eftimie from comment #41)
> > > +function testToolbarCheck() {
> > > +  toolbar.open();
> > 
> > Shouldn't we add a check that the open call was successful?
> This check is already made as part of toolbar.open()

Ok, so I checked again and have seen that I don't see a method to actually close the toolbar. Do I miss something?

> > > +function Downloads(aToolBar) {
> > 
> > There is no check that aToolBar is not undefined/null.
> 
> We're only calling this from inside the toolBar class as this isn't exposed
> to the outside world.
> Adding another check here is superfluous IMHO.

Makes sense. Lets keep as it is then.

Comment 43

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #42)
> Ok, so I checked again and have seen that I don't see a method to actually
> close the toolbar. Do I miss something?

Hmmm, do we have any way of specifically closing these?
I think they close automatically once they are not being used anymore / lose focus.

I don't remember seeing any specific UI for these.

Comment 44

[Andreea Matei [:AndreeaMatei]]

No, it doesn't toggle with the dtd key used to open, neither works Escape to make it fade. Not sure if we would need it to close..

Comment 45

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v7.2

Updated after latest requests.

Comment 46

[Andrei Eftimie]

Comment on attachment 8367946 [details] [diff] [review]
patch v7.2

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

Looks good to me.

Comment 47

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

Comment on attachment 8367946 [details] [diff] [review]
patch v7.2

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

Mostly nits but we should get them fixed. I think with the next iteration we should be ok.

::: metrofirefox/lib/tests/testStartScreen.js
@@ +26,5 @@
> +  // Check the containers on the main page
> +  ELEMENTS.forEach(function (aElement) {
> +    var element = startScreen.getElement({type: aElement.name});
> +    expect.equal(element.getNode().localName, aElement.type,
> +                 aElement.name + " container exists");

Leave out container here, if the element changes in the future it would not apply anymore.

::: metrofirefox/lib/tests/testTabs.js
@@ +41,5 @@
> +  // Check the elements on each side and the new tab button
> +  ELEMENTS.forEach(function (aElement) {
> +    var element = tabBrowser.getElement({type: aElement.name});
> +    expect.equal(element.getNode().localName, aElement.type,
> +                 aElement.name + " button exists");

Same here for button. Just remove it.

::: metrofirefox/lib/tests/testToolbar.js
@@ +26,5 @@
> +  {name: "reloadButton", type: "toolbarbutton", parent: "locationBar"},
> +  {name: "stopButton", type: "toolbarbutton", parent: "locationBar"},
> +  {name: "menuButton", type: "toolbarbutton", parent: "toolBar"},
> +  {name: "pinButton", type: "toolbarbutton", parent: "toolBar"},
> +  {name: "starButton", type: "toolbarbutton", parent: "toolBar"}

This still requires if/else cases. Why don't you do it like:

ELEMENTS = {
  findbar: [...],
  locationbar: [...],
  toolbar: [...]
};

@@ +38,5 @@
> +function testDownloads() {
> +  controller.open("about:support");
> +  controller.waitForPageLoad();
> +
> +  var progressBar = toolbar.downloads.getElement({type: "downloadProgress"});

The element you are retrieving here has still the `download` prefix. That's something I already mentioned the last time. Also I talked about the element and not the variable name.

@@ +40,5 @@
> +  controller.waitForPageLoad();
> +
> +  var progressBar = toolbar.downloads.getElement({type: "downloadProgress"});
> +  expect.equal(progressBar.getNode().localName, "circularprogressindicator",
> +               "Downloads bar exists");

I would actually also include this as subelement in the elements array. Maybe more elements will come here.

@@ +80,5 @@
> +  toolbar.locationBar.focus();
> +  toolbar.locationBar.type("url");
> +  expect.equal(toolbar.locationBar.value, "url", "Correct text has been typed in location bar");
> +
> +  var suggestionsPanel = toolbar.getElement({type: "suggestionsPanel"});

I would move this down right before the expect call.

@@ +81,5 @@
> +  toolbar.locationBar.type("url");
> +  expect.equal(toolbar.locationBar.value, "url", "Correct text has been typed in location bar");
> +
> +  var suggestionsPanel = toolbar.getElement({type: "suggestionsPanel"});
> +  var closeSuggestions = toolbar.getElement({type: "closeSuggestionsButton"});

This is a button.

@@ +107,5 @@
> +  });
> +
> +  var findInPage = toolbar.findBar.getElement({type: "findInPage"});
> +  expect.equal(findInPage.getNode().localName, "richlistitem",
> +               "Find in page option is present");

Same question here as for in my last review. Why is this element not part of the findbar array? Given that this is a check for an element of the findbar class, I don't expect it to stay in the test for the toolbar.

::: metrofirefox/lib/ui/tabs.js
@@ +53,5 @@
> +   *
> +   * @returns {Number} Number of open tabs
> +   */
> +  get length() {
> +    return this._controller.tabs.length;

I still don't see that a new bug has been filed about this issue yet. When will that happen?

@@ +135,5 @@
> +        break;
> +      case "tabsList":
> +        elem = new findElement.ID(document, "tabs");
> +        break;
> +      case "tray":

What is that new element about? I haven't seen a comment what it is.

@@ +160,5 @@
> +
> +    var root = this.getElement({type: "tabsList"}).getNode().strip;
> +    var nodeCollector = new domUtils.nodeCollector(root);
> +    var tabList = nodeCollector.queryNodes("documenttab");
> +

Nope. All this has to be part of getElements(). I don't want to see the usage of nodeCollector outside of this method.

@@ +171,5 @@
> +   * @returns {Boolean} True if the tabs container is visible
> +   */
> +  isVisible: function tabBrowser_isVisible() {
> +    var tabs = this.getElement({type: "tray"});
> +    return tabs.getNode().hasAttribute("expanded");

Hm, that looks way better. So do we still need the tabsContainer element? Or can this get removed? If tray is that important we should test this if it exists!

Comment 48

[Andreea Matei [:AndreeaMatei]]

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

> This still requires if/else cases. Why don't you do it like:
> 
> ELEMENTS = {
>   findbar: [...],
>   locationbar: [...],
>   toolbar: [...]
> };
Right, I could do that, thanks for suggesting.

> @@ +107,5 @@
> > +  });
> > +
> > +  var findInPage = toolbar.findBar.getElement({type: "findInPage"});
> > +  expect.equal(findInPage.getNode().localName, "richlistitem",
> > +               "Find in page option is present");
> 
> Same question here as for in my last review. Why is this element not part of
> the findbar array? Given that this is a check for an element of the findbar
> class, I don't expect it to stay in the test for the toolbar.
This is a tricky one. The element is a method to open the findbar, but it's not an element from the find bar itself. It's part of the menu button options, which is in the toolbar. We might even use controller.Menu, if we're not blocked by bug 966963. But as for where this check should be, I consider the element is part of the toolbar.

> @@ +171,5 @@
> > +   * @returns {Boolean} True if the tabs container is visible
> > +   */
> > +  isVisible: function tabBrowser_isVisible() {
> > +    var tabs = this.getElement({type: "tray"});
> > +    return tabs.getNode().hasAttribute("expanded");
> 
> Hm, that looks way better. So do we still need the tabsContainer element? Or
> can this get removed? If tray is that important we should test this if it
> exists!
Tray wraps tabs-container. I'll add a check for it's existance.
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#189

Comment 49

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v7.3

Updated the latest requests. Filed bug for the tabs.js module (bug 968079).
Hope it's all well now :)

Comment 50

[Andrei Eftimie]

Comment on attachment 8370607 [details] [diff] [review]
patch v7.3

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

Looks good.

I've had a bit of trouble following the findBar context menu.
To make it easy to follow which items are related here, I've added comments inline.

This said, I'm fine either way on this.
I don't find it a blocking issue.

Leaving this for Henrik to decide if we should move it of leave it as is.

::: metrofirefox/lib/ui/toolbars.js
@@ +169,5 @@
> +      case "closeButton":
> +        elem = new findElement.ID(this._controller.window.document, "findbar-close-button");
> +        break;
> +      case "findInPage":
> +        elem = new findElement.ID(this._controller.window.document, "context-findinpage");

So instead of moving for this element in the findbar class, it might be better to move this element itself in the toolbar class directly.

@@ +255,5 @@
> +
> +          var menuButton = this.toolbar.getElement({type: "menuButton"});
> +          menuButton.tap();
> +
> +          var findInPage = this.getElement({type: "findInPage"});

So this one is actually part of the context-menu in the toolbar.
The other 2 entries from this menu are "View Source" and "Relaunch in Desktop" which we have no use for.

Comment 51

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

Comment on attachment 8370607 [details] [diff] [review]
patch v7.3

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

::: metrofirefox/lib/tests/testToolbar.js
@@ +46,5 @@
> +  controller.waitForPageLoad();
> +
> +  var progressBar = toolbar.downloads.getElement({type: ELEMENTS.downloads[0].name});
> +  expect.equal(progressBar.getNode().localName, ELEMENTS.downloads[0].type,
> +               "Downloads bar exists");

I would also have done this via forEach. So if we have to add more elements, we only have to extend the array.

::: metrofirefox/lib/ui/tabs.js
@@ +142,5 @@
> +      case "tabsContainer":
> +        elem = new findElement.ID(document, "tabs-container");
> +        break;
> +      case "tabsList":
> +        var root = this.getElement({type: "tabs"}).getNode().strip;

What is strip? That's not something you should use here given that it again goes via the JS object hierarchy. Also do we ever need the tabs element somewhere else? If not we should better name this element tabs.

@@ +179,5 @@
> +   * @returns {Boolean} True if the tabs container is visible
> +   */
> +  isVisible: function tabBrowser_isVisible() {
> +    var tabs = this.getElement({type: "tray"});
> +    assert.equal(tabs.getNode().localName, "vbox", "Element has been found");

Ups, I think you misunderstood my comment. This check should be part of the library test but not part of the library itself.

::: metrofirefox/lib/ui/toolbars.js
@@ +169,5 @@
> +      case "closeButton":
> +        elem = new findElement.ID(this._controller.window.document, "findbar-close-button");
> +        break;
> +      case "findInPage":
> +        elem = new findElement.ID(this._controller.window.document, "context-findinpage");

It depends on what kind of parent it has. If it's part of the toolbar it should indeed be in the toolbar class.

@@ +255,5 @@
> +
> +          var menuButton = this.toolbar.getElement({type: "menuButton"});
> +          menuButton.tap();
> +
> +          var findInPage = this.getElement({type: "findInPage"});

If that is a context menu make sure to also add the newly filed bug as reference that controller.Menu() doesn't work here. We should not forget to get this fixed later.

Comment 52

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v8

Added a TODO for the strip part as I didn't find another way to get those tabs:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_tabs_container.js#34

Also a TODO for bug 966963 to use controller.Menu.

Comment 53

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

Comment on attachment 8370795 [details] [diff] [review]
patch v8

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

I think that looks fine now. But letting Andrei do a sanity review before we land this.

Comment 54

[Andrei Eftimie]

Comment on attachment 8370795 [details] [diff] [review]
patch v8

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

Looks great.

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

Comment 55

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

We are not going to backport this patch. So closing as fixed. Thanks Andreea.