Notify changed slots in different shadow roots during manual assignment [chromium/src : main]

0 views

Skip to first unread message

Mason Freed (Gerrit)

unread,

Jun 24, 2026, 8:45:12 PM (2 days ago) Jun 24

to android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

Attention needed from Joey Arhar

Mason Freed voted and added 3 comments

Votes added by Mason Freed

Code-Review

3 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Mason Freed . resolved

LGTM, but seems like we need a flag.

File third_party/blink/renderer/core/html/html_slot_element.cc

Line 262, Patchset 2 (Latest): if (shadow_root) {

Mason Freed . unresolved

Was it possible to get here with a null `shadow_root` before? The code on the left doesn't have this and doesn't seem (?) to crash. Perhaps CHECK instead?

Line 270, Patchset 2 (Latest):

    // A previous slot may belong to a different shadow tree than `this`, or
    // `this` may not be in a shadow tree at all. Such slots are not reached
    // by the traversal above; signal them here.
    for (HTMLSlotElement* slot : changed_slots) {
      slot->DidSlotChange(SlotChangeType::kSignalSlotChangeEvent);
    }

Mason Freed . unresolved

Should this go behind a kill switch, just in case?

Open in Gerrit

Related details

Attention is currently required from:

Joey Arhar

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

Joey Arhar (Gerrit)

unread,

2:44 PM (1 hour ago) 2:44 PM

to Mason Freed, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, jmedle...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

Attention needed from Mason Freed

Joey Arhar added 2 comments

File third_party/blink/renderer/core/html/html_slot_element.cc

Line 262, Patchset 2: if (shadow_root) {

Mason Freed . resolved

Was it possible to get here with a null `shadow_root` before? The code on the left doesn't have this and doesn't seem (?) to crash. Perhaps CHECK instead?

Joey Arhar

Apparently so, I just added a test case which hits this null shadow_root case.

Line 270, Patchset 2: // A previous slot may belong to a different shadow tree than `this`, or


    // `this` may not be in a shadow tree at all. Such slots are not reached
    // by the traversal above; signal them here.
    for (HTMLSlotElement* slot : changed_slots) {
      slot->DidSlotChange(SlotChangeType::kSignalSlotChangeEvent);
    }

Mason Freed . resolved

Should this go behind a kill switch, just in case?

Joey Arhar

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

Reply all

Reply to author

Forward

0 new messages