Closed Bug 714777 Opened 9 years ago Closed 9 years ago

Refactor fragment parsing out of nsHtml5Parser

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(2 files, 1 obsolete file)

Move fragment parsing out of nsHtml5Parser in order to avoid mixing incremental parsing and synchronous parsing from a string.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #589506 - Flags: review?(bugs)
Before this patch, nsHtml5Parser was used in two very different and disjoint ways. With this patch, it's clearer what's used for what, which makes the foundation nicer for adding HTML support to DOMParser.
Oh, and I called nsIParserBase "nsI", because it inherits from nsISupports and works in nsCOMPtr. I didn't add the ability to QI into nsIParserBase, though, because there's no need to support that and I don't want anyone else to start QIing into it and making deCOMtamination harder later. DeCOM is bug 719023.
Oops. Forgot to make a destructor virtual.
Attachment #589506 - Attachment is obsolete: true
Attachment #589506 - Flags: review?(bugs)
Attachment #589511 - Flags: review?(bugs)
Attachment #589511 - Attachment is patch: true
Comment on attachment 589511 [details] [diff] [review]
Refactor fragment parsing out of nsHtml5Parser, with virtual destructor for nsHtml5StringParser

>   // Drop our reference to the parser to get rid of a circular
>   // reference.
>-  nsCOMPtr<nsIParser> kungFuDeathGrip(mParser.forget());
>+  nsCOMPtr<nsIParserBase> kungFuDeathGrip(mParser.forget());
So this doesn't cause any "QueryInterface needed" assertions?
nsCOMPtr should be used only with interfaces (which you can QI to)
I think you could just use nsRefPtr


>-  nsCOMPtr<nsIParser>           mParser;
>+  nsCOMPtr<nsIParserBase>       mParser;
Also here

>   if (!sHTMLFragmentParser) {
>-    sHTMLFragmentParser =
>-      static_cast<nsHtml5Parser*>(nsHtml5Module::NewHtml5Parser().get());
>+    NS_ADDREF(sHTMLFragmentParser = new nsHtml5StringParser());
>     // Now sHTMLFragmentParser owns the object
>   }
>   nsresult rv =
>     sHTMLFragmentParser->ParseHtml5Fragment(aSourceBuffer,
>                                             aTargetNode,
>                                             aContextLocalName,
>                                             aContextNamespace,
>                                             aQuirks,
>                                             aPreventScriptExecution);
>-  sHTMLFragmentParser->Reset();
Fragment parser doesn't ever keep anything alive?


>+nsIParser*
>+nsXMLContentSink::GetParser()
>+{
>+  return static_cast<nsIParser*> (mParser.get());
>+}
Extra space after >

> nsXMLFragmentContentSink::DidBuildModel(bool aTerminated)
> {
>-  nsCOMPtr<nsIParser> kungFuDeathGrip(mParser);
>+  nsCOMPtr<nsIParserBase> kungFuDeathGrip(mParser);
nsRefPtr


>   // avoid crashing near EOF
>   nsRefPtr<nsHtml5TreeOpExecutor> kungFuDeathGrip(this);
>-  nsCOMPtr<nsIParser> parserKungFuDeathGrip(mParser);
>+  nsRefPtr<nsIParserBase> parserKungFuDeathGrip(mParser);
> 
And here you are actually using nsRefPtr...


>+#ifndef nsAParserBase_h_
>+#define nsAParserBase_h_
>+
>+#include "nsIChannel.h"
>+
>+class nsIParserBase : public nsISupports
Since this isn't a real interface, I'd prefer nsParserBase
And ifndef should reflect the class/file name
Attachment #589511 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> nsCOMPtr should be used only with interfaces (which you can QI to)
> I think you could just use nsRefPtr

Switched to using nsRefPtr everywhere.

> Fragment parser doesn't ever keep anything alive?

It only keeps its members (tokenizer, tree builder, executor) alive. At the end of the parse (before returning), it always makes the executor drop references to various things the executor likes to hold during the parse.

> >+nsIParser*
> >+nsXMLContentSink::GetParser()
> >+{
> >+  return static_cast<nsIParser*> (mParser.get());
> >+}
> Extra space after >

Fixed.

> Since this isn't a real interface, I'd prefer nsParserBase
> And ifndef should reflect the class/file name

Renamed.

Thanks for the review.
https://hg.mozilla.org/mozilla-central/rev/69a75d09b8c1
https://hg.mozilla.org/mozilla-central/rev/8bb4941a7f7d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.