620538 - Mozmill test for Panorama create a new tab

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Clay Earl Uyenghua]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10
Build Identifier: 

https://litmus.mozilla.org/show_test.cgi?id=12653

Reproducible: Always

Steps to Reproduce:
1. Open Panorama/Group view.
2. In Group Your Tabs, click on the new-tab button, the little [+] icon, inside a tab group.


Expected Results:  
Verify a new blank tab is created inside the set and that the tab goes to focus in the browser.

Comment 1

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653

Comment 2

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

Comment on attachment 498937 [details] [diff] [review]
Automation script for testcase 12653

>+ * Contributor(s):
>+ * Clay Earl Uyenghua <[e-mail]> (original author)

Please use a 2 spaces indentation, so it is aligned with "n".

>+// Include the required modules
>+var tabs = require("../../shared-modules/tabs");
>+var tabViews = require("../../shared-modules/tabview");

As given on the other bug, capital letters are preferred at the beginning of the module variable.

>+const TIMEOUT = 5000;

That's the default value in our framework. So you can remove it from the test.

>+const LOCAL_TEST_PAGE = {url: LOCAL_TEST_FOLDER + 'layout/mozilla.html',
>+							name: 'community'};

Please update the formatting, so it uses 2 spaces indentation from the beginning of the line.

