567032 - [mozmill] Undo tab closure from context menu

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: patch-wip-v1

This bug will cover the work needed to automate a test for Litmus - Testcase ID #10267
https://litmus.mozilla.org/show_test.cgi?id=10267

Comment 2

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

Comment on attachment 446411 [details] [diff] [review]
patch-wip-v1

>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2009

You are the initial developer. Please update those lines regarding the templates we have in our mozmill-tests repository.

>+ var setupModule = function(module)
>+ {
>+   module.controller = mozmill.getBrowserController();
>+   tabBrowser = new TabbedBrowsingAPI.tabBrowser(controller);

You are testing closing/opening of tabs. Please make sure that you have a known number of tabs open before the test gets started.

>+  for(var i = 0; i < 3; i++){

You can use for each which would make it better readable.

>+  // Close a tab via File > Close tab:
>+  tabBrowser.closeTab({type: 'menu'});

I would suggest to not close the last tab. Lets take tab number 2. That will allow us to check if it gets restored at the correct position.

>+  // Open the tab browser context menu
>+  var tabBar = new elementslib.ID(controller.window.document, 'tabbrowser-tabs');

Please use the getElement function from the API here.

>+  controller.rightClick(tabBar);
>+  controller.waitForElement(tabBar);

That has been mixed up? But the tabbar should always be present, so there is no need to wait.

>+  // Check that 'Undo Close Tab' is available
>+  var contextMenuItem = new elementslib.ID(controller.window.document, 
>+                        'context_undoCloseTab');
>+  controller.assertJS('subject.hidden == false', contextMenuItem.getNode());

That can fail. Please think about that formerly closed tabs in the test-run have already been added to that list of undo tabs. We shouldn't have this dependency. Also please use the an object to pass the value into the expression. That gives us the change to have a better name in the output. There are a lot of tests which make use of that style.

>+  // Check that the recently closed tab is reopened
>+  //controller.assertJS("subject.activeTab.location == " + websites[2], controller.tabs);

That line doesn't look like it's ready yet. See my comment from above regarding assertJS. I also would like to see more tests here. Which ones you would like to add?

>+  // Check that 'Undo Close Tab' is unavailable
>+  controller.assertJS('subject.hidden == true', contextMenuItem.getNode());

You should bring that in the correct order.

>+  UtilsAPI.closeContentAreaContextMenu(controller);

Make sure that this gets run in the teardown function. Otherwise it will stay open if the 2nd last test fails.

Comment 3

[u279076]

>>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>>+ * Portions created by the Initial Developer are Copyright (C) 2009
>
>You are the initial developer. Please update those lines regarding the
>templates we have in our mozmill-tests repository.

Henrik, I disagree with this.  Aaron is contributing code to a Mozilla project.  The copyright date should be updated to 2010 though.

Comment 4

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

Anthony, we already had this discussion a couple of months ago. Aaron is not an employee, so the initial developer cannot be the Mozilla Foundation.

Comment 5

[u279076]

(In reply to comment #4)
> Anthony, we already had this discussion a couple of months ago. Aaron is not an
> employee, so the initial developer cannot be the Mozilla Foundation.

He doesn't have to be.  As soon as the code is checked into the Mozilla code-base, it becomes Mozilla's.  Aaron is a contributor to Mozilla code.

Comment 6

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

(In reply to comment #5)
> (In reply to comment #4)
> > Anthony, we already had this discussion a couple of months ago. Aaron is not an
> > employee, so the initial developer cannot be the Mozilla Foundation.
> 
> He doesn't have to be.  As soon as the code is checked into the Mozilla
> code-base, it becomes Mozilla's.  Aaron is a contributor to Mozilla code.

Gerv, could you please comment?

Comment 7

[Gervase Markham [:gerv]]

Aaron: thanks for the patch! :-)

The Initial Developer in a new file should be the person who owns the copyright to the written code. In the case of employees of or contractors of a Mozilla entity (Foundation, Corporation, Messaging etc.) that is the Mozilla Foundation in all cases. In the case of an employee of another company who is doing the work on company time, it's that company. In the case of a private individual doing the work in their own time, it's that individual. (These are general rules of thumb, which could of course be overridden by the contracts in a particular case.)

You cannot perform a copyright assignment by just putting a different original developer, and Mozilla doesn't accept copyright assignments anyway.

When the code is checked into the Mozilla codebase, it does not in fact "become Mozilla's". It becomes available to anyone under our set of licences, but that's not the same as us owning it.

I hope that helps :-)

Gerv

Comment 8

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

Thanks Gerv for the clarification. I will remember that bug and comment if questions come up again.

Comment 9

[u279076]

(In reply to comment #8)
> Thanks Gerv for the clarification. I will remember that bug and comment if
> questions come up again.

Yes, thank you Gerv for clearing that up.

Comment 10

[Aaron Train [:aaronmt]]

Thanks for the feedback, I am working on an update with some fixes. Henrik, you recommended I not test for visibility of the Undo Close Tab element, what would you recommend instead?

Comment 11

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

(In reply to comment #10)
> Thanks for the feedback, I am working on an update with some fixes. Henrik, you
> recommended I not test for visibility of the Undo Close Tab element, what would
> you recommend instead?

You have to make sure in setupModule that the list of tabs which could be restored is emptied. There should be a function in one of the XPCOM components which can help you.

Comment 12

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

Aaron, do you need any more help to continue your work on this test?

Comment 13

[Aaron Train [:aaronmt]]

Yes actually. As per Comment #11, I took a tour through nsISessionStore but am uncertain as to the proper functions that would make sense to call. Any ideas?

Comment 14

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

You could check how the list of tabs under "Recently Closed Tabs" gets built-up. It's highly possible that the same component offers a function to reset this list. Otherwise you could ask zpao on IRC, who should be able to point you to a better direction as I can at the moment without diving deeper into this problem. Finally this would be a perfect helper function for our SessionStoreAPI.

Comment 15

[Aaron Train [:aaronmt]]

(In reply to comment #14)
> You could check how the list of tabs under "Recently Closed Tabs" gets
> built-up. It's highly possible that the same component offers a function to
> reset this list. Otherwise you could ask zpao on IRC, who should be able to
> point you to a better direction as I can at the moment without diving deeper
> into this problem. Finally this would be a perfect helper function for our
> SessionStoreAPI.

The easiest way is to setPref("browser.sessionstore.max_tabs_undo", 0);

I will need to also do a check against the closed tab count, can I add this to the shared module as a property?

Comment 16

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

(In reply to comment #15)
> The easiest way is to setPref("browser.sessionstore.max_tabs_undo", 0);

Looks good. We have to set it to 0 and reset it immediately to restore the default value. That's a good example for a helper function.

> I will need to also do a check against the closed tab count, can I add this to
> the shared module as a property?

That's outside of the scope of this test. We can't really check it easily via the menu. Those sub menus will be dynamically updated once they get opened and we cannot synthesize this actions on OS X.

Comment 17

[Aaron Train [:aaronmt]]

Attachment: patch-wip-v2

This patch addresses the problems raised in comment #16 and comment #2 , and I think covers the work of the litmus test. I added a function in the shared module that retrieves the undo closed tab count to which I can compare against. Please let me know if there should be more tests and what (i.e., how far should this test cover compared to the chrome-browser tests)

Comment 18

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

Comment on attachment 447841 [details] [diff] [review]
patch-wip-v2

>+ var MODULE_REQUIRES = ['TabbedBrowsingAPI', 'UtilsAPI', 
>+                       'PrefsAPI', 'SessionStoreAPI'];

nit: please use an alphabetical order.

>+ var setupModule = function(module)
>+ {
>+   module.controller = mozmill.getBrowserController();

We don't wanna include tests into other tests. So please remove all "module" prefixes from setupModule.

>+   module.session = new SessionStoreAPI.aboutSessionRestore(controller);

You aren't using about:sessionrestore in that test, right? So it's not necessary.

>+   // Clear sessionstore and disable 'Recently Closed Tabs' pref
>+   PrefsAPI.preferences.setPref("browser.sessionstore.max_tabs_undo", 0);

You should immediately clear this user value again to bring the browser into the default state again. Please also move that code into the SessionStoreAPI.

>+ var testUndoTabFromContextMenu = function()
>+ {
>+  var websites = ['http://www.google.com',
>+                  'http://www.yahoo.com',
>+                  'http://www.bing.com'];

Please use local test files. Check the testOpenTabsInBackground test under the tabbed browing folder. 

>+  var tabCount = SessionStoreAPI.getClosedTabCount(controller);
>+  controller.assertJS("subject == 0", tabCount);

You should use an object to fold values into the assertJS function. That will give much clearer test output. See the following example. There is also no need for an extra variable.

>+  // Close 2nd tab via File > Close tab:
>+  tabBrowser.selectedIndex = 2;

That's the 3rd tab. Our index starts with 0. Also wait until really 2 tabs are open.

>+  tabCount = SessionStoreAPI.getClosedTabCount(controller);
>+  controller.assertJS("subject == 1", tabCount);

Same as above.

>+  var contextMenuItem = new elementslib.ID(controller.window.document, 
>+                          'context_undoCloseTab');

nit: please check indentation. 

>+  tabCount = SessionStoreAPI.getClosedTabCount(controller);
>+  controller.assertJS("subject == 0", tabCount);

Same as above.

>+  // Check if 'Undo Close Tab' is hidden
>+  controller.rightClick(tabBar);
>+  controller.assertProperty(contextMenuItem, 'hidden', true);

Please also use that test before you start the test. So we also test the initial state.


>+function getClosedTabCount(controller)

Please also use an alphabetical order for functions in that file.

>+{
>+ return sessionStoreService.getClosedTabCount(controller.window);
>+}

nit: please fix the indentation (2 spaces on the left side)

Comment 19

[Aaron Train [:aaronmt]]

(In reply to comment #18)
> >+   // Clear sessionstore and disable 'Recently Closed Tabs' pref
> >+   PrefsAPI.preferences.setPref("browser.sessionstore.max_tabs_undo", 0);
> 
> You should immediately clear this user value again to bring the browser into
> the default state again. Please also move that code into the SessionStoreAPI.
 
> >+  var tabCount = SessionStoreAPI.getClosedTabCount(controller);
> >+  controller.assertJS("subject == 0", tabCount);
> 
> You should use an object to fold values into the assertJS function. That will
> give much clearer test output. See the following example. There is also no need
> for an extra variable.

1) What type of function would you like to see? No parameters and just calls prefsAPI.preferences.setPref("browser.sessionstore.max_tabs_undo", 0);?

2. Not sure what you mean by 'use an object to fold values'. getClosedTabCount just returns the number of restorable tabs in the window.

Comment 20

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

(In reply to comment #19)
> > >+  var tabCount = SessionStoreAPI.getClosedTabCount(controller);
> > >+  controller.assertJS("subject == 0", tabCount);
> > 
> > You should use an object to fold values into the assertJS function. That will
> > give much clearer test output. See the following example. There is also no need
> > for an extra variable.
> 
> 1) What type of function would you like to see? No parameters and just calls
> prefsAPI.preferences.setPref("browser.sessionstore.max_tabs_undo", 0);?

A function which will reset the list of closed tabs. We don't need any parameter here, correct. And as said above, also clear the user value immediately.

> 2. Not sure what you mean by 'use an object to fold values'. getClosedTabCount
> just returns the number of restorable tabs in the window.

Sorry, I forgot to past the URL of the example. See this line and others in the test: http://hg.mozilla.org/qa/mozmill-tests/file/cfb139777b02/firefox/restartTests/testExtensionInstallUninstall/test1.js#l88

Comment 21

[Aaron Train [:aaronmt]]

Attachment: patch-wip-v3.patch

This patch addresses the requests made in comment #18.

Comment 22

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

Comment on attachment 448298 [details] [diff] [review]
patch-wip-v3.patch

>+ const localTestFolder = collector.addHttpResource('../testTabbedBrowsing/files');

In a follow-up patch we should move all of those local tests to firefox/files. But it's not something you have to take care about here. It will change in bug 568008.

>+ var teardownModule = function(module)
>+ {

Nit: We wanna make sure to follow the design patterns for Javascript. Please always place the opening bracket in the same line as the function declaration.

>+  PrefsAPI.preferences.setPref("browser.sessionstore.max_tabs_undo", 1);

Please call the helper function here. We have to reset the list of closed tabs and not limit it to only 1 tab for later tests.

>+  // Check 'Recently Closed Tabs' count, should be 0
>+  controller.assertJS("subject == 0", SessionStoreAPI.getClosedTabCount(controller));

I miss the object usage here as what you have already updated some lines below.

>+  // Open 3 tabs with pages in the local test folder
>+  for (var i = 0; i < 3; i++){
>+   tabBrowser.openTab({type: 'menu'});
>+   controller.open(localTestFolder + "openinnewtab.html");
>+   controller.waitForPageLoad();  
>+  }

We have to open three different pages to check if the correct one has been restored at the wanted position. Therefor you should use e.g. 'openinnewtab_target.html?id=' + i. Also check the indentation of the bracket in the first line.

Once you have closed tab 2 make sure that the link id of the new second tab is 3. Once restored it should be 2 again. Also switching to tab 1 before restoring tab 2 will make sure that we also can test if we restore the recently closed tab at the correct position.

>+  // Open the tab browser context menu
>+  controller.rightClick(tabBar);
>+
>+  // Check if 'Undo Close Tab' exists and is visible
>+  controller.assertProperty(contextMenuItem, 'hidden', false);

Can you shorten the comment and just use it as below:

>+  // Check if 'Undo Close Tab' is hidden
>+  controller.rightClick(tabBar);
>+  controller.assertProperty(contextMenuItem, 'hidden', true);
>+
>+ }

nit: please remove the empty line

>+++ b/shared-modules/testSessionStoreAPI.js
> /**
>+ * Resets the list of recently closed tabs by setting and clearing the user preference
>+ *
>+ */
>+function clearRecentlyClosedTabs()

Please rename it to resetRecentlyClosedTabs. Also please remove the empty line from the comment.

>+  var prefSrv = collector.getModule('PrefsAPI').preferences;

It's not the pref service we are using here. Call it prefs or preferences to not give readers a wrong impression.

>+  prefSrv.setPref("browser.sessionstore.max_tabs_undo", 0);
>+  prefSrv.clearUserPref("browser.sessionstore.max_tabs_undo");

For the preference name we have to add a global constant.

Aaron, if you have questions please contact me on IRC so we can figure out those issues really quick without the delays for reviews.

Comment 23

[Aaron Train [:aaronmt]]

Attachment: patch-wip-v4

This patch addresses the requests made in comment #22

Comment 24

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

Comment on attachment 448639 [details] [diff] [review]
patch-wip-v4

>+var teardownModule = function(module) {
>+  UtilsAPI.closeContentAreaContextMenu(controller);
>+  SessionStoreAPI.resetRecentlyClosedTabs();
>+}

Please also close all open tabs after the test-run. Sorry forgot to mention it the last time.

>+var testUndoTabFromContextMenu = function() {
>+
>+  // Open the tab browser context menu
>+  var tabBar = tabBrowser.getElement({type: 'tabs'});

nit: remove that empty line.

>+  // Open 3 tabs with pages in the local test folder
>+  for (var i = 0; i < 3; i++) {
>+   controller.open(localTestFolder + 'openinnewtab_target.html?id=' + i);
>+   tabBrowser.openTab({type: 'menu'});
>+   controller.waitForPageLoad();  
>+  }

Even we are fast when loading local pages this could cause race conditions. waitForPageLoad has to be called before you open a new tab. In the case above it checks the new tab instead the one where we load the web page.

>+  // Check if 'Undo Close Tab' is hidden
>+  controller.rightClick(tabBar);
>+  controller.assertProperty(contextMenuItem, 'hidden', true);
>+  UtilsAPI.closeContentAreaContextMenu(controller);

While this works with Namoroka it constantly fails with Minefield:

Controller.assertProperty(ID: context_undoCloseTab) : true == false'

Aaron, on which platforms you can test?

Comment 25

[Aaron Train [:aaronmt]]

Changes made as per request above. Henrik, I am testing on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.5pre) Gecko/20100602 Namoroka/3.6.5pre as initially requested.

Comment 26

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

Can you please check why it is failing on trunk and incorporate the last review comments too? As seen the "Undo Close Tab" is still visible after we have restored the tab. While running a quick test showed me that opening the context menu a second time will work. That said it sounds like we are opening the menu too early.

Comment 27

[Aaron Train [:aaronmt]]

I believe the test is failing with the rightClick, because tabBar is null (when run with trunk)

My suspicion (from inspecting on trunk) is that with recent changes with tabs on trunk, there is a different anonid/id (i.e., TabsToolbar/tabbrowser-tabs) and the tabBrowser_getElement will have to reflect the differences in changes from 1.9.2 and trunk.

Comment 28

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

I have updated the API to use all the new elements with bug 567486. Are you using the default branch? It should work with the latest changeset. I don't get null returned.

Comment 29

[Aaron Train [:aaronmt]]

Attachment: patch v5

This patch addresses the requests made in comment #24 and was tested on both branches.

Comment 30

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

Landed on all branches as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b9623dcdce3f
http://hg.mozilla.org/qa/mozmill-tests/rev/92975965a35c
http://hg.mozilla.org/qa/mozmill-tests/rev/0bc2a6cc5a95

Comment 31

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

Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.