481824 - nsMsgDBView grouping logic is buggy, needs unit tests

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

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

Attachment: v1 dbview fixes from bug 474701 plus a new unit test, adds files, use qimport

I am splitting the nsMsgDBView changes off from bug 474701.  Without these fixes, when using a search view, group-by logic is both crashy and incorrect.  

The provided patch provides a first-pass at a unit test that wants to at least verify my changes and the use case from bug 474701.

I was originally turfing this to bienvenu because I thought this was crashy, but it turns out that I don't know of any crashers with this patch (The crash I was seeing was because I had not updated the patch properly to account for changes on trunk.)  So now I'm turfing it because bienvenu hinted at wanting better searching logic, and because we really really need unit tests for nsMsgDBView and friends and I don't have time to take it further or to clean up the changes I have already made.  (nsMsgDBView has crazy many variants and permutations and needs coverage.)

Comment 1

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

The very limited set of changes on messageGenerator.js have been mooted by http://hg.mozilla.org/comm-central/rev/99320ed86776.  The rejections should be ignored.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Since this blocks [b3ux] work, it needs to be [b3ux] itself.  Getting it on the radar and targetting.

Comment 3

[David :Bienvenu]

I've been running with this a bit - I'll try to figure out why the db isn't valid.

Comment 4

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

Do you mean why I needed to add m_db guards?  If so, it's because gloda is creating a search view which doesn't have an m_db.  We just cram stuff in via onSearchHit.

Comment 5

[David :Bienvenu]

No, I meant why the db you try to get after getting the folder loaded notification  after parsing the msg db wasn't valid. aka the GAAAAAAAA comment :-)

Comment 6

[David :Bienvenu]

updateFolder does not currently return an error when the db is invalid and will be reparsed, but it always sends a folderLoaded notification at the appropriate time (either immediately, or when the folder has been reparsed). I don't know when it stopped returning an error - perhaps jcranmer or rkent or I changed it at some point. But if I remove all the needToWait logic from mailTestUtils.js, then I can get rid of the timeout in mailTestUtils.js, and the one in test_nsMsgDBView.js, and all the tests pass. So I'm probably going to go with that approach.

Comment 7

[David :Bienvenu]

Attachment: updated fix

this cleans up the test code so that we don't need settimeouts - I'm going to have a look through this patch and probably r/sr it if it looks ok, and land it since it's blocking the search bar stuff.

Comment 8

[David :Bienvenu]

patch checked in,  so other search bar stuff can be unblocked.