>+var setupModule = function(module) {
>+var testCreateNewTab = function() {

It should be named functions.

>+  var activeGroup = tabView.getElement({
>+      type: "groups",
>+      subtype: "active"
>+    });

Here you have 4 instead of 2 spaces indentation.

>+  // Get the number of tabs for the active group
>+  var tabCount = activeGroup.getNode().iQData.item.getChildren().length;

Therefor you can use "TabView.getTabs({filter: "group", value: activeGroup})" to get all the tabs. The object which is returned, is an array.

>+  // Click the new tab button of the active group
>+  controller.click(tabView.getElement({type: "group_newTabButton", 
>+                                       parent: activeGroup}));

The element should be instantiated before the click call. See https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Style_Guide#Elements

>+  // Get the current active tab's url and check that new tab is opened
>+  var currentTabURI = controller.tabs.activeTab.documentURI;
>+  controller.assertJS("subject.currentTabURI=='about:blank'",
>+                      { currentTabURI: currentTabURI });

Use controller.assert and the locationBar class here. The latter contains the urlbar reference:
https://developer.mozilla.org/en/Mozmill_Tests/Shared_Modules/ToolbarAPI/locationBar

Further we should check, that a new tab has been added to the list of open tabs in the tabbar. It's obvious for testers running manual tests but important in this case, because closeAllTabs already leaves an about:blank tab open. Given this fact we could miss a regression.

>+  activeGroup = tabView.getElement({
>+      type: "groups",
>+      subtype: "active"
>+    });
>+  var currentTabCount = activeGroup.getNode().iQData.item.getChildren().length;

See my comments from above.

>+  controller.assertJS("subject.tabCount==subject.currentTabCount-1",
>+                      { tabCount: tabCount,
>+                        currentTabCount: currentTabCount });

Change it to assert() too.

Also I miss the full screen part of this test. Means we should let this execute twice.

Comment 3

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 1

revised automation script. Incorporated suggested changes.

Comment 4

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 1

Added the full screen part of this test.

Comment 5

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

Comment on attachment 499465 [details] [diff] [review]
Automation script for testcase 12653 revision 1

>+function setupModule() {

Please re-add the module parameter.

>+  tabView = new TabView.tabView(controller);
>+  
>+  locationBar = new Toolbars.locationBar(controller);

You can remove the blank line in-between those two lines.

>+  for ( var i=0; i < 2; i++ ) {
>+    // Open the local page and wait for page to load
>+    controller.open(LOCAL_TEST_PAGE.url);
>+    controller.waitForPageLoad();

As given in the other test, you probably want to have a separate function which gets called twice from the test function. 

>+    var elem = new elementslib.Name(controller.tabs.activeTab,
>+      LOCAL_TEST_PAGE.name);

There is a difference for parameter like those, and ones which are enclosed in {}. While the latter ones visualize a block and need a 2 spaces indentation, you should align those exactly at the same column.

>+    var newTabButton = tabView.getElement({type: "group_newTabButton", 
>+      parent: activeGroup});

The first parameter also has to be on its own line. See the other calls to getElement in this file.

>+    controller.assert(function () { return locationBar.value=='' });

Please see our style guide, how to format this block correctly. Beside that use the === operator and add surrounding spaces.

>+    var currentTabCount = tabView.getTabs({filter: "group", 
>+      value: activeGroup}).length;

Same formatting issues. Also it would be better to split it into two expressions (get the element, and get the count). 

>+    controller.assert(function () { 
>+      return tabCount==currentTabCount-1 });

Please correct the formatting and insert blanks around the operators.

>+    var fullScreen = new elementslib.Elem(controller.menus['view-menu'].fullScreenItem);
>+    controller.click(fullScreen);

As given in my other review, we need a tearDownModule function to make sure we don't stay in full screen mode after this test has been finished.

r- based on the missing tearDownModule function. The other ones are mostly style issues and can quickly be fixed.

Comment 6

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 2

Revised automation script. Created the teardownModule.

Comment 7

[u279076]

Comment on attachment 500244 [details] [diff] [review]
Automation script for testcase 12653 revision 2

>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
Please use 2011.

>+// Include the required modules
>+var Tabs = require("../../shared-modules/tabs");
>+var TabView = require("../../shared-modules/tabview");
>+var Toolbars = require("../../shared-modules/toolbars");
>+var Utils = require("../../shared-modules/utils");
All variables should begin lower-case.

>+
>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
>+const LOCAL_TEST_PAGE = {url: LOCAL_TEST_FOLDER + 'layout/mozilla.html',
>+  name: 'community'};
Please move the url and name onto a newline.

>+  tabView = new TabView.tabView(controller);
You'll have to use a different name for this variable (maybe activeTabView).

>+function teardownModule() {
>+  var view = controller.window.document.defaultView;
>+  var state = view.fullScreen;
>+  if ( state === 'true' ) {
>+    controller.mainMenu.click("#fullScreenItem");
>+  }
>+}
Can you add a comment to this code? It's unclear what you are trying to do here.


>+function testCreateNewTab() {
>+  // Perform test twice for normal and full screen
>+  for ( var i=0; i < 2; i++ ) {
Use the formatting from the styleguide.

>+    createNewTabTest();
I'm not sure why you extracted the test into it's own method.  Just do it within the current method.

>+function createNewTabTest() {
>+  // Open the local page and wait for page to load
>+  controller.open(LOCAL_TEST_PAGE.url);
>+  controller.waitForPageLoad();
>+  var elem = new elementslib.Name(controller.tabs.activeTab,
>+  LOCAL_TEST_PAGE.name);
>+  controller.assertNode(elem);
Please break these up into blocks -- I suggest a newline after waitForPageLoad.
Also, please use more meaningful variable names -- "elem" is not meaningful.

Comment 8

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 3

I have revised the automation script according to changes required by the review. The teardown module is to make sure that we are not in full screen at the end of the test. I previously extracted the test according to the review done by Henrik on Bug 620589 since both tests perform test twice for normal view and fullscreen view. I have made changes and removed the createNewTabTest function.

Comment 9

[u279076]

Comment on attachment 502376 [details] [diff] [review]
Automation script for testcase 12653 revision 3

>+// Include the required modules
>+var tabs = require("../../shared-modules/tabs");
>+var tabView = require("../../shared-modules/tabview");
>+var toolbars = require("../../shared-modules/toolbars");
Please capitalize the first letters -- sorry for the confusion.

>+    // Check that the local test page opened successfully
>+    var pageName = new elementslib.Name(controller.tabs.activeTab,
>+    LOCAL_TEST_PAGE.name);

Please align LOCAL_ with controller.tabs

>+    // The tab will focus and we wait until tabView is closed
>+    controller.waitFor(function () {
>+      return activeTabView.isOpen == false
>+    });
Please refer to the waitFor() documentation for proper usage:
https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#waitFor.28.29

Do this for all waitFor() in all patches.  Thanks.

>+    // Check current tab count has one more tab than previous tab count
>+    controller.assert(function () {
>+      return tabCount == currentTabCount-1
>+    });

Use a waitFor() here.

Comment 10

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 4

Implemented necessary changes. Changed assert() calls to waitFor().

Comment 11

[u279076]

Comment on attachment 503072 [details] [diff] [review]
Automation script for testcase 12653 revision 4

>diff --git a/firefox/testTabView/testCreateNewTab.js b/firefox/testTabView/testCreateNewTab.js
>new file mode 100644
>--- /dev/null
>+++ b/firefox/testTabView/testCreateNewTab.js
>@@ -0,0 +1,136 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is MozMill Test code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Clay Earl Uyenghua <[e-mail]>.
>+ *
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Clay Earl Uyenghua <[e-mail]> (original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Include the required modules
>+var Tabs = require("../../shared-modules/tabs");
>+var TabView = require("../../shared-modules/tabview");
>+var Toolbars = require("../../shared-modules/toolbars");
>+
>+const PERFORM_TEST = 2;
>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
>+const LOCAL_TEST_PAGE = {
>+  url: LOCAL_TEST_FOLDER + 'layout/mozilla.html',
>+  name: 'community'
>+};

I missed this before, but the test page constant should be LOCAL_TEST_PAGES even if there is just one page.  This is so it is the same across all tests.

>+function teardownModule() {
>+  // If we are still on full screen then exit full screen

Don't forget the module parameter.

>+function testCreateNewTab() {
>+  // Perform test twice for normal and full screen
>+  for (var i = 0; i < PERFORM_TEST; i++) {
>+    // Open the local page and wait for page to load
>+    controller.open(LOCAL_TEST_PAGE.url);

url should be URL since it's an acronym.

>+    // The tab will focus and we wait until tabView is closed
>+    controller.waitFor(function () {
>+      return activeTabView.isOpen == false
>+    }, null, null, "Expected Tab View to close");

Please update the error message to include "got" and "expected" values.

>+    // Check that a new tab is opened
>+    controller.waitFor(function () {
>+      return locationBar.value === ''
>+    }, null, null, "Expected a new tab to open");

Same here.

>+    // Check current tab count has one more tab than previous tab count
>+    controller.waitFor(function () {
>+      return tabCount == currentTabCount-1
>+    }, null, null, "Expected tab count to increment after opening a new tab");

Same here.

Comment 12

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 4

Revised automation script. Implemented required changes.

Comment 13

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 4

Comment 14

[u279076]

Comment on attachment 504927 [details] [diff] [review]
Automation script for testcase 12653 revision 4

>diff --git a/firefox/testTabView/testCreateNewTab.js b/firefox/testTabView/testCreateNewTab.js
>+function teardownModule(module) {
>+  // If we are still on full screen then exit full screen
>+  var view = controller.window.document.defaultView;
>+  var state = view.fullScreen;
>+  if ( state === 'true' ) {
>+    controller.mainMenu.click("#fullScreenItem");
>+  }
>+}

If state is a string then this is correct.  However, if it is a boolean, this should fail.  Booleans do not need quotes.

>+    // The tab will focus and we wait until tabView is closed
>+    controller.waitFor(function () {
>+      return activeTabView.isOpen == false
>+    }, "Expected Tab View to close - got " + activeTabView.isOpen 
>+    + ", expected " +false);

Use ===.  Also, since this is boolean, it's enough to just say "Tab view should now be closed" (no need for got, expected).

>+    // Check that a new tab is opened
>+    controller.waitFor(function () {
>+      return locationBar.value === ''
>+    }, "Expected a new tab to open - got " + locationBar.value
>+    + ", expected no URL");

This is probably not a reliable way to check if a new tab is open.  This only checks that the location bar is empty.  I believe their should be a way to count tabs before and tabs after.  We might also be able to check the tab title for "New Tab" or about:blank loaded.  Using DOM Inspector will help you narrow down the properties to check.

>+    // Check current tab count has one more tab than previous tab count
>+    controller.waitFor(function () {
>+      return tabCount == currentTabCount-1
>+    }, "Expected tab count to increment after opening new tab - got tabCount "
>+	+ tabCount + ", expected tabCount" + currentTabCount-1);

Use ===.

Comment 15

[u279076]

It might help you to look at testTabbedBrowsing/testNewTab.js for how we check a tab is loaded.

Comment 16

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 6

Implemented necessary changes.

Comment 17

[u279076]

Comment on attachment 507372 [details] [diff] [review]
Automation script for testcase 12653 revision 6

This looks good to me.  Henrik, can you spotcheck in case I might have missed something?  Thanks.

Comment 18

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

Comment on attachment 507372 [details] [diff] [review]
Automation script for testcase 12653 revision 6

>+const PERFORM_TEST = 2;

Why do we need the PERFORM_TEST constant? We will always run the loop twice, so I would better place it hard-coded in the for header. Also with the name it's hard to understand for what it is used for. 

>+function teardownModule(module) {
>+  // If we are still on full screen then exit full screen
>+  var view = controller.window.document.defaultView;
>+  var state = view.fullScreen;
>+  if ( state === true ) {
>+    controller.mainMenu.click("#fullScreenItem");
>+  }
>+}

While it is great to work with the menu item, we should really only operate on the fullScreen property of the window here. The teardown is meant to hard-reset all changes which happened in a test. And clicking on the menu item can fail. So please move the click into the test itself and in the teardown set controller.window.fullScreen to false. No need to check for the current state.

>+function testCreateNewTab() {
[..]
>+    // The tab will focus and we wait until tabView is closed
>+    controller.waitFor(function () {
>+      return activeTabView.isOpen === false
>+    }, "Tab View should now be closed");

The value we compare to is already a boolean. So it can be reduced to 'return !activeTabView.isOpen. Also please rename 'should now be' to 'has been closed'. Messages have to express the real expected state and no a vague one. We know what we want to have. :)

>+    // Check that a new tab is opened
>+    controller.waitFor(function () {
>+      return controller.tabs.activeTab.URL === 'about:blank'
>+    }, "Expected a new tab to open - got ['"
>+    + controller.tabs.activeTab.URL + "'], expected ['about:blank']");

Why have the brackets put around the quotes? As we agreed on we want to use: "- got 'xyz', expected 'zyx'". And the value we are comparing to is not an array.

>+    // Check current tab count has one more tab than previous tab count
>+    controller.waitFor(function () {
>+      return tabCount === currentTabCount-1
>+    }, "Expected tab count to increment after opening new tab - got tabCount ["
>+	+ tabCount + "], expected tabCount [" + currentTabCount-1 + "]");

Same here.

>+    // Close tab view and click on full screen for next test
>+    activeTabView.close();
>+    controller.mainMenu.click("#fullScreenItem");

Can we please add a final check that we are in or out of fullscreen for real? A waitFor which incorporates the loop index should work perfectly here.

Looks like we are close to get it checked-in. I think only one more iteration, Clay. Thanks!

Comment 19

[u279076]

(In reply to comment #18)
> >+    // Check that a new tab is opened
> >+    controller.waitFor(function () {
> >+      return controller.tabs.activeTab.URL === 'about:blank'
> >+    }, "Expected a new tab to open - got ['"
> >+    + controller.tabs.activeTab.URL + "'], expected ['about:blank']");
> 
> Why have the brackets put around the quotes? As we agreed on we want to use: "-
> got 'xyz', expected 'zyx'". And the value we are comparing to is not an array.

Using 'xyz' only makes sense when we are expecting a string value. The square brackets are used because we are expecting a particular index value.

Comment 20

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

(In reply to comment #19)
> Using 'xyz' only makes sense when we are expecting a string value. The square
> brackets are used because we are expecting a particular index value.

There is no difference between strings, numbers, or booleans. The quotes are used to separate the values from the rest of the message.

Comment 21

[u279076]

(In reply to comment #20)
> (In reply to comment #19)
> > Using 'xyz' only makes sense when we are expecting a string value. The square
> > brackets are used because we are expecting a particular index value.
> 
> There is no difference between strings, numbers, or booleans. The quotes are
> used to separate the values from the rest of the message.

Ok, thanks for clearing that up, Henrik.  Clay, please make these revisions and I'll test the patch once it gets r+ by Henrik.  Thanks.

Comment 22

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12653 revision 7 [backed-out]

Implemented necessary changes.

Comment 23

[u279076]

Comment on attachment 509356 [details] [diff] [review]
Automation script for testcase 12653 revision 7 [backed-out]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/9decb81ef7df [default]

Comment 24

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

(In reply to comment #21)
> Ok, thanks for clearing that up, Henrik.  Clay, please make these revisions and
> I'll test the patch once it gets r+ by Henrik.  Thanks.

Hm, I never gave my r+ on the last updated patch. Clay, please don't set r+ without explicit approval.

Comment 25

[u279076]

(In reply to comment #24)
> (In reply to comment #21)
> > Ok, thanks for clearing that up, Henrik.  Clay, please make these revisions and
> > I'll test the patch once it gets r+ by Henrik.  Thanks.
> 
> Hm, I never gave my r+ on the last updated patch. Clay, please don't set r+
> without explicit approval.

That was my fault.  I told him "r=me with those changes".  I'll keep my eye on that in the future.  Sorry.

Comment 26

[u279076]

Comment on attachment 509356 [details] [diff] [review]
Automation script for testcase 12653 revision 7 [backed-out]

Backed-out:
http://hg.mozilla.org/qa/mozmill-tests/rev/e88f075d7e1f [default]

This test was causing a regression of failures in all tests following with the following exception:
Expression \"id(\"navigator-toolbox\")\" returned null

This was only reproducible locally, did not get reported in today's testrun.

Clay, please fix and resubmit a patch.  Make sure you test your patch using mozmill-automation/testrun_all.py first.  I can help you set that up if needed.

Thanks

Comment 27

[Aaron Train [:aaronmt]]

From simply watching the console after this test is executed, it seems that mozmill is executing the succeeding tests before the state of the browser document has returned from tab-groups. This was tested with a manifest of this test alongside our collection of search test modules. Perhaps in the teardown the test needs to wait until the state of the browser is restored?

Comment 28

[u279076]

I'm going to take this bug. Clay, thanks for your work on it so far.  Can you please shift focus to bug 624892?

Thanks

Comment 29

[u279076]

This test requires entering and exiting Fullscreen mode.  Currently, we don't have a gracious way of handling exit of Fullscreen mode with Mozmill.

The following is a workaround I've hacked together, but we should find a better solution via the toolbar shared module:

  // Switch back to normal view mode
  var dtds = ["chrome://browser/locale/browser.dtd"];
  if (mozmill.isMac) {
    var fullscreenCmdKey = utils.getEntity(dtds, "fullScreenCmd.macCommandKey");
    controller.keypress(null, fullscreenCmdKey, {accelKey: true, shiftKey: true});
  } else {
    controller.keypress(null, "VK_F11", {});
  }

Henrik, should I file a new bug to get some work done on Fullscreen handling?

Comment 30

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

Why does the keyboard shortcut not work for exiting the tab groups view?

Comment 31

[u279076]

(In reply to comment #30)
> Why does the keyboard shortcut not work for exiting the tab groups view?

What I refer to in comment 29 is not about entering/exiting Tab Groups view; it's about entering/exiting Full Screen mode.

Comment 32

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

Which was a mistake on my part. I wanted to write fullscreen.

Comment 33

[u279076]

(In reply to comment #32)
> Which was a mistake on my part. I wanted to write fullscreen.

So, what I have written in comment 29 works.  However, I think it's a bit ugly and should exist in an API, not a test.  If you are fine with me doing it this way in the test for the time being, then so am I.

Comment 34

[u279076]

Attachment: Patch v2

I've refactored the test and corrected any problems it was causing.

Comment 35

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

(In reply to comment #33)
> So, what I have written in comment 29 works.  However, I think it's a bit ugly
> and should exist in an API, not a test.  If you are fine with me doing it this
> way in the test for the time being, then so am I.

We have multiple tests which make use of the fullscreen portion, right? We could stick it into the utils.js module for now to have the abstraction ready. For the API refactoring we can stick this method onto the browser object.

Comment 36

[Aaron Train [:aaronmt]]

Comment on attachment 517935 [details] [diff] [review]
Patch v2

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
>+const LOCAL_TEST_PAGES = {
>+  URL: LOCAL_TEST_FOLDER + 'layout/mozilla.html',
>+  name: 'community'
>+};

Upper-case name as well, or lowercase 'URL'.

>+  // Check that a new tab is opened
>+  controller.waitFor(function () {
>+    return controller.tabs.activeTab.URL === 'about:blank'
>+  }, "Expected a new tab to open - got '" + controller.tabs.activeTab.URL +
>+     "', expected 'about:blank'");

Why do you use a waitFor here and not an assert? 

>+    // Check current tab count has one more tab than previously
>+  controller.waitFor(function () {
>+    return activeGroupTabs.length === initialTabCount + 1
>+  }, "Expected tab count to increment after opening new tab - got '" +
>+     activeGroupTabs.length + "', expected tabCount '" +
>+     initialTabCount + 1 + "'");
>+
>+    // Close tab view and click on full screen for next test
>+  activeTabView.close();  
>+}

Nit: Shift your comments back two spaces

Comment 37

[u279076]

(In reply to comment #36)
> Comment on attachment 517935 [details] [diff] [review]
> Patch v2
> 
> >+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
> >+const LOCAL_TEST_PAGES = {
> >+  URL: LOCAL_TEST_FOLDER + 'layout/mozilla.html',
> >+  name: 'community'
> >+};
> 
> Upper-case name as well, or lowercase 'URL'.

We only use uppercase for acronyms. This will remain as is.

> 
> >+  // Check that a new tab is opened
> >+  controller.waitFor(function () {
> >+    return controller.tabs.activeTab.URL === 'about:blank'
> >+  }, "Expected a new tab to open - got '" + controller.tabs.activeTab.URL +
> >+     "', expected 'about:blank'");
> 
> Why do you use a waitFor here and not an assert? 

I'm using a waitFor to account for tab animation.

Comment 38

[u279076]

Attachment: Patch v2.1

Nits addressed.

Comment 39

[Aaron Train [:aaronmt]]

Comment on attachment 518402 [details] [diff] [review]
Patch v2.1

Looks good, thanks r+

Comment 40

[u279076]

Attachment: Patch v2.2

Incorporates refactored structure.

Comment 41

[Aaron Train [:aaronmt]]

Comment on attachment 518589 [details] [diff] [review]
Patch v2.2

>+  // Wait for fullscreen mode to become active
>+  var view = controller.window.document.defaultView;
>+  controller.waitFor(function () {
>+      return view.fullScreen;
>+    }, "Window switched to fullscreen mode");
Nit: Adjust the spacing of the closing brace and your return line
 
>+  
>+  // Wait for normal mode to become active
>+  var view = controller.window.document.defaultView;
>+  controller.waitFor(function () {
>+      return !view.fullScreen;
>+    }, "Window switched to normal mode");
And here too

With those nits addressed, passing onto Henrik

Comment 42

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

Comment on attachment 518589 [details] [diff] [review]
Patch v2.2

>+ * Portions created by the Initial Developer are Copyright (C) 2010

2011 please

>+function setupModule(module) {

As discussed on IRC you will also need a teardownModule function which calls tabView.reset(). Also make sure if the test fails you will exit the fullscreen mode.

>+  // Wait for fullscreen mode to become active
>+  var view = controller.window.document.defaultView;
>+  controller.waitFor(function () {
>+      return view.fullScreen;
>+    }, "Window switched to fullscreen mode");

defaultView is the same as window. So you can directly use controller.window.fullScreen.

>+  // Switch back to normal view mode
>+  if (mozmill.isMac) {
>+    var fullscreenCmdKey = utils.getEntity(["chrome://browser/locale/browser.dtd"],
>+	    				    "fullScreenCmd.macCommandKey");
>+    controller.keypress(null, fullscreenCmdKey, {accelKey: true, shiftKey: true});
>+  } else {
>+    controller.keypress(null, "VK_F11", {});

Does sending F11 on OS X not work? As long as we don't send native events, the keypress should not be interpreted by OS X. Also why does the menu shortcut doesn't work? Even the menu is not visible the event should get propagated.

>+  // Wait for the Tab Groups view to close
>+  activeTabView.waitForClosed();
>+  controller.assert(function () {
>+    return !activeTabView.isOpen
>+  }, "Tab Groups view has been closed");

No need to do another assert. There is an implicit check in waitForClosed().

>+    return activeGroupTabs.length === initialTabCount + 1

nit: please add braces around the operands of the + operator.

Comment 43

[u279076]

Attachment: Patch v2.3

> >+  // Switch back to normal view mode
> >+  if (mozmill.isMac) {
> >+    var fullscreenCmdKey = utils.getEntity(["chrome://browser/locale/browser.dtd"],
> >+	    				    "fullScreenCmd.macCommandKey");
> >+    controller.keypress(null, fullscreenCmdKey, {accelKey: true, shiftKey: true});
> >+  } else {
> >+    controller.keypress(null, "VK_F11", {});
> 
> Does sending F11 on OS X not work? As long as we don't send native events, the
> keypress should not be interpreted by OS X. Also why does the menu shortcut
> doesn't work? Even the menu is not visible the event should get propagated.

F11 is reserved on OSX for an OS level shortcut for "show desktop". 
With the menu invisible, it is impossible for me to determine what the menu might be.  Also, I don't think we should be performing actions in Mozmill tests which users could not conceivably perform. Please advise.

All other revisions have been addressed in this patch.

Comment 44

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

Comment on attachment 519927 [details] [diff] [review]
Patch v2.3

>+function teardownModule(module) {
>+  activeTabView.reset();
>+  tabBrowser.closeAllTabs();
>+}

You also have to ensure that fullscreen has been exited.

>+  // Switch to fullscreen mode
>+  controller.mainMenu.click("#fullScreenItem");
>+  
>+  // Wait for fullscreen mode to become active
>+  controller.waitFor(function () {
>+      return controller.window.fullScreen;
>+    }, "Window switched to fullscreen mode");
>+  
[..]
>+  
>+  // Switch back to normal view mode
>+  if (mozmill.isMac) {
>+    var fullscreenCmdKey = utils.getEntity(["chrome://browser/locale/browser.dtd"],
>+	    				    "fullScreenCmd.macCommandKey");
>+    controller.keypress(null, fullscreenCmdKey, {accelKey: true, shiftKey: true});
>+  } else {
>+    controller.keypress(null, "VK_F11", {});
>+  }

We should move this logic into utils.js as setFullscreen(aBoolean). It should always use the keyboard shortcuts and check if the mode has been set. The main menu part can be tested in a special test for fullscreen if necessary.

>+  // Wait for the Tab Groups view to close
>+  activeTabView.waitForClosed();
>+
>+  // Check that a new tab is opened
>+  controller.waitFor(function () {
>+    return controller.tabs.activeTab.URL === 'about:blank'
>+  }, "Expected a new tab to open - got '" + controller.tabs.activeTab.URL +
>+     "', expected 'about:blank'");

Why a waitFor? The tab should be present immediately.

>+  // Check current tab count has one more tab than previously
>+  controller.waitFor(function () {
>+    return activeGroupTabs.length === (initialTabCount + 1)
>+  }, "Expected tab count to increment after opening new tab - got '" +
>+     activeGroupTabs.length + "', expected tabCount '" +
>+     (initialTabCount + 1) + "'");

Same here.

Comment 45

[u279076]

Attachment: Patch v2.4

Comment 46

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

Comment on attachment 520057 [details] [diff] [review]
Patch v2.4

>- *   Clay Earl Uyenghua <[e-mail]>
[..]
> function assertElementVisible(controller, elem, expectedVisibility) {
>   var element = elem.getNode();
>   var visible;
> 
>-  switch (element.nodeName.toLowerCase()) {
>+  switch (element.nodeName) {
>     case 'panel':
>       visible = (element.state == 'open');
>       break;
>-    case 'div':
>-      var style = controller.window.getComputedStyle(element, '');
>-      var state = style.getPropertyValue('display');
>-      visible = (state !== 'none');
>-      break;

I assume that's by accident.

>+ * @param {MozMillController} Controller of the current window

It misses the aController right after the type definition.

>+ * @param {boolean} aFullscreen
>+ *        Whether or not to enter fullscreen mode

Put the description in the same line and use "Boolean" as type.

>+function setFullscreen(aController, aFullscreen) {
>+  // Wait for the display mode to match the expected state
>+  aController.waitFor(function () {    
>+    return aController.window.fullScreen === aFullscreen;
>+  }, "Window changed display mode: got fullscreen='" +
>+     aController.window.fullScreen + "', expected fullscreen='" +
>+     aFullscreen + "'");

We can shorten this message: "Window in fullscreen mode - got 'true/false', expected 'true/false'.

>+function teardownModule(module) {
>+  activeTabView.reset();
>+  tabBrowser.closeAllTabs();
>+}

We have to make sure to reset the fullscreen mode here. Simply move its call from testOpenNewTab to the teardownModule.

>+function testOpenNewTab() {
>+  // Test in normal view mode
>+  checkOpenNewTab();
>+ 
>+  // Switch to fullscreen mode using the menu item
>+  controller.mainMenu.click("#fullScreenItem");

Also use the new utils method here. We should keep the default action for all tests except for the one which tests all different combinations.

>+  // Check that a new tab is opened
>+  controller.waitFor(function () {
>+    return controller.tabs.activeTab.URL === 'about:blank'
>+  }, "Expected a new tab to open - got '" + controller.tabs.activeTab.URL +
>+     "', expected 'about:blank'");

You have given me no reply why you are using waitFor here.

>+  // Check current tab count has one more tab than previously
>+  controller.waitFor(function () {
>+    return activeGroupTabs.length === (initialTabCount + 1)
>+  }, "Expected tab count to increment after opening new tab - got '" +
>+     activeGroupTabs.length + "', expected tabCount '" +
>+     (initialTabCount + 1) + "'");

Same here.

Comment 47

[u279076]

Attachment: Patch v2.5

I've made most of your suggested revisions in this patch. I need more feedback on the following before they can be implemented:

>>+ * @param {boolean} aFullscreen
>>+ *        Whether or not to enter fullscreen mode
>
> Put the description in the same line

All other methods I have seen use put the description the line below for readability.  What's the reasoning behind abandoning this? Will it be covered in the shared module refactoring?

>>+  // Check that a new tab is opened
>>+  controller.waitFor(function () {
>>+    return controller.tabs.activeTab.URL === 'about:blank'
>>+  }, "Expected a new tab to open - got '" + controller.tabs.activeTab.URL +
>>+     "', expected 'about:blank'");
>
> You have given me no reply why you are using waitFor here.

This check immediately follows clicking the + button on the tab bar. Using a waitFor here makes sense to account for tab animation.

Comment 48

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

(In reply to comment #47)
> All other methods I have seen use put the description the line below for
> readability.  What's the reasoning behind abandoning this? Will it be covered
> in the shared module refactoring?

So keep it in the new line for now to be consistent. With the refactoring work it will change.

> >>+  // Check that a new tab is opened
> >>+  controller.waitFor(function () {
> >>+    return controller.tabs.activeTab.URL === 'about:blank'
> >>+  }, "Expected a new tab to open - got '" + controller.tabs.activeTab.URL +
> >>+     "', expected 'about:blank'");
> >
> > You have given me no reply why you are using waitFor here.
> 
> This check immediately follows clicking the + button on the tab bar. Using a
> waitFor here makes sense to account for tab animation.

You don't click the new tab button on the tabbar but inside the tab view. You have a waitForClosed right after which makes sure it will wait until the tab view has been closed. At this point the new tab should already be there. There is no tab open animation.

Comment 49

[u279076]

Attachment: Patch v2.6

Changed the waitFor() into an assert() as requested.

Comment 50

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

Anthony, do you have any update? What's missing on this bug?

Comment 51

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

Bug 836758 will remove Panorama from Firefox soon and make it available as add-on. That means no new tests are necessary. Closing as WONTFIX.