Ensure uncustomized element can be set to null registry [chromium/src : main]
Jayson Chen (Gerrit)
Jayson Chen added 1 comment![]( "Open in Gerrit")
I'm really debating between this implementation versus keeping `CreateUncustomizedOrUndefinedElement` as is (ignoring the passed in registry if it is nullptr) and explicitly set nullptr registry to the element when needed (in current case, in html_construction_site while parsing fragment. Would like to get your thoughts on this.
Related details
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")
PTAL at this bug fix on scoped registry, thanks!
Related details
- Joey Arhar
- 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 added 2 comments![]( "Open in Gerrit")
Jayson ChenI'm really debating between this implementation versus keeping `CreateUncustomizedOrUndefinedElement` as is (ignoring the passed in registry if it is nullptr) and explicitly set nullptr registry to the element when needed (in current case, in html_construction_site while parsing fragment. Would like to get your thoughts on this.
Re-commenting this in commit message so it's visible from Files section.
Ensure uncustomized element can be set to null registryI'm really debating between this implementation versus keeping CreateUncustomizedOrUndefinedElement as is (ignoring the passed in registry if it is nullptr) and explicitly set nullptr registry to the element when needed (in current case, in html_construction_site while parsing fragment. Would like to get your thoughts on this.
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Do you know if the other browsers pass these new tests?
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 added 1 comment![]( "Open in Gerrit")
Do you know if the other browsers pass these new tests?
Safari (the only other browser with SCER) is failing these tests as these tests also pointed out some issue in their implementation. Safari will get the null registry assignment correct but somehow accidentally upgrade some of those uncustomized elements into `WrongNewElement`, which means they somehow used the global registry in the upgrading process.
Related details
- Joey Arhar
- 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 2 comments![]( "Open in Gerrit")
std::optional<CustomElementRegistry*> registry);So this brings back the (confusing in my opinion) nullopt-vs-nullptr thing. Perhaps add a `bool no_customized_registry` or whatever wording makes sense, rather than the std::optional?
}, 'insertAdjacentHTML should use the element\'s registry even when the registry is null');This will need at least expectations files to make the bots green.
Related details
- Jayson Chen
- Joey Arhar
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 3 comments![]( "Open in Gerrit")
Votes added by Jayson Chen
| Commit-Queue | +1 |
Jayson ChenI'm really debating between this implementation versus keeping CreateUncustomizedOrUndefinedElement as is (ignoring the passed in registry if it is nullptr) and explicitly set nullptr registry to the element when needed (in current case, in html_construction_site while parsing fragment. Would like to get your thoughts on this.
Done
std::optional<CustomElementRegistry*> registry);So this brings back the (confusing in my opinion) nullopt-vs-nullptr thing. Perhaps add a `bool no_customized_registry` or whatever wording makes sense, rather than the std::optional?
Agree, I was really debating when I wrote those nullopt. I updated it to use an additional arg as a flag to indicate we want to keep the null registry or no. Lmk how you think about the naming :)
}, 'insertAdjacentHTML should use the element\'s registry even when the registry is null');This will need at least expectations files to make the bots green.
The dry run seems to be passing in the last patch, am I missing something here?
Related details
- Joey Arhar
- Mason Freed
- youssef saikouk
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 4 comments![]( "Open in Gerrit")
std::optional<CustomElementRegistry*> registry);Jayson ChenSo this brings back the (confusing in my opinion) nullopt-vs-nullptr thing. Perhaps add a `bool no_customized_registry` or whatever wording makes sense, rather than the std::optional?
Agree, I was really debating when I wrote those nullopt. I updated it to use an additional arg as a flag to indicate we want to keep the null registry or no. Lmk how you think about the naming :)
I think `wait_for_registry` makes it very clear - thanks for this change.
Only thing I'd change is to remove the default value. Otherwise it's kind of unclear what it means to pass in `nullptr`.
(registry || wait_for_registry)) {Seems like it's worth DCHECKing here that if `wait_for_registry` is true, then `registry` is nullptr.
// registry on the created element null.nit: `to null`?
}, 'insertAdjacentHTML should use the element\'s registry even when the registry is null');Jayson ChenThis will need at least expectations files to make the bots green.
The dry run seems to be passing in the last patch, am I missing something here?
- Jayson Chen
- Joey Arhar
- youssef saikouk
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 3 comments![]( "Open in Gerrit")
Votes added by Jayson Chen
| Commit-Queue | +1 |
3 comments
std::optional<CustomElementRegistry*> registry);Jayson ChenSo this brings back the (confusing in my opinion) nullopt-vs-nullptr thing. Perhaps add a `bool no_customized_registry` or whatever wording makes sense, rather than the std::optional?
Mason FreedAgree, I was really debating when I wrote those nullopt. I updated it to use an additional arg as a flag to indicate we want to keep the null registry or no. Lmk how you think about the naming :)
I think `wait_for_registry` makes it very clear - thanks for this change.
Only thing I'd change is to remove the default value. Otherwise it's kind of unclear what it means to pass in `nullptr`.
Sounds good, removed the default value.
(registry || wait_for_registry)) {Seems like it's worth DCHECKing here that if `wait_for_registry` is true, then `registry` is nullptr.
Imo the registry doesn't have to be nullptr when we set `wait_for_registry` to true. See the usage in `html_construction_site`, I was thinking this flag is there to indicate that "if you get nullptr, don't ignore it!", but it can totally still receive valid registry.
// registry on the created element null.Jayson Chennit: `to null`?
Done
Related details
- Joey Arhar
- Mason Freed
- youssef saikouk
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 2 comments![]( "Open in Gerrit")
// Wait for registry flag indicates whether we want to ignore a passed
// in null registry and let element implicitly pick up the tree scope's
// registry or keep it null and wait for a registry to be set later.This is still a bit unclear, at least to me. What about this?
```
If `wait_for_registry` is false, `registry` will be used even if it is
nullptr, meaning the element will use the TreeScope's registry. If
`wait_for_registry` is true, then `registry` will be ignored, and
the element will wait for a registry to be provided.
```
(registry || wait_for_registry)) {Jayson ChenSeems like it's worth DCHECKing here that if `wait_for_registry` is true, then `registry` is nullptr.
Imo the registry doesn't have to be nullptr when we set `wait_for_registry` to true. See the usage in `html_construction_site`, I was thinking this flag is there to indicate that "if you get nullptr, don't ignore it!", but it can totally still receive valid registry.
Man I'm still confused. I think maybe the description I wrote above might even be wrong. I thought we had three cases:
1. Wait for a registry. Don't use a higher-level registry, just don't upgrade elements.
2. Nullptr registry. Use the TreeScope's (or higher level) registry.
3. non-nullptr registry. Use that registry.
I would think these three are mutually exclusive. What does it mean to "wait for a registry" while also "using the non-nullptr registry you have"?
Related details
- Jayson Chen
- Joey Arhar
- youssef saikouk
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")
(registry || wait_for_registry)) {Jayson ChenSeems like it's worth DCHECKing here that if `wait_for_registry` is true, then `registry` is nullptr.
Mason FreedImo the registry doesn't have to be nullptr when we set `wait_for_registry` to true. See the usage in `html_construction_site`, I was thinking this flag is there to indicate that "if you get nullptr, don't ignore it!", but it can totally still receive valid registry.
Man I'm still confused. I think maybe the description I wrote above might even be wrong. I thought we had three cases:
1. Wait for a registry. Don't use a higher-level registry, just don't upgrade elements.
2. Nullptr registry. Use the TreeScope's (or higher level) registry.
3. non-nullptr registry. Use that registry.I would think these three are mutually exclusive. What does it mean to "wait for a registry" while also "using the non-nullptr registry you have"?
Hmm I may have confused you a bit. Let's reset and rename this arg to `allow_null_registry` so we don't get confused with the exisiting wait-for-registry concept. I think the confusion may be coming from that.
You're correct, these are the three cases we have and they're mutually exclusive. I'll add a small annotation as below:
1. Wait for a registry (Explicit null). Don't use a higher-level registry, just don't upgrade elements.
2. Didn't set registry (Implicit null). Use the TreeScope's (or higher level) registry.
3. non-nullptr registry. Use that registry.
The goal we're trying to achieve in this patch is that when we're creating uncustomized element, we want to have control over the created uncustomized element being in case(1) or case(2). For most of the time, we want to do case(2) so it will just pick up the tree scope's registry. However, when we're parsing fragment, aka dealing with innerHTML, the registry assignment is dependent on the container element, which can be a valid registry or nullptr. The happy path is getting a valid registry, but in the case of nullptr registry, we need to make sure the element is actually set to explicit null registry bc the treescope may have valid registry and the new uncustomized element will pick up the tree scope registry if we don't make it explicit null. This awkwardness is due to the fact that we're expecting an element acting as a scoping object but implementation wise, we have no real way to make connection between an inserted element and its original container element, so explicitly setting it to null is necessary.
Back to the usage in html construction site, it's trying to create uncustomized element while parsing fragment, and we want to turn on `allow_null_registry` flag to make sure if we actually want to use null as registry to create this uncustomizeed element, we can explicitly set it.
Does renaming the arg make the reasoning a bit clearer?
Related details
- Joey Arhar
- Mason Freed
- youssef saikouk
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")
(registry || wait_for_registry)) {Jayson ChenSeems like it's worth DCHECKing here that if `wait_for_registry` is true, then `registry` is nullptr.
Mason FreedImo the registry doesn't have to be nullptr when we set `wait_for_registry` to true. See the usage in `html_construction_site`, I was thinking this flag is there to indicate that "if you get nullptr, don't ignore it!", but it can totally still receive valid registry.
Jayson ChenMan I'm still confused. I think maybe the description I wrote above might even be wrong. I thought we had three cases:
1. Wait for a registry. Don't use a higher-level registry, just don't upgrade elements.
2. Nullptr registry. Use the TreeScope's (or higher level) registry.
3. non-nullptr registry. Use that registry.I would think these three are mutually exclusive. What does it mean to "wait for a registry" while also "using the non-nullptr registry you have"?
Hmm I may have confused you a bit. Let's reset and rename this arg to `allow_null_registry` so we don't get confused with the exisiting wait-for-registry concept. I think the confusion may be coming from that.
You're correct, these are the three cases we have and they're mutually exclusive. I'll add a small annotation as below:
1. Wait for a registry (Explicit null). Don't use a higher-level registry, just don't upgrade elements.
2. Didn't set registry (Implicit null). Use the TreeScope's (or higher level) registry.
3. non-nullptr registry. Use that registry.The goal we're trying to achieve in this patch is that when we're creating uncustomized element, we want to have control over the created uncustomized element being in case(1) or case(2). For most of the time, we want to do case(2) so it will just pick up the tree scope's registry. However, when we're parsing fragment, aka dealing with innerHTML, the registry assignment is dependent on the container element, which can be a valid registry or nullptr. The happy path is getting a valid registry, but in the case of nullptr registry, we need to make sure the element is actually set to explicit null registry bc the treescope may have valid registry and the new uncustomized element will pick up the tree scope registry if we don't make it explicit null. This awkwardness is due to the fact that we're expecting an element acting as a scoping object but implementation wise, we have no real way to make connection between an inserted element and its original container element, so explicitly setting it to null is necessary.
Back to the usage in html construction site, it's trying to create uncustomized element while parsing fragment, and we want to turn on `allow_null_registry` flag to make sure if we actually want to use null as registry to create this uncustomizeed element, we can explicitly set it.
Does renaming the arg make the reasoning a bit clearer?
Thanks for the great explanation here! I think some part of that big middle paragraph should likely make its way into the comment above `CreateUncustomizedOrUndefinedElementTemplate()` in the header file.
But given what you said, I really don't like `allow_null_registry` since that doesn't really tell me what's going on. Like "allow" in what sense? I'd rather be more explicit, and given your explanation, I actually really like `wait_for_registry` as a name. That tells me what will happen.
I'll come back to my comment - I think you should `CHECK(!wait_for_registry || !registry)`. You mentioned there were cases that would hit that - I'd modify those call sites to never do that. Because this is an unmentioned 4th case: non-nullptr registry but also wait_for_registry - which one wins? Let's just avoid that case.
Related details
- Jayson Chen
- Joey Arhar
- youssef saikouk
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 2 comments![]( "Open in Gerrit")
Votes added by Jayson Chen
| Commit-Queue | +1 |
2 comments
// Wait for registry flag indicates whether we want to ignore a passed
// in null registry and let element implicitly pick up the tree scope's
// registry or keep it null and wait for a registry to be set later.This is still a bit unclear, at least to me. What about this?
```
If `wait_for_registry` is false, `registry` will be used even if it is
nullptr, meaning the element will use the TreeScope's registry. If
`wait_for_registry` is true, then `registry` will be ignored, and
the element will wait for a registry to be provided.
```
Updated the comments and put it above `CreateUncustomizedOrUndefinedElementTemplate`
(registry || wait_for_registry)) {Jayson ChenSeems like it's worth DCHECKing here that if `wait_for_registry` is true, then `registry` is nullptr.
Mason FreedImo the registry doesn't have to be nullptr when we set `wait_for_registry` to true. See the usage in `html_construction_site`, I was thinking this flag is there to indicate that "if you get nullptr, don't ignore it!", but it can totally still receive valid registry.
Jayson ChenMan I'm still confused. I think maybe the description I wrote above might even be wrong. I thought we had three cases:
1. Wait for a registry. Don't use a higher-level registry, just don't upgrade elements.
2. Nullptr registry. Use the TreeScope's (or higher level) registry.
3. non-nullptr registry. Use that registry.I would think these three are mutually exclusive. What does it mean to "wait for a registry" while also "using the non-nullptr registry you have"?
Mason FreedHmm I may have confused you a bit. Let's reset and rename this arg to `allow_null_registry` so we don't get confused with the exisiting wait-for-registry concept. I think the confusion may be coming from that.
You're correct, these are the three cases we have and they're mutually exclusive. I'll add a small annotation as below:
1. Wait for a registry (Explicit null). Don't use a higher-level registry, just don't upgrade elements.
2. Didn't set registry (Implicit null). Use the TreeScope's (or higher level) registry.
3. non-nullptr registry. Use that registry.The goal we're trying to achieve in this patch is that when we're creating uncustomized element, we want to have control over the created uncustomized element being in case(1) or case(2). For most of the time, we want to do case(2) so it will just pick up the tree scope's registry. However, when we're parsing fragment, aka dealing with innerHTML, the registry assignment is dependent on the container element, which can be a valid registry or nullptr. The happy path is getting a valid registry, but in the case of nullptr registry, we need to make sure the element is actually set to explicit null registry bc the treescope may have valid registry and the new uncustomized element will pick up the tree scope registry if we don't make it explicit null. This awkwardness is due to the fact that we're expecting an element acting as a scoping object but implementation wise, we have no real way to make connection between an inserted element and its original container element, so explicitly setting it to null is necessary.
Back to the usage in html construction site, it's trying to create uncustomized element while parsing fragment, and we want to turn on `allow_null_registry` flag to make sure if we actually want to use null as registry to create this uncustomizeed element, we can explicitly set it.
Does renaming the arg make the reasoning a bit clearer?
Thanks for the great explanation here! I think some part of that big middle paragraph should likely make its way into the comment above `CreateUncustomizedOrUndefinedElementTemplate()` in the header file.
But given what you said, I really don't like `allow_null_registry` since that doesn't really tell me what's going on. Like "allow" in what sense? I'd rather be more explicit, and given your explanation, I actually really like `wait_for_registry` as a name. That tells me what will happen.
I'll come back to my comment - I think you should `CHECK(!wait_for_registry || !registry)`. You mentioned there were cases that would hit that - I'd modify those call sites to never do that. Because this is an unmentioned 4th case: non-nullptr registry but also wait_for_registry - which one wins? Let's just avoid that case.
Sounds good! Updated the call site to make sure the use of `wait_for_registry` flag is intentional on specific scenarios. (also updated the comments)
Related details
- Joey Arhar
- Mason Freed
- youssef saikouk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 6 comments![]( "Open in Gerrit")
// it to null and it will wait for a registry to be set later.Is this comment about the wait_for_registry parameter? If so, want to mention it by name?
// When a null registry is pasaed in, we want to have control over if thepassed?
// When a null registry is pasaed in, we want to have control over if theover whether? or does this mean something else?
// Wait for registry flag indicates whether we want to ignore a passedThe `wait_for_registry`?
// registry during fragment parsing. Set wait for registry flag to trueThe `wait_for_registry`
Jayson ChenDo you know if the other browsers pass these new tests?
Safari (the only other browser with SCER) is failing these tests as these tests also pointed out some issue in their implementation. Safari will get the null registry assignment correct but somehow accidentally upgrade some of those uncustomized elements into `WrongNewElement`, which means they somehow used the global registry in the upgrading process.
Is anyone on webkit aware of this? Should the spec be changed or a spec issue get opened?
Related details
- Jayson Chen
- Mason Freed
- youssef saikouk
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 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
// Keep the usage intentional that wait_for_registry is meant for the
// scenario where we get null registry passed in and want to create an
// element with explicit null registry.Ok as-is, but this comment likely isn't needed. That's what the DCHECK says: either registry is `nullptr` or we're not `waiting_for_registry`.
assert_equals(container_element.querySelector('new-element').customElementRegistry, null);Maybe add
```
const outer_element = container_element.querySelector('new-element');
const inner_element = outer_element.querySelector('new-element');
```
and then use those in all the places?
Related details
- Jayson Chen
- youssef saikouk
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. |