Allow interfaces in core/ to include mixin interfaces from modules/ [chromium/src : main]
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Hi,
https://issues.chromium.org/issues/40116347
This issue was marked as fixed, but I believe the problem still remains, so I created a patch. I would appreciate it if you could take a look.
At the moment, to avoid core/modules layer violations, Chromium uses some workarounds. For example, partial interfaces are used instead of mixin interfaces, not only for NavigatorBadge.
To follow the spec more closely, I thought it would be better to support calling static functions in the same way as partial interfaces. This way of calling static functions does not cause a core/modules layer violation, because the binding happens under /modules.
However, I am not completely sure. Could you please let me know if this is an appropriate way to address the problem? Thank you very much.
Related details
- Andrey Kosyakov
- Kentaro Hara
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kentaro Hara
| Code-Review | +1 |
1 comment
- Andrey Kosyakov
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 1 comment![]( "Open in Gerrit")
The idea looks interesting and reasonable, however, we may want a different condition to apply the change.
I'm afraid that this change affects a following case for example.
(a) An original interface and mixins are defined in core/.
(b) [ImplementedAs=] is applied just because of a naming reason.
(c) The original interface is *not* a singleton.
In this case, non-static member function calls should be much convenient and performant for us.
Especially (c) is important. It looks to me that your idea works well in the case of Navigator because Navigator is a singleton luckily. In a case of non-singleton, how do you get an instance object?
I understand the root motivation of this CL, but the condition of mixin-case + [ImplementedAs=] would be rough cut. I'd recommend exploring a different condition.
Related details
- Andrey Kosyakov
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 1 comment![]( "Open in Gerrit")
The idea looks interesting and reasonable, however, we may want a different condition to apply the change.
I'm afraid that this change affects a following case for example.
(a) An original interface and mixins are defined in core/.
(b) [ImplementedAs=] is applied just because of a naming reason.
(c) The original interface is *not* a singleton.
In this case, non-static member function calls should be much convenient and performant for us.Especially (c) is important. It looks to me that your idea works well in the case of Navigator because Navigator is a singleton luckily. In a case of non-singleton, how do you get an instance object?
I understand the root motivation of this CL, but the condition of mixin-case + [ImplementedAs=] would be rough cut. I'd recommend exploring a different condition.
Oops, mark as unresolved.
Related details
- Andrey Kosyakov
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Yuki ShiinoThe idea looks interesting and reasonable, however, we may want a different condition to apply the change.
I'm afraid that this change affects a following case for example.
(a) An original interface and mixins are defined in core/.
(b) [ImplementedAs=] is applied just because of a naming reason.
(c) The original interface is *not* a singleton.
In this case, non-static member function calls should be much convenient and performant for us.Especially (c) is important. It looks to me that your idea works well in the case of Navigator because Navigator is a singleton luckily. In a case of non-singleton, how do you get an instance object?
I understand the root motivation of this CL, but the condition of mixin-case + [ImplementedAs=] would be rough cut. I'd recommend exploring a different condition.
Oops, mark as unresolved.
Thank you very much for your review.
I drafted this CL mainly to confirm whether I am heading in the right direction. I will continue to look into it further and make improvements based on your comments.
(c) The original interface is not a singleton.
In this case, non-static member function calls should be much convenient and performant for us.
Especially (c) is important. It looks to me that your idea works well in the case of Navigator because Navigator is a singleton luckily. In a case of non-singleton, how do you get an instance object?
(C) was also something I had been considering, and I will put more effort into it. If you already have an idea or suggestion on how best to address this problem, I would be very grateful if you could share your guidance.
Thank you again!
Related details
- Andrey Kosyakov
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 1 comment![]( "Open in Gerrit")
Yuki ShiinoThe idea looks interesting and reasonable, however, we may want a different condition to apply the change.
I'm afraid that this change affects a following case for example.
(a) An original interface and mixins are defined in core/.
(b) [ImplementedAs=] is applied just because of a naming reason.
(c) The original interface is *not* a singleton.
In this case, non-static member function calls should be much convenient and performant for us.Especially (c) is important. It looks to me that your idea works well in the case of Navigator because Navigator is a singleton luckily. In a case of non-singleton, how do you get an instance object?
I understand the root motivation of this CL, but the condition of mixin-case + [ImplementedAs=] would be rough cut. I'd recommend exploring a different condition.
Suyeon JiOops, mark as unresolved.
Thank you very much for your review.
I drafted this CL mainly to confirm whether I am heading in the right direction. I will continue to look into it further and make improvements based on your comments.(c) The original interface is not a singleton.
In this case, non-static member function calls should be much convenient and performant for us.Especially (c) is important. It looks to me that your idea works well in the case of Navigator because Navigator is a singleton luckily. In a case of non-singleton, how do you get an instance object?
(C) was also something I had been considering, and I will put more effort into it. If you already have an idea or suggestion on how best to address this problem, I would be very grateful if you could share your guidance.
Thank you again!
I have two approaches in my mind; automatic and manual ones.
(1) Automatic approach
IIUC, whenever an interface is defined in core/ and a mixin is defined in modules/, we cannot define the corresponding C++ member function in core/. Therefore, there must exists the corresponding *static* member function in modules/.
We can produce the same code as partial interfaces in modules/ in the case of an interface in core/ + a mixin in modules/.
You can use `web_idl.Interface.components` and `web_idl.Operation.components` to determine whether it's crossing the component boundary.
It looks to me that *partial* interface and *partial* interface mixin do the job already. We may just want to extend it to non-partial boundary-crossing mixins.
(2) Manual approach
We can introduce a new IDL extended attribute (e.g. [ImplementedAsStaticMemberFunction]) for the case, and apply it to IDL operations manually when needed.
Anyway, the change you want is [`is_partial` flag here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/scripts/bind_gen/interface.py;drc=75f1cd121d77b714bb83cd91b0c1037a9e69d6f0;l=672) I think. When this flag is True, the form of a member function call becomes `TheClass::StaticMemberFunction(blink_receiver, args...)`. The function will take the blink_receiver argument, so it works for non-singleton cases.
My gut feeling is that (1) automatic approach should be nicer.
Related details
- Andrey Kosyakov
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 3 comments![]( "Open in Gerrit")
Hi, thank you very much for your feedback.
I also felt that your first suggestion would be the better approach, so I’ve updated the patch accordingly.
I would greatly appreciate it if you could take another look.
Thank you!
def collect_cross_component_mixins(interfaces, includes):I think this part could be integrated into [_merge_interface_mixins](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/scripts/web_idl/idl_compiler.py;l=510), but I’d like to get your input on this.
Related details
- Andrey Kosyakov
- Kentaro Hara
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
def collect_cross_component_mixins(interfaces, includes):I think this part could be integrated into [_merge_interface_mixins](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/scripts/web_idl/idl_compiler.py;l=510), but I’d like to get your input on this.
To add a bit more context, at first I thought the cross-component check logic should go into _merge_interface_mixins() or _merge_interface_like_irs() to avoid repeating loops. However, considering the purpose of determine_blink_headers, I concluded that handling it in this function is more appropriate. In particular, since _merge_interface_like_irs() is also called from _merge_partial_interface_likes(), adding mixin-specific branching there would not be a good approach, as it is intended to be a common function.
Yuki Shiino (Gerrit)
Yuki Shiino added 4 comments![]( "Open in Gerrit")
Thank you for the fix. The basic approach looks good to me.
and "modules" in cg_context.operation.components)I hope we can just say `cg_context.member_like.defined_across_component` or something like that (the name is TBD). See other comments.
member.code_generator_info.set_defined_in_partial(is_partial)I think `_record_defined_in_partial_and_mixin` does a very similar job, and you can add another function like _record_defined_across_component or like that.
I think it'd be useful to record attributes and operations are defined across components, not limited to mixins but also partial interfaces. We can set `member.code_generator_info.set_defined_across_component(True)` in general regardless of what `member` is and regardless of what `new_ir` is (partial interface, mixin, or partial mixin). Also we can set `new_ir.code_generator_info.set_defined_across_component(True)` in addition to `member`.
and new_ir.identifier not in cross_mixin_set):If we added `_record_defined_across_component`, we can just say `new_ir.code_generator_info.defined_across_component` without creating `cross_mixin_set`.
Related details
- Andrey Kosyakov
- Kentaro Hara
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji voted and added 3 comments![]( "Open in Gerrit")
Votes added by Suyeon Ji
| Commit-Queue | +1 |
3 comments
I hope we can just say `cg_context.member_like.defined_across_component` or something like that (the name is TBD). See other comments.
Thanks for your suggestion! I defined it as you said.
member.code_generator_info.set_defined_in_partial(is_partial)I think `_record_defined_in_partial_and_mixin` does a very similar job, and you can add another function like _record_defined_across_component or like that.
I think it'd be useful to record attributes and operations are defined across components, not limited to mixins but also partial interfaces. We can set `member.code_generator_info.set_defined_across_component(True)` in general regardless of what `member` is and regardless of what `new_ir` is (partial interface, mixin, or partial mixin). Also we can set `new_ir.code_generator_info.set_defined_across_component(True)` in addition to `member`.
Done. I extended it to partial interfaces and set this value.
If we added `_record_defined_across_component`, we can just say `new_ir.code_generator_info.defined_across_component` without creating `cross_mixin_set`.
- Andrey Kosyakov
- Kentaro Hara
- Yuki Shiino
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuki Shiino (Gerrit)
Yuki Shiino added 6 comments![]( "Open in Gerrit")
Thank you. The code structure looks good to me. Some comments below.
is_mixin_and_across_component = cg_context.member_like.owner_mixin and code_generator_info.defined_across_componentI think whether it's mixin or not is not important at all at this point. If it's across component (regardless of whether it's mixin or partial interface), the function call must be in the form of TheClass::StaticMemberFunction(blink_receiver, args...).
`is_partial or is_across_component` looks enough (and better) to me.
`is_partial` should be necessary because we want the form of TheClass::StaticMemberFunction(blink_receiver, args...) for the case of interface in core/ + partial interface in core/ for a historical reason. (Technically we can get rid of this, though.)
'defined_across_component',style nit: Probably these are sorted alphabetically. Then defined_*a*cross_component < defined_*i*n_partial.
for mixin in mixinsThere seem several confusions.
The following Web IDL snippet is valid.
```
interface X { ... };
interface Y { ... };
interface mixin M1 { ... };
interface mixin M2 { ... };
X includes M1;
X includes M2;
Y includes M1;
```
where interface X includes two mixins (M1 and M2) and mixin M1 is included by two interfaces (X and Y). You're now assuming there exists a map from a mixin to a single interface, but it doesn't exist unfortunately. We have M1 => (X, Y).
Therefore, it's possible that (X, M1) is across-component while (Y, M1) is not. We can mark M1="across component" when either of X or Y is defined across component.
About Python classes of web_idl Python component, `Includes` class is not a mixin.
```
# includes.item() returns a pair of interface_identifier and a list of Includes for the interface.
for interface_identifier, list_of_includes in includes.items():
# You can traverse each Includes instance for a list of Includes.
# Example)
# 1. X includes M1;
# 2. X includes M2;
for includes in list_of_includes:
assert(isinstance(includes, web_idl.Includes))
includes.interface_identifier # X
interfaces[includes.interface_identifier]
includes.mixin_identifier # M1 or M2
mixins[includes.mixin_identifier]
```
where
```
interfaces = self._ir_map.find_by_kind(IRMap.IR.Kind.INTERFACE)
mixins = self._ir_map.find_by_kind(IRMap.IR.Kind.INTERFACE_MIXIN)
```
The current definition of `interfaces` (line 332) looks confusing to me. We have no need to mix interfaces and mixins, I think.
original = mixin_owner_map[new_ir]I'm afraid that `if is_partial(new_ir):` would match with partial interface mixins, not only with partial interfaces. Probably you want `if is_mixin(new_ir):` first?
```
if is_mixin(new_ir):
...
elif is_partial(new_ir):
...
else:
assert(False)
```
True)I'd set not only True but also False explicitly. The value should be None by default.
I'd be happier (especially when debugging) with that callback_function.code_generator_info.defined_across_component = None (because not set explicitly) and mixin.code_generator_info.defined_across_component = True or False. If None is observed for mixin or partial interface, that's a bug.
Related details
- Andrey Kosyakov
- Kentaro Hara
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |