Last Comment Bug 661819 - Values for All Sites in about:permissions displays last selected site's values
: Values for All Sites in about:permissions displays last selected site's values
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: x86 All
-- major (vote)
: Firefox 7
Assigned To: :Margaret Leibovic
: Jared Wein [:jaws] (please needinfo? me)
Depends on:
Blocks: 662287
  Show dependency treegraph
Reported: 2011-06-03 07:27 PDT by Dave Hunt (:davehunt)
Modified: 2011-06-10 04:46 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.14 KB, patch)
2011-06-03 11:19 PDT, :Margaret Leibovic review-
Details | Diff | Splinter Review
patch v2 (1.48 KB, patch)
2011-06-03 13:17 PDT, :Margaret Leibovic review+
Details | Diff | Splinter Review
tests for hidden allow item (2.92 KB, patch)
2011-06-03 16:05 PDT, :Margaret Leibovic review+
Details | Diff | Splinter Review

Description User image Dave Hunt (:davehunt) 2011-06-03 07:27:03 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110602 Firefox/6.0a2

Steps to reproduce:
1. Open any site to populate some sites data
2. Open about:permissions
3. Click a site from the list
4. Change the values to be different from the All Sites preferences
5. Click 'All Sites'

The values should change back to the defaults for all sites

The values for Share Location and Maintain Offline Storage do not change back

Refreshing the page resolves the issue
Comment 1 User image [:Aleksej] 2011-06-03 07:39:03 PDT
Confirmed with Mozilla/5.0 (X11; Linux x86_64; rv:6.0a2) Gecko/20110602 Firefox/6.0a2
Comment 2 User image :Margaret Leibovic 2011-06-03 11:19:24 PDT
Created attachment 537189 [details] [diff] [review]

We weren't updating the menuitem value after hiding the allow item. We shouldn't do that!

I guess we should probably add a test for this, too.
Comment 3 User image :Gavin Sharp [email:] 2011-06-03 11:59:13 PDT
Comment on attachment 537189 [details] [diff] [review]

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

>   updatePermission: function(aType) {
>     let allowItem = document.getElementById(aType + "-" + PermissionDefaults.ALLOW);
>     if (!this._selectedSite &&
>         this._noGlobalAllow.indexOf(aType) != -1) {
>       allowItem.hidden = true;
>-      return;
>     }
>     allowItem.hidden = false;

This needs to be in an else.
Comment 4 User image :Margaret Leibovic 2011-06-03 13:17:56 PDT
Created attachment 537213 [details] [diff] [review]
patch v2

Oops, that was dumb.
Comment 5 User image :Gavin Sharp [email:] 2011-06-03 13:23:14 PDT
Comment on attachment 537213 [details] [diff] [review]
patch v2

We should have a test for this, yeah.
Comment 6 User image :Margaret Leibovic 2011-06-03 16:05:21 PDT
Created attachment 537260 [details] [diff] [review]
tests for hidden allow item

This tests the code that was modified by my patch.
Comment 7 User image :Gavin Sharp [email:] 2011-06-03 16:16:40 PDT
Comment on attachment 537260 [details] [diff] [review]
tests for hidden allow item

This looks OK, but only covers the hidden part of the test, right? Wouldn't have caught the original bug. Ideally we would also have a test for that. Maybe a more comprehensive test should define exactly which menuitems should be visible and what the menulist values should be for both the default "all sites" item and an site, and then check that those match up when switching back and forth between the two.
Comment 8 User image :Margaret Leibovic 2011-06-06 08:34:33 PDT

I will file a follow-up for for extensive tests, but I didn't want that to hold up landing this fix.
Comment 9 User image Simona B [:simonab ] 2011-06-10 04:46:08 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0a1) Gecko/20110609 Firefox/7.0a1

Verified issue using the STR from Comment 1 on Win XP, Win 7, MacOS X 10.6 and Ubuntu 10.10.

Issue is no longer present, setting status to - VERIFIED FIXED.

Note You need to log in before you can comment on or make changes to this bug.