User:Andrew Sutherland/MailNews/GlobalDatabase/Review

From MozillaWiki
Jump to: navigation, search

bug 450494 is tracking the process of getting gloda onto the trunk.

This page reflects the status of the feedback from the bug as of comment 39. The following comment chains have not been included because, although making for interesting discussion, aren't particularly actionable, but could form the basis of an action-y thing:

  • c21 clarkbw's responsiveness desires. followups: asuth c23

Un-Addressed

Not yet/currently being worked, no notable progress, perhaps some discussion.

bienvenu

gloda suffixtree confusing comments

(bienvenu c26 q2) Some of this code is commented out - does that make it inconsistent with the comment about 'infinity'?

   // However, if this state is a leaf node (end == 'infinity'), then 'end'
   //  isn't describing an edge at all and we want to avoid accounting for it.
   let delta;
   /*
   if (state.end != this._infinity)
     //delta = index - end + 1;
     delta = end - (index - state.length); 
   else */
   delta = index - state.length - end + 1;

dmose

gloda.js :: initMyIdentities

(dmose) (Possibly premature) optimization:

  for (let iIdentity=0; iIdentity < numIdentities; iIdentity++) {
    let msgIdentity = msgAccountManager.allIdentities.GetElementAt(iIdentity)
                                       .QueryInterface(Ci.nsIMsgIdentity);

If you cache msgAccountManager.allIdentities in a local variable before starting the for loop, you'll avoid an XPConnect crossing every iteration.

(asuth) XPConnect turns out to be oddly expensive, so this sounds reasonable and easy to do. I could also change-up to using fixIterator perhaps.


Half-Finished Comments

(dmose) (gloda.js) the kAttrDerived doxygen comment is incomplete

Noun Definition

(gloda.js)

(dmose) defineNoun: the doxygen commentary needs a bit of cleanup: I think the nounMeta property names want to be documented semi-separately in their own doxygen block, to give us a place to document the actual defineNoun function signature itself. Also make sure that the property list is complete (eg includes cacheBudget, queryClass, etc). Also, the parameter ordering in the function signature itself seems backwards from an intuitiveness point of view.

(asuth)Yes, the doxygen could use some love and its own block. Noun ID comes after noun meta because it is optional, but it's probably cleaner to just let the caller put 'id' in the meta object in the first place.


(dmose) similarly, would "instanceOf" be a clearer property name than "class"?

(asuth) This is also a good idea. I feel a pang of guilt about using something so close to a keyword, but even if we used "instanceof", spidermonkey would apparently be okay with it. I shall do so.


(dmose) defineAttribute: it's not really clear to me why it's worth having "bindName". Wouldn't it be simpler to just always use the attribute name?

(asuth) This was intentional to allow the database name to differ from the binding name. I think the idea was that the database names would make more sense standalone, whereas the binding name might be less intuitive in some ways. But I think in retrospect, the ability for them to deviate just adds potential confusion... so yes.


Being Addressed

Things being addressed. Next action is asuth's.


dmose

Need Input

asuth needs more info from peoples, grouped by peoples.

bienvenu

dmose

general

* With the new indexer, I'm seeing a moderate number of error messages of the form: 

2008-09-21 12:17:29	gloda.fundattr	ERROR	Message with subject
                        'Mark Banner' somehow lacks a valid author.  Bailing.

where in every case, the subject is purported to be the author name.
Which makes me think the code is getting confused somewhere.

I think this was a transient problem involving IMAP. Are you still seeing this? I do occasionally see things like "Message with subject somehow lacks a valid author", but I think that is a different problem either involving messages with odd MIME things happening or perhaps an event train hiccup.

gloda.js

  • in getIdentitiesForFullMailAddresses, are we actually guaranteed that the email addresses aren't relays?

I don't think the methods claim that they do this. I think the goal is that the attribute providers pierce the relay nonsense so it doesn't happen... since we basically depend on the attribute providers to mark relays as such, perhaps we could require them to do so? The result would be that either we don't know something is a relay, or we know it is and you shouldn't really hear about it. Could you clarify what your desired action would be here? (I'm sorry if my delay in getting to this point has made you forget...)


Agreed Addressed

bienvenu

dmose

Processing

Andrew is processing this set of comments. Do not touch!

dmose

gloda.js spelling typos

* Inspired by reasonable uses of triple-stores, I have tried to leverage
*  existing model and terminology rather than rolling out own for everything.

s/out/our/

gloda.js
--
* I kinda suspect introducing the concept of predicates into the explanation
of attributes is likely to make it harder for many people to understand rather
than easier.


general
--
* for all modules, all exported symbols should have associated documentation
comments (these could just be pointers) 

* API points like getters on exported classes need doxygen commentary.



datamodel.js
--
* "allowing the use of attribute-parameter as an attribute" seems
  unclear on first reading





gloda.js
--
* given that most of the rest of code-base uses the word "thread", it would
probably be good to include in the documentation of NOUN_CONVERSATION a
sentence or two about how this is (and/or isn't) different from that

* parameterized identity:
** s/constrainted/constrained/
** do we want to introduce some JS assertions about the constraints
   here?  I think Fx has a concept of such things...
** The intent of the "if (typeof [...]" is not obvious on first reading; a
   comment here would help, I think.
** shortNames need to come from .properties files, I suspect

** both bindAttribute and defineAttribute seem long enough that it's a
   little hard to keep all the necessary state in one's head at once.
   Maybe split them up?

* _bindAttribute
** please either add doxygen comments for the params or at least a
   @seealso pointing to defineAttribute
** Defining functions completely anonymously can make it hard to
   understand stacks in (e.g.) Venkman; prefer named functions.  

To Be Processed

asuth's exciting holding pen for things....


dmose

everything below here is done in the ugly-but-easy style!

dmose puts his new comments here...

collection.js
--
* doxygen comment on cacheLookupOneByUniqueValue appears to have been
copied from cacheLookupOne but not modified.  When fixing, please be
sure to mention that the caller is expected to guarantee the
"uniqueness" invariant.  Backing up a step, though, is there any
particular reason not to collapse these two functions into one by
parameterizing the array to search?  (If it turns out that makes it
hard to read, I can live with keeping it as is).

gloda.js
--
* getFolderForFolder is not a very intuitive name.  How about
getFolderForMsgFolder?

collection.js
--
* it would be good for cacheLoadUnify* to have @return and @param dox

collection.js
--
* Please add a comment explaining the reasoning behind GlodaCollection
maintaining both an array (items) and an object/hash (_idMap) that
point to the same data.


collection.js
--

* the call to this._collectionsByNoun[nounID].filter in
  registerCollection would be more readable if it used an
  expression closure.

indexer.js
--
> if (msgHdr && !(msgHder.flags&MSG_FLAG_EXPUNGED))

* The lack of spaces around the & operator goes against prevailing style
and makes it hard to read

collection.js
--

> // (_lruNext cannot be null)

* I suspect that making the above comment a bit more explicit about
why it's true will help prevent future code-maintainers from
inadvertently breaking this invariant.

gloda.js
--
* Do the constrainer functions in _bindAttribute really want to live
in query.js?  Perhaps the thing to do is to the solve the
_bindAttribute naming issue by splitting _bindAttribute itself
into _bindAttribute (which could stay in gloda.js) and
_setAttributeConstrainers (which could live in query.js).  If it's
helpful, a _setupAttribute convenience method that calls both could be
written.

* As far as I can tell, usesParameter in nounMeta is not set by any
  callers.  Should it be discarded, or should (at least)
  the parameterized-identity definition set it?

query.js
--
* The use of "explicit" in naming GlodaExplicitQueryClass is a little
non-obvious to newbies; it'd be nice if we could think of something
better.  So far, I haven't been able to, however.  :-/

* Looks like neither or()/unions/owner are actually used by any callers.  Unless you know of a specific use case for them, I'm somewhat inclined to suggest axing them in the interest of simplicity.  This would also allow the newQuery documentation comment in gloda.js to be made more understandable as well.  On the other hand, this isn't a huge win.

* The purity-of-essence part of me kinda wonders if there shouldn't be
  a separate array for all these constrainer functions, but maybe it's
  not worth it.


glautocomp.js
--

* nsAutoCompleteGlodaResult._problem doesn't appear to ever actually
  be set anywhere.  Can it be removed?

(Yes, this is historical.)

* Given that the nsAutoCompleteGlodaResult constructor sets
  this._results to [] by default, it looks to me as though the test
  for null in the matchCount getter can never be true, so it's
  probably not necessary.

(Sounds reasonable.)

* the references to "rich" in the comments a bit non-obvious; maybe be
  a bit more explicit?

* the various uses of thing.{somegetter} mostly appear to be
  abstraction violations.  :-)

more comments


* given the current game plan, removing glautocomp.js from the tree
  seems like the right thing. alternately; we could leave it in the tree
  with a NOT_YET_REVIEWED comment if anticipate future enabling, however.

mimeMsg.js
--
* please add a doxygen comment describing CallbackStreamListener

* would wrappedJSObject solve (or at least be somewhat less of a hack than)
  RESULT_RENDEVOUZ (sic)?

jsmimeemitter.js
---
/**
 * Custom nsIMimeEmitter to build a sub-optimal javascript representation of a
 *  MIME message.  The intent is that a better mechanism than is evolved to
 *  provide a javascript-accessible representation of the message.

the last sentence is missing a word

* seems like it would be trivial to factor out common code from the constructor
  and mime_emitter_complete and that would leave the code less susceptible to
  future errors of omission

other
--
* Same question about the stuff in gloda/content as already asked about
 glautocomp.js: should all of this stuff live in comm-central going forward?
 If so but we're not using it now, maybe mark it as unreviewed somehow.


mimemsg.js
--
/**
 * Starts retrieval of a MimeMessage instance for the given message header.
 *  Your callback will be called with the message header you provide and the
 * 

Trails off mid-sentence.

* Having MimeMessage.coerceBodyToPlaintext call bodies.join("") seems like it
 means that the two tokens at the body boundary will be smashed together and
 therefore not indexed correctly.  How about joining with some whitespace?

* Since it looks like the prettyString properties are purely for logging /
 debugging, how about being explicit about that in the commentary so that
 future patchers realize that they don't need to worry about l10n or about
 changing the format of the output. 

jsmimeemitter.js
--
/**
 * Custom nsIMimeEmitter to build a sub-optimal javascript representation of a
 *  MIME message.  The intent is that a better mechanism than is evolved to
 *  provide a javascript-accessible representation of the message.

The second sentence there is unclear; please rephrase.

* _partRE looks fragile in the sense that it I'd be pretty afraid of breaking
 something if I ever needed to touch it.  I suspect some unit tests would go a
 long way towards helping that.

* please file a bug about uncommenting and fixing the commented out code in
 updateCharacterSet

databind.js
--
* It would be helpful if GlodaDatabind and its methods had at least minimal
 documentation comments.

indexer.js
--
* Since iteratorUtils.jsm is in the tree now, the custom copy of fixIterator
 can presumably go away.

* The comments in this entire file are extra super awesome; nice work!  

* Should the various listener issues described there be spun off into bugs?

suffixtree.js
--
* I would suggest removing this from the tree until such time as you are
 actually committed to supporting it (presumably whenever the first front-end
 feature that uses it wants to land).  This will keep folks from stumbling on
 it and expecting it to work/be supported for whatever it is they want.  I'm
 betting "function examplar()" can go away too.  :-)

datastore.js
--
* If _mapFolder and _mapFolderId were named in a way that more explicitly
 described what they did, reading calling code would be easier.

datamodel.js
--
* I suspect pictureURL should probably just default to null.

datastore.js
--
* The commentary in MessagesByMessageIdCallback implies, but doesn't actually
  say, that if someone other than the indexer calls getMessagesByMessageID,
  things could get squirrelly.  If that's actually the case, I'd suggest
  renaming to _getMessagesByMessageID as a hint to extension authors and
  explicitly documenting it as part of the function Doxygen commentary as well.

* queryFromQuery is a fairly non-intuitive name.  In the interest of making
  calling code easier to read, maybe performQueryAsync?

* presumably, the database needs to have some sort of vacuuming strategy,
  whether that's simply using autovacuuming or periodically issuing vacuum
  commands.  This seems like it's probably a blocker for Tb3.  In a recent
  post, sdwilsh said:

  "There is almost never a good time to VACUUM, except for maybe when the
  user is idle.  In 1.9.1, we've added an asynchronous API for statement
  execution, and if you use that all the time, it shouldn't be
  noticeable to the user that you are doing a VACUUM."

* Assuming the comment "(Namely, we fail to purge items from the
  cache as they are purged from the database.)" is still correct, it's probably
  worth filing a spin-off bug about.

* The @namespace at the bottom of the big comment before GlodaDatastore appears
  to be stray, since it doesn't have an argument.

* The orphaned data problem probably wants a spin-off bug.

* Having doxygen comments documenting kConstraint* somewhere would be good.

* A comment explaining why the service is passed in to gloda_ds_init rather than
  just created locally on-demand would be nice.

* The _beginTransaction doxygen comment seems to trail off mid-sentence.  It 
  would be helpful to have a more complete description of the overall nested 
  transactional model.  As it stands, it's not very clear to the reader what 
  assumptions should be made / depended on when dealing with existing
  transactions (eg when it does and doesn't make sense to do the intermediate 
  commits that are seen in some parts of the code).

* The comment for trackAsync could stand to be fleshed out.  As it stands, it's
  not obvious from reading calling code or even the doxygen comment when or why
  someone would want to use it.  Seems like a more obvious function name could
  help.

* A quick grep suggests that there are no callers for getMessageByID or
  getMessageIDsByFolderID.  Perhaps these are something extensions might want,
  however...  If we do want to keep them, it seems like there's a high
  potential for unnecessary confusion of Message-ID headers and Gloda IDs for
  messages; perhaps a little name surgery is order.

* There are a few remaining references to APV stuff both here in and gloda.js,
  some (all?) of them would appear to be obsolete.  One example is that
  getMessageAttributes doesn't have any in-tree callers.

* comments for adjustAttributes, _convert*ByAttributeID, loadNounItem, and 
  _queryFromSQLString would improve code maintainability.


All done! You've done a great job keeping code that deals with a lot of complexity as simple and maintainable as it can be.