Closed
Bug 81384
Opened 23 years ago
Closed 23 years ago
multipart converter should be able to handle byteranges
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla0.9.1
People
(Reporter: dougt, Assigned: dougt)
References
Details
Attachments
(5 files)
7.27 KB,
patch
|
Details | Diff | Splinter Review | |
1.12 KB,
text/plain
|
Details | |
2.16 KB,
patch
|
Details | Diff | Splinter Review | |
1.55 KB,
text/plain
|
Details | |
7.61 KB,
patch
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•23 years ago
|
||
multipart converter should be able to handle byteranges
Severity: normal → critical
Summary: multipartmixed converter should be able to handle byteranges → multipart converter should be able to handle byteranges
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Comment 4•23 years ago
|
||
Comment 5•23 years ago
|
||
- some comments in the new idl would be nice (as self explainatory as it is), both iface level and method level. - while you're in here, please turn the PRInt8's into PRInt32's. PRInt8's are actually more expensive now as most arch's just build up a 32 bit int anyway and use trickery to make you think it's 8. - It's not clear to me what you're doing here: mFirstOnData = PR_FALSE; NS_ASSERTION(!mBufLen, "this is our first time through, we can't have buffered data"); const char * token = mToken; + + PRInt8 chars = 0; + if (*buffer == nsCRT::CR || *buffer == nsCRT::LF) { + if (buffer[1] == nsCRT::LF) + chars++; + chars++; + } if (bufLen < mTokenLen+2) { // we don't have enough data yet to make this comparison. // skip this check, and try again the next time OnData() // is called. mFirstOnData = PR_TRUE; - } else if (!PL_strnstr(buffer, token, mTokenLen+2)) { + } else if (!PL_strnstr(buffer + chars, token, mTokenLen+2)) { bufLen = read + mTokenLen + 1; char *tmp = (char*)nsMemory::Alloc(bufLen); if (!tmp) { a) you should just use PushOverLine() instead of expanding that linefeed char* incrementor. b) that will negate the need for the modified else if stmt. but, I don't see why you're doing this at all. - thanks for updateing w/ ignore-case. when this code was first written, we were lowercasing *all* headers. - in theory you'll have a mPartChannel all the way up until you send an OnStopRequest() to the final listener, but, after that, we explicitly null out the mPartChannel to break a circular dependency (see SendStop()). I'm a bit concerned about passing "this" as the channel on down the chain. pointer compares are common practice and the thought of a consumer getting the same channel back in two seperate On*() chain callback sets seems wierd. maybe mPartChannel (created for each "part") should be the nsIByteRangeRequest, and you should set the attribs through a a constructor for the mPartChannel.? That would protect from null mPartChannel's (those macros just look evil) and ensure that different channels get propogated to the consumer (definately a requirement at one point, maybe not anymore???). + NS_DECL_NSIBYTERANGEREQUEST + + // XXX what if mPartChannel is null + NS_FORWARD_NSICHANNEL(mPartChannel->) + NS_FORWARD_NSIREQUEST(mPartChannel->) I still can't believe there's a multipart/byteranges :-).
qa to me What would be good test case (theoretical or real world for this)?
QA Contact: tever → benc
Assignee | ||
Comment 7•23 years ago
|
||
- some comments in the new idl would be nice (as self explainatory as it is), both iface level and method level. simple enough - done. I will attach a new interface header. - while you're in here, please turn the PRInt8's into PRInt32's. PRInt8's are actually more expensive now as most arch's just build up a 32 bit int anyway and use trickery to make you think it's 8. done. - It's not clear to me what you're doing here: ... the first time the multipart/byterange data comes through, it has a CRLF that I need to remove so that the strstr does not fail. I was worried about using PushOverLine for no real reasons. - pointer compares are common practice and the thought of a consumer getting the same channel back in two seperate On*() chain callback sets seems wierd. Bad assumption!! There is no contract that this happens. - maybe mPartChannel (created for each "part") should be the nsIByteRangeRequest, and you should set the attribs through a a constructor for the mPartChannel.? Can't. mPartChannel is a generic class. I don't want to add the nsIByteRangeRequest to it. The forwarding macros should test for null if that is a problem. - I still can't believe there's a multipart/byteranges :-). I can't believe how easy it was to modify your converter to support byteranges!!! :-)
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
Comment 10•23 years ago
|
||
thanks doug. r=valeski after the _FORWARD() macros do null checking; mPartChannel can be null.
Comment 11•23 years ago
|
||
checked in the following diff to nsHttpChannel.cpp: Index: nsHttpChannel.cpp =================================================================== RCS file: /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v retrieving revision 1.10 diff -u -r1.10 nsHttpChannel.cpp --- nsHttpChannel.cpp 2001/05/17 21:38:06 1.10 +++ nsHttpChannel.cpp 2001/05/17 22:48:04 @@ -360,6 +360,7 @@ switch (httpStatus) { case 200: case 203: + case 206: rv = ProcessNormal(); break; case 300:
Assignee | ||
Comment 12•23 years ago
|
||
I checked the interface in. I will give the converter another round of testing, and I will check that in...
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•23 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 14•23 years ago
|
||
see bug #81937 for follow-up info...
You need to log in
before you can comment on or make changes to this bug.
Description
•