Status

()

Core
HTML: Parser
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Eli Friedman, Unassigned)

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 2

11 years ago
(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+
(Reporter)

Updated

11 years ago
Attachment #268803 - Flags: superreview?(jst)

Updated

11 years ago
Attachment #268803 - Flags: superreview?(jst) → superreview+
(Reporter)

Comment 4

11 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.