Expanded principals are basically a set of principals that have the union of the permissions of each individual principal they contain. They don't have a useful return value from GetURI, which means that nsPermissionManager::CommonTestPermission doesn't do anything useful with them. It should instead detect expanded principals (with a QI), and iteratively check each principal inside the nsEP in that case.

Comment on attachment 8343238 [details] [diff] [review] Part 2 - Handle nsIExpandedPrincipal instances in the permission manager API. v1 Henry, do these patches fix your problem in bug 946322? Note that they're entirely untested, because AFAICT, there isn't currently an in-tree situation where we have a permission-dependent global constructor. As such, I'd ask you (Henry) to add the tests that I want for this when you land it all. In particular, here's what it'd look like, as an addition to file_bug821850.xhtml: (1) before the win.go() call, make sure that the XBL does not see the MozWifiP2pManager constructor. (2) At the end of go(), make sure that content does not see the MozWifiP2pManager constructor. (3) Use SpecialPowers to add the permission, and then verify that content _does_ see the constructor. (4) in finish(), verify that XBL now _does_ see the MozWifiP2pManager constructor. Let me know if these patches work, and if you have any trouble adding that test coverage.

(In reply to Bobby Holley (:bholley) from comment #1) > Created attachment 8343237 [details] [diff] [review] > Part 1 - Hoist system-principal checks into CommonTestPermission. v1 I can't tell what the comment you're moving means... Feel free to rephrase :)

It seems to still not expose MozWifiP2pManager on XBL scope: https://tbpl.mozilla.org/?tree=Try&rev=f14293797ce6 https://hg.mozilla.org/try/rev/f14293797ce6#l2.12 (adding permission by SpecialPowers) I have a question though: when running mochitest dom/tests/mochitest/general/test_interfaces.html on XBL scope, what permission and what preference does the test app has? Are they all the same as the non-XBL scope test?

(In reply to Henry Chang [:henry] from comment #6) > It seems to still not expose MozWifiP2pManager on XBL scope: > https://tbpl.mozilla.org/?tree=Try&rev=f14293797ce6 > https://hg.mozilla.org/try/rev/f14293797ce6#l2.12 (adding permission by > SpecialPowers) Can you debug this locally? And can you add the test coverage from comment 4, which will be a more direct mechanism to test this bug than test_interfaces.html. If you don't want to do this, I'd request that you put together a mock constructor that reproduces this issue, so that I can debug it on desktop. > I have a question though: when running mochitest > dom/tests/mochitest/general/test_interfaces.html on XBL scope, what > permission and what preference does the test app has? Are they all the same > as the non-XBL scope test? The code running the XBL scope should have an nsExpandedPrincipal, containing the principal of the scope to which it is bound. My patch (above) makes sure that expanded principals get all of the privileges of the principals they contain. So with my patch, the code in file_interfaces.xml should have all of the privileges (and more) of the code in test_interfaces.html. But again, I don't have anything I can test this with. So there may well be a bug in the patch.

I am pretty willing to test this locally and I may spend some time studying how to run a mochitest on XBL scope since I have poor knowledge about XBL. But I can also provide a case to reproduce this. Besides, is your patch supposed to solve this issue as well: Removing "xbl: false" results in test failure (I mentioned this in the original bug.) Thanks!

(In reply to Henry Chang [:henry] from comment #8) > I am pretty willing to test this locally and I may spend some time studying > how to run a mochitest on XBL scope since I have poor knowledge about XBL. > But I can also provide a case to reproduce this. > > Besides, is your patch supposed to solve this issue as well? : Removing "xbl: > false" results in test failure (I mentioned this in the original bug.) > Removing "xbl: false" *from the entry SimpleTest* .... > Thanks!

(In reply to Henry Chang [:henry] from comment #8) > I am pretty willing to test this locally and I may spend some time studying > how to run a mochitest on XBL scope since I have poor knowledge about XBL. > But I can also provide a case to reproduce this. Providing a patch that reproduces this issue on a regular desktop build is probably the most efficient thing to do here. However, if you want to dig in and learn how all this stuff works, I'm happy to mentor you and give a basic overview. It's up to you (and your deadlines). :-) > Besides, is your patch supposed to solve this issue as well: Removing "xbl: > false" results in test failure (I mentioned this in the original bug.) I would think it would. But note that the test coverage suggested in comment 4 would be more direct.

I just dug a little deeper and found it has nothing to do with the expanded permission. My permission check function |Navigator::HasXxxSupport| makes use |Navigator::GetWindowFromGlobal|. When on XBL scope, this function returns a null window object. Not sure if it is because of http://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#594 Due to the null window object pointer, the permission check function returns false. It doesn't even delegate to nsPermissionManager for the XBL case. (Something like http://dxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1800 )

The global of an XBL scope is some XBL global, not a Window...

(In reply to Boris Zbarsky [:bz] from comment #12) > The global of an XBL scope is some XBL global, not a Window... No wonder... Probably I have to use Navigator::mWindow instead of "window from global" to do the permission check. I will try it tmr. But here's a webidl design policy I am wondering: When should we expose webidl to global and when should we only expose to Navigator? Before webidl, for example, http://dxr.mozilla.org/mozilla-central/source/dom/wifi/DOMWifiManager.manifest#4 , we use "JavaScript-navigator-property" to expose an API to navigator only. With webidl, we use "NavigatorProperty" to expose to global as well as navigator. If we only want to expose to navigator, what is the canonical way to do it with webidl? (e.g. check in constructor or ?) Or "exposing to navigator only" doesn't make any sense for webidl? Thanks :)

> Probably I have to use Navigator::mWindow instead of "window from global" to do the > permission check. That seems pretty fishy to me, honestly. But maybe in practice it ends up not mattering....

(In reply to Boris Zbarsky [:bz] from comment #14) > > Probably I have to use Navigator::mWindow instead of "window from global" to do the > > permission check. > > That seems pretty fishy to me, honestly. But maybe in practice it ends up > not mattering.... Oops... Navigator::HasXxxSupport is static, having no member variable Navigator::mWindow to use...

What are you doing with the Window, exactly? Are you just grabbing the principal and passing that into the permission manager? You should probably just get the the principal of the global with nsContentUtils::GetObjectPrincipal, and pass that directly to the permission manager. Or does your constructor depend on Window in some other way?

https://tbpl.mozilla.org/?tree=Try&rev=6557c7553f0f It totally works! Thanks you two! So can we land these two patches now? The test case you mentioned in comment 4 requires patches in https://bugzilla.mozilla.org/show_bug.cgi?id=811635 to land beforehand. Thanks!

(In reply to Henry Chang [:henry] from comment #17) > So can we land these two patches now? The test case you mentioned in comment > 4 requires patches in https://bugzilla.mozilla.org/show_bug.cgi?id=811635 > to land beforehand. Thanks! Well, the patches need review. But before that, I want to make sure that they do what I expect. Could you write the tests from comment 4 against your local tree (which presumably has bug 811635), verify that they work, and flag me for review? Once we have the test coverage ready to go (pending the landing of bug 811635), we can land these patches.

Comment on attachment 8343237 [details] [diff] [review] Part 1 - Hoist system-principal checks into CommonTestPermission. v1 I guess we might as well get these reviewed in parallel. Jonas, I know you're busy - is there anyone else who's owning the permission stuff now that mounir and justin are gone? These patches should also be very simple to review, FWIW.

Comment on attachment 8343237 [details] [diff] [review] Part 1 - Hoist system-principal checks into CommonTestPermission. v1 Review of attachment 8343237 [details] [diff] [review]: ----------------------------------------------------------------- Andrea has been digging in the permission manager lately. But I think there might be other candidates in the DOM team as well. Also, obviously bz :)

I just tried to write the tests from comment 4 and found SpecialPowers.addPermssion/removePermission could only change the permission asynchronously. (nsPermissionManager::AddFromPrincipal is called on parent process in the first place then use IPC to change the child permission.) So probably I have to change the permission and do the test asynchronously. I am not sure if it somehow breaks the original test design. What do you think of this? Thanks.

(In reply to Henry Chang [:henry] from comment #21) > I just tried to write the tests from comment 4 and found > SpecialPowers.addPermssion/removePermission > could only change the permission asynchronously. > (nsPermissionManager::AddFromPrincipal is called on parent process in the > first place then use IPC to change the child permission.) So probably I have > to change the permission and do the test asynchronously. I am not sure if it > somehow breaks the original test design. What do you think of this? It shouldn't require any restructuring. Just put the call to bound.dispatchEvent at the end of go() in the async callback. The rest of the test won't run until that call happens.

(In reply to Bobby Holley (:bholley) from comment #22) > (In reply to Henry Chang [:henry] from comment #21) > > I just tried to write the tests from comment 4 and found > > SpecialPowers.addPermssion/removePermission > > could only change the permission asynchronously. > > (nsPermissionManager::AddFromPrincipal is called on parent process in the > > first place then use IPC to change the child permission.) So probably I have > > to change the permission and do the test asynchronously. I am not sure if it > > somehow breaks the original test design. What do you think of this? > > It shouldn't require any restructuring. Just put the call to > bound.dispatchEvent at the end of go() in the async callback. The rest of > the test won't run until that call happens. Sorry I couldn't get what you said. Did you mean I can add a <handler event="addpermissionevent" action="SpecialPowers.addPermission('wifi-manage, true, window.document);"/> and add the following at the end of |go| ? bound.dispatchEvent(new CustomEvent('addpermissionevent')); ok(window.MozWifiP2pManager, "Content should see MozWifiP2pManager without permission");

From what I studies, things would happen like the followings: Child Parent ------------------------------------------------------------------------------------------------------------------ SpecialPowers.addPermission() _sendSyncMessage ----------------------------> SpecialPowersObserverAPI._recevieMessageAPI("SPPermissionManager") Services.perms.addFromPrincipal nsPermissionManager::AddFromPrincipal nsPermissionManager::AddInternal [synchronous] Add parent's permission Update nsPermissionManager::mPermissionTable ContentParent::SendAddPermission Permission not added yet <----------------------------- return (Permission added) . . . (After several tests) . . ContentChild::RecvAddPermission nsPermissionManager::AddInternal Update nsPermissionManager::mPermissionTable (Permission finally added) ----------------------------------------------------------------------------------------------------------------- Even permissions.sqlite has been added certain permission after SpecialPowers.addPermission() returns, the local cache nsPermissionManager::mPermissionTable, which is used to check the permission in nsPermissionManager::CommonTestPermission, is still not updated in child process. That's why I am wondering the timing when the permission is guaranteed added to the child process...

Re-formatted the diagram to fit the width: Child Parent ---------------------------------------------------------------------------------------- SpecialPowers.addPermission() _sendSyncMessage --------------> SpecialPowersObserverAPI. _recevieMessageAPI("SPPermissionManager") Services.perms.addFromPrincipal nsPermissionManager::AddFromPrincipal nsPermissionManager::AddInternal [synchronous] Update permissions.sqlite Update nsPermissionManager::mPermissionTable ContentParent::SendAddPermission Permission not added yet <----------------- return (Permission added to parent) . . . (After several tests) . . ContentChild::RecvAddPermission nsPermissionManager::AddInternal Update nsPermissionManager::mPermissionTable (Permission added to child)

(In reply to Henry Chang [:henry] from comment #23) > (In reply to Bobby Holley (:bholley) from comment #22) > > (In reply to Henry Chang [:henry] from comment #21) > > > I just tried to write the tests from comment 4 and found > > > SpecialPowers.addPermssion/removePermission > > > could only change the permission asynchronously. > > > (nsPermissionManager::AddFromPrincipal is called on parent process in the > > > first place then use IPC to change the child permission.) So probably I have > > > to change the permission and do the test asynchronously. I am not sure if it > > > somehow breaks the original test design. What do you think of this? > > > > It shouldn't require any restructuring. Just put the call to > > bound.dispatchEvent at the end of go() in the async callback. The rest of > > the test won't run until that call happens. > > Sorry I couldn't get what you said. Did you mean I can add a > > <handler event="addpermissionevent" > action="SpecialPowers.addPermission('wifi-manage, true, window.document);"/> > > and add the following at the end of |go| ? No. I'll attach a patch showing what I mean.

Attaching a mockup of the tests. It looks like SpecialPowers.addPermission is designed to be sync. If it's behaving async, that's either a bug that we need to fix, or it should accept a callback that will be invoked once the permission has been fully applied. I'm not the right person to say which of those is the case.

(In reply to Bobby Holley (:bholley) from comment #27) > Created attachment 8347027 [details] [diff] [review] > Tests mockup. > > Attaching a mockup of the tests. > > It looks like SpecialPowers.addPermission is designed to be sync. If it's > behaving async, that's either a bug that we need to fix, or it should accept > a > callback that will be invoked once the permission has been fully applied. I'm > not the right person to say which of those is the case. Thanks for the mockup you attached, which is exactly what I did at first. And then I found the test will fail because of the async behavior of SpecialPowers.addPermission. So do you think jgriffin is the right person for this issue? Thanks!

Yep, at least to find an owner. jgriffin - It looks like SpecialPowers.addPermission does not take effect immediately, but does not accept a callback either. Can you or someone on your team help sort this out?

Comment on attachment 8343238 [details] [diff] [review] Part 2 - Handle nsIExpandedPrincipal instances in the permission manager API. v1 Review of attachment 8343238 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +210,5 @@ > > +static bool > +IsExpandedPrincipal(nsIPrincipal* aPrincipal) > +{ > + nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(aPrincipal); do you need to increase+decrease the reference here? @@ +1108,5 @@ > + // For expanded principals, we want to iterate over the whitelist and see > + // if the permission is granted for any of them. > + nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(aPrincipal); > + if (ep) { > + nsTArray<nsCOMPtr<nsIPrincipal> >* whitelist; This additional space is not needed any more.

(In reply to Andrea Marchesini (:baku) from comment #30) > > +static bool > > +IsExpandedPrincipal(nsIPrincipal* aPrincipal) > > +{ > > + nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(aPrincipal); > > do you need to increase+decrease the reference here? As noted on IRC, QI implicitly AddRefs. > > + nsTArray<nsCOMPtr<nsIPrincipal> >* whitelist; > > This additional space is not needed any more. Between the > > you mean? That's cool! I didn't know that.

> Yep, at least to find an owner. > > jgriffin - It looks like SpecialPowers.addPermission does not take effect > immediately, but does not accept a callback either. Can you or someone on > your team help sort this out? This doesn't look like any kind of problem in specialpowers; I'd guess we'd have to ask people who understand the permission manager better. It's also possible this doesn't take effect immediately because the permission is only checked at certain times.

(In reply to Jonathan Griffin (:jgriffin) from comment #32) > > Yep, at least to find an owner. > > > > jgriffin - It looks like SpecialPowers.addPermission does not take effect > > immediately, but does not accept a callback either. Can you or someone on > > your team help sort this out? > > This doesn't look like any kind of problem in specialpowers; Well, it would be a problem with the SpecialPowers API if permissions took effect asynchronously and the API didn't provide any mechanism to trigger it. But yes, let's see what the permission people say

(In reply to Bobby Holley (:bholley) from comment #31) > Between the > > you mean? That's cool! I didn't know that. I didn't know that, too! But is it portable to all compilers other than gcc with --std=c++0x?

It is supported by all compilers that we support.

Created test case to test the permission things. Since we couldn't synchronously change the permission so I just worked around by setTimeout. The test runs correctly on the emulator. On TPBL, since MozWifiP2pManager only has implementation on gonk, it will fail on other platform. However, this test case can still verify the attached patches.

(In reply to Henry Chang [:henry] from comment #37) > Created test case to test the permission things. Since we couldn't > synchronously change the permission so I just worked around by setTimeout. > The test runs correctly on the emulator. On TPBL, since MozWifiP2pManager > only has implementation on gonk, it will fail on other platform. However, > this test case can still verify the attached patches. Yeah. And setTimeout is never really acceptable for tests running in automation (too flaky and prone to intermittent orange). So let's do this: 1 - Land the patches in the bug (but not the tests). 2 - Mark this bug as in-testsuite?, and get somebody working on figuring out the addPermission issues. 3 - Land your wifi changes. 4 - Once addPermission is sorted out, we can remove the setTimeout and land the tests. I'll do (1) and (2) right now.

I've flagged baku for info in bug 951272. Clearing the needinfo? flags here.

Looks like the answer is to use SpecialPowers.pushPermissions. Henry, can you fix up the tests to do that and flag me for review?

Try push for the patches in this bug, which can land on their own: https://tbpl.mozilla.org/?tree=Try&rev=460cbd7ccd99

(In reply to Bobby Holley (:bholley) from comment #40) > Looks like the answer is to use SpecialPowers.pushPermissions. Henry, can > you fix up the tests to do that and flag me for review? I love SpecialPowers.pushPermissions! I was using setTimeout just for verifying locally and indeed knew it would never ever appear in the tree :) BTW, in the real test case, I have to only add the test to WIDGET_GONK, maybe using flag or something else. Let me try it and fix it right now! Thanks!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c5dee37341 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/adb27b99c7d8

It works perfectly with SpecialPowers.pushPermissions and its callback! SpecialPowers is the best! But there are still two things I am worried about: 0. The test container app used by mochitest has "wifi-manage" permission in its manifest.webapps. It's fine. 1. The attached test case only tests one way: w/o permission (visible) ==> with permission (invisible). For the converse case: "with permission ==> w/o permission", the ctor is still visible after removing the permission. I am wondering if there is something cached and I am still figuring it out. 2. If we want to test the former case only, due to 0), we have to remove the permission before testing (just as what I changed in test_bug812850.html) In this case, we will call removePermission in turn pushPermissions. In SimpleTest.finish(), the permissions will be automatically reverted to the state right before calling pushPermissions, which has "no wifi-manage", making this test case have side effect. So Bob what do you say? Are you comfortable with testing the only one way (w/o permission ==> with permission)? If you are, then the last thing I need to do is to solve issue 2). If you are NOT, I may need to find out why the visibility is cached... Thanks!

(In reply to Henry Chang [:henry] from comment #44) > For the converse case: "with permission ==> w/o permission", > > the ctor is still visible after removing the permission. > I am wondering if there is something cached and I am still figuring it out. This is expected. Once ctor is created on a global, the test function will not even be called on the global. If you want to test "with permission ==> w/o permission" case, you will have to create a new global (by inserting an iframe, for example.)

(In reply to Henry Chang [:henry] from comment #44) > But there are still two things I am worried about: > > 0. The test container app used by mochitest has "wifi-manage" permission in > its manifest.webapps. It's fine. I don't understand this. Can you clarify? > > 1. The attached test case only tests one way: > > w/o permission (visible) ==> with permission (invisible). > > For the converse case: "with permission ==> w/o permission", > > the ctor is still visible after removing the permission. Yeah, as Masatoshi noted, this is to be expected.

(In reply to Bobby Holley (:bholley) from comment #46) > (In reply to Henry Chang [:henry] from comment #44) > > But there are still two things I am worried about: > > > > 0. The test container app used by mochitest has "wifi-manage" permission in > > its manifest.webapps. It's fine. > > I don't understand this. Can you clarify? > It's just the pre-condition, not issue :p > > > > 1. The attached test case only tests one way: > > > > w/o permission (visible) ==> with permission (invisible). > > > > For the converse case: "with permission ==> w/o permission", > > > > the ctor is still visible after removing the permission. > > Yeah, as Masatoshi noted, this is to be expected. Thanks Masatosh san! So Bob do you think we should test both cases?

(In reply to Henry Chang [:henry] from comment #47) > So Bob do you think we should test both cases? Removing the permission and testing the property once the constructor has already been resolved isn't really meaningful.

This test case uses SpecialPowers.pushPermissions to remove/add permission so that it will have no side effect.

https://hg.mozilla.org/mozilla-central/rev/d7c5dee37341 https://hg.mozilla.org/mozilla-central/rev/adb27b99c7d8

Please flag me for review when the patch is ready. :-)

(In reply to Bobby Holley (:bholley) from comment #51) > Please flag me for review when the patch is ready. :-) The patch is almost ready for review except for the lack of excluding non-gonk case. I'm still studying how to do this. But I'll appreciate you if you could give some preliminary feedback at this point! Thanks!

If this only works with gonk, I think it should probably be split into a separate test (you can just copy the test files here, update the bug number, and strip out the rest of the stuff). Then you can just do add skip-if = appname != "b2g" to mochitest.ini.

+ // Start with DENY_ACTION. If we get PROMPT_ACTION, keep going to see if + // we get ALLOW_ACTION from another principal. + *aPermission = nsIPermissionManager::DENY_ACTION; This is inconsistent behavior with regular principals. Should be nsIPermissionManager::UNKNOWN_ACTION. Is this just a bug or did you have a reason for this asymmetry? Note: that this is a blocking issue in the nsEP for jetpack content-script patch.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #54) > + // Start with DENY_ACTION. If we get PROMPT_ACTION, keep going to see if > + // we get ALLOW_ACTION from another principal. > + *aPermission = nsIPermissionManager::DENY_ACTION; > > This is inconsistent behavior with regular principals. Should be > nsIPermissionManager::UNKNOWN_ACTION. > Is this just a bug or did you have a reason for this asymmetry? Note: that > this is a blocking issue in the nsEP for jetpack content-script patch. Not intentional, certainly. But what kind of caller is treating UNKNOWN and DENY differently?

(In reply to Bobby Holley (:bholley) from comment #55) > Not intentional, certainly. But what kind of caller is treating UNKNOWN and > DENY differently? http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/DOMStorage.cpp#266 For "cookie" this gets UNKNOWN and then after some dance it allows access to storage, for DENY it returns false immediately.

Hm ok. I guess that makes sense.

Comment on attachment 8385180 [details] [diff] [review] Bug946815_Test-case.patch Hi Bob, I wrote a test case as we discussed before. Please see https://tbpl.mozilla.org/?tree=Try&rev=717cdea08d8d for try server result. The idea is to use SpecialPowers.pushPermission to remove the permission of 'wifi-manager' in the beginning the test. In turn test without/with 'wifi-manage' permission on content/xbl scope respectively. Thanks!

Comment on attachment 8385180 [details] [diff] [review] Bug946815_Test-case.patch Review of attachment 8385180 [details] [diff] [review]: ----------------------------------------------------------------- r=me with those changes. Thanks for following up on this! ::: dom/xbl/test/file_bug946815.xhtml @@ +23,5 @@ > + testWifiPermission(true); > + SimpleTest.finish(); > + } > + > + eval(win.testWifiPermission.toString()); The fact that we don't have a naming collision here with the testWifiPermission in the non-XBL scope is non-obvious - it depends on the fact that the XBL global (which is not a window but whose prototype is the content window) sees the content window with an XrayWrapper, which filters out the testWifiPermission property. To be safe, we should give these different names. You can do this by replacing: function testWifiPermission(..) {..} with: var testWifiPermissionFromContent = function(..) {..} and then having the xbl scope do: eval('var testWifiPermissionFromXBL = ' + win.testWifiPermissionFromContent.toSource()); @@ +59,5 @@ > + bound.dispatchEvent(new CustomEvent('testevent')); > + }); > + } > + > + function testWifiPermission(aIsXblScope) { I think testWifiPermission should have an extra boolean argument, aExpectPermission, which is true or false depending on whether the caller expects the permission to be in effect or not. Then, we can do: is(hasWifiPermission, aExpectPermission, "Permissions set up the way we expect"); ::: dom/xbl/test/test_bug946815.html @@ +9,5 @@ > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > + <script type="application/javascript"> > + > + /** Test for XBL scope behavior. **/ This comment needs updating. @@ +17,5 @@ > + // > + // NB: This two-layer setup used to exist because XBL scopes were behind a > + // pref, and we wanted to make sure that we properly set the pref before > + // loading the real test. This stuff is no longer behind a pref, but we just > + // leave the structure as-is for expediency. This one too.

Thanks for your quick review! I attached a new version with your comments and add more comment to explain my idea of this test. Do I have to ask for review again? (In reply to Bobby Holley (:bholley) from comment #60) > Comment on attachment 8385180 [details] [diff] [review] > Bug946815_Test-case.patch > > Review of attachment 8385180 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with those changes. Thanks for following up on this! > > ::: dom/xbl/test/file_bug946815.xhtml > @@ +23,5 @@ > > + testWifiPermission(true); > > + SimpleTest.finish(); > > + } > > + > > + eval(win.testWifiPermission.toString()); > > The fact that we don't have a naming collision here with the > testWifiPermission in the non-XBL scope is non-obvious - it depends on the > fact that the XBL global (which is not a window but whose prototype is the > content window) sees the content window with an XrayWrapper, which filters > out the testWifiPermission property. > > To be safe, we should give these different names. You can do this by > replacing: > > function testWifiPermission(..) {..} > > with: > > var testWifiPermissionFromContent = function(..) {..} > > and then having the xbl scope do: > > eval('var testWifiPermissionFromXBL = ' + > win.testWifiPermissionFromContent.toSource()); > > @@ +59,5 @@ > > + bound.dispatchEvent(new CustomEvent('testevent')); > > + }); > > + } > > + > > + function testWifiPermission(aIsXblScope) { > > I think testWifiPermission should have an extra boolean argument, > aExpectPermission, which is true or false depending on whether the caller > expects the permission to be in effect or not. Then, we can do: > > is(hasWifiPermission, aExpectPermission, "Permissions set up the way we > expect"); > > ::: dom/xbl/test/test_bug946815.html > @@ +9,5 @@ > > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > > + <script type="application/javascript"> > > + > > + /** Test for XBL scope behavior. **/ > > This comment needs updating. > > @@ +17,5 @@ > > + // > > + // NB: This two-layer setup used to exist because XBL scopes were behind a > > + // pref, and we wanted to make sure that we properly set the pref before > > + // loading the real test. This stuff is no longer behind a pref, but we just > > + // leave the structure as-is for expediency. > > This one too.

(In reply to Henry Chang [:henry] from comment #62) > Thanks for your quick review! > > I attached a new version with your comments and add more comment to explain > my idea of this test. > Do I have to ask for review again? The last patch was r+ed, so if you didn't make major changes outside of the stuff described in the review comments, you don't need to re-request review.

https://tbpl.mozilla.org/?tree=Try&rev=7eaa7c90052c try server result for the patch I request to check in.