Last Comment Bug 573176 - Implement Site-Specific Privacy Preferences
: Implement Site-Specific Privacy Preferences
Status: VERIFIED FIXED
[new feature] [hidden behind about:pe...
: user-doc-needed
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- enhancement with 6 votes (vote)
: Firefox 6
Assigned To: :Margaret Leibovic
:
Mentors:
: 558744 661747 (view as bug list)
Depends on: 552023 587208 588689 653010 654573 656145 657961 658097 658271 658423 658425 658530 658555 658556 658615 658618 658620 658624 658628 659087 659319 659642 659719 660187 660225 660229 661662 661806 663777 665527 676846 678735 697941 821049 955369 956104 1197482
Blocks: IndexedDB 672038 697942
  Show dependency treegraph
 
Reported: 2010-06-18 16:46 PDT by Jennifer Morrow [:Boriss] (UX)
Modified: 2015-11-15 05:37 PST (History)
44 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mockups: separate windows in current design, interated interface (1.06 MB, image/png)
2010-06-18 16:46 PDT, Jennifer Morrow [:Boriss] (UX)
no flags Details
Main guts of the SP add-on so far. (19.49 KB, application/x-javascript)
2010-06-23 13:44 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details
Initial patch that brings Site Preferences into the Preferences UI. (40.67 KB, patch)
2010-07-29 16:58 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
Starts to theme up SP as Boriss has described and drawn. (44.54 KB, patch)
2010-08-04 11:12 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
Woot, did more theming and moved code for creating UI at run-time into actual XUL. (45.47 KB, patch)
2010-08-04 20:21 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
First step in localizations. (55.65 KB, patch)
2010-08-11 19:25 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
Localized and added placeholder icons. (272.96 KB, patch)
2010-08-13 19:17 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
Removed debugging output from SP and fixed test case. (272.31 KB, patch)
2010-08-14 13:48 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
Fixed tests to account for async database queries. (274.65 KB, patch)
2010-08-16 13:09 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
Mainly UI changes. (76.61 KB, patch)
2010-08-20 15:55 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
Last patch before leaving. (286.01 KB, patch)
2010-08-27 16:25 PDT, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Splinter Review
WIP patch (30.79 KB, patch)
2011-04-26 17:03 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
WIP patch v2 (51.06 KB, patch)
2011-04-28 15:47 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
WIP v3 (63.57 KB, patch)
2011-05-06 16:24 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
WIP v4 (67.16 KB, patch)
2011-05-11 17:02 PDT, :Margaret Leibovic
sdwilsh: feedback+
Details | Diff | Splinter Review
patch (61.17 KB, patch)
2011-05-16 14:01 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch (updated) (61.60 KB, patch)
2011-05-17 16:57 PDT, :Margaret Leibovic
sdwilsh: review+
Details | Diff | Splinter Review
tests (11.90 KB, patch)
2011-05-17 16:58 PDT, :Margaret Leibovic
sdwilsh: review+
Details | Diff | Splinter Review
patch v2 (67.26 KB, patch)
2011-05-18 15:28 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
tests (updated) (11.84 KB, patch)
2011-05-18 15:29 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v3 (67.13 KB, patch)
2011-05-19 13:12 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
tests (updated again) (11.54 KB, patch)
2011-05-19 13:14 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v4 (64.97 KB, patch)
2011-05-19 14:26 PDT, :Margaret Leibovic
gavin.sharp: review+
Details | Diff | Splinter Review
tests v4 (11.51 KB, patch)
2011-05-19 14:28 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch for check-in (62.60 KB, patch)
2011-05-19 15:38 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
screenshot (849.29 KB, image/png)
2011-05-19 15:54 PDT, :Margaret Leibovic
no flags Details

Description Jennifer Morrow [:Boriss] (UX) 2010-06-18 16:46:27 PDT
Created attachment 452384 [details]
Mockups: separate windows in current design, interated interface

For Firefox 4, we are integrating the separate managers for individual privacy permissions into one site-centric interface within preferences.  Rather than having a separate window in preferences for each permission category, a single interface will display permissions per domain and allow the user to change preferences for a single or all domains.

Some windows that could be integrated into this single interface are:

- Pop-ups 
- Images 
- Add-on Installation 
- Cookies 
- Saved Passwords/Account Management
- Offline Data
- Geolocation

The problem with the current UI is that the one-window-per-preference design is framed around the implementation model rather than the user’s mental model; it’s designed in a way that corresponds with how it was built rather than how users perceive the action they want to take.  A better design would present controls in a site-centric rather than technology-centric view. If a user decides that he doesn’t trust site X and doesn’t want it to have any access, it would be more efficient to control all of site X’s access in a single preference window than hunt for the permissions individually.
Comment 1 Robert Kaiser 2010-06-23 08:53:40 PDT
As a note, SeaMonkey bug 569341 is connected to this, being about a manager for all the explicit data items stored in those buckets, and the in-development version of that is available as an add-on for both SeaMonkey and Firefox (trunk nightlies and recent alphas): https://addons.mozilla.org/addon/162068
Comment 2 Mehdi Mulani [:mmm] (I don't check this) 2010-06-23 13:44:05 PDT
Created attachment 453489 [details]
Main guts of the SP add-on so far.

The attached code is a part from the extension on AMO: https://addons.mozilla.org/addon/181744/

The AMO extension is a snapshot from my work on implementing SP.
Comment 3 Mehdi Mulani [:mmm] (I don't check this) 2010-07-29 16:58:01 PDT
Created attachment 461405 [details] [diff] [review]
Initial patch that brings Site Preferences into the Preferences UI.

Reflecting the future place of SP, we decided to move from AMO to a patch to Firefox for the Preferences UI.

Without consideration for the Preferences coding style, or UI style, this patch adds SP to Preferences with a fake icon and doesn't _seem_ to break anything.
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-08-03 16:12:32 PDT
Hey guys, just want to make sure that this makes it in to Firefox 4 as IndexedDB needs this UI to remove its database files.
Comment 5 Mehdi Mulani [:mmm] (I don't check this) 2010-08-04 11:12:54 PDT
Created attachment 462839 [details] [diff] [review]
Starts to theme up SP as Boriss has described and drawn.

This patch also adds live-updating to SP (except for cache, for which a bug should be filed soon by byronm).

Putting this patch up to get some initial UI feedback from Dao. Some shortcomings UI wise with this:
The first category (Cache) has been themed how we want but the others are still a work in progress. None of this has been localized or specified on a platform basis, this should be coming later.
Comment 6 Mehdi Mulani [:mmm] (I don't check this) 2010-08-04 20:21:55 PDT
Created attachment 463047 [details] [diff] [review]
Woot, did more theming and moved code for creating UI at run-time into actual XUL.

Previously some UI was created at run-time but it is now easier to be done in XUL for theming purposes.
Comment 7 Mehdi Mulani [:mmm] (I don't check this) 2010-08-11 19:25:54 PDT
Created attachment 465086 [details] [diff] [review]
First step in localizations.

Added localizations but not pluralizations. (All UI strings that should be localized are prefixed with an "!" to verify that they are from properties/entities, once pluralizations are added the prefix will be removed.)

In this patch, password exceptions are also present and the "Mixed vibes" options has been removed.
Comment 8 Mehdi Mulani [:mmm] (I don't check this) 2010-08-13 19:17:09 PDT
Created attachment 465925 [details] [diff] [review]
Localized and added placeholder icons.

Monster patch, not many changes from the previous except for the addition of graphics!
Comment 9 Mehdi Mulani [:mmm] (I don't check this) 2010-08-14 10:50:14 PDT
Comment on attachment 465925 [details] [diff] [review]
Localized and added placeholder icons.

Just realized I had not finished the test case for this (and it will fail). Hopefully will submit another patch later today.
Comment 10 Mehdi Mulani [:mmm] (I don't check this) 2010-08-14 13:48:53 PDT
Created attachment 466039 [details] [diff] [review]
Removed debugging output from SP and fixed test case.
Comment 11 Mehdi Mulani [:mmm] (I don't check this) 2010-08-14 22:18:40 PDT
Comment on attachment 466039 [details] [diff] [review]
Removed debugging output from SP and fixed test case.

Having a problem with the test on tryserver and sometimes failing locally when run in the suite.
Comment 12 Mehdi Mulani [:mmm] (I don't check this) 2010-08-16 13:09:48 PDT
Created attachment 466406 [details] [diff] [review]
Fixed tests to account for async database queries.

Found some problems with the test over the weekend with the help of TryServer. Current TryServer push appears to be green so I'm putting this up for review.
Comment 13 Justin Dolske [:Dolske] 2010-08-16 21:57:41 PDT
Comment on attachment 466406 [details] [diff] [review]
Fixed tests to account for async database queries.

Let sdwilsh have the first crack at this, say I!
Comment 14 Mehdi Mulani [:mmm] (I don't check this) 2010-08-17 15:40:20 PDT
Comment on attachment 466406 [details] [diff] [review]
Fixed tests to account for async database queries.

From sdwilsh, pushing this to Mano.
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-08-19 17:23:30 PDT
* Please do not diff binary files.
* General code-style comment: please add an empty line between js methods.
Comment 16 Mehdi Mulani [:mmm] (I don't check this) 2010-08-20 15:55:20 PDT
Created attachment 467936 [details] [diff] [review]
Mainly UI changes.
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-08-25 07:56:18 PDT
First part of review:

diff --git a/browser/components/preferences/applications.js b/browser/components/preferences/applications.js
--- a/browser/components/preferences/applications.js
+++ b/browser/components/preferences/applications.js

+      item.setAttribute("class", "appEntry");

Use the className property.

diff --git a/browser/components/preferences/handlers.css b/browser/components/preferences/handlers.css
--- a/browser/components/preferences/handlers.css
+++ b/browser/components/preferences/handlers.css

-richlistitem {
+richlistitem.appEntry {
   -moz-binding: url("chrome://browser/content/preferences/handlers.xml#handler");
 }
 
-richlistitem[selected="true"] {
+richlistitem.appEntry[selected="true"] {
   -moz-binding: url("chrome://browser/content/preferences/handlers.xml#handler-selected");
 }

Please double check that the style rules in browser/themes for those richlistitems don't impact you too.

--- /dev/null
+++ b/browser/components/preferences/siteprefs.css
@@ -0,0 +1,48 @@

1. Anything but -moz-binding sets should go to browser/htemes.
2. Add a license block here;

+.site {
+  -moz-binding: url("chrome://browser/content/preferences/siteprefs.xml#site");
+  padding: 2px;
+  border-bottom-width: 1px;
+  border-bottom-style: solid;
+  border-bottom-color: threedlightshadow;

You could use border-bottom here.

+  font-size: 12px;

You shouldn't hardcode font-size in pixels

+}
+.permissionPref {
+  -moz-binding: url("chrome://browser/content/preferences/siteprefs.xml#permissionPref");
+}
+.siteListImage {
+  width: 16px;
+  height: 16px;

+  margin: 2px 6px 0px -2px;

This isn't rtl friendly. Please see https://developer.mozilla.org/en/Making_Sure_Your_Theme_Works_with_RTL_Locales

+}
+.siteListTitle {
+  margin: 1px 0px 0px 0px;

Isn't setting margin-to enough?

+/*  max-width: 15em; */

Please remove that.

+/** {
+  -moz-transition-property: height, visibility, clip;
+  -moz-transition-delay: 1s;
+  -moz-transition-timing-function: linear;
+  -moz-transition-duration: 2s;
+  -moz-transform-origin: top;
+}*/

hrm?

diff --git a/browser/components/preferences/siteprefs.js b/browser/components/preferences/siteprefs.js
new file mode 100644
--- /dev/null
+++ b/browser/components/preferences/siteprefs.js

1. Name your functions
2. Use aFoo arguments format.

+let tld = Cc["@mozilla.org/network/effective-tld-service;1"]
+            .getService(Ci.nsIEffectiveTLDService);
+let ioService = Cc["@mozilla.org/network/io-service;1"]
+                .getService(Ci.nsIIOService);
+let pm = Cc["@mozilla.org/permissionmanager;1"]
+           .getService(Ci.nsIPermissionManager);
+let os = Cc["@mozilla.org/observer-service;1"]
+           .getService(Ci.nsIObserverService);
+let ac = Cc["@mozilla.org/autocomplete/search;1?name=history"]
+           .getService(Ci.nsIAutoCompleteSearch);
+let db = Cc["@mozilla.org/browser/nav-history-service;1"]
+           .getService(Ci.nsPIPlacesDatabase).DBConnection;
+let hs = Cc["@mozilla.org/browser/nav-history-service;1"]
+           .getService(Ci.nsINavHistoryService);
+let ps = Cc["@mozilla.org/preferences-service;1"].
+            getService(Ci.nsIPrefService);
+let cache = Cc["@mozilla.org/network/cache-service;1"].
+              getService(Ci.nsICacheService);
+let cookieMgr = Cc["@mozilla.org/cookiemanager;1"].
+                getService(Ci.nsICookieManager);
+let loginMgr = Cc["@mozilla.org/login-manager;1"].
+                getService(Ci.nsILoginManager);

Most of these could be replaced by Services.jsm

+let gSitePrefsPane = {
+  init: function() {
+    // Observers for all data
+    os.addObserver(this, "perm-changed", false);
+    os.addObserver(this, "passwordmgr-storage-changed", false);
+    os.addObserver(this, "cookie-changed", false);

+    this._prefBranch = ps.getBranch("");
+    this._prefBranch.QueryInterface(Ci.nsIPrefBranch2);
+    this._prefBranch.addObserver("", this, false);

Aren't you observing all preferences here?

+    else if (searchText.length == 0)
+      this._previousSitesLength = this.sites.length;
+    else
+      this._previousSitesLength = 0;
+
+    let sitesList = document.getElementById("sp.sitesList");
+    let allSites = document.createElement("richlistitem");
+    allSites.setAttribute("class", "site");

Use the className property.

+    allSites.setAttribute("title", this._string("allsiteslabel"));
+
+    let selectedItems = sitesList.selectedItems.
+                        map(function (x) parseInt(x.siteId)).concat(this._toSelect);
+    this._toSelect = selectedItems;
+    let toScrollItem;
+
+    //sitesList.clearSelection();

?
+  createSiteListItem: function(site) {
+    var item = document.createElement("richlistitem");

nit: let

+    item.setAttribute("class", "site");

className

+    item.setAttribute("name", site.name);
+    item.siteId = site._id;

See my question on the type of this property (string/int).

+  observe: function(aSubject, aTopic, aData) {
+    let updateCurrentSites = false;
+    let updateDefaults = false;
+    let updateSitesList = false;
+    var site;

why var?


+    }
+    if (updateCurrentSites) {
+      gSitePrefsPane.updateCurrentSites();
+    }
+    if (updateDefaults) {
+      gSitePrefsPane.updateDefaults();
+    }
+    if (updateSitesList) {
+      gSitePrefsPane.updateSitesList();
+    }

1. Remove braces around single line blocks.
2. s/gSitePrefsPane/this/

+  },
+
+  sites: [],
+  _sitesHostToId: {},
+  preferences: {},
+
+  _permissionToPreference: {
+    cookie: "cookies", install: "addons", image: "image", popup: "popups",
+  },
+
+  get currentSites() {
+    let items = document.getElementById("sp.sitesList").selectedItems;
+    let toRet = [];
+    for each (let i in items) {
+      if (typeof(i.siteId) == "undefined")

How would that happen?

+        return [null];

And if it could, shouldn't it throw?

+      toRet.push(this.getSiteFromId(i.siteId));
+    }
+    if (toRet.length == 0)
+      toRet.push(null);

Why not return []?

+  showAllSites: function() {
+    document.getElementById("sp.cookieStatus").selectedIndex =
…
You should either add a _getElt helper (that also prefixes "sp") or cache all of these elements, or both.

+  removeData: function(site) {
+    // This feature is also called "Forget About This Site".
+    let pb = Cc["@mozilla.org/privatebrowsing;1"].
+               getService(Ci.nsIPrivateBrowsingService);

nit: Here and in few other places, please fix the indent.

+    let domain = site.domain;
+    pb.removeDataFromDomain(domain);
+    dump("data removed");

Please make sure to remove all debug code.

+  toggleExceptionsList: function(event) {
+    let type = event.target.parentNode.getAttribute("type");
+    let list = document.getElementById("sp." + type + "ExceptionsList");
+    list.style.visibility = list.style.visibility == "collapse" ? "visible"
+                                                                : "collapse";

Why don't you use hidden?

+function website(domain, parentSite, id) {

1. Please find a better name for this Object.
2. I saw that you call parseInt for id/siteId. Why is it a string?

+
+gSitePrefsPane._statement =
+"/* do not warn (bug ...order by frecency is intentional) */" +

Please check with mak a possible API here.  You shouldn't access the db directly from this level.

--- /dev/null
+++ b/browser/components/preferences/siteprefs.xml

+<binding id="site" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
+  <content>
+    <xul:hbox style="padding-left: 5px" maxheight="35" minheight="35">

All of these attributes don't belong here.

+      <xul:vbox pack="start">
+        <xul:image xbl:inherits="src=icon" class="siteListImage"/>
+      </xul:vbox>
+      <xul:vbox pack="center">
+        <xul:description xbl:inherits="value=title" crop="end" class="siteListTitle"/>
+        <xul:description xbl:inherits="value=name,selected" class="siteListUrl"/>

Inheriting the selected attribute seems wrong.

--- /dev/null
+++ b/browser/components/preferences/siteprefs.xul

+  <prefpane id="paneSitePrefs"
+            onpaneload="gSitePrefsPane.init()"
+            helpTopic="prefs-privacy"

+            maxheight="600">

Why is maxheight needed? It sure doesn't belong here.

+    <stringbundle id="bundleSitePrefs"

nit: sitePrefsBundle

+<hbox flex="1" maxheight="400">
+<vbox flex="1" id="sp.prefsBox" style="overflow: auto;">
+<vbox minwidth="250" maxwidth="250">

ditto.

+        <richlistbox id="sp.sitesList"
+                     flex="1"
+                     class="list"

This class is used by the addons manager.  It will do nothing here.

+ <image class="prefItemImage"
+        src="chrome://browser/skin/preferences/sp_cache.png"/>
+ <image class="prefItemImage"
+        src="chrome://browser/skin/preferences/sp_cookies.png"/>
+ etc.

Theme file?

+                <menuitem label="Mixed vibes" hidden="true"/>

What's that?


+              <button label="&showPasswords.label;" oncommand=""/>
+              <button label="&clearSelectedPasswords.label;" oncommand=""/>
hrm?

+          <vbox id="sp.popupContent" type="popup">

Please don't use the generic type attribute here.  I also don't like that you had to go up to the vbox to check what an elements stands for.  Instead, you could make a btter use of the id prefixes ( sp.popup.(..) ).

Also, please remove the unnecessary comments from this file.

diff --git a/browser/locales/en-US/chrome/browser/preferences/sitePrefs.dtd b/browser/locales/en-US/chrome/browser/preferences/sitePrefs.dtd
new file mode 100644
--- /dev/null
+++ b/browser/locales/en-US/chrome/browser/preferences/sitePrefs.dtd

Any accesskeys?

--- a/browser/themes/*/browser/preferences/preferences.css
+++ b/browser/themes/*/browser/preferences/preferences.css
..
+radio[pane=paneSitePrefs] {
+	list-style-image: url("chrome://browser/skin/preferences/Options-siteprefs.png");
+}

Don't use hard tabs.

That's it for now.  I'm keeping the rest for the next revision.
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-08-25 07:57:12 PDT
Comment on attachment 467936 [details] [diff] [review]
Mainly UI changes.

Not ready for prime time.  Canceling review request for now.
Comment 19 Shawn Wilsher :sdwilsh 2010-08-25 08:30:05 PDT
(In reply to comment #17)
> 1. Remove braces around single line blocks.
Per coding style docs, we are supposed to do this now (https://developer.mozilla.org/En/Developer_Guide/Coding_Style).
Comment 20 Mehdi Mulani [:mmm] (I don't check this) 2010-08-27 16:25:31 PDT
Created attachment 470075 [details] [diff] [review]
Last patch before leaving.

As this was in internship, I must leave at some point.
This is a patch for SP with some debugging info and some improvements, such as, async history loading, speed gain on getCookies (about 25%).

To-do:
- bias site representatives towards favicon & title
- make search consider sites without history
- handle URLs such at http://to./some_text and IPs?
- add text zoom and other Content Preferences (see Myk/his extension) and IndexedDB
- when clicking another site, snap right pane to the top
- similarly, when modifying sites, don't close exception panels that were open

More tests:
- make test for history visits changing
- add test for sites maintaining selection as a search is typed

I am more than happy to clarify on any of these points!
Comment 21 Mehdi Mulani [:mmm] (I don't check this) 2010-08-27 16:26:28 PDT
Whoa, almost forgot:
- look at implementation of Forget About This Site and ensure that all data from there is represented/handled in SP
Comment 22 Jennifer Morrow [:Boriss] (UX) 2010-09-21 11:23:38 PDT
I'm marking this feature for future, where future is Firefox 4.1
Comment 23 Jo Hermans 2010-10-31 04:06:27 PDT
Note that this GUI doesn't yet give the opportunity to reset the zoom-level. Are there other items from the current 'Site Preferences' that are missing ?
Comment 24 Jo Hermans 2010-10-31 04:06:47 PDT
*** Bug 558744 has been marked as a duplicate of this bug. ***
Comment 25 Mehdi Mulani [:mmm] (I don't check this) 2010-10-31 06:41:46 PDT
(In reply to comment #23)
> Note that this GUI doesn't yet give the opportunity to reset the zoom-level.
> Are there other items from the current 'Site Preferences' that are missing ?

none of the Content Preferences, a quick interface found at
https://addons.mozilla.org/en-US/firefox/addon/4066/
are modifiable through this.
Comment 26 :Margaret Leibovic 2011-04-26 17:03:40 PDT
Created attachment 528488 [details] [diff] [review]
WIP patch

I'm taking over this bug. Work on this feature is now being tracked on this wiki page: https://wiki.mozilla.org/Privacy/Features/Site-based_data_management_UI.

This is a rough WIP patch. It has basic functionality working for a first pass at this feature, but it still needs theme/localization work. It also needs UX work, which I am going to work on with Boriss.
Comment 27 :Margaret Leibovic 2011-04-28 15:47:19 PDT
Created attachment 528975 [details] [diff] [review]
WIP patch v2

I added localization/theme support and observers for data changes. I also added a "Forget About This Site" button. There's still more work to do, but it's coming along. I figured I'd upload my updated patch in case anyone has comments!
Comment 28 :Margaret Leibovic 2011-05-06 16:24:41 PDT
Created attachment 530767 [details] [diff] [review]
WIP v3

I think this patch is good enough to start asking for some formal feedback. Shawn, I'm asking you because I think you looked at some of this stuff back when Mehdi was working on it over the summer. My test file is very much a work in progress, and I'll need to follow up with Boriss for some UX feedback, but the functionality seems to be basically all there.

My main remaining concern is finding a way to relate multiple URIs to a single host. The APIs to set permissions take URIs, and I haven't looked into the way those permissions are stored/applied, but if user sets a permission for a host, we'd want that to apply to all URIs for that host. Right now, if I only have a host string, I'm just prepending "http://" to it to create a URI, but I'm not sure if that will result in the permissions being set the way the user would expect.
Comment 29 :Margaret Leibovic 2011-05-10 19:50:56 PDT
I pushed a more recent version of my patch to the new UX project branch. If you want to try it out, builds will be available in the ux-[platform] directories here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/.
Comment 30 Leman Bennett [Omega] 2011-05-11 04:05:54 PDT
Unless I missed something, the Windows UX build doesn't have this enabled. Also, hard locks happen when navigating to some sites. It requires killing from task manager.
Comment 31 :Margaret Leibovic 2011-05-11 10:28:45 PDT
(In reply to comment #30)
> Unless I missed something, the Windows UX build doesn't have this enabled.

I guess I failed to mention that the new design implements this preference interface as an in-content about: page that you can open by navigating to "about:permissions" from the location bar.

> Also, hard locks happen when navigating to some sites. It requires killing
> from task manager.

This sounds bad, but unrelated to my patch here. My patch shouldn't be doing anything unless the about:permissions page is open.
Comment 32 Leman Bennett [Omega] 2011-05-11 13:56:52 PDT
Ahh, Thank you!

The new interface works well. The "Forget about this site" button causes a hard lock. It might be related to the navigation bug.
Comment 33 :Margaret Leibovic 2011-05-11 17:02:15 PDT
Created attachment 531808 [details] [diff] [review]
WIP v4

I think this just about fixes all known issues, disabling some parts that have problems without clear solutions.

Dolske, Shawn is pretty busy lately, so you're more than welcome to take a look at it if you get there first! You can both ignore the test file, because I think I broke it with some of my recent changes :)
Comment 34 :Margaret Leibovic 2011-05-11 17:28:50 PDT
(In reply to comment #33)
> Created attachment 531808 [details] [diff] [review] [review]
> WIP v4

I also just pushed this updated patch to the UX branch, so you can try new builds when they are ready (see comment 29).
Comment 35 Leman Bennett [Omega] 2011-05-12 14:19:06 PDT
Another issue I've noticed, using a url without the "www" prefix that resolves to a domain with the "www" prefix will cause two entries.

For example, cnn.com will resolve to www.cnn.com. However the cookie entries will be marked under cnn.com instead of www.cnn.com (which also has the favicon). 

Any site that does this will have double entries. The first entry will always have the related cookies while the resolved entry will have none.
Comment 36 Shawn Wilsher :sdwilsh 2011-05-13 11:01:02 PDT
Comment on attachment 531808 [details] [diff] [review]
WIP v4

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

I'm wondering if this should maybe go in toolkit instead of browser, but I don't have a strong opinion there.  You might want to poke Mossop and see what he thinks.

::: browser/base/content/browser.js
@@ +4117,5 @@
>    overLink: "",
>    startTime: 0,
>    statusText: "",
>    isBusy: false,
> +  inContentWhitelist: ["about:addons", "about:permissions"],

We sure need that new protocol Mossop has ideas for sooner rather than later :/

::: browser/components/preferences/aboutPermissions.js
@@ +29,5 @@
> +let Ci = Components.interfaces;
> +let Cc = Components.classes;
> +
> +let indexedDBService = Cc["@mozilla.org/dom/indexeddb/manager;1"].
> +                       getService(Ci.nsIIndexedDatabaseManager);

Does this actually need to be global?  If yes, please prefix it with g.

@@ +36,5 @@
> +                    getService(Ci.nsIStringBundleService);
> +let aboutPermissionsBundle =
> +  bundleService.createBundle("chrome://browser/locale/preferences/aboutPermissions.properties");
> +
> +let TEST_EXACT_PERM_TYPES = ["geo"];

Please add a comment stating what this is for.

@@ +46,5 @@
> +  this.host = host;
> +  this.listitem = null;
> +
> +  this.httpURI = Services.io.newURI("http://" + this.host, null, null);
> +  this.httpsURI = Services.io.newURI("https://" + this.host, null, null);

I would prefer to see you use NetUtil.newURI here (which has the added benefit of you not needing to pass null for the last two arguments).  This comment applies to the whole file.

@@ +63,5 @@
> +        this._favicon = fs.getFaviconForPage(this.httpURI).spec;
> +      } catch (e) {
> +        // getFaviconImageForPage returns the default favicon if no stored favicon is found.
> +        this._favicon = fs.getFaviconImageForPage(this.httpsURI).spec;
> +      }

Note that bug 655270 is likely to add an async API for favicons that we'll want to use here.  Alternatively, just constructing our own moz-anno://favicon: url might be sufficient without that new API.  That should return the default favicon if we don't have one stored (I don't recall the exact syntax there though).  The fact that this is synchronous now is sadfaces.

@@ +76,5 @@
> +   *        e.g. "cookie", "geo", "indexedDB", "install", "popup", "image"
> +   * @param aResultObj
> +   *        An object that stores the permission value set for aType.
> +   *
> +   * @returns A boolean indicating whether or not a permission is set.

global-nit: it's @return not @returns

@@ +83,5 @@
> +    let permissionValue;
> +    if (TEST_EXACT_PERM_TYPES.indexOf(aType) == -1)
> +      permissionValue = Services.perms.testPermission(this.httpURI, aType);
> +    else
> +      permissionValue = Services.perms.testExactPermission(this.httpURI, aType);

global-nit: brace all ifs

@@ +346,5 @@
> +  /**
> +   * Creates Site objects for the top-frecency sites in the places database and stores
> +   * them in _sites. The number of sites created is controlled by PLACES_SITES_LIMIT.
> +   */
> +  getSitesFromPlaces: function(aCallback) {

documenting what arguments the callback gets would be nice

@@ +355,5 @@
> +    let query = "SELECT get_unreversed_host(rev_host) AS host " +
> +                "FROM moz_places WHERE rev_host <> '.' " +
> +                "AND visit_count > 0 GROUP BY rev_host " +
> +                "ORDER BY MAX(frecency) DESC LIMIT :limit";
> +    let stmt = db.createAsyncStatement(query);

<3

@@ +371,5 @@
> +        }
> +      },
> +      handleError: function(aError) {
> +        // If there's an error, there's not really much we can do about it, and
> +        // it won't kill the interface.

Probably worth logging it to the error console though.

@@ +375,5 @@
> +        // it won't kill the interface.
> +      },
> +      handleCompletion: function(aReason) {
> +        // Notify oberservers for testing purposes.
> +        Services.obs.notifyObservers(null, "browser-permissions-statement-completed", null);

I feel like dispatching an event might be better, FWIW.

::: browser/components/preferences/aboutPermissions.xul
@@ +114,5 @@
> +                      type="password"
> +                      oncommand="AboutPermissions.onPermissionCommand(event);">
> +              <menupopup>
> +                <menuitem id="password#1" value="1" label="&permission.allow;"/>
> +                <menuitem id="password#2" value="2" label="&permission.block;"/>

pretty sure # is not a valid character in an id
Comment 37 :Margaret Leibovic 2011-05-16 09:49:36 PDT
(In reply to comment #36)
> I'm wondering if this should maybe go in toolkit instead of browser, but I
> don't have a strong opinion there.  You might want to poke Mossop and see
> what he thinks.

I think we can leave it in browser for now. I talked to Mossop and he has no strong opinion.

> ::: browser/base/content/browser.js
> @@ +4117,5 @@
> >    overLink: "",
> >    startTime: 0,
> >    statusText: "",
> >    isBusy: false,
> > +  inContentWhitelist: ["about:addons", "about:permissions"],
> 
> We sure need that new protocol Mossop has ideas for sooner rather than later
> :/

I also talked to Mossop about this, and we don't think that this should block this bug. It would also be nice to have common in-content style rules, as opposed to the way I just copied styles from extensions.css, but I think that also falls into the "good follow-up" category.


> @@ +46,5 @@
> > +  this.host = host;
> > +  this.listitem = null;
> > +
> > +  this.httpURI = Services.io.newURI("http://" + this.host, null, null);
> > +  this.httpsURI = Services.io.newURI("https://" + this.host, null, null);
> 
> I would prefer to see you use NetUtil.newURI here (which has the added
> benefit of you not needing to pass null for the last two arguments).  This
> comment applies to the whole file.

I'm not sure it's worth importing NetUtil just for the newURI method. It looks like it does the same thing as Services.io.newURI the way I'm using it.

> @@ +63,5 @@
> > +        this._favicon = fs.getFaviconForPage(this.httpURI).spec;
> > +      } catch (e) {
> > +        // getFaviconImageForPage returns the default favicon if no stored favicon is found.
> > +        this._favicon = fs.getFaviconImageForPage(this.httpsURI).spec;
> > +      }
> 
> Note that bug 655270 is likely to add an async API for favicons that we'll
> want to use here.  Alternatively, just constructing our own
> moz-anno://favicon: url might be sufficient without that new API.  That
> should return the default favicon if we don't have one stored (I don't
> recall the exact syntax there though).  The fact that this is synchronous
> now is sadfaces.

I tried using "moz-anno:favicon:" + URL (the syntax I found in MXR), but that only gave me default favicons. It looks like moz-anno:favicon: requires a favicon image URL, not a generic URL for a host, so I don't think this will work. Is synchronous I/O to get these favicons a deal-breaker? Or can we file a follow-up to make these calls async once we get the API for it?
Comment 38 Shawn Wilsher :sdwilsh 2011-05-16 09:57:43 PDT
(In reply to comment #37)
> I also talked to Mossop about this, and we don't think that this should block
> this bug. It would also be nice to have common in-content style rules, as
> opposed to the way I just copied styles from extensions.css, but I think that
> also falls into the "good follow-up" category.
Yeah, I wasn't suggesting we block on that.  Sorry if I came across that way!

> I'm not sure it's worth importing NetUtil just for the newURI method. It looks
> like it does the same thing as Services.io.newURI the way I'm using it.
The cost of importing NetUtil is going to be effectively nil for this since it's already imported in lots of places.  This isn't primary UI, so importing it will also not impact startup in any way.

> I tried using "moz-anno:favicon:" + URL (the syntax I found in MXR), but that
> only gave me default favicons. It looks like moz-anno:favicon: requires a
> favicon image URL, not a generic URL for a host, so I don't think this will
> work. Is synchronous I/O to get these favicons a deal-breaker? Or can we file a
> follow-up to make these calls async once we get the API for it?
Oh right, I forgot that expects a specific url.  Bug 655270 has a patch up with an API, so it's likely to land soonish.  We've been pretty strict about not adding things that add new synchronous disk I/O when it can be avoided, and I think that makes sense here too.
Comment 39 :Margaret Leibovic 2011-05-16 14:01:19 PDT
Created attachment 532731 [details] [diff] [review]
patch

I broke the test file out into a separate patch that I'm working on now. Unless I find problems while writing tests, this main patch should be done, so I figured it would be good to put it up for review sooner rather than later.

I left the favicon code as-is for now, but hopefully bug 655270 can land before this and its tests are finished being reviewed. If not, we could get rid of favicons for now and add them back later.

I got rid of the code that controls things we decided to disable, since it was unused. I have it in my user repo and we can add it back if we add those features in follow-up bugs. I also think any visual polish can be pushed off to follow-up bugs, as long as the UI isn't totally busted.
Comment 40 :Margaret Leibovic 2011-05-17 16:57:21 PDT
Created attachment 533120 [details] [diff] [review]
patch (updated)

I updated the observer code in this patch to correctly handle more cases, especially when "All Sites" is selected.
Comment 41 :Margaret Leibovic 2011-05-17 16:58:20 PDT
Created attachment 533123 [details] [diff] [review]
tests
Comment 42 Shawn Wilsher :sdwilsh 2011-05-18 11:30:38 PDT
Comment on attachment 533120 [details] [diff] [review]
patch (updated)

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

r=sdwilsh

::: browser/components/preferences/aboutPermissions.js
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1

EEP.  This license block is not correct.  Please copy https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

@@ +31,5 @@
> +let Cc = Components.classes;
> +
> +let gPlacesDatabase = Cc["@mozilla.org/browser/nav-history-service;1"].
> +                      getService(Ci.nsINavHistoryService).
> +                      QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;

Two things:
1) just do getService(Ci.nsPIPlacesDatabase) and drop the QI
2) also clone this as a read-only datbase connection so the DB intensive bits here do not kill other database activity work.

@@ +56,5 @@
> +Site.prototype = {
> +
> +  get favicon() {
> +    if (!this._favicon) {
> +      // TODO: Make this async when bug 655270 is fixed.

I know we just talked about this, but I'm fine with doing this in a follow-up, just make sure it happens please :)

(I'm saying this because I realized that making this all work async is a bit harder to do and requires a bit of refactoring which I'm fine with punting on for now)

@@ +58,5 @@
> +  get favicon() {
> +    if (!this._favicon) {
> +      // TODO: Make this async when bug 655270 is fixed.
> +      let fs = Cc["@mozilla.org/browser/favicon-service;1"].
> +               getService(Ci.nsIFaviconService);

We should cache this so we don't have to hit the component manager every time.

@@ +73,5 @@
> +
> +  /**
> +   * @param aCallback
> +   *        A function that takes the visit count (a number) as a parameter.
> +   */

nit: I'd prefer to see a description of what this method does, even if it does seem mostly obvious

@@ +77,5 @@
> +   */
> +  getVisitCount: function Site_getVisitCount(aCallback) {
> +    let query = "SELECT SUM(visit_count) AS count " +
> +                "FROM moz_places WHERE get_unreversed_host(rev_host) = :host";
> +    let stmt = gPlacesDatabase.createAsyncStatement(query);

We really ought to cache this statement instead of recreating it each time (and per site).  This makes us do a lot more work on the DB thread that we don't need to do.

@@ +83,5 @@
> +    stmt.executeAsync({
> +      handleResult: function(aResults) {
> +        let row = aResults.getNextRow();
> +        let count = row.getResultByName("count") || 0;
> +        aCallback(count);

It might be worthwhile to wrap this in a try-catch block in case the callback ever throws.  In the catch, just Cu.reportError(e).

@@ +89,5 @@
> +      handleError: function(aError) {
> +        Components.utils.reportError("AboutPermissions: " + aError);
> +      },
> +      handleCompletion: function(aReason) {
> +      }

You don't actually need to provide handleCompletion if you don't need it.

@@ +95,5 @@
> +    stmt.finalize();
> +  },
> +
> +  /**
> +   * Gets the permission value stored for a specified permission type.

nit: newline after this

@@ +116,5 @@
> +
> +    return permissionValue != Ci.nsIPermissionManager.UNKNOWN_ACTION;
> +  },
> +
> +  setPermission: function Site_setPermission(aType, aPerm) {

global-nit: java-doc style comment on these please

@@ +201,5 @@
> +    let value = (aValue == this.ALLOW);
> +    Services.prefs.setBoolPref("signon.rememberSignons", value);
> +  },
> +  
> +  // network.cookie.cookieBehavior: 0-Accept, 1-Don't accept foreign, 2-Don't use

Please pull the magic numbers here out into constants so we don't have to rely on this comment.

@@ +273,5 @@
> +  },
> +  set install(aValue) {
> +    let value = (aValue == this.DENY);
> +    Services.prefs.setBoolPref("xpinstall.whitelist.required", value);
> +  }

I think Mossop was talking about changing how the permissions here are used.  You should check with him to make sure that won't break this (or at least make him aware of it)

@@ +317,5 @@
> +    this.getSitesFromPlaces();
> +    this.enumerateServices();
> +
> +    // Attach observers in case data changes while the page is open.
> +    Services.prefs.addObserver("", this, false);

Can we maybe use a finer grained observer strategy here?  I'd hate to be notified about every pref change.

@@ +350,5 @@
> +      case "nsPref:changed":
> +        this._supportedPermissions.forEach(function(aType){
> +          AboutPermissions.updatePermission(aType);
> +        });
> +        break;

We can be more explicit in this case.  We know which pref changes in this callback (it's in aData), so we should only update that.

@@ +376,5 @@
> +  getSitesFromPlaces: function() {
> +    let query = "SELECT get_unreversed_host(rev_host) AS host " +
> +                "FROM moz_places WHERE rev_host <> '.' " +
> +                "AND visit_count > 0 GROUP BY rev_host " +
> +                "ORDER BY MAX(frecency) DESC LIMIT :limit";

We should also cache this statement.

For all the Places DB statements, can you please run them by mak and make sure he doesn't have any suggestions please?

::: browser/components/preferences/aboutPermissions.xml
@@ +1,1 @@
> +<?xml version="1.0"?>

I think this file probably should have a license block.

::: browser/locales/en-US/chrome/browser/preferences/aboutPermissions.dtd
@@ +16,5 @@
> +<!ENTITY permission.allowForSession      "Allow for Session">
> +<!ENTITY permission.block                "Block">
> +
> +<!ENTITY password.label                  "Store Passwords">
> +<!ENTITY password.manage                 "Manage Passwords…">

Something tells me this isn't right for rtl stuff.  Can you get l10n feedback on this please?
Comment 43 Shawn Wilsher :sdwilsh 2011-05-18 11:38:18 PDT
Comment on attachment 533123 [details] [diff] [review]
tests

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

r=sdwilsh

::: browser/components/preferences/tests/browser_permissions.js
@@ +1,4 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> +  Any copyright is dedicated to the Public Domain.
> +  http://creativecommons.org/publicdomain/zero/1.0/
> + * ***** END LICENSE BLOCK ***** */

https://www.mozilla.org/MPL/boilerplate-1.1/pd-c
Comment 44 Adam Kowalczyk 2011-05-18 12:32:07 PDT
(In reply to comment #42)
> @@ +89,5 @@
> > +      handleError: function(aError) {
> > +        Components.utils.reportError("AboutPermissions: " + aError);
> > +      },
> > +      handleCompletion: function(aReason) {
> > +      }
> 
> You don't actually need to provide handleCompletion if you don't need it.

I don't think that's true, see bug 555260.
Comment 45 Shawn Wilsher :sdwilsh 2011-05-18 12:43:13 PDT
(In reply to comment #44)
> I don't think that's true, see bug 555260.
Well, it's fine, it just has console spew.  We should keep it in this patch though.
Comment 46 Marco Bonardo [::mak] 2011-05-18 13:11:31 PDT
Comment on attachment 533120 [details] [diff] [review]
patch (updated)

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

::: browser/components/preferences/aboutPermissions.js
@@ +76,5 @@
> +   *        A function that takes the visit count (a number) as a parameter.
> +   */
> +  getVisitCount: function Site_getVisitCount(aCallback) {
> +    let query = "SELECT SUM(visit_count) AS count " +
> +                "FROM moz_places WHERE get_unreversed_host(rev_host) = :host";

This query is skipping the index on rev_host due to the get_unreversed_host function, you are forcing it to a table scan.
you should rather do a WHERE rev_host = :rev_host and reverse it in js (copying the code from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Helpers.cpp#211)
Yeah, I know this sucks, but skipping indices is worse.

Make it readable though:
SELECT
FROM
WHERE

@@ +376,5 @@
> +  getSitesFromPlaces: function() {
> +    let query = "SELECT get_unreversed_host(rev_host) AS host " +
> +                "FROM moz_places WHERE rev_host <> '.' " +
> +                "AND visit_count > 0 GROUP BY rev_host " +
> +                "ORDER BY MAX(frecency) DESC LIMIT :limit";

I don't think we can make this more performant, it is slow mostly due to the order by that has to build a temporary index, having a flattening with per-host frecency would help :(
Something you could change here is using "WHERE rev_host > '.'" instead of "<> '.'", since I can imagine situations where rev_host is null and visit_count > 0 (I have some of these in my db)
And please make it readable
SELECT
FROM
WHERE
GROUP
ORDER
LIMIT

Actually, I have a much faster query (about 30 times faster), but is far less precise. It filters the rev_hosts for the most frecent pages, rather than filtering the most frecent rev_hosts. It is faster because it immediately reduces the resultset to about 300 pages using frecency order, and then groups and filters what's left. But it has to do a first (limited) scan and generates 2 temp indices. Thus I'd suggest to retain what you have, and we can evaluate less precise alternatives in future, if it should perform badly in the wild.
Comment 47 :Margaret Leibovic 2011-05-18 15:28:59 PDT
Created attachment 533445 [details] [diff] [review]
patch v2

Shawn and Marco, thanks for the comments! I updated the patch to address everything except for these two points:

(In reply to comment #42)
> @@ +350,5 @@
> > +      case "nsPref:changed":
> > +        this._supportedPermissions.forEach(function(aType){
> > +          AboutPermissions.updatePermission(aType);
> > +        });
> > +        break;
> 
> We can be more explicit in this case.  We know which pref changes in this
> callback (it's in aData), so we should only update that.

Right now those prefs strings aren't stored anywhere; they're just used in the DefaultPermissions object. I think the only way to do this would be another switch statement on all the pref strings. Is this worth it? I feel like updating the UI should be cheap enough that it's not a huge deal, given that we're now only listening for prefs we care about.

> ::: browser/locales/en-US/chrome/browser/preferences/aboutPermissions.dtd
> @@ +16,5 @@
> > +<!ENTITY permission.allowForSession      "Allow for Session">
> > +<!ENTITY permission.block                "Block">
> > +
> > +<!ENTITY password.label                  "Store Passwords">
> > +<!ENTITY password.manage                 "Manage Passwords…">
> 
> Something tells me this isn't right for rtl stuff.  Can you get l10n
> feedback on this please?

I'm not sure what you're referring to. It seems like RTL would work fine with this.
Comment 48 :Margaret Leibovic 2011-05-18 15:29:38 PDT
Created attachment 533446 [details] [diff] [review]
tests (updated)

Addressed license block comment.
Comment 49 Shawn Wilsher :sdwilsh 2011-05-18 15:39:01 PDT
(In reply to comment #47)
> Right now those prefs strings aren't stored anywhere; they're just used in
> the DefaultPermissions object. I think the only way to do this would be
> another switch statement on all the pref strings. Is this worth it? I feel
> like updating the UI should be cheap enough that it's not a huge deal, given
> that we're now only listening for prefs we care about.
Yeah, that's fine.

> I'm not sure what you're referring to. It seems like RTL would work fine
> with this.
As you can see, bugzilla botched the unicode character there, and I misunderstood what it was supposed to be.  You can ignore that comment.
Comment 50 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-18 19:54:39 PDT
Comment on attachment 533445 [details] [diff] [review]
patch v2

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

>+  clearCookies: function Site_clearCookies(aCookieArray) {

Bit odd that this method isn't specific to any Site instance (unlike the others). Since the only caller passes in Site.cookies, you could get rid of aCookieArray here and just use this.cookies.

>+  get logins() {
>+    // There could be more logins for different schemes/ports, but this covers
>+    // the vast majority of cases.

Maybe we should have a followup filed to expose way to do a looser search on nsILoginManager so that we don't miss these edge cases?

>+let PermissionDefaults = {

>+  get cookie() {

>+    if (Services.prefs.getIntPref("network.cookie.lifetimePolicy") == this.COOKIE_DENY) {
>+      return this.SESSION;

>+  set cookie(aValue) {

>+    if (aValue == this.SESSION) {
>+      Services.prefs.setIntPref("network.cookie.lifetimePolicy", this.COOKIE_DENY);

It's confusing to use COOKIE_DENY here, since the value "2" for lifetimePolicy has a different meaning than "2" for cookieBehavior. Also it looks like switching from "allow for session" to "allow" will not set lifetimePolicy back to 0 (so you'll always be stuck on "allow for session", e.g. after reloading the page).

>+  get geo() {
>+  get indexedDB() {

>+    // We always ask for permission to share location with a specific site, so
>+    // there is no global ALLOW.

Shouldn't these have their "Allow" menu items hidden, then?

>+  set install() {

Hmm, I'm not sure we want to allow toggling this globally at all, particularly given the broken pref situation. Can we just hide it when "all sites" is selected? And similarly to geo/indexeddb, the options here are really more like "ask a lot" or "ask me once", so I'm not sure it makes sense to include this item in this UI at all.

>+let AboutPermissions = {

>+  _sitesList: null,

sitesList

>+  _stringBundle: Cc["@mozilla.org/intl/stringbundle;1"].
>+                 getService(Ci.nsIStringBundleService).

nit: Services.strings

>+  init: function() {

>+    // Notify oberservers for testing purposes.
>+    Services.obs.notifyObservers(null, "browser-permissions-initialized", null);

Hmm, shouldn't this actually be in getSitesFromPlaces()'s callback?

>+  cleanUp: function() {
>+    Services.prefs.removeObserver("signon.rememberSignons", this, false);

In theory it's possible for this to run without onload having run (if you e.g. load about:permissions and hold down Cmd+R), so it may be nice to set a flag in init() to indicate that observers were added, and then check that before trying to remove them. But it's also possible to cause other errors that way (e.g. "AboutPermissions is not defined" if unload tries to fire before permissions.js is loaded), so maybe just ignore that case for simplicity's sake (no sane user would reload about:permissions repeatedly and then complain about JS console errors!).

>+    gVisitStmt.finalize();

Similarly, if the lazy getter for this hadn't been called yet, this would create it only to finalize it. So maybe just get rid of the lazygetter for it.

>+  observe: function (aSubject, aTopic, aData) {
>+    switch(aTopic) {
>+      case "perm-changed":
>+        let permission = aSubject.QueryInterface(Ci.nsIPermission);

aSubject is null when nsIPermisionManager::removeAll() is called, so you should probably handle that.

>+        if (this._selectedSite && permission.type in PermissionDefaults) {

Maybe worth a comment that you can't compare selectedSite.host and permission.host here because you need to handle the case where a parent domain was changed in a way that affects the subdomain.

>+      case "nsPref:changed":
>+        this._supportedPermissions.forEach(function(aType){
>+          AboutPermissions.updatePermission(aType);
>+        });

nit: you could pass "this" to forEach, and then use "this.updatePermission" inside the function. That applies in a bunch of places where you use "AboutPermissions." explicitly.

>+  enumerateServices: function() {

>+    disabledHosts.forEach(function(aHostname) {
>+      // aHostname is a string in origin URL format (e.g. "http://foo.com")
>+      let uri = NetUtil.newURI(aHostname);

Maybe add a try/catch here too, to be on the safe side?

>+      if (!(host in AboutPermissions._sites)) {
>+        let site = new Site(host)
>+        AboutPermissions._sites[host] = site;
>+        AboutPermissions.addToSitesList(site);
>+      }

Could put thses few lines in a local addHost() helper.

>+  deleteFromSitesList: function(aHost) {
>+    for each (let site in this._sites) {
>+      if (site.host.hasRootDomain(aHost)) {
>+        if (site == this._selectedSite) {
>+          // Clear site data from the DOM to maximize privacy.

Wouldn't it be better to just set selectedItem to "all sites" entry? You could then move the site-label label clearing into onSitesListSelect, and get rid of the permissions-box hiding stuff.

>+  restoreDefaultPermissions: function(aSite) {

Seems like this method is a better fit on the Site prototype, conceptually.

>+  restoreAllDefaultPermissions: function() {
>+  forgetSite: function() {

I wonder whether these should prompt, or something. Seems like bad buttons to hit accidentally (maybe less so for the latter).

>+  updatePermission: function(aType) {

>+    permissionMenulist.disabled = false;

The menulists never seem to be disabled anywhere.

>+  onPermissionCommand: function(event) {

>+    } else if (permissionType == "password") {

Seems like this special casing (and the equivalent in updatePermission) could move into the site object's setPermission/getPermission.

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

>+<bindings id="prefsWindowBindings"

nit: could just remove this ID.

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

>+          <description id="site-description">
>+            &permissions.header;<label id="site-label"/>
>+          </description>

I'm not sure this is completely l10n-friendly - you should probably just make this a formatted string, since you're setting the value dynamically anyways.

>diff --git a/browser/locales/en-US/chrome/browser/preferences/aboutPermissions.dtd b/browser/locales/en-US/chrome/browser/preferences/aboutPermissions.dtd

>+<!ENTITY aboutPermissions.title          "about:permissions">

this might be a tricky one for localizers. can we omit it?

>+<!ENTITY permissions.manageDefaults      "Manage Default Permissions">
>+<!ENTITY permissions.restoreDefaults     "Restore Default Permissions">
>+<!ENTITY image.label                     "Load Images">

These are unused.

>+<!ENTITY indexedDB.label                 "Maintain Offline Database Storage">

Page info calls this just "offline storage". I guess we need to sort out strings seperately but maybe it's worth adding an L10N note here (mentioning permIndexedDB string) and being consistent for the time being.

>diff --git a/browser/locales/en-US/chrome/browser/preferences/aboutPermissions.properties b/browser/locales/en-US/chrome/browser/preferences/aboutPermissions.properties

>+passwordsCountAllSites=#1 password is stored for all sites.;#1 passwords are stored for all sites.
>+cookiesCountAllSites=#1 cookie is set for all sites.;#1 cookies are set for all sites.

also unused
Comment 51 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-18 19:58:07 PDT
(In reply to comment #50)
> >+    // Notify oberservers for testing purposes.
> >+    Services.obs.notifyObservers(null, "browser-permissions-initialized", null);
> 
> Hmm, shouldn't this actually be in getSitesFromPlaces()'s callback?

Oh, I see that it's currently fired twice. I think it should only be sent once (from the getSitesFromPlaces callback), which seems to be most useful for your test anyways.
Comment 52 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-18 20:00:53 PDT
Comment on attachment 533446 [details] [diff] [review]
tests (updated)

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

>+function cleanUp() {

>+  waitForClearHistory(finishCleanUp);

Unfortunately you can't call methods that do their work asynchronously from cleanup functions, because they won't complete before the next test runs. You should this it at the end of your test before calling finish().
Comment 53 Shawn Wilsher :sdwilsh 2011-05-18 20:05:45 PDT
(In reply to comment #50)
> >+    gVisitStmt.finalize();
> 
> Similarly, if the lazy getter for this hadn't been called yet, this would
> create it only to finalize it. So maybe just get rid of the lazygetter for it.
It's pretty cheap to create an async statement (not true of a statement created by createStatement, however), so I'm not sure it's worthwhile.
Comment 54 :Margaret Leibovic 2011-05-19 13:12:07 PDT
Created attachment 533767 [details] [diff] [review]
patch v3

(In reply to comment #50)

I appreciate the additional comments, and I addressed most of them, but I think others would be better addressed in follow-ups. I think it would be best to land what we have to get it in front of users, rather then spend a lot of cycles perfecting the code first, especially since the UX design can't be finalized before we get more feedback. I think I changed enough code that this should be re-reviewed now, but hopefully that won't be too hard. And that's what you get for giving me lots of comments! :P

> >+  get logins() {
> >+    // There could be more logins for different schemes/ports, but this covers
> >+    // the vast majority of cases.
> 
> Maybe we should have a followup filed to expose way to do a looser search on
> nsILoginManager so that we don't miss these edge cases?

Filed bug 658271.

> >+  get geo() {
> >+  get indexedDB() {
> 
> >+    // We always ask for permission to share location with a specific site, so
> >+    // there is no global ALLOW.
> 
> Shouldn't these have their "Allow" menu items hidden, then?
>
> >+  set install() {
> 
> Hmm, I'm not sure we want to allow toggling this globally at all,
> particularly given the broken pref situation. Can we just hide it when "all
> sites" is selected? And similarly to geo/indexeddb, the options here are
> really more like "ask a lot" or "ask me once", so I'm not sure it makes
> sense to include this item in this UI at all.

I hid the geo/indexedDB/install items in the "All Sites" interface, but breaks the "Apply to All Sites" button because that button clears all permissions, including these hidden permission types. Therefore, clicking that button would likely result in unexpected data loss. I decided to remove it for now to avoid accidentally upsetting people, and we can add it back in a follow-up.

> >+  deleteFromSitesList: function(aHost) {
> >+    for each (let site in this._sites) {
> >+      if (site.host.hasRootDomain(aHost)) {
> >+        if (site == this._selectedSite) {
> >+          // Clear site data from the DOM to maximize privacy.
> 
> Wouldn't it be better to just set selectedItem to "all sites" entry? You
> could then move the site-label label clearing into onSitesListSelect, and
> get rid of the permissions-box hiding stuff.

This affects a decent amount of the code, and it's not currently broken, so can we punt this to a follow-up?

> >+  restoreDefaultPermissions: function(aSite) {
> 
> Seems like this method is a better fit on the Site prototype, conceptually.

Sure, but the permissions we want to restore are defined in AboutPermissions. Right now Site doesn't know about them. I got rid of this for now anyway because I got rid of the button that triggered it (see above). Also, we weren't using this on a per-site basis because of confusing permission inheritance rules, so we can just use nsIPermissionManager::removeAll() in restoreAllDefaultPermissions when we add that method back, and then also do something separately about login saving.

> >+  restoreAllDefaultPermissions: function() {
> >+  forgetSite: function() {
> 
> I wonder whether these should prompt, or something. Seems like bad buttons
> to hit accidentally (maybe less so for the latter).

This is a first attempt, and there are still UX details to work out, so can we punt this to a follow-up?

> >+  onPermissionCommand: function(event) {
> 
> >+    } else if (permissionType == "password") {
> 
> Seems like this special casing (and the equivalent in updatePermission)
> could move into the site object's setPermission/getPermission.

Also not broken and affects a decent amount of code. Follow-up?

> >diff --git a/browser/components/preferences/aboutPermissions.xul b/browser/components/preferences/aboutPermissions.xul
> 
> >+          <description id="site-description">
> >+            &permissions.header;<label id="site-label"/>
> >+          </description>
> 
> I'm not sure this is completely l10n-friendly - you should probably just
> make this a formatted string, since you're setting the value dynamically
> anyways.

I'm styling the label separately from the other text in the description, and I can't do that with one formatted string. I added another entity after the label for localizers, and just made it an empty string in en-US. We do the same thing in aboutDialog.xul.
Comment 55 :Margaret Leibovic 2011-05-19 13:14:53 PDT
Created attachment 533771 [details] [diff] [review]
tests (updated again)

These still pass with the new patch. I just addressed the waitForClearHistory comment and updated the observer to deal with only one notification.
Comment 56 :Margaret Leibovic 2011-05-19 14:26:44 PDT
Created attachment 533791 [details] [diff] [review]
patch v4

Patch updated to address more feedback.
Comment 57 :Margaret Leibovic 2011-05-19 14:28:08 PDT
Created attachment 533792 [details] [diff] [review]
tests v4

Updated tests to reflect changes in patch.
Comment 58 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-19 14:45:10 PDT
Comment on attachment 533791 [details] [diff] [review]
patch v4

you can remove install.label now, too! r=me with that
Comment 59 :Margaret Leibovic 2011-05-19 15:38:38 PDT
Created attachment 533825 [details] [diff] [review]
patch for check-in

Also got rid of %ifdef WINSTRIPE_AERO block that was causing an error because WINSTRIPE_AERO isn't defined. We can make aero-specifc theming in a follow-up if we want.
Comment 60 :Margaret Leibovic 2011-05-19 15:51:36 PDT
http://hg.mozilla.org/mozilla-central/rev/1a9a234a6fc7
http://hg.mozilla.org/mozilla-central/rev/75fc3e9b1a6e

I will make sure follow-ups get filed for all the issues we talked about in this bug.
Comment 61 :Margaret Leibovic 2011-05-19 15:54:27 PDT
Created attachment 533830 [details]
screenshot

For all those following along, this is what the final patch looks like.
Comment 62 Leman Bennett [Omega] 2011-05-19 17:00:34 PDT
Looks like the double entry issue is in the screenshot. Is that just a quirk with how cookies are handled by the system?
Comment 63 :Margaret Leibovic 2011-05-19 17:22:26 PDT
(In reply to comment #62)
> Looks like the double entry issue is in the screenshot. Is that just a quirk
> with how cookies are handled by the system?

Sites are uniquely identified by complete host strings, so sites with different subdomains are separated, if that's what you're referring to.
Comment 64 Leman Bennett [Omega] 2011-05-19 17:48:07 PDT
Not exactly. Notice "getpersonas.com" and "www.getpersonas.com" are both listed. In the last build I tested, if visiting a site without the "www" prefix was used, the site was sometimes listed twice. One with the "www" prefix and one without.

The old dialog doesn't do this. Most likely because the "www" prefix is stripped. But still, it doesn't seem to confuse the two.
Comment 65 Jim Jeffery not reading bug-mail 1/2/11 2011-05-20 02:21:06 PDT
(In reply to comment #61)
> Created attachment 533830 [details]
> screenshot
> 
> For all those following along, this is what the final patch looks like.

Pretty pictures... where is it ?  I'm running latest hourly that should contain the patch, and I see no place or nothing like the screenshot...
Comment 66 Marco Bonardo [::mak] 2011-05-20 02:27:30 PDT
(In reply to comment #65)
> Pretty pictures... where is it ?  I'm running latest hourly that should
> contain the patch, and I see no place or nothing like the screenshot...

about:permissions
Comment 67 Jim Jeffery not reading bug-mail 1/2/11 2011-05-20 02:38:36 PDT
(In reply to comment #66)
> (In reply to comment #65)
> > Pretty pictures... where is it ?  I'm running latest hourly that should
> > contain the patch, and I see no place or nothing like the screenshot...
> 
> about:permissions

Thanks, is or are there plans to make this available through UI, like a Menu item, or from the 'Options Panel->Privacy' ?  Seems a bit obscure at the moment, especially for new users.
Comment 68 Marco Bonardo [::mak] 2011-05-20 02:47:20 PDT
check dependencies, like Bug 588689
Comment 69 Shawn Wilsher :sdwilsh 2011-05-20 08:24:42 PDT
(In reply to comment #67)
> Thanks, is or are there plans to make this available through UI, like a Menu
> item, or from the 'Options Panel->Privacy' ?  Seems a bit obscure at the
> moment, especially for new users.
See comment 54.
Comment 70 IU 2011-05-20 18:00:07 PDT
No way to manually add sites?  Is this an oversight?  I sure hope Mozilla's not planning on ditching Options --> Privacy --> Exceptions... until that is resolved.
Comment 71 George Carstoiu 2011-05-27 02:02:08 PDT
Verfied on Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2.
Comment 72 Himanshu 2011-06-03 05:23:19 PDT
*** Bug 661747 has been marked as a duplicate of this bug. ***
Comment 73 JK 2011-07-16 06:58:33 PDT
I added this: https://bugzilla.mozilla.org/show_bug.cgi?id=672038 Permissions manager should show cookies, passwords inline

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