Closed Bug 1446097 Opened 2 years ago Closed 2 years ago

HTML parser regeneration broken by gkatoms changes

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 2 obsolete files)

When I try to regenerate the parser per the directions in parser/html/java/README.txt I get:

  Missing SVG element: set_

and the resulting output has the wrong constructor function for the element.  This was introduced by bug 1445117, apparently...

I suspect that what happened is that the parser generator looked through nsGkAtoms and found the set_ thing that was present there, which is why the generated code ended up using it.  But changing the element _name_ in our element lists so it no longer matches known element names is not the way to go here.  The right answer was to just regenerate the parser.
Since this is hard-blocking me right now....
Assignee: nobody → bzbarsky
And the nsGkAtoms.h move broke the regeneration makefile, though that's at least a simple fix.
Apologies for the breakage. It's hard to avoid when there are obscure dependencies that aren't tested on automation. Note also that nsGkAtoms.cpp moved from dom/base/ to xpcom/ds/.

TBH I'm pretty fed up with the arrangement of the HTML5 parser. The presence of generated code in mozilla-central that is generated from an external repo is very hostile to development. It has caused me multiple problems recently.
njn, could you review the non-parser bits?  Henri, can you review the parser bits?
Attachment #8959279 - Flags: review?(n.nethercote)
Attachment #8959279 - Flags: review?(hsivonen)
Comment on attachment 8959279 [details] [diff] [review]
Switch to "set" as the canonical nsGkAtoms name of the string "set", so it matches the actual tag name "set" in SVG

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

r=me for the non-parser parts.

::: xpcom/ds/nsGkAtomList.h
@@ -1990,5 @@
>  GK_ATOM(separator_, "separator")
>  GK_ATOM(separators_, "separators")
>  GK_ATOM(sep_, "sep")
>  GK_ATOM(setdiff_, "setdiff")
> -GK_ATOM(set_, "set")

For consistency with other cases like this, instead of deleting this entry I would comment it out, and add a '"set" is present above" comment.
Attachment #8959279 - Flags: review?(n.nethercote) → review+
> instead of deleting this entry I would comment it out, and add a '"set" is present above" comment.

Why?  Who would ever go looking for "nsGkAtoms::set_"?  I mean, I'm happy to do this if there's a reason, but is there one?
Flags: needinfo?(n.nethercote)
Maybe the header reordering is the parser generation code, not clang-format, by the way.
Apparently the header-reordering, whoever did it, does not compile.  Updated patch coming up with that removed....
Comment on attachment 8959279 [details] [diff] [review]
Switch to "set" as the canonical nsGkAtoms name of the string "set", so it matches the actual tag name "set" in SVG

r+ on the assumption that this
 1) changes the set atom
 2) changes the atom file path
 3) the rest comes from clang-format
Attachment #8959279 - Flags: review?(hsivonen) → review+
> 1) changes the set atom

Yes.

> 2) changes the atom file path

Yes.

> 3) the rest comes from clang-format

Some of it comes from the parser generating the cpp files... for example the header reorderings sure do.
Attached patch Actually compiling (obsolete) — Splinter Review
Attachment #8959279 - Attachment is obsolete: true
Attachment #8959297 - Attachment is obsolete: true
> Why?  Who would ever go looking for "nsGkAtoms::set_"?  I mean, I'm happy to
> do this if there's a reason, but is there one?

Is this list that "set" is part of -- "saturation", "saturate", "set", "seed", etc. -- from some predefined list of things? If so, it makes sense to leave it in. But if that list has just accumulated over time, then I agree that removing it is reasonable.
Flags: needinfo?(n.nethercote)
Looks like "set" was added for the SVG <set> tag.  It's just there in alphabetical order in the original gkatoms list.  The "set_" bit is for the MathML element.  It used to be in a separate atom list and then got merged into gkatoms and then renamed.

I guess given that, it makes sense to just leave the commented-out thing in the mathml element, I guess.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6af1f5ac596b
Switch to "set" as the canonical nsGkAtoms name of the string "set", so it matches the actual tag name "set" in SVG.  r=hsivonen,njn
https://hg.mozilla.org/mozilla-central/rev/6af1f5ac596b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.