570175 - Preferences UI: add tests for editing cookie exception permissions

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch to fix behaviour.

When editing a permission in the Permissions UI, it may be possible to not change a permission under the following conditions:

- the Permissions UI is loaded with an Allow and Deny button
- the permission attempting to be edited was previously modified by the Permissions UI since it was loaded

Steps to reproduce:
Open Preferences -> Content, choose Exceptions next to "Load images automatically".
Enter "test.com" and choose Allow. Enter "test.com" and choose Deny. Lastly, enter "test.com" and choose Allow.

Behaviour:
UI shows "test.com" with status "Block".

Expected:
UI shows "test.com" with status "Allow".

Comment 1

[Shawn Wilsher :sdwilsh]

We should write a test for this too probably.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

This code is strange - why do Permission objects have both a "capability" and "perm" properties, when the former seems to always just be the result of calling this._getCapabilityString for the latter? And talk about confusing names... that's partially nsIPermission[Manager]s fault, I suppose.

Comment 3

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #2)
> This code is strange - why do Permission objects have both a "capability" and
> "perm" properties, when the former seems to always just be the result of
> calling this._getCapabilityString for the latter? And talk about confusing
> names... that's partially nsIPermission[Manager]s fault, I suppose.

"capability" seems to be a short form of "capabilityString" as you discovered and it is used to store the string which will be displayed in the UI. On the other hand, "perm" could be a short form of "permissionCapability" as it stores the capability property from the permission.

