Closed Bug 569341 (DataManager) Opened 14 years ago Closed 14 years ago

Implement a Data Manager unifying cookie, permission, password, and form data management

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set
normal

Tracking

(blocking-seamonkey2.1 -, seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1b1
Tracking Status
blocking-seamonkey2.1 --- -
seamonkey2.1 --- wanted

People

(Reporter: kairo, Assigned: kairo)

References

(Blocks 13 open bugs, )

Details

Attachments

(1 file, 8 obsolete files)

I've started work on a Data Manager, for now as an add-on, but targeted for inclusion in SeaMonkey 2.1.
This should unify cookie, permission, password, and form data management and probably also obsolete the management windows we have right now for those.
Blocks: 480734
https://addons.mozilla.org/addon/162068 has the experimental add-on containing my work on this, btw.
I think its safe to call this wanted. (If you want a more "official" council member, feel free to re-request)
blocking-seamonkey2.1: --- → -
Blocks: 295727
This patch merges the Data Manager 1.0 add-on into SeaMonkey code.

This does the first step to get it into the tree - followups will be needed at least on being able to call certain domains and possibly tabs from outside and on being able to add permissions before we can switch over from other managers to Data Managers and finally remove them from our UI.
Attachment #461575 - Flags: superreview?(neil)
Attachment #461575 - Flags: review?(iann_bugzilla)
Comment on attachment 461575 [details] [diff] [review]
merge Data Manager 1.0 into SeaMonkey

I gave this a quick whirl on a boring profile and the most annoying behaviour is that when switching domain the focus should remain on the domain list.

One thing that Data Manager seems to be unable to do is to change permissions when no specific permission exists yet.

>+/* generic item gets used for permissions that don't need any special treatment */
>+richlistitem.permission:not([type="cookie"]):not([type="geo"]):not([type="password"]) {
As pointed out on IRC the :not()s are unnecessary as this rule is less specific than the, um, specific rules above. And it would make sense to make this first.

>+window.addEventListener("load", initialize, false);
>+window.addEventListener("unload", shutdown, false);
Nit: prefer onload/onunload in the XUL.

>+// locally loaded services
>+var gLocSvc = {};
Don't you use all of these services on startup anyway? In which case, no use loading them lazily.

>+XPCOMUtils.defineLazyServiceGetter(gLocSvc, "cpref",
>+                                   "@mozilla.org/content-pref/service;1",
>+                                   "nsIContentPrefService"); // rv: >= 2.0b3 - Services.contentPrefs
And therefore unnecessary here, right?

>+  interfaces: [Components.interfaces.nsIObserver,
>+               Components.interfaces.nsIContentPrefObserver,
>+               Components.interfaces.nsISupports],
>+
>+  QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>+    if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
>+      throw Components.results.NS_ERROR_NO_INTERFACE;
>+    return this;
>+  },
Why not use XPCOMUtils.generateQI?

>+  observe: function changeobserver_observe(aSubject, aTopic, aState) {
Nit: third parameter normally called aData
Nit: function name normally CO_observe or co_observe or observe or ""

>+        if (/^hostSaving/.test(aState)) {
>+          gPerms.reactToChange(aSubject, aState);
>+        }
>+        else {
>+          gPasswords.reactToChange(aSubject, aState);
>+        }
Nit: don't need {}s

>+    Services.obs.addObserver(gChangeObserver, "cookie-changed", false);
>+    Services.obs.addObserver(gChangeObserver, "perm-changed", false);
>+    Services.obs.addObserver(gChangeObserver, "passwordmgr-storage-changed", false);
>+    gLocSvc.cpref.addObserver(null, gChangeObserver);
>+    Services.obs.addObserver(gChangeObserver, "satchel-storage-changed", false);
Presumably these are in tab order?

>+    for (let i = 0; i < gCookies.cookies.length; i++) {
>+      this.addDomainOrFlag(gCookies.cookies[i].rawHost, "hasCookies");
>+    }
Correct usage of let ;-) But {}s are optional.

>+      let nextPermission = enumerator.getNext();
>+      nextPermission = nextPermission.QueryInterface(Components.interfaces.nsIPermission);
Nit: hardly worth two statements

>+  shutdown: function domain_shutdown() {
Should clear all tree views here too. (For some reason they get cleared very late when loading a new page into the tab.)

>+    var cache_idx = this.xlcache_hosts.indexOf(aHostname);
Why not use a hash instead of an array?

>+      // return vars for nsIURLParser must all be objects
>+      // see bug 568997 for improvements to that interface
>+      var schemePos = {}, schemeLen = {}, authPos = {}, authLen = {}, pathPos = {},
>+          pathLen = {}, usernamePos = {}, usernameLen = {}, passwordPos = {},
>+          passwordLen = {}, hostnamePos = {}, hostnameLen = {}, port = {};
>+      gLocSvc.url.parseURL(aHostname, -1, schemePos, schemeLen, authPos, authLen,
>+                          pathPos, pathLen);
>+      var auth = aHostname.substring(authPos.value, authPos.value + authLen.value);
>+      gLocSvc.url.parseAuthority(auth, authLen.value, usernamePos, usernameLen,
>+                                passwordPos, passwordLen, hostnamePos, hostnameLen, port);
>+      var hostName = auth.substring(hostnamePos.value, hostnamePos.value + hostnameLen.value);
What's this actually doing? You start with a host name, and you end up with... a host name?

>+    if (!this.domainObjects.some(
Why not use a hash?

>+    if (idx >= 0 && !this.ignoreUpdate) {
idx is always set by this point.

>+      if (domAdded)
>+        this.search(this.searchfield.value);
Could possibly move this into the hash miss case.

>+        this.ignoreUpdate = true;
>+        this.select();
>+        this.ignoreUpdate = false;
Just pass a parameter to select()?

>+    this.domainObjects[idx][aFlag] = false
Nit: missing semicolon

>+    // Now, purge all empty doamins.
Typo

>+    this.ignoreUpdate = true;
>+    gDatamanUtils.restoreSelectionFromIDs(this.tree, this._getObjID,
>+                                          selectionCache);
>+    this.ignoreUpdate = false;
restoreSelectonFromIDs doesn't read ignoreUpdate does it?

>+      this.selectedDomain = {title: false};
"" or null would seem more sensible.

>+    let prevtab = gTabs.tabbox.selectedTab || gTabs.cookiesTab;
>+    let stoptab = null;
>+    while (gTabs.tabbox.selectedTab != stoptab &&
>+           (gTabs.tabbox.selectedTab.disabled || gTabs.tabbox.selectedTab.hidden)) {
>+      gTabs.tabbox.tabs.advanceSelectedTab(1, true);
>+      if (!stoptab)
>+        stoptab = prevtab;
>+    }
I prefer the code in xblBindings.js

>+    for (let i = 0; i < this.domainObjects.length; i++) {
Can we not use .filter() here?

>+      if (this.domainObjects[i] &&
When can that be null?

>+          this.domainObjects[i].title.toLocaleLowerCase().indexOf(aSearchString) != -1)
aSearchString needs to be lowercased too, no?

>+    this.tree.treeBoxObject.endUpdateBatch();
>+    this.sort();
Nit: sort first please

>+    gDatamanUtils.restoreSelectionFromIDs(this.tree, this._getObjID,
>+                                          selectionCache);
>+    this.ignoreSelect = false;
>+    // make sure we clear the data pane when selection has been removed
>+    if (!this.tree.view.selection.count && selectionCache.length)
>+      this.select();
Haven't I seen this code block before once or tiwce?

>+
>+
Why not fill the space up with a comment ;-)

>+      switch (this.activePanel) {
>+        case "cookiesPanel":
>+          gCookies.shutdown();
>+          break;
>+        case "permissionsPanel":
>+          gPerms.shutdown();
>+          break;
>+        case "preferencesPanel":
>+          gPrefs.shutdown();
>+          break;
>+        case "passwordsPanel":
>+          gPasswords.shutdown();
>+          break;
>+        case "formdataPanel":
>+          gFormdata.shutdown();
>+          break;
>+        case "forgetPanel":
Nit: need some sort of panel/object map. gPanels[this.activePanel].shutdown();

>+          this.forgetTab.hidden = true;
Doesn't that get handled by domain.select anyway?

>+    this.activePanel = this.tabbox.selectedPanel.id;
Nit: set and use this.activePanel first.

>+    document.getElementById("cookies-context-selectall").disabled =
>+      (this.tree.view.selection.count >= this.tree.view.rowCount);
I do hope it's never greater ;-)

>+var cookieTreeView = {
I'm not convinced that it makes sense having the view as a separate object.

>+  isSeparator: function(aIndex) { return false; },
>+  isSorted: function() { return false; },
>+  isContainer: function(aIndex) { return false; },
>+  cycleHeader: function(aCol) {},
>+  getRowProperties: function(aRow, aProp) {},
>+  getColumnProperties: function(aColumn, aProp) {},
>+  getCellProperties: function(aRow, aColumn, aProp) {}
Might be worth defining these on a prototype to avoid repetition.

>+      if (gDomains.hostMatchesSelected(host.replace(/^\./, ""))) {
You're computing rawHost here. Use a variable and avoid repeating yourself.

>+        permElem.setAttribute("rawHost", (host.charAt(0) == ".") ? host.substring(1, host.length) : host);
Especially don't compute rawHost in two different ways ;-)

>+    while (this.list.hasChildNodes())
>+      this.list.removeChild(this.list.firstChild);
Nit: removing lastChild is faster.

>+var gPrefs = {
I don't think we use content prefs so I can't really comment on this.

>+    // Compare function for two signons
>+    let compfunc = function passwords_sort_compare(aOne, aTwo) {
If you didn't name functions I would only have to nit you on one line ;-)

>+        let sql = "SELECT fieldname, value, timesUsed, firstUsed, lastUsed, guid FROM moz_formhistory WHERE guid=:guid";
Nit: spaces around =

>+var gForget = {
Is this tab supposed to work? All the checkboxes were always disabled.

>+    this.forgetCookies.hidden = false;
...
>+    this.forgetCookiesLabel.hidden = true;
Not sure what the point of the separate label is.

>+        for (var k=min.value; k<=max.value; k++) {
Nit: spaces, spaces, spaces!

>+    for each (let rowID in aCachedIDs) {
>+      // Find out what row this is now and if possible, add it to the selection
>+      let row = -1;
>+      for (let idx = 0; idx < dataLen; idx++) {
>+        if (aIDFunction(idx) == rowID)
Eww, you're calling aIDFunction for (rows * numIDs). Make the rows the outer loop. Better still, make aCachedIDs a hash instead of an array.

>+            permLabel.value = gDatamanBundle.getString("perm." + this.type + ".label");
>+            break;
>+          default:
>+            permLabel.value = this.type;
Why not try/catch?

>+      <property name="type">
Consider readonly="true"

>+          if (this.hasAttribute("host"))
>+            return this.getAttribute("host");
>+          return null;
Are there null-checks? If not just return this.getAttribute("host"); if there are then consider this.getAttribute("host") || null; also consider onget="..."

>+          if (this.type == "password")
>+            gLocSvc.pwd.setLoginSavingEnabled(this.host, aValue == Services.perms.ALLOW_ACTION);
Password binding should override the method.

>+                     oncommand="document.getBindingParent(this).setCapability(1);"/>
>+          <xul:radio anonid="permSetting-2" label="&perm.Block;"
>+                     oncommand="document.getBindingParent(this).setCapability(2);"/>
Use ALLOW_ACTION and DENY_ACTION.

>+    <popup id="domainTreeContextMenu"
menupopup :-(
>+      <tabpanels id="tabpanels" flex="1">
>+        <vbox id="cookiesPanel" flex="1">
Nit: Children of <tabpanels> don't need flex.

>+        <vbox id="permissionsPanel" flex="1">
>+          <richlistbox id="permList" flex="1">
>+          </richlistbox>
>+        </vbox>
Nit: <richlistbox id="permList"/>

>+	aboutData.js \
Nit: nsAboutData.js

>+<!ENTITY prefs.description "Content preferences are a way for &brandShortName; to make save of its settings, like zoom levels, specifically for a website.">
Not sure what you're trying to say here.

>+<!ENTITY forget.description "This function has not been implemented in this version. In future versions, this tab will offer to forget/delete all data for the currently selected domain.">
???

> <!ENTITY passwordExpireCmd.label "Log Out">
> <!ENTITY passwordExpireCmd.accesskey "l">
Where will this go when we lose password manager?

>+  border-bottom: 1px dotted #C0C0C0;
Bah, I bet you copied this mistake from Page Info :-(
(In reply to comment #4)
> I gave this a quick whirl on a boring profile and the most annoying behaviour
> is that when switching domain the focus should remain on the domain list.

I had done that intentionally as I had thought you always want to do something with the data when you select a domain - but I obviously missed that people might want to loop through several domains with the arrow keys...
Will remove that .focus with that (good) reason. ;-)

> One thing that Data Manager seems to be unable to do is to change permissions
> when no specific permission exists yet.

Yes, that's on my list for followups, we need some way to "add" permissions. I already have a few thoughts on that, but first I want to get the current state to be good enough to pass reviews and ensure it works for people, then I can work on more functionality. ;-)

> >+// locally loaded services
> >+var gLocSvc = {};
> Don't you use all of these services on startup anyway? In which case, no use
> loading them lazily.

Well, I didn't want to check if each and every of them is really used at startup, and I wanted to mirror what Services.jsm is doing as much as possible. Also, I'm thinking about making population of the domain list a bit lazier to help with performance when reading some of the data is slow, and then we really will only load most of them at least somewhat lazily.

> >+XPCOMUtils.defineLazyServiceGetter(gLocSvc, "cpref",
> >+                                   "@mozilla.org/content-pref/service;1",
> >+                                   "nsIContentPrefService"); // rv: >= 2.0b3 - Services.contentPrefs
> And therefore unnecessary here, right?

Yup, just wanted to keep it in in the add-on version, given that we haven't released a SeaMonkey milestone with this yet.

> >+    Services.obs.addObserver(gChangeObserver, "cookie-changed", false);
> >+    Services.obs.addObserver(gChangeObserver, "perm-changed", false);
> >+    Services.obs.addObserver(gChangeObserver, "passwordmgr-storage-changed", false);
> >+    gLocSvc.cpref.addObserver(null, gChangeObserver);
> >+    Services.obs.addObserver(gChangeObserver, "satchel-storage-changed", false);
> Presumably these are in tab order?

Yes, I thought that keeping things in the same order everywhere makes sense.

> >+      // return vars for nsIURLParser must all be objects
> >+      // see bug 568997 for improvements to that interface
> >+      var schemePos = {}, schemeLen = {}, authPos = {}, authLen = {}, pathPos = {},
> >+          pathLen = {}, usernamePos = {}, usernameLen = {}, passwordPos = {},
> >+          passwordLen = {}, hostnamePos = {}, hostnameLen = {}, port = {};
> >+      gLocSvc.url.parseURL(aHostname, -1, schemePos, schemeLen, authPos, authLen,
> >+                          pathPos, pathLen);
> >+      var auth = aHostname.substring(authPos.value, authPos.value + authLen.value);
> >+      gLocSvc.url.parseAuthority(auth, authLen.value, usernamePos, usernameLen,
> >+                                passwordPos, passwordLen, hostnamePos, hostnameLen, port);
> >+      var hostName = auth.substring(hostnamePos.value, hostnamePos.value + hostnameLen.value);
> What's this actually doing? You start with a host name, and you end up with...
> a host name?

Actually, aHostname isn't always a real host name, and that's a fast way to fix it up without generating errors, even if it's cumbersome to use from JS right now. I explained this even better to you on IRC and to others in a comment in the upcoming new patch. ;-)

> >+    this.ignoreUpdate = true;
> >+    gDatamanUtils.restoreSelectionFromIDs(this.tree, this._getObjID,
> >+                                          selectionCache);
> >+    this.ignoreUpdate = false;
> restoreSelectonFromIDs doesn't read ignoreUpdate does it?

Right, not directly, though it should end up calling select() through onselect, I guess - which now, after the change of it to not use this.ignoreUpdate, might mess up the selected tab. :(
Maybe I just need to keep that, after all... Or do you have a better idea?

> >+    let prevtab = gTabs.tabbox.selectedTab || gTabs.cookiesTab;
> >+    let stoptab = null;
> >+    while (gTabs.tabbox.selectedTab != stoptab &&
> >+           (gTabs.tabbox.selectedTab.disabled || gTabs.tabbox.selectedTab.hidden)) {
> >+      gTabs.tabbox.tabs.advanceSelectedTab(1, true);
> >+      if (!stoptab)
> >+        stoptab = prevtab;
> >+    }
> I prefer the code in xblBindings.js

That loses the idea I had that we should advance to the next non-disabled one (note that a tab cab get disabled and this triggered when we remove all data from it), but you're right, it at least is easier to understand.

> >+    for (let i = 0; i < this.domainObjects.length; i++) {
> Can we not use .filter() here?

How so? We don't want to keep two copies of the data around, IMHO, and .filter() would return another copy, potentially of the whole array.

> >+      if (this.domainObjects[i] &&
> When can that be null?

When we have removed the domain, as we only set the object to null, or else we'd need to move all the pointers in displayedDomains.

> >+          this.domainObjects[i].title.toLocaleLowerCase().indexOf(aSearchString) != -1)
> aSearchString needs to be lowercased too, no?

Good catch, thanks!

> >+    gDatamanUtils.restoreSelectionFromIDs(this.tree, this._getObjID,
> >+                                          selectionCache);
> >+    this.ignoreSelect = false;
> >+    // make sure we clear the data pane when selection has been removed
> >+    if (!this.tree.view.selection.count && selectionCache.length)
> >+      this.select();
> Haven't I seen this code block before once or tiwce?

In some (slight) variation (like that ignoreSelect on restoring), we need to do this everywhere the domain selection can be lost, and that's exactly three places, yes.

> >+      switch (this.activePanel) {
> >+        case "cookiesPanel":
> >+          gCookies.shutdown();
> >+          break;
> >+        case "permissionsPanel":
> >+          gPerms.shutdown();
> >+          break;
> >+        case "preferencesPanel":
> >+          gPrefs.shutdown();
> >+          break;
> >+        case "passwordsPanel":
> >+          gPasswords.shutdown();
> >+          break;
> >+        case "formdataPanel":
> >+          gFormdata.shutdown();
> >+          break;
> >+        case "forgetPanel":
> Nit: need some sort of panel/object map. gPanels[this.activePanel].shutdown();

Sounds elegant, but right now I'm not sure how you mean that one to be done. Can you help me there?

> >+          this.forgetTab.hidden = true;
> Doesn't that get handled by domain.select anyway?

It should be, just call me paranoid for ensuring it here. Or do you want it removed here?

> >+    document.getElementById("cookies-context-selectall").disabled =
> >+      (this.tree.view.selection.count >= this.tree.view.rowCount);
> I do hope it's never greater ;-)

Me as well, but I learned somewhere to never trust such cases if you don't have to. Should I (i.e. make that a == instead)?

> >+var cookieTreeView = {
> I'm not convinced that it makes sense having the view as a separate object.

So you think I should merge those with the main panel objects? I pondered and wasn't sure, both if I should and if it should work alright.

> >+  isSeparator: function(aIndex) { return false; },
> >+  isSorted: function() { return false; },
> >+  isContainer: function(aIndex) { return false; },
> >+  cycleHeader: function(aCol) {},
> >+  getRowProperties: function(aRow, aProp) {},
> >+  getColumnProperties: function(aColumn, aProp) {},
> >+  getCellProperties: function(aRow, aColumn, aProp) {}
> Might be worth defining these on a prototype to avoid repetition.

If I know how, I'm happy to. Can you give me pointers?

> >+var gPrefs = {
> I don't think we use content prefs so I can't really comment on this.

I actually found recently that we do, as we apparently use them to keep the last used folder for file inputs on a site.

> >+    // Compare function for two signons
> >+    let compfunc = function passwords_sort_compare(aOne, aTwo) {
> If you didn't name functions I would only have to nit you on one line ;-)

not sure if understand what you mean there (btw, naming functions is helpful in debugging, as I'm told, and as I think I also saw already).

> >+var gForget = {
> Is this tab supposed to work? All the checkboxes were always disabled.

Gah, I found that I had missed to fix up activation of them for some optimization I recently made.

> >+    this.forgetCookies.hidden = false;
> ...
> >+    this.forgetCookiesLabel.hidden = true;
> Not sure what the point of the separate label is.

Yes, because you didn't see it in work. ;-)
Actually, after a forget operation, it's used for providing feedback of what has actually been deleted.

> >+        for (var k=min.value; k<=max.value; k++) {
> Nit: spaces, spaces, spaces!

Evidently, I copied that from somewhere else. ;-)

> >+    for each (let rowID in aCachedIDs) {
> >+      // Find out what row this is now and if possible, add it to the selection
> >+      let row = -1;
> >+      for (let idx = 0; idx < dataLen; idx++) {
> >+        if (aIDFunction(idx) == rowID)
> Eww, you're calling aIDFunction for (rows * numIDs). Make the rows the outer
> loop. Better still, make aCachedIDs a hash instead of an array.

I've lost you on the hash there, how could this be one? All we can cache is the IDs (usually strings that uniquely identify each entry) of the actual data, and any of them could be at any row on restoration, esp. due to re-sorting.

> >+            permLabel.value = gDatamanBundle.getString("perm." + this.type + ".label");
> >+            break;
> >+          default:
> >+            permLabel.value = this.type;
> Why not try/catch?

Because I didn't think of it ;-)

> >+          if (this.type == "password")
> >+            gLocSvc.pwd.setLoginSavingEnabled(this.host, aValue == Services.perms.ALLOW_ACTION);
> Password binding should override the method.

> >+    <popup id="domainTreeContextMenu"
> menupopup :-(

D'oh! Sure.

> >+	aboutData.js \
> Nit: nsAboutData.js

Actually, I think the ns* prefix should probably fall on all of those - but as you like...

> >+<!ENTITY prefs.description "Content preferences are a way for &brandShortName; to make save of its settings, like zoom levels, specifically for a website.">
> Not sure what you're trying to say here.

Hmm, maybe I should make that sentence be English.

> >+<!ENTITY forget.description "This function has not been implemented in this version. In future versions, this tab will offer to forget/delete all data for the currently selected domain.">
> ???

I guess I should just remove it, as it'sa being replaced when the panel is called anyhow.

> > <!ENTITY passwordExpireCmd.label "Log Out">
> > <!ENTITY passwordExpireCmd.accesskey "l">
> Where will this go when we lose password manager?

I guess we'll need to figure that out then - we have similar cases with the entries to set various permissions directly from the menu.

> >+  border-bottom: 1px dotted #C0C0C0;
> Bah, I bet you copied this mistake from Page Info :-(

You win! ;-)
This patch should address (almost) all of Neil's comments, unless noted otherwise (unanswered questions). It's a rather large change to v1 of the patch, so I'm naming it v2 in terms of patches.
I'll also see that I port this back to the add-on repo tomorrow, but I've been sitting here for about 5h continuously just addressing those comments, I'm exhausted. ;-)

I did some additional rework, creating another global object that encapsulates the global initialize and shutdown functions and putting debug message functions in there, as well as making this debug pref available by a pref, even if that's only read on data manager load right now (should probably observe changes, but that can be done later).
I wonder if the observer and the general utils should actually be rolled into this new gDataman object, comments from the reviewers would be welcome.
Attachment #461575 - Attachment is obsolete: true
Attachment #461843 - Flags: superreview?(neil)
Attachment #461843 - Flags: review?(iann_bugzilla)
Attachment #461575 - Flags: superreview?(neil)
Attachment #461575 - Flags: review?(iann_bugzilla)
(In reply to comment #5)
> > >+    this.ignoreUpdate = true;
> > >+    gDatamanUtils.restoreSelectionFromIDs(this.tree, this._getObjID,
> > >+                                          selectionCache);
> > >+    this.ignoreUpdate = false;
> > restoreSelectonFromIDs doesn't read ignoreUpdate does it?
> Right, not directly, though it should end up calling select() through onselect,
> I guess - which now, after the change of it to not use this.ignoreUpdate, might
> mess up the selected tab. :(
> Maybe I just need to keep that, after all... Or do you have a better idea?
I'll have to double-check this on the updated patch.

> > >+    for (let i = 0; i < this.domainObjects.length; i++) {
> > Can we not use .filter() here?
> How so? We don't want to keep two copies of the data around, IMHO, and
> .filter() would return another copy, potentially of the whole array.
I was actually referring to the code in domain.search which currently pushes matching domains into this.displayedDomains; well actually I now see it currently pushes the indices of the matching domains, which is worse.

> > >+      if (this.domainObjects[i] &&
> > When can that be null?
> When we have removed the domain, as we only set the object to null, or else
> we'd need to move all the pointers in displayedDomains.
You only ever use displayedDomains to index into domainObjects. Why not put the actual objects directly into displayedDomains? This would mean gDomains.domainObjects[gDomains.displayedDomains[index]] becomes gDomains.displayedDomains[index] while the compfunc becomes aOne.title.localeCompare(aTwo.title).

I take it _getObjID uses gDomains instead of this because it's called globally?

> > >+      switch (this.activePanel) {
> > >+        case "cookiesPanel":
> > >+          gCookies.shutdown();
> > >+          break;
> > >+        case "permissionsPanel":
> > >+          gPerms.shutdown();
> > >+          break;
> > >+        case "preferencesPanel":
> > >+          gPrefs.shutdown();
> > >+          break;
> > >+        case "passwordsPanel":
> > >+          gPasswords.shutdown();
> > >+          break;
> > >+        case "formdataPanel":
> > >+          gFormdata.shutdown();
> > >+          break;
> > >+        case "forgetPanel":
> > Nit: need some sort of panel/object map. gPanels[this.activePanel].shutdown();
> 
> Sounds elegant, but right now I'm not sure how you mean that one to be done.
var gPanels = {
  cookiesPanel: gCookies,
  permissionsPanel: gPerms,
  ...
};

> > >+          this.forgetTab.hidden = true;
> > Doesn't that get handled by domain.select anyway?
> It should be, just call me paranoid for ensuring it here. Or do you want it
> removed here?
If it should be unnecessary, then it should be removed!

> > >+    document.getElementById("cookies-context-selectall").disabled =
> > >+      (this.tree.view.selection.count >= this.tree.view.rowCount);
> > I do hope it's never greater ;-)
> Me as well, but I learned somewhere to never trust such cases if you don't have
> to. Should I (i.e. make that a == instead)?
I don't really mind.

> > >+var cookieTreeView = {
> > I'm not convinced that it makes sense having the view as a separate object.
> So you think I should merge those with the main panel objects? I pondered and
> wasn't sure, both if I should and if it should work alright.
I know there are several developers who like using "tearoffs". And what do these tearoffs do? Why, they keep referencing the original object all the time. I just don't see the point. (There are some useful cases. Some functions on DOM nodes are currently defined on a separate interface. To save 4 bytes, this interface is only created when someone uses it. This doesn't apply here.)

> > >+  isSeparator: function(aIndex) { return false; },
> > >+  isSorted: function() { return false; },
> > >+  isContainer: function(aIndex) { return false; },
> > >+  cycleHeader: function(aCol) {},
> > >+  getRowProperties: function(aRow, aProp) {},
> > >+  getColumnProperties: function(aColumn, aProp) {},
> > >+  getCellProperties: function(aRow, aColumn, aProp) {}
> > Might be worth defining these on a prototype to avoid repetition.
> If I know how, I'm happy to. Can you give me pointers?
Well, it's a bit ugly if you aren't using constructors.
var baseTreeView = {
  setTree: function (aTree) {},
  ...
  isSeparator: function() { return false; },
  ...
};
domainTreeView = {
  __proto__: baseTreeView,
  get rowCount() {
    ...
  },
  getCellText: ...
};

> > >+var gPrefs = {
> > I don't think we use content prefs so I can't really comment on this.
> I actually found recently that we do, as we apparently use them to keep the
> last used folder for file inputs on a site.
Well I guess I do have one profile where I have a site where I use file inputs.

> > >+    // Compare function for two signons
> > >+    let compfunc = function passwords_sort_compare(aOne, aTwo) {
> > If you didn't name functions I would only have to nit you on one line ;-)
> not sure if understand what you mean there
Sorry, I forgot to include context showing that this wasn't comparing signons.

> > >+        for (var k=min.value; k<=max.value; k++) {
> > Nit: spaces, spaces, spaces!
> Evidently, I copied that from somewhere else. ;-)
You're good at finding errors to copy ;-)

> > >+    for each (let rowID in aCachedIDs) {
> > >+      // Find out what row this is now and if possible, add it to the selection
> > >+      let row = -1;
> > >+      for (let idx = 0; idx < dataLen; idx++) {
> > >+        if (aIDFunction(idx) == rowID)
> > Eww, you're calling aIDFunction for (rows * numIDs). Make the rows the outer
> > loop. Better still, make aCachedIDs a hash instead of an array.
> I've lost you on the hash there, how could this be one? All we can cache is the
> IDs (usually strings that uniquely identify each entry) of the actual data, and
> any of them could be at any row on restoration, esp. due to re-sorting.
You are currently looping through the array of cached IDs, then using an inner loop to check each row to see whether its ID is the current ID. An immediate improvement would be to loop through each row to see whether its id is in the array: (aCachedIDs.indexOf(aIDFunction(idx)) != -1) but the hash option would simplify this to (aIDFunction(idx) in aCachedIDs). (You would want getSelectedIDs to return null for no selection in this case.)

> > >+	aboutData.js \
> > Nit: nsAboutData.js
> Actually, I think the ns* prefix should probably fall on all of those - but as
> you like...
Well a mass rename would be a separate bug.

> > > <!ENTITY passwordExpireCmd.label "Log Out">
> > > <!ENTITY passwordExpireCmd.accesskey "l">
> > Where will this go when we lose password manager?
> I guess we'll need to figure that out then - we have similar cases with the
> entries to set various permissions directly from the menu.
Although we don't want, say, "Allow Popups from This Website" to turn into an army of clicks...
I filed bug 583567 on the strange behavior that it currently opens in the background.
This patch doesn't address any of the further comments, yet, its most important change is to add a test that can be run with:
make mochitest-browser-chrome TEST_PATH="suite/common/dataman"

That said, the patch also does some changes to make the test run through correctly, which includes some slight bug fixes (yay) and it finally does away with gPrefs and gPasswords not being in the correct order (i.e. the one of the tabs and which I'm using everywhere else when all panels are listed).
Attachment #461843 - Attachment is obsolete: true
Attachment #461936 - Flags: superreview?(neil)
Attachment #461936 - Flags: review?(iann_bugzilla)
Attachment #461843 - Flags: superreview?(neil)
Attachment #461843 - Flags: review?(iann_bugzilla)
(In reply to comment #7)
> > > >+  isSeparator: function(aIndex) { return false; },
> > > >+  isSorted: function() { return false; },
> > > >+  isContainer: function(aIndex) { return false; },
> > > >+  cycleHeader: function(aCol) {},
> > > >+  getRowProperties: function(aRow, aProp) {},
> > > >+  getColumnProperties: function(aColumn, aProp) {},
> > > >+  getCellProperties: function(aRow, aColumn, aProp) {}
> > > Might be worth defining these on a prototype to avoid repetition.
> > If I know how, I'm happy to. Can you give me pointers?
> Well, it's a bit ugly if you aren't using constructors.
Although that does gives you an excuse to use separate tree view objects.
var domainTreeView = new BaseTreeView();
domainTreeView.__defineGetter__("rowCount", function() { ... });
domainTreeView.getCellText = function DTV_getCellText ...;
(In reply to comment #7)
> (In reply to comment #5)
> You are currently looping through the array of cached IDs, then using an inner
> loop to check each row to see whether its ID is the current ID. An immediate
> improvement would be to loop through each row to see whether its id is in the
> array: (aCachedIDs.indexOf(aIDFunction(idx)) != -1)
Which I just noticed you've already done, so that's much better.
Comment on attachment 461843 [details] [diff] [review]
v2: address (almost) all of Neil's comments

>+  domainObjects: [],
This should be {} now, I think. (I thought you weren't able to enumerate named properties on arrays, but maybe that was changed ages ago.)

>+          <description id="forgetDesc"></description>
[What does this do? If you need it, <description id="forgetDesc"/>]
Comment on attachment 461936 [details] [diff] [review]
v2.1: slight adaptations, add a test

>+    setTimeout(function() {
...
>+    }, 0);
>+
>+    // Send a notification that we finished
>+    setTimeout(function() {
>+      gDataman.debugMsg("Domain list built: " + Date.now()/1000);
>+      Services.obs.notifyObservers(window, "dataman-loaded", null);
>+    }, 0);
A bunch of setTimeouts isn't really ideal. Unfortunately chaning setTimeout gets awkward, although you could name your functions. e.g.
function loadCookies() {
  ...
  setTimeout(loadPerms, 0);
}
...
function loadPasswords() {
  ...
  setTimeout(notifyObservers, 0);
}
An advanced technique uses a generator:
var loaderInstance;
function nextStep() {
  loaderInstance.next();
}
function loader() {
  // load cookies
  yield setTimeout(nextStep, 0);
  ...
  yield setTimeout(nextStep, 0);
  // notify observers
  yield;
}
loaderInstance = loader();
setTimeout(nextStep, 0);
Forget still seems to have a few rough edges.

After forgetting all about a site, when I selected another site, the forget results didn't immediately go away. After choosing to forget, instead of seeing the forget screen I got the results again. After switching site again the forget pane did reappear, but the tab shows as disabled.
(In reply to comment #5)
> (In reply to comment #4)
> > >+      if (this.domainObjects[i] &&
> > When can that be null?
> When we have removed the domain, as we only set the object to null, or else
> we'd need to move all the pointers in displayedDomains.
But you immediately recreate displayedDomains by calling search() anyway.
Comment on attachment 461936 [details] [diff] [review]
v2.1: slight adaptations, add a test

>+          gTabs.tabbox.selectedTab = gTabs.tabbox.tabs.childNodes[i];
Could write this as gTabs.tabbox.selectedIndex = i;

>+    forgetCtx.label = (this.selectedDomain.title == "*") ?
Nit: ()s not actually necessary here and in a few other places.

>+        permElem.setAttribute("orient", "vertical");
Could use -moz-box-orient: vertical; in the CSS instead.

>+            selections[selections.length] = k;
Could write this as selections.push(k);

>+                     oncommand="document.getBindingParent(this).setCapability(Components.interfaces.nsICookiePermission.ACCESS_SESSION);"/>
Ah, I knew it was there somewhere :-)
(In reply to comment #7)
> I was actually referring to the code in domain.search which currently pushes
> matching domains into this.displayedDomains; well actually I now see it
> currently pushes the indices of the matching domains, which is worse.

Why worse? I'd say more efficient.

> > > >+      if (this.domainObjects[i] &&
> > > When can that be null?
> > When we have removed the domain, as we only set the object to null, or else
> > we'd need to move all the pointers in displayedDomains.
> You only ever use displayedDomains to index into domainObjects. Why not put the
> actual objects directly into displayedDomains?

Because I want to avoid copying the objects around to different places, wasting memory needlessly and needing to fix up their flags in two places.

> I take it _getObjID uses gDomains instead of this because it's called globally?

Yes, due to it being called from the utils via the selection caching functions. I got errors without that.

> var gPanels = {
>   cookiesPanel: gCookies,
>   permissionsPanel: gPerms,
>   ...
> };

ah, and that references those object but doesn't copy them? I'm always unsure with things like that...

> > > >+          this.forgetTab.hidden = true;
> > > Doesn't that get handled by domain.select anyway?
> > It should be, just call me paranoid for ensuring it here. Or do you want it
> > removed here?
> If it should be unnecessary, then it should be removed!

As you might have noticed, I try to stay of the safe side where possible - this one has died in the new patch, though.

> Well I guess I do have one profile where I have a site where I use file inputs.

One where you're using Bugzilla? :P

> Although we don't want, say, "Allow Popups from This Website" to turn into an
> army of clicks...

IMHO, that one is too many clicks as it is right now. ;-)

(In reply to comment #12)
> >+          <description id="forgetDesc"></description>
> [What does this do? If you need it, <description id="forgetDesc"/>]

Good catch. I need it to display the head line of the forget panel - which changes from pre-forget to post-forget.

(In reply to comment #15)
> But you immediately recreate displayedDomains by calling search() anyway.

How does that matter to what's in domainObjects? I actually don't even know how to delete a member of an object, so I'm still nulling it out.

(In reply to comment #16)
> >+    forgetCtx.label = (this.selectedDomain.title == "*") ?
> Nit: ()s not actually necessary here and in a few other places.

Thanks, one more thing where I tend to err on the safe side. Please don't make me look up every single "(" now and check if it's really needed... ;-)

> >+                     oncommand="document.getBindingParent(this).setCapability(Components.interfaces.nsICookiePermission.ACCESS_SESSION);"/>
> Ah, I knew it was there somewhere :-)

Yes, as ugly as that reference is, 8 is actually that one. :)


I'll also look into the generator and the forget problem you mentioned, thanks for all that help!
(In reply to comment #14)
> Forget still seems to have a few rough edges.

OK, one part of this is that we didn't mark the tab enabled/disabled correctly, which made our "select first enabled tab" mechanism not kick in correctly on a domain change. The other part seems to have been that the resetting of the elements at shutdown of the forget panel didn't work correctly, I switched to making sure of the states at init instead.
OK, here's a patch that should address all of Neil's comments and those things I've been stating that I think I should also do along with that.

Due to the rather large rework contained in this instance, bumping patch version to v3.
Attachment #461936 - Attachment is obsolete: true
Attachment #463366 - Flags: superreview?(neil)
Attachment #463366 - Flags: review?(iann_bugzilla)
Attachment #461936 - Flags: superreview?(neil)
Attachment #461936 - Flags: review?(iann_bugzilla)
(In reply to comment #17)
> (In reply to comment #7)
> > var gPanels = {
> >   cookiesPanel: gCookies,
> >   permissionsPanel: gPerms,
> >   ...
> > };
> ah, and that references those object but doesn't copy them? I'm always unsure
> with things like that...
Like Java, JavaScript uses the reference model (except for primitive types). So if you write x = []; y = x; y.push(null); that changes x to [null]. (There are ways to copy an array such as y = [].concat(x); which avoids this, but in general there's no way to copy an object.)

> > Well I guess I do have one profile where I have a site where I use file inputs.
> One where you're using Bugzilla? :P
That was too obvious ;-)

> > Although we don't want, say, "Allow Popups from This Website" to turn into an
> > army of clicks...
> IMHO, that one is too many clicks as it is right now. ;-)
Well, three isn't too bad, I guess.


> (In reply to comment #15)
> > But you immediately recreate displayedDomains by calling search() anyway.
> How does that matter to what's in domainObjects?
My understanding is that you were worried that deleting from domainObjects would invalidate displayedDomains.

> I actually don't even know how
> to delete a member of an object, so I'm still nulling it out.
To delete a property p of an object, use delete object[p]; (delete object.p works if you know what p is in advance and it is a legal variable name.)
To delete an index i of an array, use array.splice(i, 1);
Comment on attachment 463366 [details] [diff] [review]
v3: address Neil's comments, do some reworking

>+              <rows>
>+
>+                <row align="center">
[Spacing between rows I can understand, but spacing before the first row and after the last row looks odd.]

>+                  <hbox align="center" pack="end">
The row is already centred, no need to align the box too.

>+                    <label value="&cookies.info.name.label;" control="cookieInfoName"/>
Alternatively, you could arrange to have text-align: right; CSS somehow, thus avoiding the <hbox> completely.

>+          <hbox>
>+            <button id="cookieRemove" disabled="true"
>+                    label="&cookies.button.remove.label;"
>+                    accesskey="&cookies.button.remove.accesskey;"
>+                    oncommand="gCookies.delete();"/>
>+            <checkbox id="cookieBlockOnRemove"
>+                      label="&cookies.blockOnRemove.label;"
>+                      accesskey="&cookies.blockOnRemove.accesskey;"
>+                      persist="checked"/>
Might be worth an align="center" on this box, in case other themes don't.

>+          <hbox id="passwordButtons">
Any reason some of these boxes have ids and others don't?
Comment on attachment 463366 [details] [diff] [review]
v3: address Neil's comments, do some reworking

>+      let min = new Object();
>+      let max = new Object();
Nit: could use {} instead of new Object()

>+            selections.push(k)
Nit: missing ;

>+        aTree.view.selection.rangedSelect(row, row, true);
[Could use toggleSelect]

>+      this.domainObjects[aDomain] = null;
delete this.domainObjects[aDomain];

>+    for (let domain in this.domainObjects) {
>+      if (this.domainObjects[domain] &&
... so you don't need this test.

>+          domain.toLocaleLowerCase().indexOf(lcSearch) != -1)
>+        this.displayedDomains.push(domain);
If you push this.domainObjects[domain] then you don't have to keep looking up this.displayedDomains[this.domainObjects[domain]] - this.domainObjects[domain] is already the correct object.

>+    for (let i = 0; i < this.cookies.length; i++) {
>+      if (this.cookies[i] &&
>+          gDomains.hostMatchesSelected(this.cookies[i].rawHost))
>+        this.displayedCookies.push(i);
If you stored the cookie objects directly in displayedCookies then you could actually use filter to build displayedCookies from cookies.

>+          this.cookies[idx] = null;
Use this.cookies.splice(idx, 1);
Then you don't need to null-check cookies above.

Also once you've found the cookie to operate on, you can use indexOf to see if it exists in displayedCookies.

>+  prefs: [],
>+  displayedPrefs: [],
Making displayedPrefs an array of 0 .. N looks particularly odd, when you could just use prefs directly.
(In reply to comment #19)
> Created attachment 463366 [details] [diff] [review]
> v3: address Neil's comments, do some reworking
> 
> OK, here's a patch that should address all of Neil's comments and those things
> I've been stating that I think I should also do along with that.
> 
> Due to the rather large rework contained in this instance, bumping patch
> version to v3.

This patch has now bitrotted in modern's jar.mn
(In reply to comment #23)
> This patch has now bitrotted in modern's jar.mn

Due to places bookmarks, build bustages and other craziness in the recent days, I haven't gotten around to any Data Manager work, will come back to it once I've worked through the list of places bookmarks post-landing reviews and a few other bugs, and will attach a new patch once I addressed the further reviews from Neil in this bug. The bitrotting shouldn't be much and trvial to solve, though, I'd guess it's also related to my places bookmarks stuff.
(In reply to comment #21)
> >+                  <hbox align="center" pack="end">
> The row is already centred, no need to align the box too.
> 
> >+                    <label value="&cookies.info.name.label;" control="cookieInfoName"/>
> Alternatively, you could arrange to have text-align: right; CSS somehow, thus
> avoiding the <hbox> completely.

I tried but found no way to do this, sorry. I would like it as well (this comes pretty directly from the current cookie viewer, btw).

> >+          <hbox id="passwordButtons">
> Any reason some of these boxes have ids and others don't?

I probably should add an ID for all of them, just to make it easier for someone to extend this, e.g. via another add-on.

New patch upcoming - I just need to finish work to use .filter(). ;-)
This patch should address all recent comments, the largest change is to do away with all the index pointers completely and use the objects directly in displayed* where needed (prefs just sort the main array).
Spot-check testing looks good and the included test reports 131 PASS and 0 FAIL or TODO, so I guess everything works as expected even after those changes. :)
Attachment #463366 - Attachment is obsolete: true
Attachment #467014 - Flags: superreview?(neil)
Attachment #467014 - Flags: review?(iann_bugzilla)
Attachment #463366 - Flags: superreview?(neil)
Attachment #463366 - Flags: review?(iann_bugzilla)
Alias: DataManager
Blocks: 588415
Blocks: 588417
Blocks: 588418
Blocks: 588419
Blocks: 588421
Blocks: 588422
Comment on attachment 467014 [details] [diff] [review]
v4: remove index pointers, address more comments from Neil

>+    // Find out wwhich current rows match a cached selection and add them to the selection.
Typo: which

>+    var selectionCache = gDataman.getSelectedIDs(this.tree, this._getObjID);
>+    this.tree.view.selection.clearSelection();
>+    gDataman.debugMsg("removed flag " + aFlag + " from " + aDomain);
>+    this.domainObjects[aDomain][aFlag] = false;
>+    if (!this.domainObjects[aDomain].hasCookies &&
>+        !this.domainObjects[aDomain].hasPermissions &&
>+        !this.domainObjects[aDomain].hasPreferences &&
>+        !this.domainObjects[aDomain].hasPasswords &&
>+        !this.domainObjects[aDomain].hasFormData) {
>+      gDataman.debugMsg("removed domain: " + aDomain);
>+      delete this.domainObjects[aDomain];
>+      this.search(this.searchfield.value);
Ideally you would work out which row corresponds to the domain object and delete that row from the view. If that's not straightforward, you should at least avoid rebuilding the selection unless you're deleting the domain. You might find it easier to do an early return in the tab state update case.

>+      this.select({aNoTabSelect: true});
You don't actually use this as an object, so this.select(true); works.

>+    this.tree.view.selection.clearSelection();
>+    this.tree.treeBoxObject.beginUpdateBatch();
...
>+    this.tree.treeBoxObject.endUpdateBatch();
>+    gDataman.restoreSelectionFromIDs(this.tree, this._getObjID, selectionCache);
Nit: slightly better to nest the selection changes inside the batch too.

>+    this.tree.treeBoxObject.endUpdateBatch();
>+    this.tree.treeBoxObject.invalidate();
Nit: endUpdateBatch does an invalidate anyway.

>+      (this.tree.view.selection.count >= this.tree.view.rowCount);
Nit: don't need the ()s

>+    // Usual notifications for added, changed, deleted - do "surgical" updates.
>+    aSubject.QueryInterface(Components.interfaces.nsICookie2);
>+    let domain = gDomains.getDomainFromHost(aSubject.rawHost);
>+    // Does change affect possibly loaded Cookies pane?
>+    let affectsLoaded = this.displayedCookies.length &&
>+                        gDomains.hostMatchesSelected(aSubject.rawHost);
>+    if (aData == "added") {
>+      this.cookies.push(this._makeCookieObject(aSubject));
Now, the potential problem I see here, is that we don't know whether the list has been loaded yet, since that only happens the first time the cookie tab is shown, but the observer is added when the window loads.

>+      let idx = 1; disp_idx = -1; domainCookies = 0;
Did you mean to use commas?

>+          let cookie = this.displayedCookies[i];
>+          if (cookie && cookie.host == aSubject.host &&
>+              cookie.name == aSubject.name && cookie.path == aSubject.path) {
Nit: shouldn't need to check cookie && (x2)

>+            idx = this.cookies.indexOf(this.displayedCookies[i]); disp_idx = i;
Two lines please? (Also happens later on somewhere.)

>+    this.allSignons = [];
>+    this.allSignons = gLocSvc.pwd.getAllLogins();
First line is pointless!
Blocks: 591324
(In reply to comment #27)
> >+      this.select({aNoTabSelect: true});
> You don't actually use this as an object, so this.select(true); works.

I know it *works* but I did it that way specifically as I was not sure if we'd add other options and "true" alone is quite unintuitive as to what it actually does.

> >+    this.tree.view.selection.clearSelection();
> >+    this.tree.treeBoxObject.beginUpdateBatch();
> ...
> >+    this.tree.treeBoxObject.endUpdateBatch();
> >+    gDataman.restoreSelectionFromIDs(this.tree, this._getObjID, selectionCache);
> Nit: slightly better to nest the selection changes inside the batch too.

In such cases, could you *please* give me context of where you mean this, as those statements are used in all kinds of places and it's hard to see which ones I should look into, search is useless for those.

> >+    this.tree.treeBoxObject.endUpdateBatch();
> >+    this.tree.treeBoxObject.invalidate();
> Nit: endUpdateBatch does an invalidate anyway.

Same here.

I tried to follow those I found, not sure if I caught everything, it's hard with such unspecific comments.

> Now, the potential problem I see here, is that we don't know whether the list
> has been loaded yet, since that only happens the first time the cookie tab is
> shown, but the observer is added when the window loads.

Er, the list is shown at initialization of the whole Data Manager, isn't it?
Also, if it's not there, what problem does it generate?

> >+      let idx = 1; disp_idx = -1; domainCookies = 0;
> Did you mean to use commas?

Heh, yes, good catch!
OK, this patch addresses the latest of the neverending rounds of comments. ;-)

The test needed rather larger changes, though, due to bug 546857 adding a large number of default permissions to the test profiles.
Attachment #467014 - Attachment is obsolete: true
Attachment #469935 - Flags: superreview?(neil)
Attachment #469935 - Flags: review?(iann_bugzilla)
Attachment #467014 - Flags: superreview?(neil)
Attachment #467014 - Flags: review?(iann_bugzilla)
(In reply to comment #28)
> (In reply to comment #27)
> > >+      this.select({aNoTabSelect: true});
> > You don't actually use this as an object, so this.select(true); works.
> I know it *works* but I did it that way specifically as I was not sure if we'd
> add other options and "true" alone is quite unintuitive as to what it actually
> does.
Maybe to make it clear you should test the aNoTabSelect property too. (And maybe you should call it noTabSelect, since the a prefix is for arguments.)

> > Nit: slightly better to nest the selection changes inside the batch too.
> In such cases, could you *please* give me context of where you mean this, as
> those statements are used in all kinds of places and it's hard to see which
> ones I should look into, search is useless for those.
All of them? ;-)
In fact I notice that even in the latest patch you clear the selection before beginning the batch in four places:
gDomains.search
gPasswords.reactToChange
gFormdata.search
gFormdata.reactToChange
While on the subject of batching, it occurs to me that your initialize methods for gCookies, gPrefs and gPasswords could be optimised by setting the view after loading the data in to the array; this makes the batch unnecessary.

> > Now, the potential problem I see here, is that we don't know whether the list
> > has been loaded yet, since that only happens the first time the cookie tab is
> > shown, but the observer is added when the window loads.
> Er, the list is shown at initialization of the whole Data Manager, isn't it?
> Also, if it's not there, what problem does it generate?
As far as I can tell, gCookies.initialize is first called by gTabs.select when you first view the cookies tab. Until then, gCookies isn't initialised; in particular gCookies.cookies is empty.

Unfortunately the cookie-changed observer is added by gDataman.initialize, so that if a cookie arrives after gDataman adds the cookie-changed observer, it will be added to gCookies.cookies whose length will no longer be zero. Then when you do switch to the cookies tab, it won't show your existing cookies.
Comment on attachment 469935 [details] [diff] [review]
v4.1: address latest comments from Neil, adjust test

I unfortunately managed to crash Data Manager. I was looking to see whether you had implemented the STS permissions yet (you have not given them labels yet). Steps to reproduce crash:
1. Open Data Manager to the permissions for the mozilla.org domain
2. Visit Bugzilla in another tab
Note: I had previously visited Bugzilla; I don't know if that was necessary.
This results in a stack overflow, as for some reason the permission observer tries to set the permission that it was observing.
I don't know why this doesn't crash if I change, say, image permissions.
I tried adding STS support to page info (is there a bug yet for adding password/xulxbl/sts permissions to page info?) and that didn't crash.
So I'm confused.

>+  getDefault: function permissions_getDefault(aType) {
...
>+    return false;
Not sure that this is a useful return value from this function.
Attachment #469935 - Flags: superreview?(neil) → superreview-
Bah, it doesn't crash my debug build...
I eventually managed to get it to crash my symbols build. But I accidentally trashed the process state trying to debug the crash :-(
I managed to trigger it just interacting with about:data and not even opening a page in another window or tab. This is the JS stack before it overflowed:
0 [native frame]
1 setCapability(aValue = 2) ["chrome://communicator/content/dataman/dataman.xml":115]
    radio = [object XULElement @ 0x829d740 (native @ 0x8230f40)]
    this = [object XULElement @ 0x829c4b0 (native @ 0x79d00c0)]
2 permissions_reactToChange(aData = "changed", aSubject = [xpconnect wrapped nsIPermission @ 0x7d77a40 (native @ 0x79541d0)]) ["chrome://communicator/content/dataman/dataman.js":1215]
    this = [object Object]
3 co_observe(aData = "changed", aTopic = "perm-changed", aSubject = [xpconnect wrapped nsIPermission @ 0x7d77a40 (native @ 0x79541d0)]) ["chrome://communicator/content/dataman/dataman.js":118]
    this = [object Object]
4 [native frame]
...
268 [native frame]
269 setCapability(aValue = 2) ["chrome://communicator/content/dataman/dataman.xml":115]
    radio = [object XULElement @ 0x829d740 (native @ 0x8230f40)]
    this = [object XULElement @ 0x829c4b0 (native @ 0x79d00c0)]
270 permissions_reactToChange(aData = "changed", aSubject = [xpconnect wrapped nsIPermission @ 0x82a9870 (native @ 0x79518c0)]) ["chrome://communicator/content/dataman/dataman.js":1215]
    this = [object Object]
271 co_observe(aData = "changed", aTopic = "perm-changed", aSubject = [xpconnect wrapped nsIPermission @ 0x82a9870 (native @ 0x79518c0)]) ["chrome://communicator/content/dataman/dataman.js":118]
    this = [object Object]
272 [native frame]
273 setCapability(aValue = 2) ["chrome://communicator/content/dataman/dataman.xml":115]
    radio = [object XULElement @ 0x829d740 (native @ 0x8230f40)]
    this = [object XULElement @ 0x829c4b0 (native @ 0x79d00c0)]
274 oncommand(event = [object XULCommandEvent @ 0x82a9df0 (native @ 0x8484910)]) ["about:data":1]
    this = [object XULElement @ 0x829d740 (native @ 0x8230f40)]
(In reply to comment #34)
> This is the JS stack before it overflowed

As I have no idea how JS code can cause a crash or how to fix this, I'm suspending Data Manager work indefinitely until someone can tell me how to fix this and what's wrong in the first place.
Comment on attachment 469935 [details] [diff] [review]
v4.1: address latest comments from Neil, adjust test

OK, so there are a combination of things wrong:

1. Simply observing permission changes causes the bindings to try to set permissions, since the observer calls setCapability which calls perms.add
2. Permission manager now supports session and timed permissions. But the datamanager permission bindings only try to set permanent permissions.
3. Permission manager forgets to update its internal permission duration. So when it fires the permission change for the permission that we just observed, and we try to readd the permission, it sees it as another change.

So at the very least we need to make it so that setCapability doesn't call perms.add unless it's in response to an actual user interaction.
(In reply to comment #30)
> As far as I can tell, gCookies.initialize is first called by gTabs.select when
> you first view the cookies tab. Until then, gCookies isn't initialised; in
> particular gCookies.cookies is empty.

Not entirely true. gCookies.loadList() is setting gCookies.cookies and is in the first loadInstance to be called in gDomains.initizalize(), which is called just after adding the observers. So, all in all, the potential window for observing a change with not having .cookies filled is quite small in reality.

> Unfortunately the cookie-changed observer is added by gDataman.initialize, so
> that if a cookie arrives after gDataman adds the cookie-changed observer, it
> will be added to gCookies.cookies whose length will no longer be zero. Then
> when you do switch to the cookies tab, it won't show your existing cookies.

(In reply to comment #31)
> >+  getDefault: function permissions_getDefault(aType) {
> ...
> >+    return false;
> Not sure that this is a useful return value from this function.

I'm also not 100% sure what value is best there, but I'd prefer to return *something* as we can be called with an aType not in the switch and the caller expects a return value. And IIRC, with returning something _not_ equaling a known permission value, we should end up not having any radio bullet selected when the item for such an "unknown" aType ends up being set to default. At least that's the intention here.

(In reply to comment #36)
> OK, so there are a combination of things wrong:
> 
> 1. Simply observing permission changes causes the bindings to try to set
> permissions, since the observer calls setCapability which calls perms.add

Right, that's not ideal. Should be fixed in the new upcoming patch.

> 2. Permission manager now supports session and timed permissions. But the
> datamanager permission bindings only try to set permanent permissions.

Hmm, we might want to add support for this, but I'd prefer to do that in a followup. Care to file it? Are the interfaces for dealing with session/timed stuff documented somewhere?

> 3. Permission manager forgets to update its internal permission duration. So
> when it fires the permission change for the permission that we just observed,
> and we try to readd the permission, it sees it as another change.

That's obviously a permission backend bug, which you have already filed. Thanks for that.
Here's the patch that updates to the recent comments, including the permission set loop that caused Neil's crash.

I also tested the case where the "false" return value from permissions_getDefault() comes into play, i.e. an "unknown" permission type in comparison to a known one, and it acts correctly in the UI - the known one sets the disabled radio to have the default value selected, while the "unknown" one sets no selected radio item, which is exactly the intended behavior.
Attachment #469935 - Attachment is obsolete: true
Attachment #472153 - Flags: superreview?(neil)
Attachment #472153 - Flags: review?(iann_bugzilla)
Attachment #469935 - Flags: review?(iann_bugzilla)
(In reply to comment #37)
> Not entirely true. gCookies.loadList() is setting gCookies.cookies and is in
> the first loadInstance to be called in gDomains.initizalize(), which is called
> just after adding the observers. So, all in all, the potential window for
> observing a change with not having .cookies filled is quite small in reality.
So does gCookies.initialize() still need to call loadList()?

> (In reply to comment #31)
> > >+  getDefault: function permissions_getDefault(aType) {
> > ...
> > >+    return false;
> > Not sure that this is a useful return value from this function.
> I'm also not 100% sure what value is best there, but I'd prefer to return
> *something* as we can be called with an aType not in the switch and the caller
> expects a return value. And IIRC, with returning something _not_ equaling a
> known permission value, we should end up not having any radio bullet selected
> when the item for such an "unknown" aType ends up being set to default. At
> least that's the intention here.
How about Services.perms.UNKNOWN_ACTION ?

> > 2. Permission manager now supports session and timed permissions. But the
> > datamanager permission bindings only try to set permanent permissions.
> Hmm, we might want to add support for this, but I'd prefer to do that in a
> followup. Care to file it? Are the interfaces for dealing with session/timed
> stuff documented somewhere?
Followup is fine. The only documentation I know is nsIPermissionManager.idl but I didn't look on MDC nor did I look for dev-doc-* flags in Bugzilla.
(From update of attachment 472153 [details] [diff] [review])
>+        if (affectsLoaded) {
>+          permElem.setCapability(Services.perms.ALLOW_ACTION, true);
>+        }
Nice. (Interdiff makes this clearer!)

One thing that occurred to me is that you seem to have more callers doing ui updates only than callers actually wanting to change the permissions. Would it therefore make sense to have the sense of the optional argument reversed?
(In reply to comment #39)
> So does gCookies.initialize() still need to call loadList()?

I'd like to leave those calls in as a safety measure, esp. as we only do so there when there is no loaded list yet.

> How about Services.perms.UNKNOWN_ACTION ?

Applied locally, but will not attach a new patch just for that, I'll rather wait for Ian's review comments (if you left him anything to comment on). ;-)

> Followup is fine. The only documentation I know is nsIPermissionManager.idl but
> I didn't look on MDC nor did I look for dev-doc-* flags in Bugzilla.

OK, let's look for that at that time, then.

(In reply to comment #40)
> One thing that occurred to me is that you seem to have more callers doing ui
> updates only than callers actually wanting to change the permissions. Would it
> therefore make sense to have the sense of the optional argument reversed?

I think it's more logical to have the default case be to set the permissions (i.e. keep the saved state in sync with the UI) and have the case of only updating the UI as the special case - and the difference of the usage counts isn't that high while all callers that only want to update the UI are confined in reactToChange() from what I see.

Anything else left open?

That said, thanks a lot for all the comments, they have tremendously improved this feature, which is probably the largest thing so far I really have written myself for the most part. :)
(In reply to comment #40)
> One thing that occurred to me is that you seem to have more callers doing ui
> updates only than callers actually wanting to change the permissions.
Next time I should actually count the callers ;-) I forgot that you needed to have multiple bindings, each with its own calls to setCapability.
Comment on attachment 472153 [details] [diff] [review]
v4.2: address more comments from Neil

(In reply to comment #41)
> (In reply to comment #39)
> > So does gCookies.initialize() still need to call loadList()?
> I'd like to leave those calls in as a safety measure, esp. as we only do so
> there when there is no loaded list yet.
But as per comment #30 it doesn't work as a safety measure, because it fails as soon as you get a notification. So I'd rather see this removed.
Attachment #472153 - Flags: superreview?(neil) → superreview+
With reference to bug 546857 comment 198, how easy would it be to have DM be able to add permission "allowXULXBL"?
(In reply to comment #44)
> With reference to bug 546857 comment 198, how easy would it be to have DM be
> able to add permission "allowXULXBL"?

The current version already displays that permission and lets you modify it when set, but adding any permissions is up to followup work in bug 588417 - this one will surely be in the default set for this.
Comment on attachment 472153 [details] [diff] [review]
v4.2: address more comments from Neil

Just some general testing:
Opening "Data Manager" itself I get in the error console 6 messages all saying:
Warning: reference to undefined property this.treeBoxObject.view
Source File: chrome://global/content/bindings/tree.xml
Line: 0

Also when navigating between domains in the list I get the above but also:
Warning: reference to undefined property b.view
Source File: chrome://global/content/bindings/tree.xml
Line: 1022

Selecting "Forget About This Domain", I get a new tab but if I do not check a box and then click on "Forget This Data", I get the message:
All data associated with the domain "<domain>" of the following types has been deleted:
Maybe we should present a message along the lines of:
You decided not to forget any data for the domain "<domain>"

With "Forget Global Data", in the Forget tab, it would be useful to have, an "All Cookies" option, "All Passwords" option and ones for the various types of permissions e.g. images, popups, software, etc

You have a picker next to "Domain", not sure it is really useful being there. What would be more useful is clicking on "Domain" changing the sort order (which I guess is what it is supposed to do but doesn't).

When you delete the last cookie out of a tab, when you bring up the context menu, you get in the error console:
Error: (void 0) is undefined
Source File: chrome://communicator/content/dataman/dataman.js
Line: 880
and it still has "Remove" enabled. You should deal with what happens when a tab no longer has data (switching to another tab that does or down to the next domain in the list?). Selecting "Remove" from the context menu gives in the error console:
Error: (void 0) is undefined
Source File: chrome://communicator/content/dataman/dataman.js
Line: 880
but after that the "Remove" menu item is disabled.

When you use the delete key to remove cookies, the next cookie in the list is not selected and you can not use the delete key until you select another cookie.

We don't seem to display permissions for "Cookie Websites" (Allow, Block, Session).

r- just on testing at the moment.
Attachment #472153 - Flags: review?(iann_bugzilla) → review-
Using build built from http://hg.mozilla.org/mozilla-central/rev/508d0a50827c I get, as discussed on IRC, when running the tests:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | The active tab is not the Data Manager - Didn't expect dataman-page, but got it

Also need to correct message in the isnot to "The active tab is the Data Manager" so that it makes sense.
(In reply to comment #47)
> Also need to correct message in the isnot to "The active tab is the Data
> Manager" so that it makes sense.

No, the message is correct, as the expected case there is that it's not the Data Manager, and messages are usually for the expect case, not the failure case.
I think I have a local test fix though that clears this one up, we have a behavioral change due to the "reuse empty tabs" patch that landed recently.

For the errors you have been seeing, I suspect that at least some of that is because JaegerMonkey still has some flaws and as Data Manager runs inside a tab, it gets run with that enabled in your build and newer ones.
Could you try if those errors go away if you set javascript.options.methodjit.content to false?
(In reply to comment #46)
> Just some general testing:
> Opening "Data Manager" itself I get in the error console 6 messages all saying:
> Warning: reference to undefined property this.treeBoxObject.view
> Source File: chrome://global/content/bindings/tree.xml
> Line: 0
> 
> Also when navigating between domains in the list I get the above but also:
> Warning: reference to undefined property b.view
> Source File: chrome://global/content/bindings/tree.xml
> Line: 1022

I can't reproduce these, but they somehow sound like something was busted in your build, I hope it's gone with a newer one.

> Selecting "Forget About This Domain", I get a new tab but if I do not check a
> box and then click on "Forget This Data", I get the message:
> All data associated with the domain "<domain>" of the following types has been
> deleted:
> Maybe we should present a message along the lines of:
> You decided not to forget any data for the domain "<domain>"

I think we just shouldn't activate the button when it would do nothing. Solved in new patch.

> With "Forget Global Data", in the Forget tab, it would be useful to have, an
> "All Cookies" option, "All Passwords" option and ones for the various types of
> permissions e.g. images, popups, software, etc

I have been thinking about this on and off since I created this function. I think that might be a good idea, but should into a followup.

> You have a picker next to "Domain", not sure it is really useful being there.
> What would be more useful is clicking on "Domain" changing the sort order
> (which I guess is what it is supposed to do but doesn't).

Just forgot to hide the column picker. Done in new patch.

> When you delete the last cookie out of a tab, when you bring up the context
> menu, you get in the error console:
> Error: (void 0) is undefined

That's somewhat strange, apparently we can end up with having a non-zero selection.length when the tree has no rows. I prevented our problems by dealing with this in the helper functions for getting the selections.

> When you use the delete key to remove cookies, the next cookie in the list is
> not selected and you can not use the delete key until you select another
> cookie.

Hmm, interesting idea, might really be helpful. Can I pass that into a followup, please?

> We don't seem to display permissions for "Cookie Websites" (Allow, Block,
> Session).

We do, but apparently not for setting new ones, just for those that have already been set for some time. I guess this is one more example of the new flags or whatever they are on permission in effect. I really would have liked to deal with that change in a followup. What do you think?
Here's the new patch I've been talking about in the last comment. This also addresses Ian's problems when running the test (and tests the new forget button behavior) and contains the fixes to Neil's last comments.
Attachment #472153 - Attachment is obsolete: true
Attachment #476143 - Flags: superreview+
Attachment #476143 - Flags: review?(iann_bugzilla)
Version: unspecified → Trunk
(In reply to comment #49)
> (In reply to comment #46)
> > Just some general testing:
> > Opening "Data Manager" itself I get in the error console 6 messages all saying:
> > Warning: reference to undefined property this.treeBoxObject.view
> > Source File: chrome://global/content/bindings/tree.xml
> > Line: 0
> > 
> > Also when navigating between domains in the list I get the above but also:
> > Warning: reference to undefined property b.view
> > Source File: chrome://global/content/bindings/tree.xml
> > Line: 1022
> 
> I can't reproduce these, but they somehow sound like something was busted in
> your build, I hope it's gone with a newer one.
>
I have strict javascript warnings switched on in my preferences, switching off removes these messages.
(In reply to comment #49)
> (In reply to comment #46)
> > With "Forget Global Data", in the Forget tab, it would be useful to have, an
> > "All Cookies" option, "All Passwords" option and ones for the various types of
> > permissions e.g. images, popups, software, etc
> 
> I have been thinking about this on and off since I created this function. I
> think that might be a good idea, but should into a followup.
Agreed.

> > You have a picker next to "Domain", not sure it is really useful being there.
> > What would be more useful is clicking on "Domain" changing the sort order
> > (which I guess is what it is supposed to do but doesn't).
> 
> Just forgot to hide the column picker. Done in new patch.
Sort order still does not work though.

> > When you use the delete key to remove cookies, the next cookie in the list is
> > not selected and you can not use the delete key until you select another
> > cookie.
> 
> Hmm, interesting idea, might really be helpful. Can I pass that into a
> followup, please?
I guess so, but less obvious candidate than first one.

> > We don't seem to display permissions for "Cookie Websites" (Allow, Block,
> > Session).
> 
> We do, but apparently not for setting new ones, just for those that have
> already been set for some time. I guess this is one more example of the new
> flags or whatever they are on permission in effect. I really would have liked
> to deal with that change in a followup. What do you think?
It shows up immediately in Data Manager when you do it from the Cookie Manager, so when doing it within Data Manager I'd expect the same. Perhaps being added in such a way that it bypasses the trigger used to capture it when it happens in Cookie Manager?

More testing:
If Data Manager is open in another tab and you load a web page you get a number of messages appearing in the error console:
Error: this.list is null
Source File: chrome://communicator/content/dataman/dataman.js
Line: 1177
(In reply to comment #52)
> More testing:
> If Data Manager is open in another tab and you load a web page you get a number
> of messages appearing in the error console:
> Error: this.list is null
> Source File: chrome://communicator/content/dataman/dataman.js
> Line: 1177
This only happens if you have deleted the last item out of a domain's cookie tab so all tabs are disabled and no domain selected, then load something in a new tab, so a bit of an edge case.
(In reply to comment #52)
> (In reply to comment #49)
> > (In reply to comment #46)
> > > We don't seem to display permissions for "Cookie Websites" (Allow, Block,
> > > Session).
> > 
> > We do, but apparently not for setting new ones, just for those that have
> > already been set for some time. I guess this is one more example of the new
> > flags or whatever they are on permission in effect. I really would have liked
> > to deal with that change in a followup. What do you think?
> It shows up immediately in Data Manager when you do it from the Cookie Manager,
> so when doing it within Data Manager I'd expect the same. Perhaps being added
> in such a way that it bypasses the trigger used to capture it when it happens
> in Cookie Manager?
This seems to start working, the second time you load Data Manager, perhaps one of the lazy gets is being too lazy?
(In reply to comment #54)
> (In reply to comment #52)
> > (In reply to comment #49)
> > > (In reply to comment #46)
> > > > We don't seem to display permissions for "Cookie Websites" (Allow, Block,
> > > > Session).
> > > 
> > > We do, but apparently not for setting new ones, just for those that have
> > > already been set for some time. I guess this is one more example of the new
> > > flags or whatever they are on permission in effect. I really would have liked
> > > to deal with that change in a followup. What do you think?
> > It shows up immediately in Data Manager when you do it from the Cookie Manager,
> > so when doing it within Data Manager I'd expect the same. Perhaps being added
> > in such a way that it bypasses the trigger used to capture it when it happens
> > in Cookie Manager?
> This seems to start working, the second time you load Data Manager, perhaps one
> of the lazy gets is being too lazy?
Okay, think I'm making some progress here. If you select a domain that you know has a cookie permission before deleting any cookies from a domain that you will be blocking (by checking the relevant checkbox in the cookies tab), then the permission changes show up immediately, so looks like something is initiated going that route that isn't on any other route.
Blocks: 597993
(In reply to comment #54)> (In reply to comment #52)> > It shows up immediately in Data Manager when you do it from the Cookie Manager,> > so when doing it within Data Manager I'd expect the same. Perhaps being added> > in such a way that it bypasses the trigger used to capture it when it happens> > in Cookie Manager?> This seems to start working, the second time you load Data Manager, perhaps one> of the lazy gets is being too lazy?I don't know what you mean with "doing it within Data Manager" - we don't have any possibility to create new Cookie permissions within Data Manager.Also, I don't know where any lazy get would be involved there.Given your additional analysis, I start to wonder if it's just the below-mentioned error that causes this by refusing to execute that function (that catches any permissions changes) in the future, as it had a JS error. Solving that by not having an error should just make it work in that case.(In reply to comment #52)> Error: this.list is null> Source File: chrome://communicator/content/dataman/dataman.js> Line: 1177This is when receiving a permissions change notification before ever showing a permission list, I guess we should just null-check-abort there (when you read the code, you'll know why we don't care in that instance, we rebuild the list anyhow when selecting the permissions panel).
Here's a patch that just should fix that "this.list is null" error, I'd be interested if it fixes the other one as well.
Attachment #476143 - Attachment is obsolete: true
Attachment #476779 - Flags: superreview+
Attachment #476779 - Flags: review?(iann_bugzilla)
Attachment #476143 - Flags: review?(iann_bugzilla)
Comment on attachment 476779 [details] [diff] [review]
v4.4: fix an error seen by Ian

>diff --git a/suite/common/dataman/dataman.js b/suite/common/dataman/dataman.js
>+    catch (e) {}
Nit: you are not consistent in the formatting catch, sometimes you have "} catch" other times "catch" on its own line, sometimes "catch (e)", other times "catch(ex)"

>+  shutdown: function tabs_shutdown() {
>+    gDataman.debugMsg("Shutting down tabs");
>+    if (this.activePanel) {
>+      this.panels[this.activePanel].shutdown();
>+      this.activePanel = null;
>+    }
>+  },
>+
>+  select: function tabs_select() {
>+    gDataman.debugMsg("Selecting tab");
>+    if (this.activePanel) {
>+      this.panels[this.activePanel].shutdown();
>+      this.activePanel = null;
>+    }
Can we not just call the shutdown function rather than repeating the code?

>+// :::::::::::::::::::: permissions panel ::::::::::::::::::::
>+var gPerms = {
>+  list: null,
>+
>+  initialize: function() {
Shouldn't this be "initialize: function permissions_initialize() {" to be consistent.

>+  shutdown: function() {
Shouldn't this be "shutdown: function permissions_shutdown() {" to be consistent.
>+  _askUserShowPasswords: function passwords__askUserShowPasswords() {
>+    // Confirm the user wants to display passwords.
>+    return Services.prompt.confirmEx(window,
>+                                     null,
>+                                     gDataman.bundle.getString("pwd.noMasterPasswordPrompt"),
>+                                     Services.prompt.STD_YES_NO_BUTTONS,
>+                                     null, null, null, null, { value: false }) == 0; // 0=="Yes" button
Nit: "null" could go on the same line as "window".

>diff --git a/suite/common/dataman/dataman.xul b/suite/common/dataman/dataman.xul

>+<page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+      id="dataman-page" title="&dataman.windowTitle;"
>+      onload="gDataman.initialize();" onunload="gDataman.shutdown();">
Nit: one attribute per line.

Nit: most of onpopupshowing are missing ";", about half of oncommand are missing ";"

>+      <tree id="domainTree" flex="1"
Nit: one attribute per line, also on other tree and treecol elements in this file.
>+            seltype="single"
>+            hidecolumnpicker="true"
>+            onkeypress="gDomains.handleKeyPress(event)"
Nit: Missing ; and on other onkeypress and onclick attributes in this file.
>+                    <label id="cookieInfoHostLabel" value="&cookies.info.host.label;"
>+                           value_host="&cookies.info.host.label;"
>+                           value_domain="&cookies.info.domain.label;"
>+                           control="cookieInfoHost"/>
Nit: One attribute per line.

>+            <button id="cookieRemove" disabled="true"
>+                    label="&cookies.button.remove.label;"
>+                    accesskey="&cookies.button.remove.accesskey;"
>+                    oncommand="gCookies.delete();"/>
Nit: One attribute per line and on other button elements in this file.

>+            <checkbox id="forgetCookies" disabled="true"
>+                      label="&forget.cookies.label;"
>+                      accesskey="&forget.cookies.accesskey;"
>+                      oncommand="gForget.updateOptions();"/>
Nit: One attribute per line and other checkbox elements in this file.
>+            <label id="forgetCookiesLabel" hidden="true"
>+                   value="&forget.cookies.label;"/>
Nit: One attribute per line and other label elements following checkboxes in this file.
>diff --git a/suite/common/dataman/tests/browser_dataman_basics.js b/suite/common/dataman/tests/browser_dataman_basics.js

>\ No newline at end of file
Fix?

>diff --git a/suite/common/tasksOverlay.xul b/suite/common/tasksOverlay.xul

>+          <menuitem id="tasksDataman" class="menuitem-iconic"
>+                    label="&datamanCmd.label;" accesskey="&datamanCmd.accesskey;"
>+                    command="Tasks:DataMan"/>
Nit: One attribute per line.

r=me with those nits/comments addressed.
Attachment #476779 - Flags: review?(iann_bugzilla) → review+
Couple of final things:
1/ Should we be testing keypresses/mouse clicks in the tests too?
2/ Who is going to log the followups for the items mentioned in my testing?
(In reply to comment #58)
> Nit: you are not consistent in the formatting catch, sometimes you have "}
> catch" other times "catch" on its own line, sometimes "catch (e)", other times
> "catch(ex)"

Thanks for catching those, I thought I had made it be "catch (e)"  on its own line consistently. Done now.

> Can we not just call the shutdown function rather than repeating the code?

Hmm, we can, but tabs_shutdown() is supposed to clean up everything needed to shut down the whole tab display, it just happens to not be more than what tabs_select() needs in its first step as well.

> Shouldn't this be "initialize: function permissions_initialize() {" to be
> consistent.

Good catch!

> Nit: most of onpopupshowing are missing ";", about half of oncommand are
> missing ";"

Good catches!

> Nit: One attribute per line.

I'm not too fond of those, esp. when short things like |flex="1"| land on their own lines, but then, whatever you say.

(In reply to comment #59)
> Couple of final things:
> 1/ Should we be testing keypresses/mouse clicks in the tests too?

I have a few .click() in there and tried to go through UI items to call underlying functions, but synthetic keyboard/mouse events are always a bit of a hassle, they tend to easily show up in intermittent oranges, so I try to only use them where really needed. Also, this is just a first test to have all the "basics" of data manager covered, maybe using things like the delete key might be a good idea in further tests. For now, I'm happy we have what we have. :)

> 2/ Who is going to log the followups for the items mentioned in my testing?

Didn't I file those already? There are a lot of dependencies here already. :)
Blocks: 597994
Blocks: 599097
Pushed the Data Manager as http://hg.mozilla.org/comm-central/rev/faf35fa380ca

It's done! Let's go party! (And then work on followups...)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b1
Depends on: 600229
No longer blocks: 480734
No longer blocks: 295727
Blocks: 613086
Blocks: 613087
Blocks: 613795
Blocks: 615014
Blocks: 617588
Blocks: 619098
Blocks: 629678
Blocks: 632789
Blocks: 632361
Blocks: 662096
Blocks: 665826
Blocks: 666102
Blocks: 666098, 666099
Blocks: 664574
Blocks: 667459
Blocks: 667464
Blocks: 670681
Blocks: 682193
You need to log in before you can comment on or make changes to this bug.