1252462 - Enable the test_unsafeBidiChars.xhtml test for e10s

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Florian Quèze [:florian]]

The code replacing the HelperAppLauncherDialog needs to run in the chrome process.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Comment 2

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e397aa52c974

Comment 3

[:Paolo Amadini]

Comment on attachment 8725257 [details] [diff] [review]
Patch

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

Thanks! I've three minor comments that are not mandatory to fix.

::: uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml
@@ +41,1 @@
>  SimpleTest.waitForExplicitFinish();

nit: I've noticed an add_task helper that would save you the calls to SimpleTest.waitForExplicitFinish() and SimpleTest.finish().

@@ +53,5 @@
> +            chromeScript.removeMessageListener("suggestedFileName", listener);
> +            resolve(data);
> +          });
> +        });
> +        let name = replace(test, char), expected = sanitize(test);

nit: I'd rather these to be two separate let statements.

@@ +63,4 @@
>  
> +    let promise = new Promise(function(resolve) {
> +      chromeScript.addMessageListener("unregistered", function() {
> +        resolve();

nit: I guess that for correctness we'd have to unregister this listener too, though it doesn't actually change the code flow.

Comment 4

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #3)

> Thanks! I've three minor comments that are not mandatory to fix.

Thanks for the review! :-)

> ::: uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml
> @@ +41,1 @@
> >  SimpleTest.waitForExplicitFinish();
> 
> nit: I've noticed an add_task helper that would save you the calls to
> SimpleTest.waitForExplicitFinish() and SimpleTest.finish().

I had noticed it too but initially didn't dare using it because I was afraid it wouldn't wait for the load event. This wasn't a real problem though, as add_task is spawning the task from a setTimeout.

> @@ +63,4 @@
> >  
> > +    let promise = new Promise(function(resolve) {
> > +      chromeScript.addMessageListener("unregistered", function() {
> > +        resolve();
> 
> nit: I guess that for correctness we'd have to unregister this listener too,
> though it doesn't actually change the code flow.

It's not needed because the explicit chromeScript.destroy() call a few lines later will drop the whole array of listeners: http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js?rev=fe5831958b30#485
But I made the suggested change anyway to make the test more future proof, in case someone ever expands it.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/1db864e863ed710a3e51ef795b32e070fd9cc145
Bug 1252462 - Enable the test_unsafeBidiChars.xhtml test for e10s, r=paolo.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1db864e863ed