The current form submission interface (nsIFormControl) is cumbersome (four functions where one will do); inefficient (it mallocs arrays when it does not need to); and non-reusable (intimately tied to our current set of HTML form controls). To address all these issues, form controls should implement an interface, nsISubmittable, which has one function in it: - submitNamesValues(nsIFormSubmitter* submitter, PRBool sendFiles) The form submitter's main loop will call this function once on each form control, which will look at its name and values and call one of these two functions on the form submitter: - addNameValuePair(nsAString& name, nsAString& value) - addNameValueFile(nsAString& name, nsAString& value, nsAString& filename) The form submitter, when it receives these names and values, will do whatever it needs to build the submit stream out of all names/values on the form. Because this interface allows you to add names and values one by one, it does not require the construction of *any* arrays of names and values. The form submitter can build its multipart or URL-encoded request dynamically, directly from this information. The filename will probably be replaced with a stream class or a string so that constructs can send any data they want as the "file." This will allow submittable OBJECTs to more easily send files based on arbitrary information.

can we punt the usage of nsFileSpec from the form submitter as we do this? :)

I have been doing some investigation, and it is possible to submit multiple files. Bug 44464 is filed on this. The interface must reflect it. Thus I propose we use an earlier proposal of sicking's and make the interface like this: addNameValuePair(name,value) addNameFilePair(name,filename,boolean moreFilesToCome) Reading the RFC and the method of communication, this seems to be what makes the most sense. We will do the actual "multipart/mixed" work for multiple file submission separately; the interface should just support it. Right now we will just send each file separately; since the form controls only support one file per name, that should not be a problem. To see how multiple files would be implemented, see RFC 1867 (http://www1.ics.uci.edu/pub/ietf/html/rfc1867.txt), section 6. That should give a pretty good flavor of how this will all work. bz: I think that's likely. sicking is working on a file stream class that should make our file usage much more ... glorious.

sicking's multiplexing input stream (going in as part of this) will fix bug 114106. Yay!

Just for discussion's sake, here is how Form Submission will work in the new world (after discussing and implementing I think we've come up with the best idea we can): FORMS are responsible for knowing *what* data to submit. FORM SUBMISSION objects are responsible for knowing *how* to submit. There will be multiple form submission classes, because urlencoded and multipart/form-data are encoded completely differently. In essence: 1. The form will grab the appropriate nsIFormSubmission object. 2. The form will walk through form.elements[] and have each element give their data to the naIFormSubmission object (calling AddNameValuePair() or AddNameFilePair() on it for each variable it wants to submit). 3. When the object receives data, it will build it into the query string or POST data or multipart/form-data string that it is going to send to the server. 4. The form will call "SubmitTo()" on the nsIFormSubmission object with the appropriate action and target, and the object will take the data it has marshalled, and submit it. In support of encoding the data, sicking has created an nsIMultiplexedInputStream, which essentially strings together input streams. This means we can submit the name/value pairs as a string stream, then submit the file as a file input stream, then more name/value pairs as a string stream, and then ... you get the idea. We won't have to read the files from the disk until they are actually submitted. Especially because we are getting rid of the arrays and not dumping all the data to a file, this is a big big win for performance.

This is a test comment, with the patch. Bugzilla: the ultimate form test.

OK, this has been tested against GET, POST and multipart w/files. I am actually submitting this patch using the patched build.

OK, this has been tested against GET, POST and multipart w/files. I am actually submitting this patch using the patched build.

Comment on attachment 67161 [details] [diff] [review] Patch Heh, looks like it needs work, it's not submitting the full length of file. Available() problems probably.

