Closed Bug 573176 Opened 14 years ago Closed 13 years ago

Implement Site-Specific Privacy Preferences

Categories

(Firefox :: Settings UI, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 6

People

(Reporter: jboriss, Assigned: Margaret)

References

Details

(Keywords: user-doc-needed, Whiteboard: [new feature] [hidden behind about:permissions for 6])

Attachments

(4 files, 22 obsolete files)

1.06 MB, image/png
Details
11.51 KB, patch
Details | Diff | Splinter Review
62.60 KB, patch
Details | Diff | Splinter Review
849.29 KB, image/png
Details
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.
Assignee: nobody → mmulani
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
Attached file Main guts of the SP add-on so far. (obsolete) —
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.
Attachment #453489 - Flags: feedback?(mak77)
Depends on: 578724
No longer depends on: 578524
OS: Mac OS X → All
Hardware: x86 → All
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.
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.
Blocks: IndexedDB
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.
Attachment #461405 - Attachment is obsolete: true
Attachment #462839 - Flags: feedback?
Attachment #462839 - Flags: feedback? → feedback?(dao)
Previously some UI was created at run-time but it is now easier to be done in XUL for theming purposes.
Attachment #462839 - Attachment is obsolete: true
Attachment #463047 - Flags: feedback?(dao)
Attachment #462839 - Flags: feedback?(dao)
Severity: normal → enhancement
Status: NEW → ASSIGNED
Keywords: meta
Summary: [meta] Implement Site-Specific Privacy Preferences → Implement Site-Specific Privacy Preferences
Target Milestone: Future → ---
Version: unspecified → Trunk
Attached patch First step in localizations. (obsolete) — Splinter Review
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.
Attachment #453489 - Attachment is obsolete: true
Attachment #463047 - Attachment is obsolete: true
Attachment #463047 - Flags: feedback?(dao)
Attachment #453489 - Flags: feedback?(mak77)
Monster patch, not many changes from the previous except for the addition of graphics!
Attachment #465086 - Attachment is obsolete: true
Attachment #465925 - Flags: review?(dolske)
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.
Attachment #465925 - Flags: review?(dolske)
Attachment #465925 - Attachment is obsolete: true
Attachment #466039 - Flags: review?(dolske)
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.
Attachment #466039 - Flags: review?(dolske)
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.
Attachment #466039 - Attachment is obsolete: true
Attachment #466406 - Flags: review?(dolske)
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!
Attachment #466406 - Flags: review?(dolske) → review?(sdwilsh)
Comment on attachment 466406 [details] [diff] [review]
Fixed tests to account for async database queries.

From sdwilsh, pushing this to Mano.
Attachment #466406 - Flags: review?(sdwilsh) → review?(mano)
* Please do not diff binary files.
* General code-style comment: please add an empty line between js methods.
Attached patch Mainly UI changes. (obsolete) — Splinter Review
Attachment #466406 - Attachment is obsolete: true
Attachment #467936 - Flags: review?
Attachment #466406 - Flags: review?(mano)
Attachment #467936 - Flags: review? → review?(mano)
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 on attachment 467936 [details] [diff] [review]
Mainly UI changes.

Not ready for prime time.  Canceling review request for now.
Attachment #467936 - Flags: review?(mano)
(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).
Attached patch Last patch before leaving. (obsolete) — Splinter Review
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!
Attachment #467936 - Attachment is obsolete: true
Whoa, almost forgot:
- look at implementation of Forget About This Site and ensure that all data from there is represented/handled in SP
I'm marking this feature for future, where future is Firefox 4.1
Target Milestone: --- → Future
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 ?
(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.
Depends on: 653010
Attached patch WIP patch (obsolete) — Splinter Review
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.
Assignee: mars.martian+bugmail → margaret.leibovic
Attachment #470075 - Attachment is obsolete: true
Attached patch WIP patch v2 (obsolete) — Splinter Review
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!
Attachment #528488 - Attachment is obsolete: true
Attached patch WIP v3 (obsolete) — Splinter Review
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.
Attachment #528975 - Attachment is obsolete: true
Attachment #530767 - Flags: feedback?(sdwilsh)
Depends on: 656145
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/.
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.
(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.
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.
Attached patch WIP v4 (obsolete) — Splinter Review
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 :)
Attachment #530767 - Attachment is obsolete: true
Attachment #531808 - Flags: feedback?(sdwilsh)
Attachment #531808 - Flags: feedback?(dolske)
Attachment #530767 - Flags: feedback?(sdwilsh)
(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).
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 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
Attachment #531808 - Flags: feedback?(sdwilsh)
Attachment #531808 - Flags: feedback?(dolske)
Attachment #531808 - Flags: feedback+
(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?
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #531808 - Attachment is obsolete: true
Attachment #532731 - Flags: review?(sdwilsh)
Attached patch patch (updated) (obsolete) — Splinter Review
I updated the observer code in this patch to correctly handle more cases, especially when "All Sites" is selected.
Attachment #532731 - Attachment is obsolete: true
Attachment #533120 - Flags: review?(sdwilsh)
Attachment #532731 - Flags: review?(sdwilsh)
Attached patch tests (obsolete) — Splinter Review
Attachment #533123 - Flags: review?(sdwilsh)
Depends on: 657961
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?
Attachment #533120 - Flags: review?(sdwilsh) → review+
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
Attachment #533123 - Flags: review?(sdwilsh) → review+
(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.
(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 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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #533120 - Attachment is obsolete: true
Attached patch tests (updated) (obsolete) — Splinter Review
Addressed license block comment.
Attachment #533123 - Attachment is obsolete: true
(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.
Depends on: 658097
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
(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 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().
(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.
Depends on: 658271
Attached patch patch v3 (obsolete) — Splinter Review
(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.
Attachment #533445 - Attachment is obsolete: true
Attachment #533767 - Flags: review?(gavin.sharp)
Attached patch tests (updated again) (obsolete) — Splinter Review
These still pass with the new patch. I just addressed the waitForClearHistory comment and updated the observer to deal with only one notification.
Attachment #533446 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
Patch updated to address more feedback.
Attachment #533767 - Attachment is obsolete: true
Attachment #533791 - Flags: review?(gavin.sharp)
Attachment #533767 - Flags: review?(gavin.sharp)
Attached patch tests v4Splinter Review
Updated tests to reflect changes in patch.
Attachment #533771 - Attachment is obsolete: true
Comment on attachment 533791 [details] [diff] [review]
patch v4

you can remove install.label now, too! r=me with that
Attachment #533791 - Flags: review?(gavin.sharp) → review+
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.
Attachment #533791 - Attachment is obsolete: true
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: Future → Firefox 6
Attached image screenshot
For all those following along, this is what the final patch looks like.
Depends on: 658423
Depends on: 658425
Looks like the double entry issue is in the screenshot. Is that just a quirk with how cookies are handled by the system?
(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.
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.
(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...
(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
(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.
check dependencies, like Bug 588689
Depends on: 658530
(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.
Depends on: 658555
Depends on: 658556
Depends on: 658620
Depends on: 658628
Depends on: 658624
Depends on: 658618
Depends on: 658615
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.
Depends on: 572803
No longer depends on: 578724
No longer depends on: 585125
No longer depends on: 588613
Depends on: 659087
Depends on: 659319
Depends on: 659642
Depends on: 659719
Verfied on Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2.
Status: RESOLVED → VERIFIED
Depends on: 660187
Depends on: 660229
Depends on: 660225
Depends on: 661662
Depends on: 661806
Depends on: 654573
Depends on: 663777
Depends on: 665527
I added this: https://bugzilla.mozilla.org/show_bug.cgi?id=672038 Permissions manager should show cookies, passwords inline
Blocks: 672038
Whiteboard: [new feature] [hidden behind about:permissions for 6]
Depends on: 678735
Depends on: 676846
Depends on: 697941
Blocks: 697942
Depends on: 821049
Depends on: 955369
Depends on: 956104
Depends on: 1197482
No longer blocks: 449981
No longer depends on: 572803
You need to log in before you can comment on or make changes to this bug.