1122516 - Refactor tabBrowser's closeAllTabs() method in firefox/lib/tabs.js

Status: RESOLVED FIXED

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

Description

[Teodor Druta]

This method does not clear the history, this could make follow-up tests to fail.

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Why it should clear the history? We need to be able to use this method even without clearing the history. What if we use the method and then we want to restore a tab?

Comment 2

[Teodor Druta]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #1)
> Why it should clear the history? We need to be able to use this method even
> without clearing the history. What if we use the method and then we want to
> restore a tab?

Then I think that it should be passed as an optional parameter, or maybe even better let the method clear history by default, and pass an optional parameter otherwise.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

When I talked with Teodor on IRC and meant clearing the history, it is about the backward and forward buttons in this tab. To be sure to start with a really clean new tab we should then open a new one at the end but not re-use an already open one.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Also I do not see that you mentioned which URL to load. As long as about:newtab cannot be handled correctly in Mozmill, we might have to disabling pre-loading of this page.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Btw. a fix here should really help us to get down the failure rate for waitForPageLoad() calls.

Comment 6

[Teodor Druta]

So, to sum thing up:
What actually we need here in closeAllTabs() method, is that we should open a clean new tab, with about:blank instead of about:newtab, at least till we disable the about new tab preload in mozmill, then close all other tabs.
OR:
Close all tabs besides one, the last one, open a new tab (about:blank), and then close the remaining tab.

Can you confirm this, Henrik? So we could start to work on this.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to Teodor Druta from comment #6)
> What actually we need here in closeAllTabs() method, is that we should open
> a clean new tab, with about:blank instead of about:newtab, at least till we
> disable the about new tab preload in mozmill, then close all other tabs.

That is the right set of actions, because otherwise we jump between closing and opening of tabs.

Comment 8

[Teodor Druta]

Attachment: closealltabsref.patch

So... this should be it. Henrik, can you please provide some feedback on this ? Before I'll request for a review.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8552447 [details] [diff] [review]
closealltabsref.patch

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

