Open Bug 1408754 Opened 2 years ago Updated 2 years ago

Add configuration to send page to about:tabcrashed

Categories

(Testing :: mozscreenshots, enhancement)

enhancement
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: chochri5, Assigned: chochri5, Mentored)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/60.0.3112.113 Chrome/60.0.3112.113 Safari/537.36
Mentor: jaws, mconley
Component: Untriaged → mozscreenshots
Product: Firefox → Testing
Assignee: nobody → chochri5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8918662 [details]
Bug 1408754 - Created configuration for TabCrashed

https://reviewboard.mozilla.org/r/189492/#review196880

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabCrashed.jsm:12
(Diff revision 3)
> +Cu.import("resource://testing-common/TestUtils.jsm");
> +Cu.import("resource://testing-common/ContentTask.jsm");
> +Cu.import("resource://gre/modules/Timer.jsm");

These three imports are all unused and can be deleted.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabCrashed.jsm:32
(Diff revision 3)
> +      }
> +    }
> +  },
> +};
> +
> +function closeAllButOneTab(url = "about:blank") {

This function already exists in Tabs.jsm, but we shouldn't be copying it everywhere.

You should delete this copy of it, and move the one that is in Tabs.jsm to a new ConfigurationHelpers.jsm file.
Attachment #8918662 - Flags: review?(jaws) → review-
Adding needinfo for Chris to make sure he doesn't forget about this.
Flags: needinfo?(chochri5)
Comment on attachment 8918662 [details]
Bug 1408754 - Created configuration for TabCrashed

https://reviewboard.mozilla.org/r/189492/#review208780

I think you pushed a new commit to mozreview without rolling it up with your previous changes, as a result the interdiff between versions you have pushed is confusing/broken.

Do you know why the test does not complete when TabCrashed config is run? What are some ideas you may have as to the reason?

::: browser/tools/mozscreenshots/head.js:31
(Diff revision 4)
>    info("Checking for mozscreenshots extension");
>    return new Promise((resolve) => {
>      AddonManager.getAddonByID("mozscreenshots@mozilla.org", (aAddon) => {
>        isnot(aAddon, null, "The mozscreenshots extension should be installed");
>        TestRunner = Cu.import("chrome://mozscreenshots/content/TestRunner.jsm", {}).TestRunner;
> +      ConfigurationHelpers = Cu.import("chrome://mozscreenshots/content/ConfigurationHelpers.jsm", {}).ConfigurationHelpers;

I don't see a file named `ConfigurationHelpers.jsm` in mozilla-central and your patch isn't adding it. Perhaps you forgot to `hg add` it?
Attachment #8918662 - Flags: review?(jaws) → review-
I think that the helper file isn't being correctly added to head.js.
As for ConfigurationHelpers.js file itself, I must have forgotten to add it again to the commit. I had it in the old one but I forgot to keep working on the old commit.
Flags: needinfo?(chochri5)
Comment on attachment 8918662 [details]
Bug 1408754 - Created configuration for TabCrashed

https://reviewboard.mozilla.org/r/189492/#review210700

::: browser/tools/mozscreenshots/head.js:33
(Diff revision 5)
>      AddonManager.getAddonByID("mozscreenshots@mozilla.org", (aAddon) => {
>        isnot(aAddon, null, "The mozscreenshots extension should be installed");
>        TestRunner = Cu.import("chrome://mozscreenshots/content/TestRunner.jsm", {}).TestRunner;
> +      ConfigurationHelpers = Cu.import("chrome://mozscreenshots/content/ConfigurationHelpers.jsm", {}).ConfigurationHelpers;
>        TestRunner.initTest(this);
> +      ConfigurationHelpers.initTest(this);

ConfigurationHelpers.initTest method doesn't exist. You can delete this line, as I'm not sure what type of set up ConfigurationHelpers would need to do to initialize the test.

::: browser/tools/mozscreenshots/mozscreenshots/extension/ConfigurationHelpers.jsm:13
(Diff revision 5)
> +const HOME_PAGE = "chrome://mozscreenshots/content/lib/mozscreenshots.html";
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Timer.jsm");
> +
> +function closeAllButOneTab(url = "about:blank") {

Since you've created this JSM file and defined closeAllButOneTab and hoverTab here, you should update the other tests to use this version and delete the other implementations of the same functions.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabCrashed.jsm:21
(Diff revision 5)
> +  init(libDir) {},
> +
> +  configurations: {
> +    tabCrashed: {
> +      async applyConfig() {
> +        ConfigurationHelpers.closeAllButOneTab("about:tabcrashed");

I don't think we need to use something as heavy as `closeAllButOneTab` here. It looks like the `loadPage` function defined in ControlCenter.jsm works a little better (uses BrowserTestUtils and awaits until the page is loaded) and also doesn't deal with changing tab hover state.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabCrashed.jsm:22
(Diff revision 5)
> +        await new Promise((resolve, reject) => {
> +          setTimeout(resolve, 3000);
> +        });

The `await` here wouldn't be necessary if you use `loadPage` as described above.
Attachment #8918662 - Flags: review?(jaws) → review-
Comment on attachment 8918662 [details]
Bug 1408754 - Created configuration for TabCrashed

https://reviewboard.mozilla.org/r/189492/#review211276

From your question over IRC,
> When I run the test, it skips the TabCrashed config. For a config to use functions from other files, do you know the syntax to import it?

By "skipping the TabCrashed config" I think what you're seeing is an exception getting thrown due to `loadpage` function not existing and then maybe you're not noticing the exception in the test output? To import the file you will need to call Cu.import or XPCOMUtils.defineLazyModuleGetter (lower in the review I linked you to an example of how that function is called). The general practice is to import it to a global of the same name as the module, thus you would call functions on the global like `ConfigurationHelpers.loadPage()`.

::: browser/tools/mozscreenshots/head.js:31
(Diff revisions 5 - 6)
>    info("Checking for mozscreenshots extension");
>    return new Promise((resolve) => {
>      AddonManager.getAddonByID("mozscreenshots@mozilla.org", (aAddon) => {
>        isnot(aAddon, null, "The mozscreenshots extension should be installed");
>        TestRunner = Cu.import("chrome://mozscreenshots/content/TestRunner.jsm", {}).TestRunner;
>        ConfigurationHelpers = Cu.import("chrome://mozscreenshots/content/ConfigurationHelpers.jsm", {}).ConfigurationHelpers;

Since ConfigurationHelpers isn't used by head.js, you don't need to import it here.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabCrashed.jsm:12
(Diff revisions 5 - 6)
>  this.EXPORTED_SYMBOLS = ["TabCrashed"];
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Timer.jsm");

You can import ConfigurationHelpers.jsm here, since you plan on using it within this file. You could use the Cu.import style that is used with these two, or you could use defineLazyModuleGetter like is done at [1] so that the JSM isn't imported until it is necessary.

[1] https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/browser/components/extensions/ExtensionPopups.jsm#13-14

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabCrashed.jsm:21
(Diff revisions 5 - 6)
>    init(libDir) {},
>  
>    configurations: {
>      tabCrashed: {
>        async applyConfig() {
> -        ConfigurationHelpers.closeAllButOneTab("about:tabcrashed");
> +        await loadpage("about:tabcrashed");

This needs to be ConfigurationHelpers.loadPage (note also the case sensitivity of `loadPage`)

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ControlCenter.jsm
(Diff revision 6)
>        },
>      },
>    },
>  };
>  
> -async function loadPage(url) {

You will need to update ControlCenter.jsm to load the new ConfigurationHelpers.jsm file and then call ConfigurationHelpers.loadPage since the standalone `loadPage` function doesn't exist anymore.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm
(Diff revision 6)
>      triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal()
>    });
>    browserWindow.gBrowser.selectTabAtIndex(1);
>  }
> -
> -function closeAllButOneTab(url = "about:blank") {

Since you've removed closeAllButOneTab and hoverTab from Tabs.jsm, you will need to update Tabs.jsm to import ConfigurationHelpers.jsm and then call these functions off of ConfigurationHelpers.

Or you could put these back since your new JSM isn't going to be using these functions, thus they will not be duplicated.
Attachment #8918662 - Flags: review?(jaws) → review-
You need to log in before you can comment on or make changes to this bug.