612839 - Remove nsAHtml5FragmentParser

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Henri Sivonen (:hsivonen)]

Remove nsAHtml5FragmentParser and cast to nsHtml5Parser where casts to nsAHtml5FragmentParser currently occur.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Removed nsAHtml5FragmentParser

Comment 2

[Henri Sivonen (:hsivonen)]

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)]

[e-mail], why was this assigned to me when another person has posted a patch?

Comment 4

[Ed Morley [:emorley]]

Apologies, must have accidentally used copy email context menu on comment 0 rather than comment 1; meant to set to Marco.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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)]

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

Attachment: 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]]

Attachment: Removed nsAHtml5FragmentParser v3

Now it compiles without problems.

Comment 9

[Henri Sivonen (:hsivonen)]

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)]

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

Attachment: Removed nsAHtml5FragmentParser v3

For nsContentSink.h there was a problem with my local repository, that I've now fixed.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 550334 [details] [diff] [review]
Removed nsAHtml5FragmentParser v3

r=hsivonen

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Removed nsAHtml5FragmentParser

Added author name and commit message.

Comment 14

[Dão Gottwald [:dao]]

backed out, didn't compile

Comment 15

[Marco Castelluccio [:marco]]

what was the error?

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

Attachment: Removed nsAHtml5FragmentParser

Corrected compilation problems.

Comment 18

[Marco Castelluccio [:marco]]

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 (he/him; ⌚ UTC+1/+2)]

I think exporting private headers is harmful, but that ship has apparently sailed.

Comment 20

[Dão Gottwald [:dao]]

... and backed out again.

Comment 21

[Ed Morley [:emorley]]

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]]

Attachment: Removed nsAHtml5FragmentParser v2

I need someone that could send this to a try server.

Comment 23

[Ed Morley [:emorley]]

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

Ok, in the try server the build was successful. So now this has to be checked-in.

Comment 25

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/adfa031a0129

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/145e0f391912

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/adfa031a0129

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks for taking this on Marco.