User:Andrew Sutherland/MailNews/GlobalDatabase/ReviewBelievedResolved

Believed Addressed

asuth believes these points are addressed. Please sign-off on them or move them to agreed addressed or invent a new notation or something. Next action is the person whose heading it is under.

bienvenu

indexer magic constant

(bienvenu c15 q8) might add a comment referring to the NS_ERROR define you're catching here:

     catch ( e if e.result == 0xc1f30001) {
       // this means that we need to pend on the update.
       this._log.debug("Pending on folder load...");

(asuth 9/18) This was addressed with changeset 048686317d14, see the file diff. Note that since use of updateFolder is going away soon, this will shortly be mooted.

(asuth 9/22)So, changeset e997476f295b leaves this using Cr.blah instead, although the change to Components.results was made earlier.

indexer obsolete comment

(bienvenu c15 q6) Is this comment obsolete? You're not using setTimeout, or this._messenger, in indexer.js

   // we need this for setTimeout... what to do about this?
   this._messenger = Cc["@mozilla.org/messenger;1"].
                       createInstance(Ci.nsIMessenger);
   this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

(asuth c17 a6) Yes, it's obsolete and went away sometime yesterday in a clean-up/update pass. _messenger (which oddly inherited the comment) also went away... I think that was detritus from the mime-body-fetching before it entirely migrated to mimemsg.js.

(asuth 9/18) This happened in changeset a5e649db05eb, only that file was affected.

expmess installation/usage

(bienvenu c15 q2) Should I be installing the expmess xpi to see the status of indexing? Is there a newer version of it than 0.0-m1?

(asuth c17 a2) Yes, expmess is the only way to see what is happening. The XPI is very old and probably will break things. I would install the extension directly from hg. (So, check out https://hg.mozilla.org/users/bugmail_asutherland.org/expmess/, then create a file "expmess@mozillamessaging.com" in your profile's extensions directory that provides the path to the checked-out directory.)

expmess LDAP frowny faces

(bienvenu c28 attached patch) I was never seeing anything from expmess when I clicked on a message, other than the frowny face :-( This fixes it for me. I suspect we were getting an ldap directory, which throws an exception when this is called. Andrew, let me know if you want me to just land this in your repo, or if you want to fix it another way...

(asuth c29 approving patch) LDAP, eh? Feel free to land it in the gloda repo.

(Standard8 c30) Yes, not all address book types can support this function at the moment.

(bienvenu c31 noting commit 3ac64fe0567b) finally coerced hg into letting me push

nsMsgSearchDBView::GetType fall-through

(bienvenu c33) patch provided) make seach view return a view type we were generating an error before because the base class returns an error. Gloda & friends wants this not to error out.

(asuth c35 r+ patch Patch looks good; applies and makes the problem go away. Danke.

(Neil c36 sr+) Basically this function is the inverse of the switch in CreateBareDBView, right?

(bienvenu c37 commit marking) yes, it's the same kind of thing...

code formatting: equal spaces

(bienvenu c15 q8) we generally put spaces around the '=', at least in c++

collection.js

   let iWrite=0;
   for (let iRead=0; iRead < items.length; iRead++) {

datamodel.js:

           let values = this[attrDef.boundName];
           for (let iValue=0; iValue < values.length; iValue++)

(asuth) Fixed in changeset 0f5e5f6ba559. 1561f5ddffc9 also has a related change to make "for(" become "for (".

utils variable naming

(bienvenu c26 q1)

 md5HashString: function gloda_utils_md5hash(aString) {
   let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
                   createInstance(Ci.nsIScriptableUnicodeConverter);
   let trash = {};
   converter.charset = "UTF-8";
   let emailArr = converter.convertToByteArray(aString, trash);

   let hasher = Cc['@mozilla.org/security/hash;1'].

"emailArr" seems an odd var name here, if this is a generic utility function

-)

(asuth) changed to the more generic 'data'.

dmose

startup =

At startup, I'm seeing some errors, even though the two files

 mentioned are in my source tree:

2008-10-02 17:12:48     gloda.everybody ERROR   !!! error loading resource://gloda/modules/noun_freetag.js
2008-10-02 17:12:48     gloda.everybody ERROR   (undefined:61) [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/everybody.js :: loadModule :: line 61"  data: no]
2008-10-02 17:12:48     gloda.everybody INFO    ... loading resource://gloda/modules/index_ab.js
2008-10-02 17:12:48     gloda.indexer   INFO    Event-Driven Indexing is now true
2008-10-02 17:12:48     gloda.everybody ERROR   !!! error loading resource://gloda/modules/index_ab.js
2008-10-02 17:12:48     gloda.everybody ERROR   (undefined:61)
[Exception... "Component returned failure code: 0x80520012
(NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult:
"0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame ::
file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/everybody.js
:: loadModule :: line 61"  data: no]

This was likely a transient failure, and I believe the problem has been addressed. The problem of error messages like this being somewhat non-helpful is still there, but that's more of a fundamental problem with poor error reporting from Cu.import than anything else.

indexing errors

recently when indexing, I saw about 8 of these:
2008-10-02 18:30:19 gloda.datastore ERROR Synchronous step gave up after 32000 tries.

This is no longer a possible problem since everything happen on the asynchronous thread.

shutdown sqlite/storage assertions

(dmose c38) I shut Thunderbird down while it was in the middle of downloading my giant IMAP Trash folder the first time. Saw the following debug output

2008-09-17 19:11:14     gloda.indexer   INFO    Shutting Down
2008-09-17 19:11:14     gloda.indexer   INFO    Event-Driven Indexing is now
false
2008-09-17 19:11:14     gloda.datastore INFO    Closing pending transaction out
for shutdown.
2008-09-17 19:11:14     gloda.datastore INFO    Closing async connection
WARNING: sqlite3_close failed. There are probably outstanding statements!: file
/Users/dmose/s/gloda/src/mozilla/storage/src/mozStorageConnection.cpp, line 236
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x8052000e
(NS_ERROR_FILE_IS_LOCKED) [mozIStorageConnection.close]"  nsresult: "0x8052000e
(NS_ERROR_FILE_IS_LOCKED)"  location: "JS frame ::
file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/datastore.js
:: gloda_ds_shutdown :: line 493"  data: no]
************************************************************
2008-09-17 19:11:14     gloda.datastore INFO    Closing async connection
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0xc1f30001
(NS_ERROR_NOT_INITIALIZED) [mozIStorageConnection.close]"  nsresult:
"0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame ::
file:///Users/dmose/s/gloda/obj-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/datastore.js
:: gloda_ds_shutdown :: line 493"  data: no]
************************************************************

Doing it a second time, I ended up with a few of these as well:

###!!! ASSERTION: Still pending events!: 'mPendingEvents.Count() == 0', file
/Users/dmose/s/gloda/src/mozilla/storage/src/mozStorageEvents.cpp, line 462

Both of these were running without the IMAP changes from Emre, FWIW.

(asuth status 9/18) With the indexer changes, I will have all asynchronous statements have result trackers combined with a shutdown listener to ensure that they flush before we allow shutdown to complete. Of course, the fundamental problem you encountered is that indexing currently doesn't care about shutdown and so keeps going up until the rug gets pulled out from under it... which will be addressed.

(asuth 10/7) I did what I said I would did, and it actually works as of this changeset.

Noun IDs

(dmose) does having _nextNounID not be persisted means that ids saved in the database could change across Thunderbird invocations if for some reason the ordering of calls to defineNoun change? If so, that seems fragile...

(asuth) Correct, noun id's are neither stable nor consistent (except for the internal, hard-coded ones). However, noun id's never touch the database at this time, so it's more a question of being confusing/useless for debugging. (Our full knowledge of attributes currently relies on them being registered at the start of every session; we don't persist their noun info. Given our current feature-set, this does not preclude the ability to garbage collect unused attributes.)

The existence of noun id's at this point is effectively an optimization, since their canonical identifier is either their name or the NOUN_ID constant on their data model class's prototype. They only would need to be persisted if/when we start putting them in the database for generic references that do not rely on parameterized attributes. Which is to say, we could create a "generic-reference" attribute whose parameter is the name of the noun that it is a generic-reference too. That neatly avoids having the noun id touch the database and allows easy introspection. I'm not sure that's the right solution to whatever problem would drive such a need, though.

Would you prefer that move to using some centralized noun ID registry or begin persisting the noun ID's to head off our likely future need?

(dmose) I don't think that needs to be done before landing. Please just track this issue somewhere (a bug, wiki page, wherever), and let's deal with it when a concrete need arises.

(asuth) bug 458992 logged.

general

(do now or track for later): At some point you might want to pull in a current copy of log4moz.js; the enumerateProperties stuff that landed not too long ago looks _really_ useful

pulled current copy

having indexing happening while we're on battery power is seems like it could really eat battery life. sdwilsh was talking about adding an API to Gecko to check that. In the meantime, being "offline" seems like it could be a good proxy for that. Can you file a bug on looking into what sort of strategy we want for the 3.0 timeframe?

This is implemented.

(this was observed):

###!!! ASSERTION: Still pending events!: 'mPendingEvents.Count() == 0', file
/Users/dmose/s/gloda/src/mozilla/storage/src/mozStorageEvents.cpp,
line 462

This was a mozStorage bug that is now fixed.

In the cases where you don't actually care about the index in a an Iterator:
- foreach (let [iItem, item] in Iterator(aItems))
it's clearer to explicitly ignore the index like so:
- foreach let [,item] in Iterator(aItems)
Alternately, you could use items.forEach(), possibly in combination with an expression closure:
- items.forEach(function foo() expression);

I have done the former. I was avoiding the latter because it seemed likely to be less efficient, but I didn't really have any proof of that, and with tracing now enabled, it is likely to be moot. I think I'll leave things as-is because I do prefer the former construct slightly.

collection.js

It looks like the GlodaCollection constructor duplicates a bunch of code from _onItemsAdded. Could it just call that method directly?

Good point, I have done this.

Why use apply here rather than calling this.items.push() directly?

  _onItemsAdded: function gloda_coll_onItemsAdded(aItems) {
    this.items.push.apply(this.items, aItems);

I use apply as an equivalent to the python func(*args) mechanism to spread a list of arguments for a function that expects multiple arguments rather than a list of arguments. I have done this in the hope that it is faster than repeatedly calling push, and more efficient than using concat (which, to my understanding, would create a new list and have us abandon the existing list to GC.)

datamodel.js

having raw attributes defined as a tuple of [attribute id, parameter, value] means that consumers end up accessing by integer index, which feels error prone (and makes calling code unnecessarily hard to read). How about making them be objects instead, so that the accessors are property names? e.g. { id: 1, parameter: "foo", value: "bar" }

The refactoring has eliminated the APV representation and its variants for nearly all purposes. The representation still exists for adding/removing the attribute database rows, but no code apart from the datastore's add/remove code uses them, and the representation is appropriate in that case because that's the database row format.

while all the getter magic in the MixIn is elegant, it feels to me

 like it also makes the code less approachable.  This would seem like
 another reason to move towards raw storage fields.

Post-refactoring, the MixIn's functionality to that end no longer exists. I have added some helpers that assist in dealing with the attributes generically, however. For example, if you want to walk all of the attributes on an object along with the helpers, there is a function that aids in doing this. But attribute access no longer requires any helpers, as you can simply directly access the object like a dictionary.

/**
 * @TODO Return the collection of messages belonging to this conversation.
 * (And weakly store a reference to the collection.  Once the user is rid of
 *  it, we really don't care.)
 */

What's still TODO in the above comment? Also, the last sentence isn't very clear, please be more explicit about what's intended.

This has been mooted by a proper change to the conversation object to have it construct/return a fully asynchronous collection of the messages belonging to the conversation.

Should GlodaMessage.toString() use something from a properties file? Or will this never be displayed to end users?

toString is never intended to be user-visible. It is only for debugging assistance and to ensure that use of the objects as keys in object dictionaries works reasonably without false collisions.

GlodaMessage.{_nuke,_ghost} don't appear to be private methods, so presumably they want to have doxygen comments and to not begin with an underscore.

They are not public. They are more private but friend accessible. Someone with access to a GlodaMessage instance should not be under the impression those methods are for them to use; they are only there for the indexer.

In the GlodaMessage folderMessage getter, shouldn't both of those LOGs be at a higher level than just "info"?

does the special property want to be a boolean instead of an integer?

no, this is now an enumerated value.

explain doesn't appear to be called anywhere, so it can presumably go away

It was used by expmess, but I gutted it because it was causing string issues and turned out to be less useful than I originally planned. now fully excised

seems like it would make sense to provide a boundName setter rather than allowing Gloda.js to reach inside this GlodaAttributeDef

I've moved it to just be an attribute; if we're not going to be read-only and not have any logic, no need for getters and setters. changeset

getValueFromInstance calls getAttributeInstances, which only exists on GlodaMessage objects, so presumably it will fail when called on any other type of object. Is this intentional? If so, please document...

As of my contact generalization changes getAttributeInstances is now part of a mix-in that GlodaMessage and GlodaContact both get, plus anyone else who wants in.

explattr.js

it'd be nice to have a brief (1 sentence) comment at the top describing the file and pointing to the more detailed description of explciit addrs

done

the comment above the class object definition appears to be wrong

previously done

everybody.js

instead of adding "" to an exception to force it to be a string, either call the .toString method (best) or use the String() conversion operator (ok too)

rendered moot by more extensive formatting

gloda.js

Why bother having a local let variable in getMessageForHeader?

It probably existed for debugging, but is now gone, thanks.

use @return in the documentation for getMessageForHeader, please.