714777 - Refactor fragment parsing out of nsHtml5Parser

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Henri Sivonen (:hsivonen)]

Move fragment parsing out of nsHtml5Parser in order to avoid mixing incremental parsing and synchronous parsing from a string.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Refactor fragment parsing out of nsHtml5Parser

Comment 2

[Henri Sivonen (:hsivonen)]

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.

Comment 3

[Henri Sivonen (:hsivonen)]

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.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Refactor fragment parsing out of nsHtml5Parser, with virtual destructor for nsHtml5StringParser

Oops. Forgot to make a destructor virtual.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Patch with review comments addressed, for reference

(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.

Comment 7

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69a75d09b8c1

Comment 8

[Henri Sivonen (:hsivonen)]

Windows bustage follow-up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb4941a7f7d

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/69a75d09b8c1
https://hg.mozilla.org/mozilla-central/rev/8bb4941a7f7d