Closed Bug 1252462 Opened 4 years ago Closed 4 years ago

Enable the test_unsafeBidiChars.xhtml test for e10s

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set

Tracking

(e10s+, firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: florian, Assigned: florian)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The code replacing the HelperAppLauncherDialog needs to run in the chrome process.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8725257 - Flags: review?(paolo.mozmail)
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.
Attachment #8725257 - Flags: review?(paolo.mozmail) → review+
(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.
Attached patch Patch v2Splinter Review
Attachment #8725257 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/1db864e863ed710a3e51ef795b32e070fd9cc145
Bug 1252462 - Enable the test_unsafeBidiChars.xhtml test for e10s, r=paolo.
https://hg.mozilla.org/mozilla-central/rev/1db864e863ed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.