[idlharness.js] Simplify handling of inheritance and mixins #28650
Conversation
foolip
changed the title
foolip
changed the title
foolip
force-pushed
the
foolip/idlharness-partials-includes
branch
from
5f8cfa6
to
185f655
Compare
|
@Ms2ger I expect tests will still fail here, but initial review would still be great. |
foolip
force-pushed
the
foolip/idlharness-partials-includes
branch
from
185f655
to
63f11bc
Compare
Handling of these concepts was more complicated than (now) necessary. No changes in the actual test results are intended. Inheritance: There's no reason to record inheritance separately in `IdlArray`'s `this.["inheritance"]`, since it can be determined just as easily from `this.members` via the `member.base` attributes. The concept of "consequential interfaces" in Web IDL went away with `implements` statements in whatwg/webidl#433 and here in #28619. `traverse_inherited_and_consequential_interfaces()` can be replaced with just `get_inheritance_stack()`. While this changes the behavior of `default_to_json_operation()`, there is no `toJSON` operation declared in a mixin in interfaces/, only in resources/ tests, so this change should not affect real tests. default_to_json_operation.html was updated along the lines of simplified examples in the spec: whatwg/webidl#980 Mixins: For valid `A includes B` statements, `A` is always an interface and `B` is always an interface mixin, so there are no include chains or the possibility of cycles. `recursively_get_includes()` assumed this. Instead just save the `includes` statements found in `this.includes` and apply them in `merge_mixins`, similar to partials. The handling of partials isn't changed, but `collapse_partials` is renamed to `merge_partials` to match the above. Not today: Further unification of partials and mixins is possible, and the handling of [Exposed] is missing for mixins, but this is left for later.
foolip
force-pushed
the
foolip/idlharness-partials-includes
branch
from
63f11bc
to
18bea8b
Compare
| * interface B : C {}; | ||
| * | ||
| * results in this["inheritance"] = { A: "B", B: "C" } | ||
| * Both this.partials and this.includes will be the objects as parsed by |
There was a problem hiding this comment.
As hinted at in the commit message, it would be natural to add this.mixins and not wrap mixins in IdlInterface at all. But not yet.
| @@ -249,11 +234,15 @@ IdlArray.prototype.internal_add_dependency_idls = function(parsed_idls, options) | |||
| const new_options = { only: [] } | |||
|
|
|||
| const all_deps = new Set(); | |||
| Object.values(this.inheritance).forEach(v => all_deps.add(v)); | |||
| // NOTE: If 'A includes B' for B that we care about, then A is also a dep. | |||
There was a problem hiding this comment.
This comment was misleading, there is no "that we care about" condition here.
| assert_false('K' in idlArray.members, 'K should be ignored'); | ||
| // L inherits F, so should be picked up. |
There was a problem hiding this comment.
This comment and the assert description was wrong, now fixed.
| assert_array_equals([...map.values()].map(v => v.idlType), ["DOMString", "long"]); | ||
| }, 'should return a properly ordered map that accounts for mixed-in interfaces which declare a [Default] toJSON operation.'); | ||
|
|
||
| test(function() { |
There was a problem hiding this comment.
These tests are just more convoluted forms of the earlier tests IMHO. Order, the non-inheritance of toJSON and the (now correct) mixin behavior is tested above.
|
@Ms2ger fixing the resources/ tests revealed that I had changed the order (fixed by add |
| var map = new Map(), isDefault = false; | ||
| this.traverse_inherited_and_consequential_interfaces(function(I) { | ||
| this.get_inheritance_stack().reverse().forEach(function(I) { |
There was a problem hiding this comment.
This made me a little nervous because reverse operates in-place, and get_inheritance_stack doesn't signal very clearly that it returns a fresh array. It seems all is fine, though.
Followup: of the three callers of get_inheritance_stack, one only calls it for the side effects and the two others iterate backward. Maybe change get_inheritance_stack to return the array reversed already?
There was a problem hiding this comment.
I'll send a follow-up to rename it to get_reverse_inheritance_stack.
As suggested by Ms2ger in #28650 (comment)
…28688) As suggested by Ms2ger in #28650 (comment)
…nce_stack() do the reversing, a=testonly Automatic update from web-platform-tests [idlharness.js] Let get_reverse_inheritance_stack() do the reversing (#28688) As suggested by Ms2ger in web-platform-tests/wpt#28650 (comment) -- wpt-commits: 11c46de0abfbe7bc728bb55cf2dd020fe0ec949e wpt-pr: 28688
Handling of these concepts was more complicated than (now) necessary.
No changes in the actual test results are intended.
Inheritance:
There's no reason to record inheritance separately in
IdlArray'sthis.["inheritance"], since it can be determined just as easilyfrom
this.membersvia themember.baseattributes.The concept of "consequential interfaces" in Web IDL went away with
implementsstatements in whatwg/webidl#433and here in #28619.
traverse_inherited_and_consequential_interfaces()can be replacedwith just
get_inheritance_stack().While this changes the behavior of
default_to_json_operation(), thereis no
toJSONoperation declared in a mixin in interfaces/, only inresources/ tests, so this change should not affect real tests.
default_to_json_operation.html was updated along the lines of simplified
examples in the spec: whatwg/webidl#980
Mixins:
For valid
A includes Bstatements,Ais always an interface andBis always an interface mixin, so there are no include chains orthe possibility of cycles.
recursively_get_includes()assumed this.Instead just save the
includesstatements found inthis.includesand apply them in
merge_mixins, similar to partials.The handling of partials isn't changed, but
collapse_partialsisrenamed to
merge_partialsto match the above.Not today:
Further unification of partials and mixins is possible, and the handling
of [Exposed] is missing for mixins, but this is left for later.