Allow the creation of null registry element and shadow root [chromium/src : main]
YeongHan Kim (Gerrit)
YeongHan Kim voted and added 1 comment![]( "Open in Gerrit")
Votes added by YeongHan Kim
| Commit-Queue | +1 |
1 comment
Please take a look.
This CL reflects Anne’s latest changes.
Thank you!
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 added 3 comments![]( "Open in Gerrit")
The implementation looks good to me, but it seems like it's causing some WPTs and unit tests failure. Would you be able to take a look at the unit test failures on CQ to see if it's something you're able to fix? Feel free to leave it to me to investigate if you're not sure about it.
Also, it may be a good idea to use virtual test suite for now.
// 3-2. If options[""customElementRegistry"] exists:nit: extra "
This is a testharness.js-based test.These test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
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")
Thanks for the review!
I uploaded this CL mainly to confirm the overall direction first.
I’ll take a closer look at the WPT and unit test failures on CQ and investigate what’s going wrong.
Thanks again for the helpful inputs!
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 4 comments![]( "Open in Gerrit")
Sorry for the wait! When you get a chance, could you take another look?
Thanks!
// 3-2. If options[""customElementRegistry"] exists:YeongHan Kimnit: extra "
Done
is = AtomicString(string_or_options->GetAsString());According to the specification, the element creation options parameter can accept either a string or an ElementCreationOptions dictionary, and the string must be ignored.
This behavior is covered by the `Document-createElement-customized-builtins` test.
However, the current implementation incorrectly assigns the is attribute when a string is provided.
The reason the test previously passed is that when flattening the CreateElementOptions, the default value of the registry was null, which caused the `definition` step to be skipped.
Due to the recent spec change, the default registry is now the global custom element registry, which exposed this issue.
Therefore, this patch updates the behavior so that when a string is passed as the element creation options, it is ignored as required by the specification.
These test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
Previously, if the `registry` is coming as null, `SetCustomElementRegisty()` is skipped in `CreateUncustomizedOrUndefinedElementTemplate`[1]. However, with the spec change, if ElementCreationOptions.registry is set to null, we now need to call SetCustomElementRegistry() with null.
Since `SetCustomElementRegistry` can be invoked in various situations, it should only set the `registry` when ElementCreationOptions.registry is explicitly null. To address this, I introduced an `is_explicit_null` flag.
I’m not entirely sure if this is the best approach, so I’d appreciate any feedback or suggestions for a better solution.
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 added 3 comments![]( "Open in Gerrit")
Thanks for looking into the test failures and finding this issue. Left some thoughts about how we can handle the two null cases (explicit vs implicit) in CreateElement
is = AtomicString(string_or_options->GetAsString());According to the specification, the element creation options parameter can accept either a string or an ElementCreationOptions dictionary, and the string must be ignored.
This behavior is covered by the `Document-createElement-customized-builtins` test.
However, the current implementation incorrectly assigns the is attribute when a string is provided.The reason the test previously passed is that when flattening the CreateElementOptions, the default value of the registry was null, which caused the `definition` step to be skipped.
Due to the recent spec change, the default registry is now the global custom element registry, which exposed this issue.Therefore, this patch updates the behavior so that when a string is passed as the element creation options, it is ignored as required by the specification.
Sounds good, thanks for finding this issue.
This is a testharness.js-based test.YeongHan KimThese test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
Previously, if the `registry` is coming as null, `SetCustomElementRegisty()` is skipped in `CreateUncustomizedOrUndefinedElementTemplate`[1]. However, with the spec change, if ElementCreationOptions.registry is set to null, we now need to call SetCustomElementRegistry() with null.
Since `SetCustomElementRegistry` can be invoked in various situations, it should only set the `registry` when ElementCreationOptions.registry is explicitly null. To address this, I introduced an `is_explicit_null` flag.
I’m not entirely sure if this is the best approach, so I’d appreciate any feedback or suggestions for a better solution.
Thanks for looking into this issue! Now it seems like the main issue is that CreateElement function needs to know whether the passed in nullptr is an explicit null or implicit null (so default registry). I wonder if this will be a good chance for us to overload CreateElement function (as mentioned in the comment in code), one without registry argument so we provide the default registry in the impl and one with registry argument and we always respect the passed in value. This way we don't need to differentiate two different kinds of nullptr in CreateElement and also don't need to have extra explicit null optional argument that may be confusing down the line. This may require some changes to update CreateElement usages though.
One related question: in flatten element creation option, do we need `is_explicit_null`? Given that we by default use the default registry if not provided by option, is it possible to have a null registry value that should actually be considered as default value? If we do get nullptr from `document->customElementRegistry()` as the default registry value, we'd want to consider that an explicit null registry, right?
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")
Thanks for the review!
This is a testharness.js-based test.YeongHan KimThese test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
Jayson ChenPreviously, if the `registry` is coming as null, `SetCustomElementRegisty()` is skipped in `CreateUncustomizedOrUndefinedElementTemplate`[1]. However, with the spec change, if ElementCreationOptions.registry is set to null, we now need to call SetCustomElementRegistry() with null.
Since `SetCustomElementRegistry` can be invoked in various situations, it should only set the `registry` when ElementCreationOptions.registry is explicitly null. To address this, I introduced an `is_explicit_null` flag.
I’m not entirely sure if this is the best approach, so I’d appreciate any feedback or suggestions for a better solution.
Thanks for looking into this issue! Now it seems like the main issue is that CreateElement function needs to know whether the passed in nullptr is an explicit null or implicit null (so default registry). I wonder if this will be a good chance for us to overload CreateElement function (as mentioned in the comment in code), one without registry argument so we provide the default registry in the impl and one with registry argument and we always respect the passed in value. This way we don't need to differentiate two different kinds of nullptr in CreateElement and also don't need to have extra explicit null optional argument that may be confusing down the line. This may require some changes to update CreateElement usages though.
One related question: in flatten element creation option, do we need `is_explicit_null`? Given that we by default use the default registry if not provided by option, is it possible to have a null registry value that should actually be considered as default value? If we do get nullptr from `document->customElementRegistry()` as the default registry value, we'd want to consider that an explicit null registry, right?
Thanks for the guidance. I think overloading the CreateElement without a registry parameter is a great approach. Really appreciate the idea!
Regarding your question: I believe the spec intends for an explicit null registry only when ElementCreationOptions.registry is explicitly set to null. So the nullptr returned from `document->customElementRegistry()` as the default registry should not count as an explicit null. (And this distinction is necessary for the tests to pass.)
Right now, `is_explicit_null` is used during flattening so that CreateElement can determine whether the null was explicitly provided. But if we move forward with overloading CreateElement to have a version without the registry argument, then I think we can remove is_explicit_null entirely from the flattening step.
I’ll update the patch soon and ask for another review. Thanks so much for your help!
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. |
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. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Related details
- Jayson Chen
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")
Sorry for the repeated review requests. I’ve addressed the feedback now.
Thanks so much!
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);I removed the overload and explicitly pass the default registry as an argument.
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 3 comments![]( "Open in Gerrit")
Votes added by Jayson Chen
| Code-Review | +1 |
3 comments
Like the direction and LGTM. Just few small nits.
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);That makes sense to me! I like the idea of having a static method to capture the missing concept here. Instead of `ImplicitNullRegistry()`, I wonder if it makes sense to just call it `DefaultRegistry()` as that will effectively make the argument fall into these three choices:
- Default
- Null
- Non-null registry
which aligns with the spec more; whereas implicit null is more like an implementation detail that is harder to understand while reading the code.
registry = customElementRegistry();If we call the static method `DefaultRegistry()`, I'd suggest that we use the static method here as well.
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 4 comments![]( "Open in Gerrit")
Thanks for the review! I’ve addressed your comments.
Element* CreateElement(const QualifiedName&,
const CreateElementFlags,
const AtomicString& is);I agree, `DefaultRegistry()` seems like a more appropriate name.
Thanks for the suggestion!
CustomElementRegistry::DefaultRegistry(*document);I’ve made the same change here to use `DefaultRegistry()`.
If we call the static method `DefaultRegistry()`, I'd suggest that we use the static method here as well.
- 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 Code-Review+1![]( "Open in Gerrit")
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 5 comments![]( "Open in Gerrit")
This looks good to me, other than a rename/move, and some nits. Thanks for discussing the overloaded function!
"the "Nit: please recombine this with the next line.
/*registry*/ CustomElementRegistry::DefaultRegistry(document)));nit: you don't need `/*registry*/` here since it's clear from the argument that you're passing a registry.
/*registry*/ CustomElementRegistry::DefaultRegistry(GetDocument())));ditto here and below
CustomElementRegistry* CustomElementRegistry::DefaultRegistry(
Document& document) {
return document.customElementRegistry();
}Rather than `CustomElementRegistry::DefaultRegistry(document)`, perhaps just rename `TreeScope::customElementRegistry` to `TreeScope::DefaultCustomElementRegistry` and change all of the callers to this function to just `document.DefaultCustomElementRegistry()`? (And then delete `CustomElementRegistry::DefaultRegistry`.)
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 5 comments![]( "Open in Gerrit")
Thank you for review!
I left a few comments regarding the rename.
"the "YeongHan KimNit: please recombine this with the next line.
Done
/*registry*/ CustomElementRegistry::DefaultRegistry(document)));nit: you don't need `/*registry*/` here since it's clear from the argument that you're passing a registry.
Done
/*registry*/ CustomElementRegistry::DefaultRegistry(GetDocument())));YeongHan Kimditto here and below
Done
CustomElementRegistry* CustomElementRegistry::DefaultRegistry(
Document& document) {
return document.customElementRegistry();
}Rather than `CustomElementRegistry::DefaultRegistry(document)`, perhaps just rename `TreeScope::customElementRegistry` to `TreeScope::DefaultCustomElementRegistry` and change all of the callers to this function to just `document.DefaultCustomElementRegistry()`? (And then delete `CustomElementRegistry::DefaultRegistry`.)
`TreeScope::customElementRegistry()` is used as a mixin in [document_or_shadow_root.idl](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document_or_shadow_root.idl;l=31), so renaming it would require implementing `customElementRegistry()` on both `Document` and `ShadowRoot`.
Could you please confirm whether this direction is what you intended?
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. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
LGTM! Thanks for working on this.
CustomElementRegistry* CustomElementRegistry::DefaultRegistry(
Document& document) {
return document.customElementRegistry();
}YeongHan KimRather than `CustomElementRegistry::DefaultRegistry(document)`, perhaps just rename `TreeScope::customElementRegistry` to `TreeScope::DefaultCustomElementRegistry` and change all of the callers to this function to just `document.DefaultCustomElementRegistry()`? (And then delete `CustomElementRegistry::DefaultRegistry`.)
`TreeScope::customElementRegistry()` is used as a mixin in [document_or_shadow_root.idl](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/document_or_shadow_root.idl;l=31), so renaming it would require implementing `customElementRegistry()` on both `Document` and `ShadowRoot`.
Could you please confirm whether this direction is what you intended?
Oh boo, I forgot that was web exposed. Ok, it's fine as-is. Thanks for checking first.
Related details
- Jayson Chen
- YeongHan Kim
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 Code-Review+1![]( "Open in Gerrit")
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
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")
It looks like the Review-Enforcement requirement hasn’t been satisfied yet.
Sorry for the trouble, but could you please take a look and check on this?
Thank you so much. I really appreciate it!
Related details
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 added 1 comment![]( "Open in Gerrit")
I think it has something to do with me never setting up reauth properly. Lemme try to fix it.
Related details
- YeongHan Kim
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 Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- YeongHan Kim
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 Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- YeongHan Kim
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 added 1 comment![]( "Open in Gerrit")
Should be good now. Thanks again for working on this :))
YeongHan Kim (Gerrit)
YeongHan Kim voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Allow the creation of null registry element and shadow root
As discussed in [1], support is being added to allow elements to have
a null customElementRegistry without Shadow DOM. To support this, the
specification has been updated [2] to allow null as a valid argument for
creating elements and shadow roots.
This patch implements that update.
[1] https://github.com/whatwg/dom/issues/1413
[2] https://github.com/whatwg/dom/pull/1424
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/dom/element_creation_options.idl
- M third_party/blink/renderer/core/editing/commands/editing_commands_utilities.cc
- M third_party/blink/renderer/core/editing/commands/indent_outdent_command.cc
- M third_party/blink/renderer/core/html/custom/custom_element_registry.cc
- M third_party/blink/renderer/core/html/custom/custom_element_registry.h
- M third_party/blink/renderer/core/html/custom/custom_element_test_helpers.h
- M third_party/blink/renderer/core/html/parser/html_construction_site.cc
- M third_party/blink/renderer/core/xml/parser/xml_document_parser.cc
- M third_party/blink/renderer/core/xml/parser/xml_document_parser_rs.cc
- D third_party/blink/web_tests/external/wpt/custom-elements/Document-createElement-customized-builtins-expected.txt
- D third_party/blink/web_tests/external/wpt/custom-elements/Document-createElementNS-customized-builtins-expected.txt
- D third_party/blink/web_tests/platform/mac/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/element-mutation-expected.txt
- M third_party/blink/web_tests/platform/mac/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/template.window-expected.txt
- D third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/Document-createElement-customized-builtins-expected.txt
- D third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/Document-createElementNS-customized-builtins-expected.txt
- M third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/Document-createElement-expected.txt
- M third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/Document-createElementNS-expected.txt
- M third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/Element-customElementRegistry-expected.txt
- M third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/ShadowRoot-init-customElementRegistry-expected.txt
- M third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/scoped-custom-element-registry-customelementregistry-attribute-expected.txt
- M third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/scoped-registry-append-does-not-upgrade-expected.txt
- M third_party/blink/web_tests/virtual/scoped-custom-element-registry/external/wpt/custom-elements/registries/template.window-expected.txt
Code-Review: +1 by Jayson Chen, +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |