Last Comment Bug 612839 - Remove nsAHtml5FragmentParser
: Remove nsAHtml5FragmentParser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: mozilla8
Assigned To: Marco Castelluccio [:marco]
:
:
Mentors:
Depends on:
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2010-11-17 01:21 PST by Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03)
Modified: 2011-08-15 03:44 PDT (History)
7 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removed nsAHtml5FragmentParser (10.64 KB, patch)
2011-07-28 07:22 PDT, Marco Castelluccio [:marco]
hsivonen: review+
Details | Diff | Splinter Review
Removed nsAHtml5FragmentParser v2 (13.25 KB, patch)
2011-08-02 14:12 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Removed nsAHtml5FragmentParser v3 (13.47 KB, patch)
2011-08-02 15:23 PDT, Marco Castelluccio [:marco]
hsivonen: review+
Details | Diff | Splinter Review
Removed nsAHtml5FragmentParser v3 (12.66 KB, patch)
2011-08-03 03:03 PDT, Marco Castelluccio [:marco]
hsivonen: review+
Details | Diff | Splinter Review
Removed nsAHtml5FragmentParser (12.34 KB, patch)
2011-08-03 10:57 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Removed nsAHtml5FragmentParser (13.59 KB, patch)
2011-08-04 06:22 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Removed nsAHtml5FragmentParser v2 (14.22 KB, patch)
2011-08-14 04:46 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review

Description Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2010-11-17 01:21:33 PST
Remove nsAHtml5FragmentParser and cast to nsHtml5Parser where casts to nsAHtml5FragmentParser currently occur.
Comment 1 Marco Castelluccio [:marco] 2011-07-28 07:22:07 PDT
Created attachment 549104 [details] [diff] [review]
Removed nsAHtml5FragmentParser
Comment 2 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-08-02 02:15:56 PDT
Comment on attachment 549104 [details] [diff] [review]
Removed nsAHtml5FragmentParser

