Created attachment 268803 [details] [diff] [review] Patch Okay, summary of this patch: nsIParser::Parse(nsIInputStream* aStream...) is unused. This is a good thing, because that codepath didn't work right anyway (assumes ASCII encoding). Also, the nsScanner constructor that has an aCreateStream parameter is always passed PR_FALSE. This allows for the primary simplification of this patch, getting rid of the mInputStream member of nsScanner. The rest of the patch is just fallout from that and removing unused code. Note that since mInputStream never gets set, FillBuffer() calls simplify to kEOF. Overall, should make nsScanner a bit more readable. (If we wanted to simplify nsScanner more, we should consider getting rid of nsScannerString.) (I should probably simplify the signature of the nsScanner constructor since aCreateStream is always false, but I'm not sure how; both constructors take an AString, and ACString, and a int.)
Attachment #268803 - Flags: review?(mrbkap)
(In reply to comment #0) > nsIParser::Parse(nsIInputStream* aStream...) is unused. My patch in bug 22942 uses it.
(In reply to comment #1) > (In reply to comment #0) > > nsIParser::Parse(nsIInputStream* aStream...) is unused. > > My patch in bug 22942 uses it. I would suggest not using it; that codepath is hasn't been in use for ages, it can't handle non-ASCII files, and you'll end up blocking the main thread for the entire load. You don't really need it anyway; just use your async codepath unconditionally. I doubt the performance ends up being significantly different. (On a side note, spinning an event loop in parser is unlikely to make reviewers very happy.)
Comment on attachment 268803 [details] [diff] [review] Patch I think that nsScannerString actually does have a measurable impact on performance (positively, of course), so we'll need to step carefully if we want to remove it.
Attachment #268803 - Flags: review?(mrbkap) → review+
Attachment #268803 - Flags: superreview?(jst) → superreview+
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.