Closed Bug 1269490 Opened 3 years ago Closed 2 years ago

share atoms between the html parser and nsGkAtomList

Categories

(Core :: HTML: Parser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox49 --- affected
firefox55 --- fixed

People

(Reporter: froydnj, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We have a lot of duplicate atoms (and string buffers, etc.) that could be shared between the HTML5 parser and nsGkAtomList.  We should share those atoms, which would save ~50-60K of libxul size on a 64-bit platform, and make startup a wee bit faster by not having to intern everything.
Blocks: 1254777
Depends on: 1275755
Duplicate of this bug: 1357795
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Component: General → HTML: Parser
Attached patch Translator changes (obsolete) — Splinter Review
Attachment #8859984 - Flags: review?(wchen)
Attachment #8859927 - Flags: review?(wchen)
Comment on attachment 8859984 [details] [diff] [review]
Translator changes

Review of attachment 8859984 [details] [diff] [review]:
-----------------------------------------------------------------

::: translator-src/nu/validator/htmlparser/cpptranslate/CppTypes.java
@@ +146,5 @@
> +            String line;
> +            while ((line = atomReader.readLine()) != null) {
> +                manualAtoms.append(line);
> +                manualAtoms.append('\n');
> +                if (line.startsWith("// BEGIN GENERATED")) {

The list is generated atoms is long and the "// BEGIN GENERATED" warning block is far from the end of the file so it seems likely that someone will append to the middle or end of the file when adding a new atom. At the least we should add a comment at the end, something like "// END GENERATED ATOMS, DO NOT ADD CODE BELOW THIS LINE" and maybe also add a comment after each generated atoms saying "// ATOM GENERATED BY HTML PARSER TRANSLATOR" to make it even more clear so that we don't get atoms accidentally added to the middle of the list.

Alternatively, I think it would be even better if we generated the parser atoms to another file that we include in nsGkAtomList.h so then we don't have to worry about atoms manually added in the wrong place.
Attachment #8859984 - Flags: review?(wchen)
Comment on attachment 8859927 [details]
Bug 1269490 - Merge nsHtml5Atoms into nsGkAtoms.

https://reviewboard.mozilla.org/r/131988/#review138668
Attachment #8859927 - Flags: review?(wchen)
Attachment #8859984 - Attachment is obsolete: true
Attachment #8864099 - Flags: review?(wchen)
Comment on attachment 8859927 [details]
Bug 1269490 - Merge nsHtml5Atoms into nsGkAtoms.

https://reviewboard.mozilla.org/r/131988/#review138980
Attachment #8859927 - Flags: review?(wchen) → review+
Attachment #8864099 - Flags: review?(wchen) → review+
Blocks: 1362338
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/355a23c6c2bd
Merge nsHtml5Atoms into nsGkAtoms. r=wchen
https://hg.mozilla.org/mozilla-central/rev/355a23c6c2bd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1362954
Depends on: 1363021
No longer depends on: 1363021
Blocks: 1363478
Depends on: 1377859
You need to log in before you can comment on or make changes to this bug.