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
Status: VERIFIED FIXED
[testday-20110603]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: x86 All
: -- major (vote)
: Firefox 7
Assigned To: :Margaret Leibovic
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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'

Expected:
The values should change back to the defaults for all sites

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

Note:
Refreshing the page resolves the issue
Comment 1 [: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 :Margaret Leibovic 2011-06-03 11:19:24 PDT
Created attachment 537189 [details] [diff] [review]
patch

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 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-03 11:59:13 PDT
Comment on attachment 537189 [details] [diff] [review]
patch

>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 :Margaret Leibovic 2011-06-03 13:17:56 PDT
Created attachment 537213 [details] [diff] [review]
patch v2

Oops, that was dumb.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 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 :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 :Gavin Sharp [email: gavin@gavinsharp.com] 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 :Margaret Leibovic 2011-06-06 08:34:33 PDT
http://hg.mozilla.org/mozilla-central/rev/e235aa1ea834
http://hg.mozilla.org/mozilla-central/rev/7c736690d72d

I will file a follow-up for for extensive tests, but I didn't want that to hold up landing this fix.
Comment 9 Simona B [:simonab ] -PTO- back Sept 5th 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.