::: firefox/lib/tabs.js
@@ +518,5 @@
>     */
>    closeAllTabs : function tabBrowser_closeAllTabs() {
> +    // A new tab will open with default 'about:newtab'
> +    // set the 'browser.newtab.url' preference to 'about:blank'
> +    prefs.setPref(PREF_NEWTAB_URL, "about:blank");

Thinking more about it we might want to keep about:newtab but disable pre-loading as earlier discussed. Using this page would make more sense given that we hit lots of real problems in the past.

@@ +526,5 @@
> +
> +    this.selectTab({index: 0});
> +
> +    // Restore 'browser.newtab.url' preference to 'about:newtab'
> +    prefs.setPref(PREF_NEWTAB_URL, "about:newtab");

Better to move this up right below waitForPageLoad() and put all into a try/finally to ensure we reset this pref correctly.

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

We don't want a separate file for that. It should be testTabs.js.

Comment 10

[Teodor Druta]

Attachment: closealltabsrefv2.patch

Removed the unit test, added the code to a new function in the testTabs.js .
Now before opening a new tab, the browser.newtab.preload property is set to false, a new tab is opened and the property is reset to default in a try/finally block.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8552979 [details] [diff] [review]
closealltabsrefv2.patch

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

You can request from Henrik with this solved.

::: firefox/lib/tabs.js
@@ +526,5 @@
> +      this.openTab();
> +      this._controller.waitForPageLoad();
> +    }
> +    finally {
> +      // Restore 'browser.newtab.preload' preference to false

You restore to true, the default value.

Comment 12

[Teodor Druta]

Attachment: closealltabsrefv2.1.patch

Fixed.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8553035 [details] [diff] [review]
closealltabsrefv2.1.patch

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

::: firefox/lib/tabs.js
@@ +518,5 @@
>     */
>    closeAllTabs : function tabBrowser_closeAllTabs() {
> +    // TODO: Bug 1120906
> +    // waitForPageLoad fails after opening "about:newtab" when
> +    // "browser.newtab.preload" property is set to the "true" value

"preference is set to true"

@@ +522,5 @@
> +    // "browser.newtab.preload" property is set to the "true" value
> +    prefs.setPref(PREF_NEWTAB_PRELOAD, false);
> +
> +    try {
> +      this.openTab();

Meanwhile I think it would be better to have this in openTab() given that this is the code which has to take care about it. Sorry.

@@ +523,5 @@
> +    prefs.setPref(PREF_NEWTAB_PRELOAD, false);
> +
> +    try {
> +      this.openTab();
> +      this._controller.waitForPageLoad();

I mentioned to use this.controller the last time.

@@ +527,5 @@
> +      this._controller.waitForPageLoad();
> +    }
> +    finally {
> +      // Restore 'browser.newtab.preload' preference to true
> +      prefs.clearUserPref(PREF_NEWTAB_PRELOAD);

I don't see a reason for this comment. It's clear that we clear the user value.

::: firefox/lib/tests/testTabs.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var browser = require("../ui/browser");
> +var prefs = require("../../../lib/prefs");

We agreed a while ago to have ui modules in their own block.

@@ +14,5 @@
>    url1: "about:preferences",
> +  url2: "about:accounts",
> +  urls: [BASE_URL + "layout/mozilla_mission.html",
> +         BASE_URL + "layout/mozilla_organizations.html",
> +         BASE_URL + "layout/mozilla_projects.html"]

Why do we need those URLs? You could easily use "about:blank?index=%x%.

@@ +19,3 @@
>  };
>  
> +const PREF_NEWTAB_PRELOAD = "browser.newtab.preload";

There is no need to test for the preference here.

@@ +58,5 @@
> +  expect.equal(location, "about:newtab",
> +               "'about:newtab' tab is open and is the active tab");
> +
> +  expect.equal(browserWindow.tabs.length, 1,
> +               "There is only one open tab");

You want this as the first test.

@@ +61,5 @@
> +  expect.equal(browserWindow.tabs.length, 1,
> +               "There is only one open tab");
> +
> +  expect.equal(prefs.getPref(PREF_NEWTAB_PRELOAD, false), true,
> +               "'browser.newtab.preload' preference has been set to default");

This is not a valid test because it checks against a hard-coded value. You want to remove it as mentioned above.

Comment 14

[Teodor Druta]

Attachment: closealltabsrefv3.patch

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 8553035 [details] [diff] [review]
> closealltabsrefv2.1.patch
> 
> Review of attachment 8553035 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/tabs.js
> @@ +522,5 @@
> > +    // "browser.newtab.preload" property is set to the "true" value
> > +    prefs.setPref(PREF_NEWTAB_PRELOAD, false);
> > +
> > +    try {
> > +      this.openTab();
> 
> Meanwhile I think it would be better to have this in openTab() given that
> this is the code which has to take care about it. Sorry.
> 

As discussed on IRC, we can't implement this in openTab(), so we should leave this in closeAllTabs() until Mozmill 2.1.

> ::: firefox/lib/tests/testTabs.js
> 
> @@ +14,5 @@
> >    url1: "about:preferences",
> > +  url2: "about:accounts",
> > +  urls: [BASE_URL + "layout/mozilla_mission.html",
> > +         BASE_URL + "layout/mozilla_organizations.html",
> > +         BASE_URL + "layout/mozilla_projects.html"]
> 
> Why do we need those URLs? You could easily use "about:blank?index=%x%.
> 

Removed the urls, using "about:blank?index=%x% as suggested now.

> @@ +19,3 @@
> >  };
> >  
> > +const PREF_NEWTAB_PRELOAD = "browser.newtab.preload";
> 
> There is no need to test for the preference here.
> 

Removed the check for preference.

> 
> @@ +61,5 @@
> > +  expect.equal(browserWindow.tabs.length, 1,
> > +               "There is only one open tab");
> > +
> > +  expect.equal(prefs.getPref(PREF_NEWTAB_PRELOAD, false), true,
> > +               "'browser.newtab.preload' preference has been set to default");
> 
> This is not a valid test because it checks against a hard-coded value. You
> want to remove it as mentioned above.

I discovered that the getPref() function http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/prefs.js#l64 in my opinion is not working correctly, if a default value is not specified as 2nd argument, the function will always return undefined.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8553196 [details] [diff] [review]
closealltabsrefv3.patch

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

Just a nit.

::: firefox/lib/tests/testTabs.js
@@ +40,5 @@
> + * Test tabBrowser closeAllTabs() method
> + * Bug 1122516
> + */
> +function testCloseAllTabs() {
> +  var i = 0

semicolon missing.

Comment 16

[Teodor Druta]

Attachment: closealltabsrefv3.1.patch

Fixed. Henrik can you please give a review of this ?

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Comment on attachment 8554540 [details] [diff] [review]
closealltabsrefv3.1.patch

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

Looks fine to me. Please fix the remaining nit, and request review from Andreea.

::: firefox/lib/tests/testTabs.js
@@ +46,5 @@
> +    browserWindow.tabs.openTab();
> +    browserWindow.controller.open("about:blank?index=" + i);
> +    i++;
> +  }
> +  while (i < 2);

I don't see why this has been implemented as a do/while. Please change to a for loop like `for (var i=0; i < 2; i++)` which saves us two lines.

We could also have used data: urls, but this is probably overkill here.

Comment 18

[Teodor Druta]

Attachment: closealltabsrefv3.2.patch

Changed the 'do {} while' block to a 'for () {}' one.

Comment 19

[Andreea Matei [:AndreeaMatei]]

 https://hg.mozilla.org/qa/mozmill-tests/rev/d7617d413f00 (default)

Comment 20

[Teodor Druta]

The patch applies cleanly and runs well on mozilla-aurora.
For mozilla-beta and mozilla-release it is blocked by Bug 1071566.

Comment 21

[Teodor Druta]

Attachment: clstabs_beta_release.patch

This is the patch for mozilla-release and mozilla-beta.

Comment 22

[Teodor Druta]

This patch can't be backported to esr31, due to missing prefs lib.

Comment 23

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/479f574f62b3 (aurora)
https://hg.mozilla.org/qa/mozmill-tests/rev/ff253bcb6254 (beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/d39ed067f846 (release)