This version has been tested on local *and* remote with GET, POST, and multipart/form-data with all types of controls (http://www.johnkeiser.com/cgi-bin/mozform.pl). Especial attention given tto testing in Bugzilla, including queries, comments and *ahem* patch submission. Remaining tests include internationalization and BIDI. Everything you see here submitted with the patched build. This patch includes: - new improved control submission interface! - all new dazzling nsMIMEInputStream and nsMultiplexInputStream! - complete removal of the temporary file! - significant performance and readability enhancements! - fix to nsBufferedInputStream::Available() to report size of underlying stream

The problem before, BTW, was a Necko thing--file stream was not doing the right thing when you're talking to a remote pipe (i.e. a narrow pipe). That bug is filed as bug 122849. We've fixed it by using a buffered input stream like form sub was using before, which should speed things up anyway.

P.S. 100x thanks to sicking for the awesome streams, which are all his!

+ PRInt64 size; + rv = file->GetFileSize(&size); .... + rv = NS_NewLocalFileInputStream(getter_AddRefs(rawStream), file); What does all that do in cases when the file is not readable? Will we end up creating a raw stream? Or no? (/me hopes not).

I am not actually sure where in the process it fails, but it *does* fail, and I'm guessing it's at the stream. The upshot is, if the file does not exist, it submits as though the file input were input type=text (this is how we send it in method=get and method=post with url-encoded type, so it should be safe). I did just test this, both with a bad directory and a bad file within a good directory. Both work as expected. 'Twas definitely a good point.

- *result = mFillPoint - mCursor; - return NS_OK; + nsresult rv = NS_OK; + if (mStream) { + rv = Source()->Available(result); + } + return rv; shouldn't you return Source()->Available() + mFillPoint - mCursor

Doh! I'll put it into the next version. Meantime, this shouldn't fail for anyone and testing can continue. As long as Available() returns <= the total number of possible bytes to be read, everyone is obviously happy, since that's what we were doing before.

Comment on attachment 67338 [details] [diff] [review] Patch v1.1 assuming that it was intenisve tested r= alexsavulov for the form submission part (excluding the input stream stuff) great piece of job, John! (for subsequent patch versions just transfer the r= if changes are not substantial)

Remove the size argument from AddNameFilePair, the MIME stream relies on stream->Available() returning the correct size anyway, and you don't seem to use it. Tha |aSource| argument to AddNameValuePair and AddNameFilePair should IMHO be an nsIDOMHTMLElement. Since that's what you always use it as anyway there's no point in trying to hide it. If some object that doesn't implement nsIDOMHTMLElement submits it can just provide null as aSource. How about making UnicodeToNewBytes a non-static. That way you can remove the last 4 arguments to that function. Why do you need the StartEncode() functions? why not simply put that code in the Init() function? Same with EndEncode, couldn't that be put at the beginning of SubmitTo and have the respecitve handler implement OnSubmit, the codereuse is minimal IMHO (do rods and pollman really want those debugthingies still?) Did we really need that platformencoder? wasn't that unneccesary since the <input> opens the file?

Fixes buffered stream

Comment on attachment 67538 [details] [diff] [review] Patch v1.2 >Index: xpcom/io/nsIMIMEInputStream.idl >+#include "nsIInputStream.idl" >+ >+ >+[scriptable, uuid(dcbce63c-1dd1-11b2-b94d-91f6d49a3161)] >+interface nsIMIMEInputStream : nsIInputStream >+{ >+ attribute boolean addContentLength; >+ void init(); >+ void addHeader([const] in string name, [const] in string value); >+ void setData(in nsIInputStream stream); >+}; please add documentation to these methods. eg. what does this interface do? and to nsIMultiplexInputStream.idl as well. >Index: xpcom/io/nsMIMEInputStream.cpp >+/* >+ * The MIME stream separates headers and a datastream. It also allows >+ * automatic creation of the content-length header. >+ */ how is this generic enough to belong in xpcom/io? perhaps it really belongs in necko. >+ nsStringInputStreamConstructor(nsnull, >+ NS_GET_IID(nsIStringInputStream), >+ getter_AddRefs(mHeaderStream)); >+ nsStringInputStreamConstructor(nsnull, >+ NS_GET_IID(nsIStringInputStream), >+ getter_AddRefs(mCLStream)); do not hard code against this symbol. it's not meant for mass consumption. it would be better to introduce a NS_NewStringInputStream method or simply use do_CreateInstance() until we have an alternate constructor (and file a bug). unfortunately NS_NewStringInputStream currently is used for something else... yuck :-( >+/** >+ * Forward everything else to the mStream after calling initStreams() >+ */ >+ you might want to define an inline function that checks mStartedReading before calling initStreams to avoid the overhead of always calling initStreams. >Index: xpcom/io/nsMultiplexInputStream.cpp >+ rv = stream->Read(aBuf, aCount, _retval); >+ NS_ENSURE_SUCCESS(rv, rv); what if the underlying stream returns NS_BASE_STREAM_WOULD_BLOCK and you have already read something? then you should not return NS_BASE_STREAM_WOULD_BLOCK... you should return NS_OK + whatever you've already read. >Index: netwerk/base/src/nsBufferedStreams.cpp >- *result = mFillPoint - mCursor; >- return NS_OK; >+ nsresult rv = NS_OK; >+ *result = 0; >+ if (mStream) { >+ rv = Source()->Available(result); >+ } >+ *result += (mFillPoint - mCursor); >+ return rv; you need to verify that Seek() will not break this.

1. The reason for the size argument being there is so that you can send a network stream if you want. But now that I think of it, we'd have to make network streams implement Available() anyway. So, consider it removed. 2. Make aSource nsIDOMHTMLElement. Good point. Will do. 3. I've been going back and forth on UnicodeToNewBytes(). I suppose for now it couldn't hurt to make it a class method though; and it would make the code easier to read. 4. StartEncoding() / EndEncoding(): we could move StartEncoding() into Init(), yes. Then we'd have to add the requirement that subclasses call nsFormSubmitter::Init() before they do their work. I am not sure what clean alternative there is for EndEncoding(). What you say may actually make things better; we wouldn't have to have GET return a null stream, it could just not worry about POST streams at all. But that, again, is transport, not encoding. (In fact, slapping the MIMEStream onto it is probably transport too.) I'll think about it. 5. PlatformEncoder handles the encoding of the filename. Filenames are always encoded in the encoding of the platform. I am queasy about these semantics (doesn't it seem like file values should *always* be interpreted with platform encoding?), but rather than change that too, I think we should fix it separately (it is already acting this way). 6. I'll start removing rods's and pollmann's debug thingies :)

BTW, so far I have tested this patch on GET, POST and multipart, local *and* remote, on debug, optimized and non-BIDI builds. I have tested internationalization as far as I am capable. Remaining tests: BIDI, Windows, Mac (needs stuff to check new files in on the Mac for sure).

1. Removed size arg from AddNameFilePair() 2. Changed aSource arg from nsIContent* to nsIDOMHTMLElement* 3. Made UnicodeToNewBytes non-static 4. Removed some of rods's and pollman's debug thingies None of these changes should materially affect anything. Submitting patch with patched build to be sure. Input stream changes still need to go in.

Marking nsbeta1+

not to self and jkeiser: platformencoder is due to bug 29062

more notes: if bug 124628 turns out to be a good suggestion we can get rid of the ::Init functions in nsFormSubmitter and nsMIMEInputStream if we wanted

review comments on Johns code: + if (processedValue) { + delete processedValue; + } just do |delete processedValue| (twice) + mQueryString = NS_LITERAL_CSTRING(""); mQueryString.Truncate(); Why is the aFilename argument an nsAString* while all others are nsAString&? nsAString& is better IMHO. For URL-encoded POST is there really any need to create a MIME stream? It would be more optimised to just put everything in a single string stream. Use iterators to cut-n-paste the #-stuff and the ?-stuff + if (namedAnchor.IsEmpty()) { + path.Append(namedAnchor); + } is this *really* intended ;-) IMHO you could always just append namedAnchor, appending an empty string should be as fast as the extra |if| I wonder if you should convert linebreaks in the name too in nsFSMultipartFormData::AddNameValuePair. Use the errorvalue from do_CreateInstance and do_GetService and NS_ENSURE_SUCCESS(rv, rv) instead of checking the returnvalue for null. in nsFSMultipartFormData::AddPostDataStream, do an NS_ENSURE_SUCCESS(rv, rv) after creating the stringstream + if (!*aFormSubmission) { + return NS_ERROR_OUT_OF_MEMORY; + } NS_ENSURE_TRUE(*aFormSubmission, NS_ERROR_OUT_OF_MEMORY); nsFormSubmission::GetSubmitCharset: The |nsIPresContext* aPresContext| argument doesn't seem to be used, and please make the outparam the last argument. The code doesn't pass through the BIDI part when accept-charset attribute is used, but I'm not sure if that's intentional. The function should an nsresult. Shouldn't the XXX comment be removed since we do get the charset from accept-charset first? Use iterators when searching the accept-charset value for a charset. The list of charsets is space and/or commaseparated. Though you maybe wanna do that in a different bug? nsFormSubmission::GetEncoder: + rv = nsServiceManager::GetService(kCharsetConverterManagerCID , use an nsCOMPtr and do_GetService instead (I think that's possible) + if (encoder) { + rv = NS_ERROR_FAILURE; + } uhoh :-), this seems to be an error from the first submission rewrite, and was an error even in the FormFrame (should be *encoder). Just do the NS_ENSURE_SUCCESS on rv instead. the nsFSURLEncoded::URLEncode functions seems to have strayed from the other nsFSURLEncoded functions? I wonder if you should inline UnicodeToNewBytes into EncodeVal, that seems to be the only place it's called (once the platformencoder is gone). Not sure if you had some special plans for that function thouhg? I got that impression before. That's it for nsFormSubmission.cpp

oh, and this was of course submitted with a patched build ;-)

