Closed Bug 456247 Opened 13 years ago Closed 13 years ago

Add option to streamMessage to disallow streaming over the network

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: rain1, Assigned: rain1)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch v1 (obsolete) — Splinter Review
Currently, streamMessage will stream from the network even if we're offline or we don't want to. This isn't always desirable. The patch aims to fix this.

The patch basically does two things --
- it allows streaming over the network in case 

This is a fairly straightforward patch, with some code shuffled around and a bit of the control flow changed. There is only one cache lookup now.

David -- is this worth getting in for beta 1? Also, right now I've tried to keep as similar a behaviour as possible for the search component. Jeff mentioned that there should be a UI option whether to allow the component to download over the network. What do you think of such a thing post beta 1?

(There are some changes that are just whitespace, diff -w didn't figure them out though)
Attachment #339637 - Flags: superreview?(bienvenu)
Attachment #339637 - Flags: review?(bienvenu)
Thx for doing this! In general it looks good.

I know this is code that just moved, but you only need to check NS_SUCCEEDED or aStreamListener here., not both:

+    nsCOMPtr<nsIStreamListener> aStreamListener = do_QueryInterface(aDisplayConsumer, &rv);
+    if (NS_SUCCEEDED(rv) && aStreamListener)

Am I right that with this patch, no one is passing in PR_TRUE for aLocalOnly? If not, I don't see a good reason to get this in for b1, other than getting some testing, or allowing extensions to try it...
(In reply to comment #1)
> Am I right that with this patch, no one is passing in PR_TRUE for aLocalOnly?
> If not, I don't see a good reason to get this in for b1, other than getting
> some testing, or allowing extensions to try it...

The only thing I'm concerned about is that streamMessage in its current state might attempt to generate network traffic even if we're offline.

Otherwise, the patch doesn't really provide any critically useful features for extensions, either. So do you think this should land post b1?

(Sorry about the half-finished thought there -- the full thought is:
The patch basically does two things --
- it allows streaming over the network in case we aren't offline and aLocalOnly is false
- it doesn't allow it otherwise, and returns immediately with a failure instead of much later)
Attachment #339637 - Flags: superreview?(bienvenu)
Attachment #339637 - Flags: superreview+
Attachment #339637 - Flags: review?(bienvenu)
Attachment #339637 - Flags: review+
Comment on attachment 339637 [details] [diff] [review]
patch v1

thx for doing this. One other nit: I think we should remove the "now is where" part of this comment, since we don't have context anymore.

+  // now is where our behavior differs....if the consumer is the docshell then we want to
+  // run the url in the webshell in order to display it. If it isn't a docshell then just

I think I'd like this to land after b1, since no one will be taking advantage of the new functionality until after b1
carrying forward r+sr.

I also fixed an nsresult to read NS_IMETHODIMP.
Attachment #339637 - Attachment is obsolete: true
Attachment #341932 - Flags: superreview+
Attachment #341932 - Flags: review+
Keywords: checkin-needed
Checked in, changeset id 529:70420d2ee44c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1
You need to log in before you can comment on or make changes to this bug.