668700 - centralize stream handling in search/filters

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[David :Bienvenu]

Attachment: proposed fix

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.

Comment 1

[David :Bienvenu]

Comment on attachment 543322 [details] [diff] [review]
proposed fix

I guess since this changes an interface, it needs sr.

Comment 2

[Kent James (:rkent)]

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.

Comment 3

[David :Bienvenu]

fixed on trunk, tfv tb 8, but there's no version for that yet.
http://hg.mozilla.org/comm-central/rev/cf80c93a7d8d

Comment 4

[Mark Banner (:standard8)]

(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?

Comment 5

[David :Bienvenu]

yeah, target milestone. Perhaps I needed to refresh the bug...thx for setting the milestone.