Closed Bug 678621 Opened 13 years ago Closed 13 years ago

Make the attachmentInfos url property a getter

Categories

(MailNews Core :: Database, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 10.0

People

(Reporter: protz, Assigned: protz)

Details

(Whiteboard: [gloda key])

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
So the attachmentInfos url property becomes out of date as soon as we move the message –— this is obviously very bad; what the patches does is just store the part information and reconstruct the full URL when asked.
- I had to modify fromJSON to accept a second parameter, namely, the noun: that allows the GlodaAttachment, we revived from JSON, to get a link to its parent GlodaMessage, which in turns allows it to recover the message URL through it's GlodaMessage's folderMessageURI property.
- We need to keep the full url in case the attachment is an external one, this patch takes that into account. Detaching an attachment appears to Gloda as if the message is deleted, then added back into the folder, so a full reindexing occurs.

I also changed the storage format to just be an array, to shave off some bytes.

There were tests already, but I added more to make sure the url property is properly rebuilt; the best place to test this was test_mime_emitter.js
Attachment #552762 - Flags: review?(bugmail)
FWIW, I just rebuild my gloda database with the patch on, and the size difference is negligible. I guess few messages have attachments...
Attached patch Patch v2 (obsolete) — Splinter Review
Fix a typo in the test.
Attachment #552762 - Attachment is obsolete: true
Attachment #552762 - Flags: review?(bugmail)
Attachment #552814 - Flags: review?(bugmail)
Comment on attachment 552814 [details] [diff] [review]
Patch v2

The patch looks very good, but you don't seem to actually provide a unit test for the explicit goal of the patch.  Please provide an xpcshell unit test that indexes a message in a folder, performs a fast-path move, then verifies that the URI generated in the target location is correct.

Yes, this would only likely make a disk size difference for records where it pushes us over the page size.  If you are running with 32k pages, there should be a lot of spare space in those pages.  On a 1k page size, the spillage should still be somewhat rare, and when we overflow, it's still only 1k more...
Attachment #552814 - Flags: review?(bugmail) → review-
Attached patch Patch v3Splinter Review
Just added a test, this one took me quite some time to put together, because there was some strange oddity in the test framework...
Attachment #552814 - Attachment is obsolete: true
Attachment #555228 - Flags: review?(bugmail)
Comment on attachment 555228 [details] [diff] [review]
Patch v3

I'm also unsettled by the imap offline failure you experience (noting to drive-bys that gloda 'offline' in testing parlance just means that the folder is set so we download an offline copy of the message, not that the network is offline), but the test looks good to me, so we'll cross our fingers :)

I particularly appreciate that you made sure the URLs changed in addition to making sure they are valid!
Attachment #555228 - Flags: review?(bugmail) → review+
http://hg.mozilla.org/comm-central/rev/123782592812
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0
You need to log in before you can comment on or make changes to this bug.