Remove nsAHtml5FragmentParser

RESOLVED FIXED in mozilla8

Status

()

Core
HTML: Parser
--
trivial
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: hsivonen, Assigned: marco)

Tracking

(Blocks: 1 bug)

Trunk
mozilla8
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

7 years ago
Remove nsAHtml5FragmentParser and cast to nsHtml5Parser where casts to nsAHtml5FragmentParser currently occur.
Blocks: 105431
(Assignee)

Comment 1

6 years ago
Created attachment 549104 [details] [diff] [review]
Removed nsAHtml5FragmentParser
Attachment #549104 - Flags: review?(hsivonen)

Updated

6 years ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Reporter)

Comment 2

6 years ago
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. :-(
Attachment #549104 - Flags: review?(hsivonen) → review+
(Reporter)

Comment 3

6 years ago
bmo@edmorley.co.uk, why was this assigned to me when another person has posted a patch?
Assignee: hsivonen → mar.castelluccio

Comment 4

6 years ago
Apologies, must have accidentally used copy email context menu on comment 0 rather than comment 1; meant to set to Marco.
I think we should move this to nsIParser for now and to mozilla::parser::AParser eventually. Exporting all of the parser seems sub-optimal.
(Reporter)

Comment 6

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

Comment 7

6 years ago
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.
Attachment #549104 - Attachment is obsolete: true
Attachment #550198 - Flags: review?(hsivonen)
(Assignee)

Comment 8

6 years ago
Created attachment 550233 [details] [diff] [review]
Removed nsAHtml5FragmentParser v3

Now it compiles without problems.
Attachment #550198 - Attachment is obsolete: true
Attachment #550198 - Flags: review?(hsivonen)
(Assignee)

Updated

6 years ago
Attachment #550233 - Flags: review?(hsivonen)
(Reporter)

Comment 9

6 years ago
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.
Attachment #550233 - Flags: review?(hsivonen) → review+
(Reporter)

Comment 10

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

Comment 11

6 years ago
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.
Attachment #550233 - Attachment is obsolete: true
Attachment #550334 - Flags: review?(hsivonen)
(Reporter)

Comment 12

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

r=hsivonen
Attachment #550334 - Flags: review?(hsivonen) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

6 years ago
Created attachment 550427 [details] [diff] [review]
Removed nsAHtml5FragmentParser

Added author name and commit message.
Attachment #550334 - Attachment is obsolete: true

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [inbound]
backed out, didn't compile
Whiteboard: [inbound]
(Assignee)

Comment 15

6 years ago
what was the error?
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.
(Assignee)

Comment 17

6 years ago
Created attachment 550668 [details] [diff] [review]
Removed nsAHtml5FragmentParser

Corrected compilation problems.
Attachment #550427 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 18

6 years ago
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?
I think exporting private headers is harmful, but that ship has apparently sailed.

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [inbound]
... and backed out again.
Whiteboard: [inbound]
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 :-)
(Assignee)

Comment 22

6 years ago
Created attachment 552957 [details] [diff] [review]
Removed nsAHtml5FragmentParser v2

I need someone that could send this to a try server.
Attachment #550668 - Attachment is obsolete: true
(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

:-)
(Assignee)

Comment 24

6 years ago
Ok, in the try server the build was successful. So now this has to be checked-in.
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/adfa031a0129
Keywords: checkin-needed
Whiteboard: [inbound]
Version: unspecified → Trunk
http://hg.mozilla.org/mozilla-central/rev/145e0f391912

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/adfa031a0129
Flags: in-testsuite-
Whiteboard: [inbound]
Thanks for taking this on Marco.
You need to log in before you can comment on or make changes to this bug.