nsHTMLInputElement::SubmitNamesValues should always cut the path from the filename when not submitting the file. nsHTMLSelectElement::SubmitNamesValues (some of this is old code, but it would be nice to get it fixed now): + // + // Get the value + // + nsAutoString value; + rv = GetValue(value); + if (NS_FAILED(rv)) { + return rv; + } copy'n'paste error? mOptions->ItemAsOption(optIndex, getter_AddRefs(option)); if (option) { this should always succeeded, right? use NS_ENSURE_TRUE instead rv = option->GetSelected(&isSelected); if (NS_FAILED(rv) || !isSelected) { continue; only |continue| on !isSelected, NS_ENSURE_SUCCEESS rv nsCOMPtr<nsIOptionElement> optionElement = do_QueryInterface(option); if (optionElement) { less if, more ENSURE :), or shouldn't an assertion be enough? Just nsHTMLFormElement left...

please add some small comments on the new nsHTMLFormElement methods. The |nsIPresContext* aPresContext| argument to DoReset doesn't seem to be used nsHTMLFormElement::DoSubmit, less if more ENSURE. You really only need a single if in that function. (sorry, i'm allergic to this) nsHTMLFormElement::NotifySubmitObservers: well, you know, ENSURE... + nsIDocument* document; + GetDocument(document); use mDocument instead, but make sure it's != null + nsCOMPtr<nsIFormSubmitObserver> formSubmitObserver( + do_QueryInterface(inst, &rv)); don't do use &rv here. It's an exception to the rule. Reason being that there is basically only one thing that can fail for QIs (at least i think that's the reason). + nsresult notifyStatus = formSubmitObserver->Notify(this, + nsresult notifyStatus = formSubmitObserver->Notify(this, + window, + aActionURL, + aCancelSubmit); + if (NS_FAILED(notifyStatus)) { // assert/warn if we get here? + return notifyStatus; + } why not just |rv =|? And use NS_ENSURE_SUCCESS, that way we'll get the assertion + if (*aCancelSubmit) { + return NS_OK; + } I'm not sure on this, but i think we should notify all observers, even if one tells us to cancel. What did the old code do? nsHTMLFormElement::CompareNodes: + rv = a->GetParentNode(getter_AddRefs(aParent)); + if (NS_FAILED(rv)) { + return rv; + } I'm not sure what we wanna return if some node isn't in the tree (doesn't have a parent)? Should it be an error? + nsCOMPtr<nsIContent> aParentContent(do_QueryInterface(aParent)); + nsCOMPtr<nsIContent> aContent(do_QueryInterface(a)); + rv = aParentContent->IndexOf(aContent, aIndex); you need to check if the first QI is successfull, not all nodes are nsIContents, documents and attributes for example. nsHTMLFormElement::GetActionURL: remove the comment about needing the document to notify observers since that's not done here any more. + nsIDocument* document; + GetDocument(document); use mDocument instead. please make a note in the function-description that the submission should be aborted if no url is returned, for example for security reasons. The code for when the href is empty is very strange IMHO. Now it uses the base-url as href and then resolves it against the base-url again! Just set actionURL = docURL if the href is empty. use ENSURE instead of the if after |securityManager->IsCapabilityEnabled|. bbaetz also asked that we put a comment above this code that it has never been really tested, since mailto: forms don't work in moz. Thats it! Excellent work!

Sorry, i should say that most of my nits are on code that's not origianlly Johns, but is copied from old code nsFormFrame. So thanks a million for fixing this!

Great feedback! This addresses all of sicking's issues except: 1. "just do |delete processedValue| (twice)" - I don't get this. processedValue could be null. You don't delete nulled memory. 2. aFilename was made an nsAString* so that it could be sent as |nsnull| for interfaces that didn't have a filename. But from RFC 1867, we're supposed to try real hard to supply the filename, so we'll just make 'em send blank if there is no filename. 3. MIME stream for POST -- yeah, but MIME stream is so cool! The performance problems are negligible, and you get the (also negligible in this case) benefit of re-using the code. It *does* look more pretty and easy to maintain, however, which is a very real benefit. 4. Linebreak conversion happens everywhere -- it's a MIME thing. 5. Our accept-charset functionality is somewhat broken anyway, I *would* rather fix it up in another bug. 6. UnicodeToNewBytes seems like a useful entity on its own. I'd rather leave it there rather than try to rip it out of EncodeVal when I need it later. 7. 2 or 3 others we talked about and agreed upon in IRC. We are now using this in both Linux and Windows builds. Optimized and debug has been tested. Internationalization has been tested. Remaining: - test BIDI and Mac - decide how or whether to make the streams threadsafe

> 1. "just do |delete processedValue| (twice)" - I don't get this. > processedValue could be null. You don't delete nulled memory. delete (unlike free) checks for null, so |delete nsnull;| is safe. > 3. MIME stream for POST -- yeah, but MIME stream is so cool! The performance > problems are negligible, and you get the (also negligible in this case) > benefit of re-using the code. It *does* look more pretty and easy to > maintain, however, which is a very real benefit. Sounds fine > 4. Linebreak conversion happens everywhere -- it's a MIME thing. But you have explicit code that does it for the value, but none for the name? > 5. Our accept-charset functionality is somewhat broken anyway, I *would* > rather fix it up in another bug. Sounds fine. I was terribly confused by that code. > 6. UnicodeToNewBytes seems like a useful entity on its own. I'd rather leave > it there rather than try to rip it out of EncodeVal when I need it later. Okidoki

- Made streams threadsafe - Fixed delete to not do if ()

in the declaration of GetEncoder, change |mCharset| to |aCharset| (it's right everywhere else) You've forgot to change the filename from nsAString* to nsAString& Do the changes to <input type=file> we talk about on irc; change the |if (file) {| to ENSURE, change the |} else {| to |if(!submittedValue)| Rename EndEncoding to GetEncodedSubmission the |if (dataStream) {| isn't needed in nsFSURLEncoded::EndEncoding in nsFormSubmission::SubmitTo: + if (NS_OK == aPresContext->GetLinkHandler(getter_AddRefs(handler))) { don't compare to NS_OK, use NS_SUCCEEDED instead. in nsHTMLFormElement::CompareNodes you still need to check that the QI's to nsIContent succeeds (at least on the parent). with that, r=sicking

Fixed all issues in previous comment. This is a pretty minor change.

Comment on attachment 68936 [details] [diff] [review] Patch v1.4.2 nit: don't change the indentation in nsBufferedStreams.cpp mac project file changes? otherwise, sr=darin on the netwerk and xpcom stream changes.

- Mac build changes (we are tested on the Mac now!) - small stream changes (my r= comments)

Comment on attachment 69292 [details] [diff] [review] Patch v1.4.3 r=jkeiser on streams sr=darin on streams r=sicking on form changes

There is other information on this earlier in the bug, but here is the broad overview of this patch (what happens when you call nsHTMLFormElement::OnSubmit()): GET THE SUBMISSION OBJECT 1. OnSubmit(): Gets an nsIFormSubmission instance that knows how to encode the way you want to encode. ENCODE THE NAME/VALUE PAIRS 2. Walk over all the form controls you want to submit and call SubmitNamesValues(myFormSubmission) on them. 3. Each of these form controls is then responsible for calling myFormSubmission->AddNameValuePair() (or AddNameFilePair()). 4. AddNameValuePair() takes the name and value and gets it ready for shipping (in the case of url-encoded, it appends "&name=value" to a big string). SUBMIT THE INFORMATION 5. nsIFormSubmission::SubmitTo(): knows how to take this info and send it to the specified action/target. For example, the url-encoded submission class appends the encoded string to the URL or places it into a POST data stream, depending on the method specified. MULTIPART Multipart submission uses an nsIMultiplexInputStream to store its data. This is is a stream composed of other streams--string streams and file streams, in our example--that will be concatenated together as though they were one stream (they will be read sequentially). Multipart takes name/value pairs and turns them into MIME sections of data that contain the name and value. When it receives a file, it creates the headers for the MIME section that will contain the file, wraps up a string stream with those headers, and adds that to the multiplexed stream. It then adds the file input stream to the multiplexed stream as well. For efficiency purposes, we store as many sections as possible in a string stream--for example, if there are 8 input type=texts, one input type=file, and then 4 more input type=texts, you will have 3 streams--one string stream, one file stream, and one more string stream) MIMESTREAM Both multipart and POST data streams use nsIMIMEStream(), which is a nice helper stream that lets you set the headers and also, as a bonus, calculates the size of the data for a dynamic Content-Length: header using the Available() method on the stream (which reports the size for all streams we are using). This last feature is necessary in case you reload a file submission URL (reposting the POST data stream) and the size of one of the files being submitted has changed. This is the relevant information that I can think of. I endeavored not to change charset or BIDI except to use our string classes better in some cases where it was doing excessive string copies (it is still doing more than it needs to).

- The aTarget argument to nsIFormSubmission::SubmitTo() should be const, no? And argument descriptions are missing from the header. - The aSource argument to nsIFormSubmission is of type nsIDOMHTMLElement*, should we not leave this as an nsIDOMElement* for potential future XForms n' who knows what standards that come along? - What's up with all those "protected:"'s in nsFormSubmission :-) More later...

- In nsFSURLEncoded::GetEncodedSubmission(): + mimeStream->SetData(dataStream); + + *aPostDataStream = mimeStream; + NS_ADDREF(*aPostDataStream); + + NS_ASSERTION(*aPostDataStream, + "Unable to create POST data stream! Bad bad things."); That assertion is pointless, it will never be hit, you'll crash before getting there. - ... and just below that, I'd prefere an early return if |schemeIsJavaScript| is true. - ... and below that: + path.Append(mQueryString); + + // Bug 42616: Add named anchor to end after query string + path.Append(namedAnchor); Could you not use path.Append(mQueryString + namedAnchor) here to eliminate a .Append() call? - In nsFSURLEncoded::URLEncode(): + if (!inBuf) + inBuf = ToNewCString(aString); ... + delete [] inBuf; Double space before '=', no OOM checking, and free/delete missmatch. Use nsMemory::Free() and not delete[] on the string (this would generate FMM's (Free Memory Mismatch) in Purify). - Also, if you'd make nsFSURLEncoded::URLEncode() take a nsCString& (or nsACString&) out parameter you would save an allocation per call since you don't need to allocate the returned nsCString. - The argument list in the declaration of the nsFSMultipartFormData constructor don't line up wel... - In nsFSMultipartFormData::AddNameValuePair(), using an nsCAutoString just to Adopt() something into it doesn't make much sense, or does it? nsCString seems more appropriate (and cheaper). - ... also: + mPostDataChunk += NS_LITERAL_CSTRING("--") + mBoundary + + NS_LITERAL_CSTRING(CRLF) + + NS_LITERAL_CSTRING("Content-Disposition: form-data; name=\"") + + nameStr + NS_LITERAL_CSTRING("\"" CRLF) + + NS_LITERAL_CSTRING(CRLF) This results in a double CRLF, is that intentional? If so, you could eliminate one string concatenation if you move the latter up into the one before it. - In nsFSMultipartFormData::AddNameFilePair(), inconsistent variable initialization: + nsString* processedValue; + processedValue = ProcessValue(aSource, aName, aFilename); How about: + nsString* processedValue = + ProcessValue(aSource, aName, aFilename); in stead? Also nsCAutoString/Adopt here... - ... also: + filenameStr.Adopt(nsLinebreakConverter::ConvertLineBreaks(filenameStr.get(), + nsLinebreakConverter::eLinebreakAny, + nsLinebreakConverter::eLinebreakNet)); bad next line argument indentation. - ... and futher down: + + NS_LITERAL_CSTRING(CRLF) + + NS_LITERAL_CSTRING(CRLF); make that NS_LITERAL_CSTRING(CRLF CRLF), no? - In nsFSMultipartFormData::Init(): + mPostDataStream = do_CreateInstance( + "@mozilla.org/io/multiplex-input-stream;1", &rv); Moving the |do_CreateInstance(| down to the next line would be clearer, no? - In nsFSMultipartFormData::GetEncodedSubmission(): + nsCOMPtr<nsIMIMEInputStream> mimeStream( + do_CreateInstance("@mozilla.org/network/mime-input-stream;1", &rv)); use var = ... in stead of var(...) here for clarity. - ... and below that: + mimeStream->AddHeader("Content-Type", + PromiseFlatCString( + NS_LITERAL_CSTRING("multipart/form-data; boundary=") + + mBoundary).get()); We know NS_LITERAL_STRING(...) + mBoundary is not a flat string, so I would guess using a nsCAutoString here directly would be cheaper. - In nsFSMultipartFormData::AddPostDataStream(): + rv = NS_NewCStringInputStream(getter_AddRefs(postDataChunkStream), + mPostDataChunk); ^<-------------| - ... and: + mPostDataChunk = NS_LITERAL_CSTRING(""); I guess this works, but why not simply mPostDataChunk.Truncate()? - In GetSubmissionFromForm(): + nsCOMPtr<nsIFormProcessor> formProcessor = + do_GetService(kFormProcessorCID, &rv); two space indentation is enough here... - ... and: + ((nsFormSubmission*)*aFormSubmission)->Init(); use NS_STATIC_CAST here to make the compiler catch more potential future problems... More comments on its way...

- In nsFormSubmission::SubmitTo(): + nsCOMPtr<nsGenericElement> formElement = do_QueryInterface(aForm); Eek, you can't do that. nsGenericElement doesn't have an IID, you don't know what'll come out of that. QI to an interface you know nsGenericElement implements, and static cast from that. Hmm, it seems like the only reason you do this is to get to an nsIContent interface pointer, you should QI directly to nsIContent then. - ... also: + if (NS_SUCCEEDED(aPresContext->GetLinkHandler(getter_AddRefs(handler)))) { + if (handler) { No need for the double error checking here, just the null check is enough. - In nsFormSubmission::GetSubmitCharset(): + if (NS_CONTENT_ATTR_HAS_VALUE == rv) { + if (eHTMLUnit_String == value.GetUnit()) { What you're asking here is if var == constant, not if constant == var, so swap them around (I bet you copied this from somewhere :-) and maybe combine those two checks into one if check too? - ... that method could use some string iterator love, you could eliminate a .Mid() call with Substring()'s n' all that cool stuff, your call. - In nsFormSubmission::UnicodeToNewBytes(), inconsisten use of spaces around operators. - In nsFormSubmission::GetEnumAttr(): no default value for the out parameter. (and backwards constant == val check too :-) - In nsFormSubmission::ProcessValue(): Do we really need to do so many string copies there? Can we use nsAutoString if we really do need all those copies? Couldn't the method take a nsAString& out parameter in stead of having to allocate the result string? Also, one space missing from the next line arbument in the declaration. More comments later...

Changed everything so far except: - The aSource element to AddNameValuePair() should stay as an nsIDOMHTMLElement until we change the nsIFormProcessor interface to be an nsIDOMElement (filed as bug 125555) - string iterator love in GetSubmitCharset(): sicking had the same comment :) That function was copied almost verbatim and I would rather not mess with it ATM if I could. charsets are going to be getting some lovin' later anyway, I can mess with it then. (The main reason I don't want to do it now is Bo Don't Know Iterators and these are the last days of this patch.) - In nsFormSubmission::UnicodeToNewBytes(), inconsistent use of spaces around operators: I just went through and tried to consistent-ize them as much as possible, but note that there is still not *always* a space between the operator and the args; I try to use my judgement on readability there. When in an = I use spaces; when there are only two args and it's in brackets or a for() I tend to not use them. (The inconsistent spacing before was due to copied code, BTW--I was trying not to modify character encoding code.) - In nsFormSubmission::GetEnumAttr(), there can be no proper default value. Callers supply that themselves, it is different for every attribute. - In nsFormSubmission::ProcessValue(), again, this is a faulty nsIFormProcessor interface. Bug 125555 handles the nsString issue as well. The return value of nsString* is due to the interface again--nsCAutoString won't work. I think we are doing a minimum of copies; unfortunately, the minimum is a lot. (The return value of nsString* rather than dumping stuff into nsCString& is also because mFormProcessor does not exist a lot of the time and I don't want to even bother *initializing* a string for its return value if I don't have to.) I'll wait to post the updated patch until you are done with the sr, unless you want to see it now?

- In nsHTMLButtonElement::SubmitNamesValues(): + rv = aFormSubmission->AddNameValuePair(this,name,value); Space-after-commas for consistency. - In nsHTMLFormElement::NotifySubmitObservers(), make contractId static. - In nsHTMLFormElement::CompareNodes(), don't name local variables |aFoo|, i.e. rename aParent, aContent, and aParentContent, those names make them look like method arguments in the mozilla code. - In nsHTMLFormElement::WalkFormElements(): + nsCOMPtr<nsIFormControl> image = +do_QueryInterface(imageElementNode); Indent two spaces from the line above, if the whole thing doesn't fit in 80 columns. - In nsHTMLFormElement::GetActionURL(): + if (!mDocument) return NS_OK; // No doc means don't submit, see Bug 28988 Don't put the if condition and the statement on the same line, and add braces too. There's more of those in that same function. Fix those issues as well (unless you disagree, then we'll talk, your comments above make sense), and you'll have sr=jst

- fixes all JST's issues except those listed in my comment

OK, this patch has passed the Bugzilla Acid Test. jst, sicking, you OK with it?

Comment on attachment 69573 [details] [diff] [review] Patch v1.4.4 Fine with me if you went through all those issues, which it seems like you did. sr=jst

Comment on attachment 69573 [details] [diff] [review] Patch v1.4.4 r=sicking

On a linux CVS build, i've started to get a lot of crashes very recently. Not sure all are related: i've crashed when clicking links as well as submit buttons. One that has repeated several times now is when clicking "Submit Query" or "Commit" button here in bugzilla. I believe this checkin was made in the timeframe between good/bad build. Attaching backtrace, in case it's worth looking at.

Hmm. I am not seeing the crash, but looking at the stack trace and the code, it's probably crashing when it tries to get nsCharsetConverterManager's contract ID: mCharsetConverterManager = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv); rkaa, what Internationalization settings do you have on your browser? We might be able to reproduce better with that.

Actually, if you could do me a favor and open a new bug report on that, it would be appreciated. Fix checked in.

rkaa: I managed to reproduce, I think I've found the problem. I'll file the bug and make the patch when I have surfed it for a while and it doesn't crash.

This is total dogfood for me...I've crashed six times in the past five minutes doing typical work with Mozilla: searching lxr, querying bugzilla, looking up incidents on talkback. I'm going to have to use 4.x this weekend. In the future, it seems like such a large rewrite of fundamental functionality would benefit from test builds.

Blake: Why 4.x and not 0.9.8?

> I'll file the bug > and make the patch when I have surfed it for a while and it doesn't crash. jkeiser: bug # ?

It's fixed already (fix went in Saturday) ... bug 125950.

Cannot verify this since it is code level. Can one of the developers please verify?

looking at lxr i see that the new code sucks much less then the old :-) verifying