Allow the creation of null registry element and shadow root [chromium/src : main]
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Sorry for the delay.
Please review it when you get a chance. Thanks!
Related details
- Jayson Chen
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jayson Chen (Gerrit)
Jayson Chen voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jayson Chen
| Code-Review | +1 |
2 comments
Overall looks good to me! Thanks for making the change.
const bool wait_for_registry) {Do we still need this argument? (Correct me if I'm wrong) In the default registry scenario (registry not provided to `CreateElement`), if we get null by looking up document's custom element registry by calling `customElementRegistry()`, we do want to set null registry to the element right?
Is it possible to remove this argument, and in line 1632 below use `!registry` for the argument for `CreateUncustomizedOrUndefinedElement`?
Related details
- Mason Freed
- YeongHan Kim
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 2 comments![]( "Open in Gerrit")
Thank you so much for reviewing this!
Do we still need this argument? (Correct me if I'm wrong) In the default registry scenario (registry not provided to `CreateElement`), if we get null by looking up document's custom element registry by calling `customElementRegistry()`, we do want to set null registry to the element right?
Is it possible to remove this argument, and in line 1632 below use `!registry` for the argument for `CreateUncustomizedOrUndefinedElement`?
Oh! I had thought it needed to be set only when `ElementCreationOptions` included a registry, but it seems I misunderstood.
You’re right, and it looks like we can remove both the `wait_for_registry` argument and the extra `CreateElement()` overload.
Thanks!
Related details
- Jayson Chen
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jayson Chen (Gerrit)
Jayson Chen voted and added 4 comments![]( "Open in Gerrit")
Votes added by Jayson Chen
| Code-Review | +1 |
4 comments
LGTM with few nits. Thanks!
Element* Document::CreateElement(const QualifiedName& q_name,nit: can we get a comment explaining this overload behavior of "not providing registry in arg (default case) so use document's registry?
// 2. If registry is "default", set registry to the result of looking
// up a custom element registry given document.nit: can we also have these lines in the overload function? Since that is the entry point for "registry is default"
// Note that this step is currently only applicable to scenario when
// scoped registry is disabled as a valid registry should be assigned for
// default cases while flattening options.nit: Prob not entirely accurate now, perhaps something along the line of
"Note that we need to assign default registry when scoped registry is disabled"
to explain the following line of code.
Related details
- Mason Freed
- YeongHan Kim
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. |
Mason Freed (Gerrit)
Mason Freed voted and added 6 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
6 comments
LGTM, thanks for working on this bug! I do have a few nits, but nothing major I think.
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);I think it might (might!) be better *not* to add this overload, and instead explicitly pass in the registry argument? There are 12 callers or so, which isn't too many. I'm just concerned that all callers really should be thinking about custom element registries, rather than perhaps accidentally relying on the default behavior.
// 3-1. If options["is"] exists, then set is to it.`"is"` - see comment below
"The custom element registry and is option can't be set at the "Perhaps `and "is" option` (with quotation marks) or something? It's too bad "is" is the name of this thing, it makes writing it down confusing.
Element* Document::CreateElement(const QualifiedName& q_name,nit: can we get a comment explaining this overload behavior of "not providing registry in arg (default case) so use document's registry?
...also, I think it'd be easier to read if this overload were defined in the `.h` file instead.
*this, q_name, flags, is, registry, !registry);nit: `, /*wait_for_registry*/ !registry)`
Related details
- YeongHan Kim
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. |
Jayson Chen (Gerrit)
Jayson Chen added 1 comment![]( "Open in Gerrit")
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);I think it might (might!) be better *not* to add this overload, and instead explicitly pass in the registry argument? There are 12 callers or so, which isn't too many. I'm just concerned that all callers really should be thinking about custom element registries, rather than perhaps accidentally relying on the default behavior.
I’d argue that we need the overload for default case bc
- The spec does outline that create element could have a registry given, null or default as registry. Without overloading, we’re not able to differentiate the implicit null (default case) or explicit null (actually want null case) without introducing another argument asking if null is explicit or implicit. I find overloading here clearer and less confusing than having another argument like wait_for_registry. Patch 7 demonstrates what it’s like if we don’t do overload but have another argument.
- Correct me if I’m wrong, it feels like it is actually the desired behavior to use default registry, unless the caller explicitly wants the registry to specified, so I wouldn’t worry too much about relying on the default behavior.
But ofc open to thoughts and discussions :)
YeongHan Kim (Gerrit)
YeongHan Kim added 8 comments![]( "Open in Gerrit")
Thanks for the review!
I’ve applied the nit suggestions, and I’ve also shared some of my thoughts in the discussion.
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);Jayson ChenI think it might (might!) be better *not* to add this overload, and instead explicitly pass in the registry argument? There are 12 callers or so, which isn't too many. I'm just concerned that all callers really should be thinking about custom element registries, rather than perhaps accidentally relying on the default behavior.
I’d argue that we need the overload for default case bc
- The spec does outline that create element could have a registry given, null or default as registry. Without overloading, we’re not able to differentiate the implicit null (default case) or explicit null (actually want null case) without introducing another argument asking if null is explicit or implicit. I find overloading here clearer and less confusing than having another argument like wait_for_registry. Patch 7 demonstrates what it’s like if we don’t do overload but have another argument.
- Correct me if I’m wrong, it feels like it is actually the desired behavior to use default registry, unless the caller explicitly wants the registry to specified, so I wouldn’t worry too much about relying on the default behavior.
But ofc open to thoughts and discussions :)
Thank you for the helpful thoughts!
I’d like to share my thoughts as well, though cautiously.
When the registry is not explicitly provided (implicit null), we previously passed nullptr, but now it has been changed so that we pass the default (`customElementRegistry()`) instead.
Based on that, the way I understand Mason’s suggestion is that instead of using an overload, we could explicitly pass the default registry. In that case, unlike Patch 7, we wouldn’t need an additional argument
That said, even so, when the registry isn’t explicitly provided, I feel it’s cleaner to rely on the default behavior through an overload rather than requiring callers to explicitly pass the default registry
`"is"` - see comment below
Done!
"The custom element registry and is option can't be set at the "Perhaps `and "is" option` (with quotation marks) or something? It's too bad "is" is the name of this thing, it makes writing it down confusing.
Done!
Element* Document::CreateElement(const QualifiedName& q_name,Mason Freednit: can we get a comment explaining this overload behavior of "not providing registry in arg (default case) so use document's registry?
...also, I think it'd be easier to read if this overload were defined in the `.h` file instead.
I’ve added the comment to the `.h` file. Thanks!
// 2. If registry is "default", set registry to the result of looking
// up a custom element registry given document.nit: can we also have these lines in the overload function? Since that is the entry point for "registry is default"
Done!
// Note that this step is currently only applicable to scenario when
// scoped registry is disabled as a valid registry should be assigned for
// default cases while flattening options.nit: Prob not entirely accurate now, perhaps something along the line of
"Note that we need to assign default registry when scoped registry is disabled"
to explain the following line of code.
Done!
*this, q_name, flags, is, registry, !registry);YeongHan Kimnit: `, /*wait_for_registry*/ !registry)`
Done!
Related details
- Jayson Chen
- Mason Freed
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. |
Jayson Chen (Gerrit)
Jayson Chen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jayson Chen
| Code-Review | +1 |
1 comment
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);Jayson ChenI think it might (might!) be better *not* to add this overload, and instead explicitly pass in the registry argument? There are 12 callers or so, which isn't too many. I'm just concerned that all callers really should be thinking about custom element registries, rather than perhaps accidentally relying on the default behavior.
YeongHan KimI’d argue that we need the overload for default case bc
- The spec does outline that create element could have a registry given, null or default as registry. Without overloading, we’re not able to differentiate the implicit null (default case) or explicit null (actually want null case) without introducing another argument asking if null is explicit or implicit. I find overloading here clearer and less confusing than having another argument like wait_for_registry. Patch 7 demonstrates what it’s like if we don’t do overload but have another argument.
- Correct me if I’m wrong, it feels like it is actually the desired behavior to use default registry, unless the caller explicitly wants the registry to specified, so I wouldn’t worry too much about relying on the default behavior.
But ofc open to thoughts and discussions :)
Thank you for the helpful thoughts!
I’d like to share my thoughts as well, though cautiously.When the registry is not explicitly provided (implicit null), we previously passed nullptr, but now it has been changed so that we pass the default (`customElementRegistry()`) instead.
Based on that, the way I understand Mason’s suggestion is that instead of using an overload, we could explicitly pass the default registry. In that case, unlike Patch 7, we wouldn’t need an additional argument
That said, even so, when the registry isn’t explicitly provided, I feel it’s cleaner to rely on the default behavior through an overload rather than requiring callers to explicitly pass the default registry
Ah yes that makes sense that we wouldn't need an additional argument then. With that in mind, I still slightly lean towards having the overload function as default registry case is explicitly stated in the spec, but I don't feel as strongly about it as before. I guess the question will come down to, moving forward for all the create element caller who don't care about custom element registry, should we expect them to know that they should pass in `document->customElementRegistry()` as the default value? Or we want to offload that responsibility of knowing registry to the overload function.
Related details
- Mason Freed
- YeongHan Kim
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Related details
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);Jayson ChenI think it might (might!) be better *not* to add this overload, and instead explicitly pass in the registry argument? There are 12 callers or so, which isn't too many. I'm just concerned that all callers really should be thinking about custom element registries, rather than perhaps accidentally relying on the default behavior.
YeongHan KimI’d argue that we need the overload for default case bc
- The spec does outline that create element could have a registry given, null or default as registry. Without overloading, we’re not able to differentiate the implicit null (default case) or explicit null (actually want null case) without introducing another argument asking if null is explicit or implicit. I find overloading here clearer and less confusing than having another argument like wait_for_registry. Patch 7 demonstrates what it’s like if we don’t do overload but have another argument.
- Correct me if I’m wrong, it feels like it is actually the desired behavior to use default registry, unless the caller explicitly wants the registry to specified, so I wouldn’t worry too much about relying on the default behavior.
But ofc open to thoughts and discussions :)
Jayson ChenThank you for the helpful thoughts!
I’d like to share my thoughts as well, though cautiously.When the registry is not explicitly provided (implicit null), we previously passed nullptr, but now it has been changed so that we pass the default (`customElementRegistry()`) instead.
Based on that, the way I understand Mason’s suggestion is that instead of using an overload, we could explicitly pass the default registry. In that case, unlike Patch 7, we wouldn’t need an additional argument
That said, even so, when the registry isn’t explicitly provided, I feel it’s cleaner to rely on the default behavior through an overload rather than requiring callers to explicitly pass the default registry
Ah yes that makes sense that we wouldn't need an additional argument then. With that in mind, I still slightly lean towards having the overload function as default registry case is explicitly stated in the spec, but I don't feel as strongly about it as before. I guess the question will come down to, moving forward for all the create element caller who don't care about custom element registry, should we expect them to know that they should pass in `document->customElementRegistry()` as the default value? Or we want to offload that responsibility of knowing registry to the overload function.
My concern here is confusion and bugs in future code. Since there are explicitly three choices:
- Implicit null
- Explicit null
- Explicit non-null registry
these three choices need to be apparent in the interface. Right now, there are two overloads, and one of them chooses one of these choices without it being obvious. For example, imagine [this method](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/svg/svg_script_element.cc;l=205;drc=13c2ee3faddbad7baed3819883d2c6438fe01247) were being added *after* this change lands. It might have been possible then, for the author to just not add the fourth argument, and miss that now SVG scripts would not properly construct elements with the right registry. That's bad, right? That relies on tests to catch such bugs, and it relies on the author of the change to know that there's a possible interaction that they might need to add a test for. If nothing breaks when they add it, they'll likely not even notice that there is an interaction.
So based on that, I'm still in favor of requiring the `registry` argument, and I think further we should have a static method on CustomElementRegistry called `CustomElementRegistry::ImplicitNullRegistry()` or similar, which is the one used where that makes sense. That feels easier to understand than just passing `customElementRegistry()`, because "customElementRegistry()" doesn't tell me what this means "implicit registry". I agree with the above that the two argument approach is a hassle. But this method still makes the choice explicit at the call site, rather than implicit.
I'm still open to being talked out of this, so let me know what I'm missing here. I.e. let's discuss before you make a big change to the API again. I want to avoid making you go through the pain if I've got the wrong idea here.
Related details
- YeongHan Kim
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Thank you for the detailed explanation!
I agree with your point.
What do you think, Jayson?
Related details
- Jayson Chen
- Mason Freed
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. |