Closed Bug 1395828 Opened 2 years ago Closed 2 years ago

Remove the parser service

Categories

(Core :: DOM: HTML Parser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox57 --- affected

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(4 files, 1 obsolete file)

nsIParserService/nsParserService is a stateless wrapper around static methods in nsHTMLTags and nsHTMLElement, and hence an unnecessary layer of indirection that just adds complexity and slowness.
This mirrors the existing nsHTMLElement::IsContainer() function.
Attachment #8903462 - Flags: review?(mrbkap)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
The patch uses the nsParserService method names (minus the "HTML" prefix)
because they are more descriptive. This will make it easier to replace
nsParserService method calls with nsHTMLTags method calls.

The patch also adds nsHTMLTags::AtomTagToId(), to match up with
nsParserService::HTMLAtomTagToId().
Attachment #8903463 - Flags: review?(mrbkap)
It's a bit strange for the editor to distrust the parser service in this way.
The double-checking seems unnecessary, especially given that it is *buggy*: it
incorrectly includes `col`, `colgroup`, and `legend` as block elements, but
excludes `pre`. (It must only be called in situations where these
incorrectly-classified elements are not passed in, otherwise it would have
triggered by now.) So this patch removes it.

The patch also removes `li` and `pre` from the IsAnyOfHTMLElements() test in
HTMLEditor::NodeIsBlockStatic. Contrary to what the comment in
NodeIsBlockStatic() says, they *are* identified as block elements by the parser
service.
Attachment #8903464 - Flags: review?(mrbkap)
It a stateless wrapper around static methods in nsHTMLTags and nsHTMLElement,
and hence an unnecessary layer of indirection that just adds complexity and
slowness. This patch removes it, cutting almost 300 lines of code.

This requires making nsElementTable.h an exported header, to expose the
nsHTMLElement methods.
Attachment #8903465 - Flags: review?(mrbkap)
It would be a nice follow-up to move the remaining files away from parser/ to alleviate the impression that a parser still uses them (when it's mostly the editor still using this stuff).
Attachment #8903462 - Flags: review?(mrbkap) → review+
Attachment #8903463 - Flags: review?(mrbkap) → review+
Attachment #8903464 - Flags: review?(mrbkap) → review+
Attachment #8903465 - Flags: review?(mrbkap) → review+
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6238f14363f2
(part 1) - Add nsHTMLElement::IsBlock(). r=mrbkap.
Keywords: leave-open
I'm getting some test failures due to part 4. Child processes are sometimes asserting in nsHTMLTags::CaseSensitiveAtomTagToId() because gAtomTabTable is null.

gAtomTableTable is set in nsParserModule.cpp:Initialize(), which is called when the nsParserModule is initialized. I figure nsParserModule initialization was triggered by the calls to nsContentUtils::GetParserService(), but now that they have been removed it's not longer occurring.

I think I can fix this by finding another place to trigger nsParserModule initialization (or the nsHTMLTags::{AddRefTable,Release}Table() pair).
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d369b47a20
(part 3.1) - Remove assertion expectation for 759249-1.html to fix unexpected passes. r=expectation-update
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1e1ee5aa23
(part 3.2) - Remove assertion expectation for 415394-1.xhtml to fix unexpected passes. r=expectation-update
(In reply to Nicholas Nethercote [:njn] from comment #8)
> I think I can fix this by finding another place to trigger nsParserModule
> initialization (or the nsHTMLTags::{AddRefTable,Release}Table() pair).

Would it work to initialize this stuff from nsContentUtils' initialization?
> It's a bit strange for the editor to distrust the parser service in this way.
> The double-checking seems unnecessary, especially given that it is *buggy*: it
> incorrectly includes `col`, `colgroup`, and `legend` as block elements, but
> excludes `pre`. (It must only be called in situations where these
> incorrectly-classified elements are not passed in, otherwise it would have
> triggered by now.)

Turns out we had a couple of existing test failures, about `col`, in bug 1195474 and bug 1324651. The relevant tests had just been marked as potentially failing but nobody had investigated the problem, so that just confirms the uselessness of the assertion :)

Thank you archaeopteryx for updating the test expections.
> Would it work to initialize this stuff from nsContentUtils' initialization?

Yes! I have a local patch doing just that and it seems to be working.
Updated version which adds an AddRefTable()/ReleaseTable() pair in
nsContentUtils.
Attachment #8907375 - Flags: review?(mrbkap)
Attachment #8903465 - Attachment is obsolete: true
Attachment #8907375 - Flags: review?(mrbkap) → review+
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49862e6cc323
(part 4) - Remove nsIParserService/nsParserService. r=mrbkap.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.