Closed
Bug 1393837
Opened 7 years ago
Closed 7 years ago
Codegen.py should include all parent interface headers
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: valentin, Assigned: valentin)
References
Details
Attachments
(1 file, 1 obsolete file)
In bug 1263722 I tried adding a new interface called PerformanceNavigationTiming which extends PerformanceResourceTiming which in turn extends PerformanceEntry. The generated code produced the following error: mozilla-central/obj-ff-dbg/dom/bindings 0:20.55/PerformanceNavigationTimingBinding.cpp:424:8: error: use of undeclared identifier 'PerformanceEntryBinding'; did you mean 'PerformanceEntry'? 0:20.55 if (!PerformanceEntryBinding::JsonifyAttributes(cx, obj, self, result)) { 0:20.55 ^~~~~~~~~~~~~~~~~~~~~~~ 0:20.55 PerformanceEntry This is because PerformanceEntryBinding.h wasn't included in the implementation file.
Assignee | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: 5UWExNmYyME
Attachment #8901250 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
Comment on attachment 8901250 [details] [diff] [review] Make sure codegen includes all the parent interfaces Review of attachment 8901250 [details] [diff] [review]: ----------------------------------------------------------------- While I have touched Codegen.py many times, I am not confident in my ability to review this patch. Forwarding to peterv.
Attachment #8901250 -
Flags: review?(nfroyd) → review?(peterv)
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=687f225a3e876e05a261ee1d054185c13f2a49e0
Comment 4•7 years ago
|
||
Comment on attachment 8901250 [details] [diff] [review] Make sure codegen includes all the parent interfaces Review of attachment 8901250 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +1078,5 @@ > # ancestor with an interface object too, so we can use its > # interface object as the proto of our interface object. > if iface.hasInterfaceObject(): > parent = iface.parent > + while parent and parent.hasInterfaceObject(): This is wrong, see the comment above. It seems to me you want to add another loop that mirrors CGJsonifierMethod, so add a loop over descriptors, if a descriptor has a operations['Jsonifier'], then loop over the descriptors for its parents and if any of those have a operations['Jsonifier'] then add them to ancestors.
Attachment #8901250 -
Flags: review?(peterv) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8901250 [details] [diff] [review] Make sure codegen includes all the parent interfaces I attached a new patch. I hope this is the solution you had in mind. I don't really know this code.
Attachment #8901250 -
Attachment is obsolete: true
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8902857 [details] Bug 1393837 - Make sure codegen includes all the parent interfaces https://reviewboard.mozilla.org/r/174554/#review181310 Did you try this with the code that used to fail? Because I don't think this works unless you append the descriptor instead of the interface, but I'd like to know why you didn't run into a failure when testing. ::: dom/bindings/Codegen.py:1084 (Diff revision 1) > parent = parent.parent > if parent: > ancestors.append(parent) > interfaceDeps.extend(ancestors) > + > + # We need to ensure that all parent interface headers (bug 1393837) "Include parent interface headers needed for jsonifier code." ::: dom/bindings/Codegen.py:1087 (Diff revision 1) > interfaceDeps.extend(ancestors) > + > + # We need to ensure that all parent interface headers (bug 1393837) > + jsonInterfaceParents = [] > + for desc in descriptors: > + parent = desc.interface.parent I think you should first check desc.operations['Jsonifier'], no need to loop over the parents if we won't generate a jsonifier. ::: dom/bindings/Codegen.py:1091 (Diff revision 1) > + for desc in descriptors: > + parent = desc.interface.parent > + while parent: > + parentDesc = desc.getDescriptor(parent.identifier.name) > + if parentDesc.operations['Jsonifier']: > + jsonInterfaceParents.append(parentDesc.interface) Shouldn't this append parentDesc?
Attachment #8902857 -
Flags: review?(peterv) → review-
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #7) > ::: dom/bindings/Codegen.py:1091 > (Diff revision 1) > > + for desc in descriptors: > > + parent = desc.interface.parent > > + while parent: > > + parentDesc = desc.getDescriptor(parent.identifier.name) > > + if parentDesc.operations['Jsonifier']: > > + jsonInterfaceParents.append(parentDesc.interface) > > Shouldn't this append parentDesc? It's because I later call > interfaceDeps.extend(jsonInterfaceParents) and interfaceDeps contains interfaces, not descriptors. See the block above: > interfaceDeps = [d.interface for d in descriptors]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
I also pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b4441f29b271601ec76395a7a21c1080a6d0676
Comment 11•7 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #8) > > Shouldn't this append parentDesc? > > It's because I later call > > interfaceDeps.extend(jsonInterfaceParents) > > and interfaceDeps contains interfaces, not descriptors. See the block above: > > interfaceDeps = [d.interface for d in descriptors] Yeah, you're right. Got confused by the d in "self.getDeclarationFilename(d) for d in interfaceDeps".
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8902857 [details] Bug 1393837 - Make sure codegen includes all the parent interfaces https://reviewboard.mozilla.org/r/174554/#review181664 r+ if you remove the break and append instead. ::: dom/bindings/Codegen.py:1093 (Diff revision 2) > + continue > + parent = desc.interface.parent > + while parent: > + parentDesc = desc.getDescriptor(parent.identifier.name) > + if not parentDesc.operations['Jsonifier']: > + break You should have kept this the way it was before (appending instead of the break). You still need to walk all the parents here, regardless of whether they have Jsonifier.
Attachment #8902857 -
Flags: review?(peterv) → review+
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/9698cc8c6752 Make sure codegen includes all the parent interfaces r=peterv
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9698cc8c6752
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•