Add option to streamMessage to disallow streaming over the network

RESOLVED FIXED in Thunderbird 3.0b1

Status

MailNews Core
Backend
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sid0, Assigned: sid0)

Tracking

(Blocks: 1 bug)

unspecified
Thunderbird 3.0b1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 339637 [details] [diff] [review]
patch v1

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)
(Assignee)

Updated

9 years ago
Attachment #339637 - Flags: superreview?(bienvenu)
Attachment #339637 - Flags: review?(bienvenu)

Comment 1

9 years ago
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...
(Assignee)

Comment 2

9 years ago
(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)

Updated

9 years ago
Attachment #339637 - Flags: superreview?(bienvenu)
Attachment #339637 - Flags: superreview+
Attachment #339637 - Flags: review?(bienvenu)
Attachment #339637 - Flags: review+

Comment 3

9 years ago
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
(Assignee)

Comment 4

9 years ago
Created attachment 341932 [details] [diff] [review]
patch for checkin

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+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Checked in, changeset id 529:70420d2ee44c
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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.