504419 - SearchSpec.updateSession 'filtering' variable is incorrectly initialized or documented

Status: RESOLVED DUPLICATE of bug 537820

(Thunderbird :: Folder and Message Lists, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 fix filtering logic

While going to comment on another bug, I was reviewing when we force searches to be offline in searchSpec.js and noticed that the initialization of "filtering" is confusing and inconsistent with the comment above it and the bug comment it references, https://bugzilla.mozilla.org/show_bug.cgi?id=474701#c73

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/searchSpec.js#382

The current initialization of:
  let filtering = this._virtualFolderTerms == null &&
                  this._viewTerms == null;
...implies that only this._userTerms has a value.  (As must be the case because we would not be in the method if all three were null.)  This effectively makes the logic only applicable for quick-searches on non-virtual-folders with no mail-view applied.

Based on the comment, a better (and literal) translation is:
  let filtering = this._viewTerms != null ||
                  this._userTerms != null;

In terms of impact on virtual folders, this logic causes us to perform an offline search for virtual folders when a mail-view and/or quick-search is active as long as there are no body terms involved.  If the virtual folder was an onlineSearch (and the mail-view was not loaded from the persisted folder state on entry), it should already have compelled the headers to get up-to-date via the search.

The danger cases are:
a) That case where the mail-view was restored automatically and the (multi-folder) virtual folder is an online search.
b) If haveBodyTerm does not accurately capture all the predicates where we need the IMAP server to actually check things for us.

Danger case 'a' is something that would be mitigated if we triggered updateFolder on each folder backing the (multi-folder) virtual-folder in sequence on entry.

Requesting reviews from sid0 for logic verification (don't think you've gotten to review this code yet! :) and bienvenu for the greater virtual-folder concerns.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 388786 [details] [diff] [review]
v1 fix filtering logic

Do not review before your vacation is over!

Comment 2

[David :Bienvenu]

Comment on attachment 388786 [details] [diff] [review]
v1 fix filtering logic

the only other kinds of search terms that can require offline bodies are headers that we don't store in the db - but we have ways to cause specific headers to get added to the .msf file.

Comment 3

[Rain [:rain1] (inactive)]

Comment on attachment 388786 [details] [diff] [review]
v1 fix filtering logic

Looks good.

This is mailnews/ code -- wouldn't this require sr?

Comment 4

[David :Bienvenu]

you can take my r+ as an sr.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

dmose and bienvenu have signed off on the mailnews/ portions of the pure-JS FolderDisplayWidget use-case (so DBViewWrapper/SearchSpec/etc.) only requiring review as long as I make sure to get a review from bienvenu (or other appropriate reviewer) for when non-logic concerns involve greater issues of virtual folders/message db views/etc.  I had an action item to expose that to mdat and kick off an SR discussion, which I apparently have neglected to do...

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

This never landed but bug 537820 ended up with an identical fix (after a regression in bug 511131 which removed the (incorrect) logic).