>-      asFragmentParser->ParseHtml5Fragment(aFragment,
>+      asParser->ParseHtml5Fragment(aFragment,
>                                            fragment,
>                                            contextAsContent->Tag(),
>                                            contextAsContent->GetNameSpaceID(),
>                                            (document->GetCompatibilityMode() ==
>                                                eCompatibility_NavQuirks),
>                                            PR_FALSE);
>     } else {
>-      asFragmentParser->ParseHtml5Fragment(aFragment,
>+      asParser->ParseHtml5Fragment(aFragment,
>                                            fragment,
>                                            nsGkAtoms::body,
>                                            kNameSpaceID_XHTML,
>                                            (document->GetCompatibilityMode() ==
>                                                eCompatibility_NavQuirks),
>                                            PR_FALSE);

Please realign the lines under the ones you change so that all the arguments start on the same column.

>+    asParser->ParseHtml5Fragment(aInnerHTML,
>                                          this,
>                                          Tag(),
>                                          GetNameSpaceID(),
>                                          doc->GetCompatibilityMode() ==
>                                              eCompatibility_NavQuirks,
>                                          PR_TRUE);

Same here.

>     NS_IMETHOD ParseHtml5Fragment(const nsAString& aSourceBuffer,
>                                   nsIContent* aTargetNode,
>                                   nsIAtom* aContextLocalName,
>                                   PRInt32 aContextNamespace,
>                                   PRBool aQuirks,
>                                   PRBool aPreventScriptExecution);

This could now be made non-virtual, right?

r=me with those nits addressed. However, chances are that this patch doesn't apply as-is anymore. :-(
Comment 3 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-08-02 02:16:48 PDT
bmo@edmorley.co.uk, why was this assigned to me when another person has posted a patch?
Comment 4 Ed Morley [:emorley] 2011-08-02 02:35:30 PDT
Apologies, must have accidentally used copy email context menu on comment 0 rather than comment 1; meant to set to Marco.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-08-02 09:24:14 PDT
I think we should move this to nsIParser for now and to mozilla::parser::AParser eventually. Exporting all of the parser seems sub-optimal.
Comment 6 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-08-02 10:31:40 PDT
(In reply to comment #5)
> I think we should move this to nsIParser for now and to
> mozilla::parser::AParser eventually.

I'd like to put only the real commonality in mozilla::parser::AParser. The fragment parsing entry points need different arguments for HTML and XML, so I think they don't belong on mozilla::parser::AParser.

> Exporting all of the parser seems sub-optimal.

sicking disagreed in bug 599588 comment 31. I disagree, too (now).
Comment 7 Marco Castelluccio [:marco] 2011-08-02 14:12:04 PDT
Created attachment 550198 [details] [diff] [review]
Removed nsAHtml5FragmentParser v2

The patch is a little different, because there was some changes in the repository.
nsHtml5Parser.h includes nsContentSink.h, that doesn't exist. I don't know if it'll be added with another patch, so I leaved the inclusion be.
Comment 8 Marco Castelluccio [:marco] 2011-08-02 15:23:00 PDT
Created attachment 550233 [details] [diff] [review]
Removed nsAHtml5FragmentParser v3

Now it compiles without problems.
Comment 9 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-08-02 23:17:45 PDT
Comment on attachment 550233 [details] [diff] [review]
Removed nsAHtml5FragmentParser v3

>-  static nsAHtml5FragmentParser* sHTMLFragmentParser;
>+  static nsHtml5Parser* sHTMLParser;
>   static nsIParser* sXMLFragmentParser;

Please keep the variable name sHTMLFragmentParser to indicate that the object is to be used for fragment parsing only and for consistency with sXMLFragmentParser.

With that, r=me.
Comment 10 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-08-02 23:40:12 PDT
(In reply to comment #7)
> nsHtml5Parser.h includes nsContentSink.h, that doesn't exist.

How do you mean? http://hg.mozilla.org/mozilla-central/file/tip/content/base/src/nsContentSink.h seems to exist, still.
Comment 11 Marco Castelluccio [:marco] 2011-08-03 03:03:43 PDT
Created attachment 550334 [details] [diff] [review]
Removed nsAHtml5FragmentParser v3

For nsContentSink.h there was a problem with my local repository, that I've now fixed.
Comment 12 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-08-03 05:47:07 PDT
Comment on attachment 550334 [details] [diff] [review]
Removed nsAHtml5FragmentParser v3

r=hsivonen
Comment 13 Marco Castelluccio [:marco] 2011-08-03 10:57:25 PDT
Created attachment 550427 [details] [diff] [review]
Removed nsAHtml5FragmentParser

Added author name and commit message.
Comment 14 Dão Gottwald [:dao] 2011-08-04 02:05:23 PDT
backed out, didn't compile
Comment 15 Marco Castelluccio [:marco] 2011-08-04 03:21:40 PDT
what was the error?
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2011-08-04 05:17:17 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312441221.1312441362.1615.gz&fulltext=1#err0

error: nsContentSink.h: No such file or directory

Is nsContentSink.h exported? This is one of the reasons I'm not happy with the patch.
Comment 17 Marco Castelluccio [:marco] 2011-08-04 06:22:32 PDT
Created attachment 550668 [details] [diff] [review]
Removed nsAHtml5FragmentParser

Corrected compilation problems.
Comment 18 Marco Castelluccio [:marco] 2011-08-04 06:25:09 PDT
Ms2ger, nsContentSink.h wasn't exported, now I've changed the Makefile.in to export it.
Isn't it better to have less code and more exports, than more code and less exports?
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2011-08-06 07:56:18 PDT
I think exporting private headers is harmful, but that ship has apparently sailed.
Comment 20 Dão Gottwald [:dao] 2011-08-07 07:58:55 PDT
... and backed out again.
Comment 21 Ed Morley [:emorley] 2011-08-07 08:05:53 PDT
http://dev.philringnalda.com/tbpl/?tree=Mozilla-Inbound&rev=e60f96b64dfa

Win32:
{
e:/builds/moz2_slave/m-in-w32/build/editor/libeditor/html/nsHTMLDataTransfer.cpp(87) : fatal error C1083: Cannot open include file: 'nsAHtml5FragmentParser.h': No such file or directory
}
(and other platforms too)

Marco, if you don't have try server access, just ask once the patch is updated and one of the people on the CC list will be more than happy to send to try for you :-)
Comment 22 Marco Castelluccio [:marco] 2011-08-14 04:46:58 PDT
Created attachment 552957 [details] [diff] [review]
Removed nsAHtml5FragmentParser v2

I need someone that could send this to a try server.
Comment 23 Ed Morley [:emorley] 2011-08-14 04:56:48 PDT
(In reply to Marco Castelluccio from comment #22)
> I need someone that could send this to a try server.

http://dev.philringnalda.com/tbpl/?tree=Try&rev=6dba44e1c91f

:-)
Comment 24 Marco Castelluccio [:marco] 2011-08-14 09:32:20 PDT
Ok, in the try server the build was successful. So now this has to be checked-in.
Comment 25 Daniel Holbert [:dholbert] 2011-08-14 10:44:32 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/adfa031a0129
Comment 26 :Ms2ger (⌚ UTC+1/+2) 2011-08-14 10:45:40 PDT
http://hg.mozilla.org/mozilla-central/rev/145e0f391912
Comment 27 :Ms2ger (⌚ UTC+1/+2) 2011-08-15 02:39:38 PDT
http://hg.mozilla.org/mozilla-central/rev/adfa031a0129
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-15 03:44:41 PDT
Thanks for taking this on Marco.

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