Closed
Bug 1395828
Opened 7 years ago
Closed 7 years ago
Remove the parser service
Categories
(Core :: DOM: HTML Parser, enhancement)
Core
DOM: HTML Parser
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox57 | --- | affected |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
This mirrors the existing nsHTMLElement::IsContainer() function.
Attachment #8903462 -
Flags: review?(mrbkap)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
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).
Updated•7 years ago
|
Attachment #8903462 -
Flags: review?(mrbkap) → review+
Updated•7 years ago
|
Attachment #8903463 -
Flags: review?(mrbkap) → review+
Updated•7 years ago
|
Attachment #8903464 -
Flags: review?(mrbkap) → review+
Updated•7 years ago
|
Attachment #8903465 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 6•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6238f14363f27df8cc8d570e7e4d8dce25299ba1
Bug 1395828 (part 1) - Add nsHTMLElement::IsBlock(). r=mrbkap.
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6238f14363f2
(part 1) - Add nsHTMLElement::IsBlock(). r=mrbkap.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 8•7 years ago
|
||
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).
Comment 9•7 years ago
|
||
bugherder |
Assignee | ||
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/104172464a8f93a29a0f9d101427ce1d62eeabff
Bug 1395828 (part 2) - Rename nsHTMLTags methods. r=mrbkap.
https://hg.mozilla.org/integration/mozilla-inbound/rev/25180e78cdc7374ee2ed57d65087962e75d1ce9b
Bug 1395828 (part 3) - Remove AssertParserServiceIsCorrect(). r=mrbkap.
Comment 11•7 years ago
|
||
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
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
(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?
Comment 14•7 years ago
|
||
bugherder |
Assignee | ||
Comment 15•7 years ago
|
||
> 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.
Assignee | ||
Comment 16•7 years ago
|
||
> 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.
Assignee | ||
Comment 17•7 years ago
|
||
Updated version which adds an AddRefTable()/ReleaseTable() pair in
nsContentUtils.
Attachment #8907375 -
Flags: review?(mrbkap)
Assignee | ||
Updated•7 years ago
|
Attachment #8903465 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8907375 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/49862e6cc323f6d3a3f498b803436e32cddc9824
Bug 1395828 (part 4) - Remove nsIParserService/nsParserService. r=mrbkap.
Comment 19•7 years ago
|
||
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49862e6cc323
(part 4) - Remove nsIParserService/nsParserService. r=mrbkap.
Comment 20•7 years ago
|
||
bugherder |
Assignee | ||
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•