Jayson Chen (Gerrit)

unread,

Jun 17, 2026, 7:37:40 PM (8 days ago) Jun 17

to Mason Freed, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dominicc+...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention needed from Mason Freed

Jayson Chen voted and added 1 comment

Votes added by Jayson Chen

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Jayson Chen . resolved

Hey Mason, here's the promised CL for SCER logic clean up. I was exploring using enum, but ended up finding using optional to be a fairly clean solution here. Lemme know how you think and we can iterate on it :)

Open in Gerrit

Related details

Attention is currently required from:

Mason Freed

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mason Freed (Gerrit)

unread,

Jun 18, 2026, 11:51:26 AM (7 days ago) Jun 18

to Jayson Chen, Chromium LUCI CQ, chromium...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dominicc+...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention needed from Jayson Chen

Mason Freed added 3 comments

Commit Message

Line 17, Patchset 6 (Latest):explanatory comments ateach site.

Mason Freed . unresolved

nit: at each

Line 22, Patchset 6 (Latest):

Element::SetCustomElementRegistry interpret as the single authority:
  * std::nullopt        -> inherit (resolve to the document's global registry)
  * std::optional(null) -> wait for a scoped registry to be assigned later
  * std::optional(reg)  -> explicit registry assignment

Mason Freed . unresolved

I still think this sets us up for bugs in the future:

`nullptr` - wait
`nullopt` - inherit

those are the same down to moving two letters and replacing one. Very easy to misread, and you always have to go look up which one means what.

How about you introduce a holding class that makes this all very clear. E.g.

```
class RegistryAssignment {
 public:
  enum class Type {
    kInherit,
    kExplicit,
    kWaitForScopedRegistry
  };
  static RegistryAssignment Inherit() { 
    return RegistryAssignment(Type::kInherit, nullptr); 
  }
  static RegistryAssignment Wait() { 
    return RegistryAssignment(Type::kWaitForScope, nullptr); 
  }
  static RegistryAssignment Explicit(CustomElementRegistry*
                                     registry) {
    return RegistryAssignment(Type::kExplicit, registry);
  }

  bool IsInherit() const { return type_ == Type::kInherit; }
  bool IsWait() const { return type_ == Type::kWaitForScope; }
  bool IsExplicit() const { return type_ == Type::kExplicit; }

  CustomElementRegistry* Registry() const {
    DCHECK(IsExplicit()); // Or whatever
    return registry_;
  }

 private:
  RegistryAssignment(Type type, CustomElementRegistry* registry)
      : type_(type), registry_(registry) {}

  Type type_;
  CustomElementRegistry* registry_;
};
```

Or something like that.

File third_party/blink/renderer/core/dom/element.cc

Line 7410, Patchset 6 (Latest): // std::nullopt means "inherit": the element resolves its registry through the

Mason Freed . unresolved

e.g. you shouldn't need this comment

Open in Gerrit

Related details

Attention is currently required from:

Jayson Chen

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jayson Chen (Gerrit)

unread,

Jun 22, 2026, 12:54:50 PM (3 days ago) Jun 22

to android-bu...@system.gserviceaccount.com, Mason Freed, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dominicc+...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention needed from Mason Freed

Jayson Chen added 4 comments

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Jayson Chen . resolved

Ready for another review

Commit Message

Line 17, Patchset 6:explanatory comments ateach site.

Mason Freed . resolved

nit: at each

Jayson Chen

Done

Line 22, Patchset 6:Element::SetCustomElementRegistry interpret as the single authority:


  * std::nullopt        -> inherit (resolve to the document's global registry)
  * std::optional(null) -> wait for a scoped registry to be assigned later
  * std::optional(reg)  -> explicit registry assignment

Mason Freed . resolved

Jayson Chen

Done.

File third_party/blink/renderer/core/dom/element.cc

Line 7410, Patchset 6: // std::nullopt means "inherit": the element resolves its registry through the

Mason Freed . resolved

e.g. you shouldn't need this comment

Jayson Chen

Done

Open in Gerrit

Related details

Attention is currently required from:

Mason Freed

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mason Freed (Gerrit)

unread,

Jun 24, 2026, 12:06:38 PM (yesterday) Jun 24

to Jayson Chen, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, blink-revi...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, dominicc+...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention needed from Jayson Chen

Mason Freed added 18 comments

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Mason Freed . resolved

Generally, I like this new approach. It's very explicit and clear at the call sites what's going on. Thanks for the conversion. Some comments, but I think we're on the right track.

Commit Message

Line 18, Patchset 10 (Latest):


Replace the (pointer, bool) pair with a single
std::optional<CustomElementRegistry*> "assignment" that both
TreeScope::SetCustomElementRegistry and


Element::SetCustomElementRegistry interpret as the single authority:
  * std::nullopt        -> inherit (resolve to the document's global registry)
  * std::optional(null) -> wait for a scoped registry to be assigned later
  * std::optional(reg)  -> explicit registry assignment

Mason Freed . unresolved

I think this needs a re-write.

File third_party/blink/renderer/core/dom/element.h

Line 1091, Patchset 10 (Latest): CustomElementRegistryAssignment registry_assignment,

Mason Freed . unresolved

nit: I think everywhere here, the variable name could likely just be `registry` and it'd still be pretty clear.

File third_party/blink/renderer/core/dom/element.cc

Line 1167, Patchset 10 (Latest):

      if (RuntimeEnabledFeatures::ScopedCustomElementRegistryEnabled() &&
          shadow_root->IsWaitingForScopedRegistry()) {
        registry_assignment = CustomElementRegistryAssignment::Wait();
      } else if (shadow_root_registry) {
        registry_assignment =
            CustomElementRegistryAssignment::Explicit(shadow_root_registry);
      }

Mason Freed . unresolved

nit:

```
CustomElementRegistryAssignment registry_assignment =
    CustomElementRegistryAssignment::Inherit();
if (RuntimeEnabledFeatures::ScopedCustomElementRegistryEnabled()) {
  if (shadow_root->IsWaitingForScopedRegistry()) {
    registry_assignment = CustomElementRegistryAssignment::Wait();
  } else if (shadow_root_registry) {
    registry_assignment =
        CustomElementRegistryAssignment::Explicit(shadow_root_registry);
  }
} else {
  DCHECK(!shadow_root_registry);
}
```

Line 7405, Patchset 10 (Latest): CustomElementRegistryAssignment assignment,

Mason Freed . unresolved

nit: `registry`

Line 7406, Patchset 10 (Latest): bool explicitly_set) {

Mason Freed . unresolved

It seems like `explicitly_set` needs to be removed, right? And replaced with a check of the `CustomElementRegistryAssignment` from the first param?

Line 7407, Patchset 10 (Latest):

  if (!RuntimeEnabledFeatures::ScopedCustomElementRegistryEnabled()) {
    return;
  }

Mason Freed . unresolved

Why can't this stay a DCHECK?

Line 7653, Patchset 10 (Latest): mode, focus_delegation, slot_assignment, registry, serializable, clonable,

Mason Freed . unresolved

This is currently implicitly converting to `CustomElementRegistryAssignment`. Likely this needs to be manually converted here/above.

Line 7692, Patchset 10 (Latest):


  CustomElementRegistryAssignment registry_assignment =
      CustomElementRegistryAssignment::Inherit();
  if (waiting_for_scoped_registry) {
    registry_assignment = CustomElementRegistryAssignment::Wait();
  } else if (registry) {
    registry_assignment = CustomElementRegistryAssignment::Explicit(registry);
  }

Mason Freed . unresolved

Since roughly this pattern happens a couple places, maybe make a constructor like

```
CustomElementRegistryAssignment(bool wait_for_registry,
                                CustomElementRegistry* registry)
```

?

Line 7771, Patchset 10 (Latest): shadow_root.SetCustomElementRegistry(registry_assignment);

Mason Freed . unresolved

Ahh - this answers my question from above. I think it'd be better to just do this here:

```
if (RuntimeEnabledFeatures::ScopedCustomElementRegistryEnabled()) {
  shadow_root.SetCustomElementRegistry(registry);
}
```

File third_party/blink/renderer/core/dom/tree_scope.cc

Line 361, Patchset 10 (Latest): waiting_for_registry_ = false;

Mason Freed . unresolved

Do we need this on treescope? Perhaps just also store a CustomElementRegistryAssignment?

File third_party/blink/renderer/core/frame/local_dom_window.cc

Line 2119, Patchset 10 (Latest): document_->SetCustomElementRegistry(custom_elements_.Get());

Mason Freed . unresolved

Another implicit conversion

File third_party/blink/renderer/core/html/custom/custom_element_registry_assignment.h

Line 64, Patchset 10 (Latest): : type_(type), registry_(registry) {}

Mason Freed . unresolved

Maybe also DCHECK here that things are correct, e.g. if it's Inherit, then registry should be null, etc.

Line 47, Patchset 10 (Latest): return CustomElementRegistryAssignment(Type::kExplicit, registry);

Mason Freed . unresolved

Maybe `DCHECK(registry)` here?

Line 36, Patchset 10 (Latest): CustomElementRegistryAssignment(CustomElementRegistry* registry)

Mason Freed . unresolved

This should definitely be `explicit` to avoid several implicit conversions above, some likely bugs.

Line 26, Patchset 10 (Latest):

// This is a transient, stack-only value (it holds a raw pointer and is never
// traced); do not store it as a member.

Mason Freed . unresolved

Comment not needed - that's implied by `STACK_ALLOCATED()`.

Line 20, Patchset 10 (Latest):

//   * Inherit          -> resolve to the document's global registry (the
//                         default; nothing is stored).
//   * Wait             -> the registry is explicitly null and waiting for a
//                         scoped registry to be assigned later.
//   * Explicit(reg)    -> use `reg` (a scoped or global registry).

Mason Freed . resolved

But this is great.

Line 16, Patchset 10 (Latest):

// registry when one is assigned. This is an explicit, self-documenting
// replacement for a `(CustomElementRegistry*, bool waiting)` pair, whose three
// meaningful states were easy to confuse (see crbug.com/512514744):

Mason Freed . unresolved

This sounds AI-ish, and likely isn't needed. 😊

Open in Gerrit

Related details

Attention is currently required from:

Jayson Chen

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Express custom element registry assignment as std::optional [chromium/src : main]

Jayson Chen (Gerrit)

Jayson Chen voted and added 1 comment

Votes added by Jayson Chen

1 comment

Related details

Mason Freed (Gerrit)

Mason Freed added 3 comments

Related details

Jayson Chen (Gerrit)

Jayson Chen added 4 comments

Related details

Mason Freed (Gerrit)

Mason Freed added 18 comments

Related details