Closed
Bug 678621
Opened 13 years ago
Closed 13 years ago
Make the attachmentInfos url property a getter
Categories
(MailNews Core :: Database, defect)
MailNews Core
Database
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 10.0
People
(Reporter: protz, Assigned: protz)
Details
(Whiteboard: [gloda key])
Attachments
(1 file, 2 obsolete files)
21.44 KB,
patch
|
asuth
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•13 years ago
|
||
FWIW, I just rebuild my gloda database with the patch on, and the size difference is negligible. I guess few messages have attachments...
Assignee | ||
Comment 2•13 years ago
|
||
Fix a typo in the test.
Attachment #552762 -
Attachment is obsolete: true
Attachment #552762 -
Flags: review?(bugmail)
Attachment #552814 -
Flags: review?(bugmail)
Comment 3•13 years ago
|
||
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-
Assignee | ||
Comment 4•13 years ago
|
||
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 5•13 years ago
|
||
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+
Assignee | ||
Comment 6•13 years ago
|
||
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.
Description
•