Closed Bug 685652 Opened 8 years ago Closed 7 years ago

we need a pushPermissionsEnv equivalent to pushPrefEnv in SpecialPowers

Categories

(Testing :: Mochitest, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

(Whiteboard: [specialpowers])

Attachments

(1 file, 1 obsolete file)

in bug 621363, we add pushPrefEnv and this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=544087

has an example of doing the same for permissions.
Note that the patch linked uses a nested event loop, which is bad. We want to duplicate the semantics of pushPrefEnv instead.
Blocks: 826058
Clint - Getting automation for the install/update apps API for b2g is blocked on this (unless you know of a way to workaround the problem presented in bug 826058). Can you help find an assignee on this bug to look into this?
Flags: needinfo?(ctalbert)
See also bug 846057 which deals with a SpecialPowers API for getting permissions.
Ted and Joel, would one of you have a chance to take a look at this?
Flags: needinfo?(ctalbert)
I asked about this in bug 826058 comment 27 when I was reviewing the patch there. I don't actually know how this stuff works, but assuming we can get a notification when a value has been set (like we can for preferences), this should be straightforward to add.
Do we still need this now that bug 846057 landed?
(In reply to Fabrice Desré [:fabrice] from comment #7)
> Do we still need this now that bug 846057 landed?

At a lesser priority, yes - if we build something similar to pushPrefEnv, we could greatly keep our tests simpler for "always run tests with these perms."
It'd still be nice to have this function, and have it synchronously modify the permission both in the child and the parent process.

But I think the more urgent question is if this blocks bug 826058 still. I think that is something we'll have to answer in bug 826058.
(In reply to Jonas Sicking (:sicking) from comment #9)
> It'd still be nice to have this function, and have it synchronously modify
> the permission both in the child and the parent process.
> 
> But I think the more urgent question is if this blocks bug 826058 still. I
> think that is something we'll have to answer in bug 826058.

Fabrice mentions in IRC that we actually do still need this bug to land the webapps tests.
this patch works well and I have converted an existing test case to use this.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #727812 - Flags: review?(fabrice)
Comment on attachment 727812 [details] [diff] [review]
add in pushPermissions to SpecialPowers (1.0)

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

That looks ok to me, but I'd like someone more familiar with SpecialPowersAPI to review instead.

::: content/base/test/test_XHR_anon.html
@@ +170,5 @@
> +function setup() {
> +  am.init();
> +  //SpecialPowers.addPermission("systemXHR", true, document);
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPermissions([["systemXHR", true, document]], runTests);

I find the syntax here unintuitive. What about using an array of objects:
{ permission: "systemXHR", XXX: true, YYY: document }

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +480,4 @@
>      }
>  
>      SpecialPowers.executeAfterFlushingMessageQueue(function() {
> +        cleanUpCrashDumpFiles();        

nit: trailing whitespace
Attachment #727812 - Flags: review?(fabrice) → feedback+
Attachment #727812 - Flags: review?(josh)
Comment on attachment 727812 [details] [diff] [review]
add in pushPermissions to SpecialPowers (1.0)

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

I agree with Fabrice's comments.

::: content/base/test/test_XHR_anon.html
@@ +38,4 @@
>    },
>  }
>  
> +//var tests = [ test1, test2, test2a, test3, test3, test3, test4, test4, test4, test5, test5, test5 ];

Nix.

@@ +168,5 @@
>  }
>  
> +function setup() {
> +  am.init();
> +  //SpecialPowers.addPermission("systemXHR", true, document);

Nix.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +497,4 @@
>      return crashDumpFiles;
>    },
>  
> +  pushPermissions: function(inPermissions, callback) {

Please document the parameters, especially the format of inPermissions.

@@ +505,5 @@
> +        var permission = inPermissions[p];
> +        var originalValue = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> +        if (this.testPermission(permission[0], Ci.nsIPermissionManager.ALLOW_ACTION, permission[2])) {
> +          originalValue = Ci.nsIPermissionManager.ALLOW_ACTION;
> +        } else if (this.testPermission(permission[0], Ci.nsIPermissionManager.DENY_ACTION, permission[2])) {

Having to make two sync IPC requests to determine that we don't have a stored permission is pretty awful; I'm not sure why the SpecialPowers testPermission API is designed the way it is. Oh well.

@@ +561,5 @@
> +        }
> +        content.window.setTimeout(delayAgain, 0);
> +      }
> +      let cb = callback ? delayedCallback : null;
> +      /* Each pop will have a valid block of preferences */

I don't understand what this means.

@@ +577,5 @@
> +    this.popPermissions(callback);
> +  },
> +
> +
> +  permissionObserver: {

_permissionObserver

@@ +621,5 @@
> +    this.permissionObserver._nextCallback = function () {
> +            self._applyingPermissions = false;
> +            // Now apply any permissions that may have been queued while we were applying
> +            self._applyPermissions();
> +          }

Indentation.
Attachment #727812 - Flags: review?(josh) → review+
updated with review comments, including documenting the parameters and switching to an object instead of an array.
Attachment #727812 - Attachment is obsolete: true
Attachment #730270 - Flags: review+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Backed out for possibly causing OSX 10.6 mochitest-a11y failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f22ec75a02d7
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21171998&tree=Mozilla-
> Inbound

I don't see how this patch could be causing those failures since it doesn't seem to touch chrome://mochitests/content/a11y/accessible/states/test_tree.xul at all.
It does affect the overall harness, though. And indeed, the problem did go away after this backout.
This is because we call SimpleTest.finish() explicitly for OSX 10.6 only here:
https://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/states/test_tree.xul#98
https://hg.mozilla.org/mozilla-central/rev/68a3cd154732
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.