Closed
Bug 252344
Opened 21 years ago
Closed 21 years ago
FTP does never show "looking up host" status message
Categories
(Core Graveyard :: Networking: FTP, defect)
Core Graveyard
Networking: FTP
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8alpha3
People
(Reporter: Biesinger, Assigned: Biesinger)
Details
Attachments
(1 file, 2 obsolete files)
7.90 KB,
patch
|
darin.moz
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
when going to an ftp url, the status bar does not show any messages about
"looking up host", and not about "connecting to host" initially either.
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #153832 -
Flags: review?(darin)
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha3
Comment 2•21 years ago
|
||
Comment on attachment 153832 [details] [diff] [review]
patch
aren't there cases where you might leak |eventSink|? Perhaps you should wrap
the newly created eventSink with a nsRefPtr prior to passing it to
nsFtpControlConnection::Connect.
also, i don't think you really want to pass NULL as the second parameter to
SetEventSink. doing that causes the events to happen on the socket transport
thread, and your code appears to then call out to the nsIProgressEventSink impl
on the socket thread!
call NS_GetCurrentEventQ and pass that as the second parameter. see for
example the code that sets a nsITransportEventSink for the data connection.
Attachment #153832 -
Flags: review?(darin) → review-
Assignee | ||
Comment 3•21 years ago
|
||
(In reply to comment #2)
> (From update of attachment 153832 [details] [diff] [review])
> aren't there cases where you might leak |eventSink|? Perhaps you should wrap
> the newly created eventSink with a nsRefPtr prior to passing it to
> nsFtpControlConnection::Connect.
oh, hm, yeah. will fix.
> also, i don't think you really want to pass NULL as the second parameter to
> SetEventSink. doing that causes the events to happen on the socket transport
> thread, and your code appears to then call out to the nsIProgressEventSink impl
> on the socket thread!
Yes, but I think that's fine. the nsIProgressEventSink is the nsFTPChannel, and
that has a proxy to the "real" event sink:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFTPChannel.cpp#508
Assignee | ||
Comment 4•21 years ago
|
||
Attachment #153832 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #154591 -
Flags: review?(darin)
Comment 5•21 years ago
|
||
Comment on attachment 154591 [details] [diff] [review]
patch v2
oh, ok... i didn't realize mSink was already a proxy. that's probably somewhat
leftover from the days when FTP allocated an actual thread for the state logic.
ok... looks good to me.
though, you should know that the transport event sink proxy object is nice
because it avoids clogging the event queue with more than one status/progress
event at a time. it can be configured to be lossy like that in cases where
some loss of status/progress information is tolerable.
doesn't TransportEventForwarder need to support threadsafe AddRef and Release
at least?
Comment 6•21 years ago
|
||
Comment on attachment 154591 [details] [diff] [review]
patch v2
r=darin with NS_IMPL_THREADSAFE for TransportEventForwarder.
Attachment #154591 -
Flags: review?(darin) → review+
Assignee | ||
Comment 7•21 years ago
|
||
Comment on attachment 154591 [details] [diff] [review]
patch v2
hmm, as mSink is an nsFTPChannel, this patch causes assertions about ftp
channel's lack of threadsafe isupports.
all in all, it's probably better to specify an eventq.
Attachment #154591 -
Attachment is obsolete: true
Assignee | ||
Comment 8•21 years ago
|
||
now using current thread's eq
Assignee | ||
Updated•21 years ago
|
Attachment #155211 -
Flags: review?(darin)
Updated•21 years ago
|
Attachment #155211 -
Flags: review?(darin) → review+
Assignee | ||
Updated•21 years ago
|
Attachment #155211 -
Flags: superreview?(bzbarsky)
![]() |
||
Comment 9•21 years ago
|
||
Comment on attachment 155211 [details] [diff] [review]
patch v3
sr=bzbarsky
Attachment #155211 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 10•21 years ago
|
||
Checking in netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v <--
nsFtpConnectionThread.cpp
new revision: 1.287; previous revision: 1.286
done
Checking in netwerk/protocol/ftp/src/nsFtpControlConnection.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpControlConnection.cpp,v <--
nsFtpControlConnection.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in netwerk/protocol/ftp/src/nsFtpControlConnection.h;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpControlConnection.h,v <--
nsFtpControlConnection.h
new revision: 1.23; previous revision: 1.22
done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•1 year ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•