Closed Bug 192284 Opened 22 years ago Closed 21 years ago

support nsIChannel::open for all protocols


(Core :: Networking, enhancement, P5)






(Reporter: darin.moz, Assigned: darin.moz)



(Keywords: topembed-)


(1 file, 2 obsolete files)

support nsIChannel::open for all protocols.  i know we once had a bug filed for
this, but i just can't seem to locate it.  anyhow, we decided yesterday that
there is need to provide implementations of the sync load for all protocols. 
the solution will most likely involve pushing an event queue to "fake" blocking
i/o on top of AsyncOpen.  this can be implemented as a helper class for use with
all the remote necko protocols.
Priority: -- → P3
Target Milestone: --- → mozilla1.4alpha
Plussing topembed nomination, discussed at 2/11 EDT meeting.
Keywords: topembedtopembed+
Target Milestone: mozilla1.4alpha → mozilla1.4beta
Discussed in topembed bug triage.  Minusing.
Keywords: topembed+topembed-
Severity: normal → enhancement
Priority: P3 → P5
Target Milestone: mozilla1.4beta → mozilla1.5beta
this implementation is not quite complete... remaining issues:

1- when can the consumer of the HTTP channel check the response headers? 
shouldn't we wait to return from nsHttpChannel::Open until we have the headers?
 NOTE: i think this can be easily implemented by just calling Available on the
nsIInputStream returned from NS_NewSyncStreamListener.

2- in this patch, i'm not pushing or popping event queues.  i'm just using the
existing/current event queue, which i think is ok.  i don't see much value in
pushing an event queue in this case.  NOTE: the event loop happens whenever
Available would return zero bytes available (assuming OnStopRequest has not yet
been called).
Blocks: 100022
Target Milestone: mozilla1.5beta → mozilla1.6alpha
Attached patch v1 patch (obsolete) — Splinter Review
final patch.  the only issue i have with this patch is that only some of the
channels implement nsIChannel::Open using AsyncOpen.  as a result, load group
and progress notifications will occur only for those channels which implement
Open via AsyncOpen.  this might lead to subtle bugs when a component starts
sync loading URLs of a different scheme.  we might want to fix this by making
all channels implement Open via AsyncOpen (or perhaps only when a load group
and/or progress event sink exist).
Attachment #128935 - Attachment is obsolete: true
Attachment #131697 - Flags: review?(dougt)
Comment on attachment 131697 [details] [diff] [review]
v1 patch

worse is better.  lets file a new bug for the known issue and check what what
you got in.  

Attachment #131697 - Flags: review?(dougt) → review+
Attachment #131697 - Flags: superreview?(bz-vacation)
Comment on attachment 131697 [details] [diff] [review]
v1 patch

erm, isn't this patch missing the new files?
sicking: yes it is.. oops!  though there was no change from the v0 patch in that
Attachment #131697 - Attachment is obsolete: true
Attachment #132155 - Flags: superreview?(bzbarsky)
Attachment #131697 - Flags: superreview?(bzbarsky)
once the patch for bug 210125 goes in, it would not be that difficult to
implement a load flag which would cause the sync load to not trigger any events
other than those needed by the nsSyncStreamListener.  such a load flag could be
used in places where we need sync loading, but cannot risk re-entrancy problems.
Blocks: 194198
Comment on attachment 132155 [details] [diff] [review]
v1.1 patch - including the new files this time ;-)

>Index: netwerk/base/public/nsISyncStreamListener.idl

>+interface nsISyncStreamListener : nsIStreamListener
>+    /**
>+     * Returns an input stream that when read will fetch data delivered
>+     * to the sync stream listener.  The nsIInputStream implementation
>+     * will wait for OnDataAvailable events before returning from Read.
>+     */

Do we want to explicitly comment that it will spin the current thread event
queue to get those events and that callers should be ready for it?

>Index: netwerk/base/public/nsNetUtil.h

>+ * Implement the nsIChannel::Open(nsIInputStream**) method using the channel's
>+ * AsyncOpen method.
>+ */

And maybe the same warning here.

>Index: netwerk/base/src/nsSyncStreamListener.cpp

>+nsSyncStreamListener::OnDataAvailable(nsIRequest     *request,
>+    nsresult rv = mPipeOut->WriteFrom(stream, count, &bytesWritten);
>+    if (NS_FAILED(rv))
>+        return rv;

Add a comment about why we can ignore bytesWritten (it'll be count for an
infinite pipe; we may want to add an assert to that effect) and maybe why we
don't set mStatus to rv here?

sr=bzbarsky with those nits addressed.	I'm sort of curious as to how the
existing callers of Open() will deal with this patch; perhaps we should make a
list of them and see how likely they are to hit this code?
Attachment #132155 - Flags: superreview?(bzbarsky) → superreview+
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.