Last Comment Bug 714777 - Refactor fragment parsing out of nsHtml5Parser
: Refactor fragment parsing out of nsHtml5Parser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla12
Assigned To: Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
:
:
Mentors:
Depends on: 715103
Blocks: 102699
  Show dependency treegraph
 
Reported: 2012-01-03 07:00 PST by Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
Modified: 2012-01-21 06:56 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Refactor fragment parsing out of nsHtml5Parser (60.32 KB, patch)
2012-01-18 07:48 PST, Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
no flags Details | Diff | Splinter Review
Refactor fragment parsing out of nsHtml5Parser, with virtual destructor for nsHtml5StringParser (60.32 KB, patch)
2012-01-18 08:06 PST, Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
bugs: review+
Details | Diff | Splinter Review
Patch with review comments addressed, for reference (60.32 KB, patch)
2012-01-20 02:22 PST, Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
no flags Details | Diff | Splinter Review

Description Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-03 07:00:58 PST
Move fragment parsing out of nsHtml5Parser in order to avoid mixing incremental parsing and synchronous parsing from a string.
Comment 1 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-18 07:48:00 PST
Created attachment 589506 [details] [diff] [review]
Refactor fragment parsing out of nsHtml5Parser
Comment 2 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-18 07:49:17 PST
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) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-18 07:51:26 PST
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) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-18 08:06:21 PST
Created attachment 589511 [details] [diff] [review]
Refactor fragment parsing out of nsHtml5Parser, with virtual destructor for nsHtml5StringParser

Oops. Forgot to make a destructor virtual.
Comment 5 Olli Pettay [:smaug] (reviewing overload) 2012-01-19 14:30:43 PST
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) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-20 02:22:51 PST
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.
Comment 7 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-20 03:21:39 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a75d09b8c1
Comment 8 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2012-01-20 04:05:15 PST
Windows bustage follow-up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb4941a7f7d

Note You need to log in before you can comment on or make changes to this bug.