Closed Bug 668700 Opened 8 years ago Closed 8 years ago

centralize stream handling in search/filters

Categories

(MailNews Core :: Search, defect)

x86
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 8.0

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Details

Attachments

(1 file)

Attached patch proposed fixSplinter Review
This is fallout from the pluggable store work, but can be extracted out to reduce the size of that patch. The idea is to centralize the input stream handling in nsMsgSearchTerm, and get rid of unneeded params. (with pluggable stores, we'll get an input stream for the msg hdr using the pluggable store interface). This passes all unit tests (for me) so if it does break something, we'll probably need more unit tests.
Attachment #543322 - Flags: review?(kent)
Comment on attachment 543322 [details] [diff] [review]
proposed fix

I guess since this changes an interface, it needs sr.
Attachment #543322 - Flags: superreview?(neil)
Attachment #543322 - Flags: superreview?(neil) → superreview+
Comment on attachment 543322 [details] [diff] [review]
proposed fix

r=me with one small nit:

+  nsCOMPtr<nsISeekableStream> seekableStream(do_QueryInterface(m_inputStream));
+  rv = seekableStream->Seek(PR_SEEK_SET, offset);

You need a test to confirm non-null seekableStream. Yes I understand that with the current design, that is always true, but it would be very easy in the future to add the ability to set the inputStream, assuming that you start that stream from the beginning, and then this is a hidden trap. I would be happy if you just did not do the seek in that case, perhaps with a warning for now.
Attachment #543322 - Flags: review?(kent) → review+
fixed on trunk, tfv tb 8, but there's no version for that yet.
http://hg.mozilla.org/comm-central/rev/cf80c93a7d8d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(In reply to comment #3)
> fixed on trunk, tfv tb 8, but there's no version for that yet.
> http://hg.mozilla.org/comm-central/rev/cf80c93a7d8d

Did you mean target milestone?
Target Milestone: --- → Thunderbird 8.0
yeah, target milestone. Perhaps I needed to refresh the bug...thx for setting the milestone.
You need to log in before you can comment on or make changes to this bug.