(In reply to comment #1)
> We should write a test for this too probably.

Testing for this would be pretty useful. I will try writing a test this Monday.

Comment 4

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Added test case.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 450416 [details] [diff] [review]
Added test case.

Good find, and good job with the test! Some minor nits below:

>diff --git a/browser/components/preferences/permissions.js b/browser/components/preferences/permissions.js

>+            this._permissions[i].perm = permission.capability;
>             this._permissions[i].capability = this._getCapabilityString(permission.capability);

These variable names still make my cry. Unrelated to this bug, but might as well take this opportunity to fix - can you rename "perm" to "permissionType" and "capability" to "permissionString"?

>diff --git a/browser/components/preferences/tests/browser_cookies_exceptions.js b/browser/components/preferences/tests/browser_cookies_exceptions.js

>+ * By doing this we test the Permissions UI.
>+ * Bug 570175: "Preferences UI: editing permission for same host does not always show
>+ * a change"

Don't need to include these comments, they're not particularly useful (the info in the second one can be retrieved from blame).

>+function test() {

>+  function prefObserver(subject, topic, data) {

prefObserver is a confusing name, since we actually have unrelated things called pref observers - prefWindowObserver?

>+  function cookiesWindowObserver(subject, topic, data) {

>+    win.addEventListener("load", function(event) {
>+      setTimeout(function() {

Presumably this is here because your load handler fires before the dialog's - it would be cleaner to just add an observer service notification to the end of the dialog's onLoad handler and use that to trigger the test. See e.g. http://hg.mozilla.org/mozilla-central/annotate/db3e8b1ecb26/toolkit/content/commonDialog.js#l247 and http://hg.mozilla.org/mozilla-central/annotate/db3e8b1ecb26/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadmonitor.js#l61 .

>+        is(statusText, allowText, "permission set correctly");

"permission text set should be set correctly"

>+        is(statusText, denyText, "permission set correctly");

"permission text should be updated after changing permission type"

>+        is(statusText, allowText, "permission set correctly");

"permission text should be updated after resetting permission type"

(keeping test descriptions informative and unique helps when debugging failures)

For bonus points, it'd be nice to extend this test to also actually check that the changes were applied, by checking that the expected values are set via nsIPermissionManager. Could even factor the checks into a checkState() that compares the expected values, nsIPermissionManager-obtained values, and displayed values after each step.

Comment 6

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #5)
> These variable names still make my cry. Unrelated to this bug, but might as
> well take this opportunity to fix - can you rename "perm" to "permissionType"
> and "capability" to "permissionString"?

"permission type" is also used to indicate the type of permission in general (e.g. "cookie", "geo", etc.). How about renaming "perm" to "permissionAction" and "capability" to "permissionString"?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

sure

Comment 8

[Mehdi Mulani [:mmm] (I don't check this)]

Just discovered that this UI doesn't seem to remove items from the permissions list when they are removed by a different method.

Haven't looked into how this is caused yet, will do along with renaming and test changes.

Comment 9

[Mehdi Mulani [:mmm] (I don't check this)]

woot! another bug found.

if you add a permission which type does not match the dialog being displayed, the dialog will add a blank line or a line with content (which is clickable).

Steps to reproduce:
1. open Cookies exception window.
2. add cookie exception for domain A.
3. one of 3a or 3b:
3a. add popup exception for domain B.
at this point domain B will show up in the Cookies exception window.
3b. add popup exception for domain A.
a blank line will show up in the Cookies exception window.

have not looked into this yet but seems reasonable to add a fix for this to the bundle.

Comment 10

[Mehdi Mulani [:mmm] (I don't check this)]

per Shawn's advice, I have split comment 8 and 9 into two bugs: bug 574460 and bug 574463 respectively.

Comment 11

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changes test method.

Did not clean up too much of the permissions.js code but I did change how the test suite works.

I'm planning on using this test suite for the other two bugs, not sure how good of an idea this is.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 454162 [details] [diff] [review]
Changes test method.

Sounds like a fine idea to me. This patch looks incomplete, but the test parts look fine. I'd probably rename "before" to "test" and "after" to "check" or something, but that doesn't matter very much.

Comment 13

[Michael Pruett]

Attachment: Patch

I've updated Mehdi's original patch, removing all changes unrelated to the new tests. I've also added a regression test for bug 1006461.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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

Thanks for updating the patches, Michael! Forwarding these to Blair who should be able to help with quicker reviews.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

Er, what about the original change in permissions.js that actually fixes what this bug is about?

::: browser/components/preferences/tests/browser_cookies_exceptions.js
@@ +162,5 @@
> +      finish();
> +      return;
> +    }
> +
> +    dump("Running test #" + (currentTest + 1) + "\n");

Nit: Use info() to print informational text

Comment 16

[Michael Pruett]

(In reply to Blair McBride [:Unfocused] from comment #15)
> Er, what about the original change in permissions.js that actually fixes
> what this bug is about?

The fix for bug 956151 obviated Mehdi's proposed change in behavior and also addressed some of his proposed naming changes.

I've updated the title of this bug to reflect its narrowed scope of adding tests for editing cookie exception permissions.

Comment 17

[Michael Pruett]

Attachment: Patch

(In reply to Blair McBride [:Unfocused] from comment #15)
> ::: browser/components/preferences/tests/browser_cookies_exceptions.js
> @@ +162,5 @@
> > +      finish();
> > +      return;
> > +    }
> > +
> > +    dump("Running test #" + (currentTest + 1) + "\n");
> 
> Nit: Use info() to print informational text

Thanks. I've updated the patch accordingly.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Aha! bug 956151 wasn't mentioned anywhere in this bug until now. That makes far more sense now :)

Comment 19

[Carsten Book [:Tomcat]]

Hi,

can we get a try link here (see also https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices) :)

Thanks!

Comment 20

[Michael Pruett]

Here are the results from the try server:

https://tbpl.mozilla.org/?tree=Try&rev=3928734eb334

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/b91f44c3b298

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b91f44c3b298

Comment 23

[Liz Henry (:lizzard)]

sdwilsh, is this a test you're planning to write? Thanks!

Comment 24

[Michael Pruett]

Liz, I believe that the test to which Shawn referred was written by Mehdi and has been committed.

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

What comment 24 said.

Comment 26

[Mihaela Velimiroviciu (:mihaelav)]

Setting in-testsuite+ as the test was created and landed.