Closed Bug 1393837 Opened 2 years ago Closed 2 years ago

Codegen.py should include all parent interface headers

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

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.
MozReview-Commit-ID: 5UWExNmYyME
Attachment #8901250 - Flags: review?(nfroyd)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
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)
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 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 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-
(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]
(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 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+
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9698cc8c6752
Make sure codegen includes all the parent interfaces r=peterv
https://hg.mozilla.org/mozilla-central/rev/9698cc8c6752
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.