Refactor fragment parsing out of nsHtml5Parser

RESOLVED FIXED in mozilla12

Status

()

Core
HTML: Parser
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Move fragment parsing out of nsHtml5Parser in order to avoid mixing incremental parsing and synchronous parsing from a string.
(Assignee)

Updated

6 years ago
Depends on: 715103
(Assignee)

Comment 1

5 years ago
Created attachment 589506 [details] [diff] [review]
Refactor fragment parsing out of nsHtml5Parser
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #589506 - Flags: review?(bugs)
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 589511 [details] [diff] [review]
Refactor fragment parsing out of nsHtml5Parser, with virtual destructor for nsHtml5StringParser

Oops. Forgot to make a destructor virtual.
Attachment #589506 - Attachment is obsolete: true
Attachment #589506 - Flags: review?(bugs)
Attachment #589511 - Flags: review?(bugs)

Updated

5 years ago
Attachment #589511 - Attachment is patch: true

Comment 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 590156 [details] [diff] [review]
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.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a75d09b8c1
(Assignee)

Comment 8

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

Comment 9

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