Repository: servo/servo
File: components/script/dom/html/htmldetailselement.rs (Rust)

Code before:
    fn ensure_details_exclusivity(
        &self,
        conflict_resolution_behaviour: ExclusivityConflictResolution,
    ) {
        // NOTE: This method implements two spec algorithms that are very similar to each other, distinguished by the
        // `conflict_resolution_behaviour` argument. Steps that are different between the two are annotated with two
        // spec comments.

        // Step 1. Assert: element has an open attribute.
        // Step 1. If element does not have an open attribute, then return.
        if !self.Open() {
            if conflict_resolution_behaviour ==
                ExclusivityConflictResolution::CloseExistingOpenElement
            {
                unreachable!()
            } else {
                return;
            }
        }

        // Step 2. If element does not have a name attribute, or its name attribute
        // is the empty string, then return.
        let name = self.Name();
        if name.is_empty() {
            return;
        }

        // Step 3. Let groupMembers be a list of elements, containing all elements in element's
        // details name group except for element, in tree order.
        // Step 4. For each element otherElement of groupMembers:
        //     Step 4.1 If the open attribute is set on otherElement, then:
        //         Step 4.1.1 Assert: otherElement is the only element in groupMembers that has the open attribute set.
        //         Step 4.1.2 Remove the open attribute on otherElement.
        //         Step 4.1.3 Break.
        //
        //         Step 4.1.1 Remove the open attribute on element.
        //         Step 4.1.2 Break.
        // NOTE: We implement an optimization that allows us to easily find details group members when the
        // root of the tree is a document or shadow root, which is why this looks a bit more complicated.
        let other_open_member = if let Some(shadow_root) = self.containing_shadow_root() {
            shadow_root
                .details_name_groups()
                .group_members_for(&name, self)
                .find(|group_member| group_member.Open())
        } else if self.upcast::<Node>().is_in_a_document_tree() {
            self.owner_document()
                .details_name_groups()
                .group_members_for(&name, self)
                .find(|group_member| group_member.Open())
        } else {
            // This is the slow case, which is hopefully not too common.

Review comment (refactor):
This step as well as the checks in `{un}register_details_element` are all duplicate. Given that we call them multiple times, should we make this an assert instead and handle it when handling attribute changes? I think if the name is empty, we can skip a whole bunch of logic, can we not?

Code after:
    fn ensure_details_exclusivity(
        &self,
        conflict_resolution_behaviour: ExclusivityConflictResolution,
    ) {
        // NOTE: This method implements two spec algorithms that are very similar to each other, distinguished by the
        // `conflict_resolution_behaviour` argument. Steps that are different between the two are annotated with two
        // spec comments.

        // Step 1. Assert: element has an open attribute.
        // Step 1. If element does not have an open attribute, then return.
        if !self.Open() {
            if conflict_resolution_behaviour ==
                ExclusivityConflictResolution::CloseExistingOpenElement
            {
                unreachable!()
            } else {
                return;
            }
        }

        // Step 2. If element does not have a name attribute, or its name attribute
        // is the empty string, then return.
        let name = self.Name();
        if name.is_empty() {
            return;
        }

        // Step 3. Let groupMembers be a list of elements, containing all elements in element's
        // details name group except for element, in tree order.
        // Step 4. For each element otherElement of groupMembers:
        //     Step 4.1 If the open attribute is set on otherElement, then:

        // NOTE: We implement an optimization that allows us to easily find details group members when the
        // root of the tree is a document or shadow root, which is why this looks a bit more complicated.
        let other_open_member = if let Some(shadow_root) = self.containing_shadow_root() {
            shadow_root
                .details_name_groups()
                .group_members_for(&name, self)
                .find(|group_member| group_member.Open())
        } else if self.upcast::<Node>().is_in_a_document_tree() {
            self.owner_document()
                .details_name_groups()
                .group_members_for(&name, self)
                .find(|group_member| group_member.Open())
        } else {
            // This is the slow case, which is hopefully not too common.
            self.upcast::<Node>()
                .GetRootNode(&GetRootNodeOptions::empty())
                .traverse_preorder(ShadowIncluding::No)
                .flat_map(DomRoot::downcast::<HTMLDetailsElement>)
                .filter(|details_element| {