[scoped-registry] Support cross document adoption [chromium/src : main]
Jayson Chen (Gerrit)
Jayson Chen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jayson Chen
| Commit-Queue | +1 |
1 comment
Hey all, here's the last big CL for the scoped registry for now. It implements the logic relating to cross document adoption of a node and how we take care of the registry in these scenarios.
Some note:
- In these scenarios, I found that in certain situation we DO want to know if an element's registry is null instead of by default falling back to the tree scope's registry, but instead of creating new data structure like we did before, I'm leveraging the rare data vector give us this flexibility on registry information storage.
- Given that it is the last chunk of implementation of the current spec/WPT requirement, I'm playing around with an optimization idea (patch 3) that should be able to reduce the need of storing registry in an element by a good amount by removing the field in rare data if we can just fallback to the tree scope's registry. Except in some cases (before we move a node cross scope) we want to ensure we retain knowledge from previous scope, for the rest of the majority cases, we could just erase the registry field in rare data if we can rely on the tree scope's registry.
PTAL, thanks!
Related details
- Joey Arhar
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 4 comments![]( "Open in Gerrit")
bool explicit_set = false);want to add a comment here explaining what explicit_set does?
std::optional<CustomElementRegistry*> GetCustomElementRegistry() const;want to add a comment here explaining the difference between nullptr and nullopt?
// We intentionally don't use SetField function because SetField will eraseI don't understand this... if we aren't supposed to be using SetField, then why is SetField being called here?
// Custom element registry of a tree scope can only be set once except the treeexcept when the?
Related details
- Jayson Chen
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 10 comments![]( "Open in Gerrit")
Generally looks good, just some concerns about the `nullptr` vs. `nullopt` thing.
CustomElementRegistry* EffectiveGlobalCustomElementRegistry() const;This could use a comment explaining what it is (i.e. what "effective" means). I don't love the name either - maybe something closer to `CustomElementRegistryIfGlobal()` or something? I don't know.
// If the registry is the same as the tree scope's registry, we typically
// can clear the registry field in rare data and have the registry implicitlyDoes this break in some circumstances? E.g. is there some way to later insert an element in between this one and the root, which uses a custom registry, and have that take effect for this element?
// inferred. There are certain scenarios where we want the registry to be+1 to Joey's comment - it'd be good to enumerate those scenarios.
std::optional<CustomElementRegistry*> GetCustomElementRegistry() const;want to add a comment here explaining the difference between nullptr and nullopt?
Yeah +1 to this. I'd perhaps go a step further and split it into two methods. One like `bool HasCustomElementRegistrySet()` that is used instead of `std::optional`, and then this one that does `DCHECK(HasCustomElementRegistrySet())` (so you only call it when you've already checked it), and then just returns the pointer. That's a lot easier to keep track of in your head, I think.
if (fields_.HasField(FieldId::kCustomElementRegistry)) {
return static_cast<CustomElementRegistry*>(
fields_.GetField(FieldId::kCustomElementRegistry).Get());
}
// Return nullopt to signify that the field has never been set.Yeah, the logic around nullptr vs. nullopt is really hard to wrap your head around.
if (GetField(FieldId::kCustomElementRegistry)) {
DCHECK(static_cast<CustomElementRegistry*>(
GetField(FieldId::kCustomElementRegistry))
->IsGlobalRegistry());
}Better not to have DCHECK-only code inside an `if`. Perhaps this instead?
```
DCHECK(!GetField(FieldId::kCustomElementRegistry) ||
static_cast<CustomElementRegistry*>(
GetField(FieldId::kCustomElementRegistry))
->IsGlobalRegistry());
```
// We intentionally don't use SetField function because SetField will eraseI don't understand this... if we aren't supposed to be using SetField, then why is SetField being called here?
I think he means ElementRareDataVector::SetField, rather than the one on `fields_`.
// the field if we set null to the field. However, there are scenarios whereAgain, good to enumerate scenarios.
// keep track of it in case the registry is implicitly implied by thenit: `implicitly implied` -> `implied`
But really this comment is a little hard to parse, if there's anything that can be simplified/clarified?
Related details
- Jayson Chen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |