918069 - Use a mock prompt service instead of a WindowListener in browser_homeDrop.js

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

browser_homeDrop.js uses a WindowListener to test that the Set Homepage dialog appears when a link is dragged to the home button. It would be more reliable if it used a mock prompt service.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch switches the test to use a mock prompt service. With this patch applied and the patch for bug 896291 I no longer get hangs within browser_homeDrop.js.

This patch depends on the patch in bug 917887 which moves the test directory.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Forgot to delete a few lines.

Comment 3

[:Paolo Amadini]

Comment on attachment 806907 [details] [diff] [review]
Patch v1.1

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

Thanks for the patch! I've just an observation on the order of the events and some minor comments.

::: browser/base/content/test/general/browser_homeDrop.js
@@ +11,5 @@
> +MockPromptService.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptService]),
> +  confirmEx: function() {
> +    ok(true, "dialog appeared in response to home button drop");
> +    return 1; // 1 = cancel

We should make sure that the test continues only after we get here. You can do this either by invoking a callback or resolving a promise. We should have an additional executeSoon here like the WindowListener did.

nit: I think we now have the chance to verify that some parameters passed to confirmEx (like the button types) are those we expect, right?

@@ +15,5 @@
> +    return 1; // 1 = cancel
> +  }
> +};
> +
> +let mockPromptServiceInstance = new MockObjectRegisterer("@mozilla.org/embedcomp/prompt-service;1",  MockPromptService);

This should be called mockPromptServiceRegisterer, as it's not an instance of nsIPromptService.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

The test now only continues once the prompt has been reached, and I added a verification to check that the prompt has the Yes/No buttons.

 5:11.22 INFO TEST-START | Shutdown
 5:11.22 Browser Chrome Test Summary
 5:11.22        Passed: 4680
 5:11.22        Failed: 0
 5:11.22        Todo: 3

Comment 5

[:Paolo Amadini]

Comment on attachment 809406 [details] [diff] [review]
Patch v1.2

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

Thanks!

::: browser/base/content/test/general/browser_homeDrop.js
@@ +7,3 @@
>  
> +let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +                     getService(Ci.mozIJSSubScriptLoader);

nit: you may reuse scriptLoader for the import above.

@@ +18,5 @@
> +MockPromptService.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptService]),
> +  confirmEx: function(...args) {
> +    ok(true, "dialog appeared in response to home button drop");
> +    is(args[3], Services.prompt.STD_YES_NO_BUTTONS, "dialog should have yes+no buttons");

nit: I'm not a fan of magic numbers, I'd rather use the real confirmEx parameter names in the declaration.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

The mock service has a issue (bug 920616) so it will be easier to just leave this test as-is and remove the mock service from the tree. Bug 896291 is avoiding using the mock service.