ronantakizawa/codereview-bench huggingface dataset:
Repository: servo/servo
File: resources/debugger.js (JavaScript)

Code before:
    const script = sourceIdsToScripts.get(spidermonkeyId);
    function getPossibleBreakpointsRecursive(script) {
        const result = script.getPossibleBreakpoints(/* TODO: `query` */);
        for (const child of script.getChildScripts()) {
            for (const location of getPossibleBreakpointsRecursive(child)) {
                result.push(location);
            }
        }
        return result;
    }
    getPossibleBreakpointsResult(event, getPossibleBreakpointsRecursive(script));
});

addEventListener("setBreakpoint", event => {
    const {spidermonkeyId, offset} = event;
    const script = sourceIdsToScripts.get(spidermonkeyId);
    function breakpointHandler(...args) {
        // TODO: notify script to pause
        // tell spidermonkey to pause
       return {throw: "1"}
    }

    function getPossibleBreakpointsRecursive(script) {
        script.setBreakpoint(offset, { hit: breakpointHandler })
        for (const child of script.getChildScripts()) {
            getPossibleBreakpointsRecursive(child)
        }
    }

    getPossibleBreakpointsRecursive(script);
    setBreakpointResult(event, true)
});


Review comment (bug):
Let's add this doc here: https://firefox-source-docs.mozilla.org/js/Debugger/Conventions.html#resumption-values, should explain throw here(for now)

Code after:
    const script = sourceIdsToScripts.get(spidermonkeyId);
    let result = [];

    function getPossibleBreakpointsRecursive(script) {
        for (const location of script.getPossibleBreakpoints()) {
            location["scriptId"] = script.sourceStart;
            result.push(location);
        }
        for (const child of script.getChildScripts()) {
            getPossibleBreakpointsRecursive(child);
        }
    }
    getPossibleBreakpointsRecursive(script);

    getPossibleBreakpointsResult(event, result);
});

addEventListener("setBreakpoint", event => {
    const {spidermonkeyId, scriptId, offset} = event;
    const script = sourceIdsToScripts.get(spidermonkeyId);

    // <https://firefox-source-docs.mozilla.org/js/Debugger/Conventions.html#resumption-values>
    function breakpointHandler(...args) {
        // TODO: notify script to pause
        // tell spidermonkey to pause
       return {throw: "1"}
    }

    function setBreakpointRecursive(script) {
        if (script.sourceStart == scriptId) {
            script.setBreakpoint(offset, { hit: breakpointHandler });
            return;
        }
        for (const child of script.getChildScripts()) {
            setBreakpointRecursive(child);
        }
    }
    setBreakpointRecursive(script);
});
Repository: servo/servo
File: components/shared/base/generic_channel/shared_memory.rs (Rust)

Code before:
                // reconstruct the Box.
                #[expect(unsafe_code)]
                let arc = unsafe { Arc::from_raw(ptr) };
                Ok(GenericSharedMemory(GenericSharedMemoryVariant::InProcess(
                    arc,
                )))
            },
        }
    }
}

impl<'a> Deserialize<'a> for GenericSharedMemory {
    fn deserialize<D>(d: D) -> Result<GenericSharedMemory, D::Error>
    where
        D: Deserializer<'a>,
    {
        d.deserialize_enum(
            "GenericSharedMemory",
            &["Ipc", "InProcess"],
            GenericSharedMemoryVisitor {},
        )
    }
}

#[cfg(test)]
mod single_process_callback_test {
    use std::sync::Arc;

    use ipc_channel::ipc::IpcSharedMemory;

    use super::GenericSharedMemory;
    use crate::generic_channel::{self};

    #[test]
    fn test_ipc() {
        let bytes = vec![0xba; 10];
        let bytes_copy = bytes.clone();
        let shared_memory = GenericSharedMemory(super::GenericSharedMemoryVariant::Ipc(
            IpcSharedMemory::from_bytes(&bytes),
        ));

        let (send, recv) = generic_channel::channel().unwrap();
        send.send(shared_memory).expect("Could not send");
        assert_eq!(recv.recv().unwrap().to_vec(), bytes_copy);
    }

    #[test]
    fn test_inprocess() {
        let bytes = vec![0xba; 10];
        let bytes_copy = bytes.clone();
        let shared_memory = GenericSharedMemory(super::GenericSharedMemoryVariant::InProcess(

Review comment (suggestion):
```suggestion
mod single_process_shared_memory_test {
```

Code after:
                // reconstruct the Arc.
                #[expect(unsafe_code)]
                let arc = unsafe { Arc::from_raw(ptr) };
                Ok(GenericSharedMemory(GenericSharedMemoryVariant::InProcess(
                    arc,
                )))
            },
        }
    }
}

impl<'a> Deserialize<'a> for GenericSharedMemory {
    fn deserialize<D>(d: D) -> Result<GenericSharedMemory, D::Error>
    where
        D: Deserializer<'a>,
    {
        d.deserialize_enum(
            "GenericSharedMemory",
            &["Ipc", "InProcess"],
            GenericSharedMemoryVisitor {},
        )
    }
}

#[cfg(test)]
mod single_process_shared_memory_test {
    use std::sync::Arc;

    use ipc_channel::ipc::IpcSharedMemory;

    use super::GenericSharedMemory;
    use crate::generic_channel::{self};

    #[test]
    fn test_ipc() {
        let bytes = vec![0xba; 10];
        let bytes_copy = bytes.clone();
        let shared_memory = GenericSharedMemory(super::GenericSharedMemoryVariant::Ipc(
            IpcSharedMemory::from_bytes(&bytes),
        ));

        let (send, recv) = generic_channel::channel().unwrap();
        send.send(shared_memory).expect("Could not send");
        assert_eq!(recv.recv().unwrap().to_vec(), bytes_copy);
    }

    #[test]
    fn test_inprocess() {
        let bytes = vec![0xba; 10];
        let bytes_copy = bytes.clone();
        let shared_memory = GenericSharedMemory(super::GenericSharedMemoryVariant::InProcess(
Repository: servo/servo
File: components/script/dom/bindings/conversions.rs (Rust)

Code before:
        if !JS_HasProperty(*cx, object, name.as_ptr(), &mut found) || !found {
            if JS_IsExceptionPending(*cx) {
                return Err(Error::JSFailed);
            }
            return Ok(());
        }

        JS_GetProperty(*cx, object, name.as_ptr(), rval);
        if JS_IsExceptionPending(*cx) {
            return Err(Error::JSFailed);
        }
        Ok(())
    }
}

/// Get a property from a JS object, and convert it to a Rust value.
pub(crate) fn get_property<T>(
    cx: JSContext,
    object: HandleObject,
    name: &ffi::CStr,
    option: T::Config,
) -> Fallible<Option<T>>
where
    T: FromJSValConvertible,
{
    debug!("Getting property {:?}.", name);
    rooted!(in(*cx) let mut result = UndefinedValue());

    get_property_jsval(cx, object, &name, result.handle_mut())?;
    if result.is_undefined() {
        debug!("No property {:?}.", name);
        return Ok(None);
    }
    debug!("Converting property {:?}.", name);
    let value = unsafe { T::from_jsval(*cx, result.handle(), option) };
    match value {
        Ok(ConversionResult::Success(value)) => Ok(Some(value)),
        Ok(ConversionResult::Failure(_)) => Ok(None),
        Err(()) => Err(Error::JSFailed),
    }
}


Review comment (bug):
The debug impl for CStr says that the bytes will be interpreted as UTF-8 and displayed like a normal string: https://doc.rust-lang.org/std/ffi/struct.CStr.html#impl-Debug-for-CStr

I haven't tested that, but it means that these debug logs should still be useful.

Code after:
        if !JS_HasProperty(*cx, object, name.as_ptr(), &mut found) || !found {
            if JS_IsExceptionPending(*cx) {
                return Err(Error::JSFailed);
            }
            return Ok(());
        }

        JS_GetProperty(*cx, object, name.as_ptr(), rval);
        if JS_IsExceptionPending(*cx) {
            return Err(Error::JSFailed);
        }
        Ok(())
    }
}

/// Get a property from a JS object, and convert it to a Rust value.
pub(crate) fn get_property<T>(
    cx: JSContext,
    object: HandleObject,
    name: &ffi::CStr,
    option: T::Config,
) -> Fallible<Option<T>>
where
    T: FromJSValConvertible,
{
    debug!("Getting property {:?}.", name);
    rooted!(in(*cx) let mut result = UndefinedValue());

    get_property_jsval(cx, object, name, result.handle_mut())?;
    if result.is_undefined() {
        debug!("No property {:?}.", name);
        return Ok(None);
    }
    debug!("Converting property {:?}.", name);
    let value = unsafe { T::from_jsval(*cx, result.handle(), option) };
    match value {
        Ok(ConversionResult::Success(value)) => Ok(Some(value)),
        Ok(ConversionResult::Failure(_)) => Ok(None),
        Err(()) => Err(Error::JSFailed),
    }
}
Repository: servo/servo
File: tests/wpt/tests/FileAPI/blob/Blob-text.any.js (JavaScript)

Code before:
// META: global=window,worker
// META: title=Blob text() method tests

"use strict";

promise_test(async () => {
  const blob = new Blob(["HELLO"]);
  const result = await blob.text();
  assert_equals(result, "HELLO");
}, "Blob.text() returns correct string");

promise_test(async () => {
  const blob = new Blob([]);
  const result = await blob.text();
  assert_equals(result, "");
}, "Blob.text() returns empty string for empty blob");


Review comment (suggestion):
Actually, after looking it up, we don't really need additional tests for `Blob::text`, but only for classic script workers.
So please revert this change.

Code after:
// META: title=Blob Text
// META: script=../support/Blob.js
'use strict';

promise_test(async () => {
  const blob = new Blob(["PASS"]);
  const text = await blob.text();
  assert_equals(text, "PASS");
}, "Blob.text()")

promise_test(async () => {
  const blob = new Blob();
  const text = await blob.text();
  assert_equals(text, "");
}, "Blob.text() empty blob data")

promise_test(async () => {
  const blob = new Blob(["P", "A", "SS"]);
  const text = await blob.text();
  assert_equals(text, "PASS");
}, "Blob.text() multi-element array in constructor")

promise_test(async () => {
  const non_unicode = "\u0061\u030A";
  const input_arr = new TextEncoder().encode(non_unicode);
  const blob = new Blob([input_arr]);
Repository: servo/servo
File: components/script/dom/security/csp.rs (Rust)

Code before:
        let Some(csp_list) = self else {
            return false;
        };

        let (allowed_by_csp, violations) = csp_list
            .should_sink_type_mismatch_violation_be_blocked_by_csp(sink, sink_group, source);

        global.report_csp_violations(violations, None, None);

        allowed_by_csp == CheckResult::Blocked
    }

    /// <https://www.w3.org/TR/CSP3/#allow-base-for-document>
    fn is_base_allowed_for_document(
        &self,
        global: &GlobalScope,
        base: &servo_url::ServoUrl,
        self_origin: &servo_url::ImmutableOrigin,
    ) -> bool {
        let Some(csp_list) = self else {
            return true;
        };

        let (is_base_allowed, violations) = csp_list.is_base_allowed_for_document(
            &base.clone().into_url(),
            &self_origin.clone().into_url_origin(),
        );

        global.report_csp_violations(violations, None, None);

        is_base_allowed == CheckResult::Allowed
    }

    fn concatenate(self, new_csp_list: Option<CspList>) -> Option<CspList> {
        let Some(new_csp_list) = new_csp_list else {
            return self;
        };

        match self {
            None => Some(new_csp_list),
            Some(mut old_csp_list) => {
                old_csp_list.append(new_csp_list);
                Some(old_csp_list)
            },
        }
    }
}

pub(crate) struct SourcePosition {
    pub(crate) source_file: String,
    pub(crate) line_number: u32,

Review comment (suggestion):
Rather than doing `into_*` here, can we add these types as arguments to `is_base_allowed_for_document`? That way the caller takes care of it and we avoid accidentally cloning twice.

Code after:
        let Some(csp_list) = self else {
            return false;
        };

        let (allowed_by_csp, violations) = csp_list
            .should_sink_type_mismatch_violation_be_blocked_by_csp(sink, sink_group, source);

        global.report_csp_violations(violations, None, None);

        allowed_by_csp == CheckResult::Blocked
    }

    /// <https://www.w3.org/TR/CSP3/#allow-base-for-document>
    fn is_base_allowed_for_document(
        &self,
        global: &GlobalScope,
        base: &url::Url,
        self_origin: &url::Origin,
    ) -> bool {
        let Some(csp_list) = self else {
            return true;
        };

        let (is_base_allowed, violations) =
            csp_list.is_base_allowed_for_document(base, self_origin);

        global.report_csp_violations(violations, None, None);

        is_base_allowed == CheckResult::Allowed
    }

    fn concatenate(self, new_csp_list: Option<CspList>) -> Option<CspList> {
        let Some(new_csp_list) = new_csp_list else {
            return self;
        };

        match self {
            None => Some(new_csp_list),
            Some(mut old_csp_list) => {
                old_csp_list.append(new_csp_list);
                Some(old_csp_list)
            },
        }
    }
}

pub(crate) struct SourcePosition {
    pub(crate) source_file: String,
    pub(crate) line_number: u32,
    pub(crate) column_number: u32,
}
Repository: servo/servo
File: components/script/dom/documentorshadowroot.rs (Rust)

Code before:
    pub(crate) fn get_fullscreen_element(
        node: &Node,
        fullscreen_element: Option<DomRoot<Element>>,
    ) -> Option<DomRoot<Element>> {
        // Step 1. If this is a shadow root and its host is not connected, then return null.
        if let Some(shadow_root) = node.downcast::<ShadowRoot>() {
            if !shadow_root.Host().is_connected() {
                return None;
            }
        }

        // Step 2. Let candidate be the result of retargeting fullscreen element against this.
        let retargeted = fullscreen_element?
            .upcast::<EventTarget>()
            .retarget(node.upcast());
        let candidate = retargeted.downcast::<Element>().unwrap();

        // Step 3. If candidate and this are in the same tree, then return candidate.
        let are_in_same_tree = candidate
            .upcast::<Node>()
            .GetRootNode(&GetRootNodeOptions::empty()) ==
            node.GetRootNode(&GetRootNodeOptions::empty());

        if are_in_same_tree {
            return Some(DomRoot::from_ref(candidate));
        }

        // Step 4. Return null.
        None
    }
}


Review comment (suggestion):
```suggestion
        if candidate
            .upcast::<Node>()
            .GetRootNode(&GetRootNodeOptions::empty()) ==
            node
        {
            return Some(DomRoot::from_ref(candidate));
        }
```
I believe since retarget already trying to get the element and the target in the same tree, and the node is a root of the tree all we need to do is this. 

Code after:
    pub(crate) fn get_fullscreen_element(
        node: &Node,
        fullscreen_element: Option<DomRoot<Element>>,
    ) -> Option<DomRoot<Element>> {
        // Step 1. If this is a shadow root and its host is not connected, then return null.
        if let Some(shadow_root) = node.downcast::<ShadowRoot>() {
            if !shadow_root.Host().is_connected() {
                return None;
            }
        }

        // Step 2. Let candidate be the result of retargeting fullscreen element against this.
        let retargeted = fullscreen_element?
            .upcast::<EventTarget>()
            .retarget(node.upcast());
        // It's safe to unwrap downcasting to `Element` because `retarget` either returns `fullscreen_element` or a host of `fullscreen_element` and hosts are always elements.
        let candidate = DomRoot::downcast::<Element>(retargeted).unwrap();

        // Step 3. If candidate and this are in the same tree, then return candidate.
        if *candidate
            .upcast::<Node>()
            .GetRootNode(&GetRootNodeOptions::empty()) ==
            *node
        {
            return Some(candidate);
        }

        // Step 4. Return null.
        None
    }
}
Repository: servo/servo
File: ports/servoshell/prefs.rs (Rust)

Code before:
    ///  Config directory following xdg spec on linux platform.
    #[bpaf(argument("~/.config/servo"))]
    config_dir: Option<PathBuf>,

    ///
    ///  Run as a content process and connect to the given pipe.
    #[bpaf(argument("servo-ipc-channel.abcdefg"))]
    content_process: Option<String>,

    ///
    ///  A comma-separated string of debug options. Pass help to show available options.
    #[bpaf(
        short('Z'),
        argument("layout_grid_enabled=true,dom_async_clipboard_enabled"),
        long,
        map(map_debug_options),
        fallback(vec![])
    )]
    debug: Vec<String>,

    ///
    ///  Device pixels per px.
    #[bpaf(argument("1.0"))]
    device_pixel_ratio: Option<f32>,

    /// Start remote devtools server on port listening on this address
    #[bpaf(argument("127.0.0.1:7000"))]
    devtools: Option<String>,

    ///
    ///  Whether or not to enable experimental web platform features.
    #[bpaf(long)]
    enable_experimental_web_platform_features: bool,

    // Exit after Servo has loaded the page and detected a stable output image.
    #[bpaf(short('x'), long)]
    exit: bool,

    // Use ipc_channel in singleprocess mode.
    #[bpaf(short('I'), long("force-ipc"))]
    force_ipc: bool,

    /// Exit on thread failure instead of displaying about:failure.
    #[bpaf(short('f'), long)]
    hard_fail: bool,

    /// Headless mode.
    #[bpaf(short('z'), long)]
    headless: bool,

    ///

Review comment (suggestion):
I think this deserves some additional clarification, that both `<address>:<port>` and `<port>` are valid values.

Code after:
    ///  Config directory following xdg spec on linux platform.
    #[bpaf(argument("~/.config/servo"))]
    config_dir: Option<PathBuf>,

    ///
    ///  Run as a content process and connect to the given pipe.
    #[bpaf(argument("servo-ipc-channel.abcdefg"))]
    content_process: Option<String>,

    ///
    ///  A comma-separated string of debug options. Pass help to show available options.
    #[bpaf(
        short('Z'),
        argument("layout_grid_enabled=true,dom_async_clipboard_enabled"),
        long,
        map(map_debug_options),
        fallback(vec![])
    )]
    debug: Vec<String>,

    ///
    ///  Device pixels per px.
    #[bpaf(argument("1.0"))]
    device_pixel_ratio: Option<f32>,

    /// Start remote devtools server on port listening on this address. <address>:<port> and <port> are valid values.
    #[bpaf(argument("127.0.0.1:7000"))]
    devtools: Option<String>,

    ///
    ///  Whether or not to enable experimental web platform features.
    #[bpaf(long)]
    enable_experimental_web_platform_features: bool,

    // Exit after Servo has loaded the page and detected a stable output image.
    #[bpaf(short('x'), long)]
    exit: bool,

    // Use ipc_channel in singleprocess mode.
    #[bpaf(short('I'), long("force-ipc"))]
    force_ipc: bool,

    /// Exit on thread failure instead of displaying about:failure.
    #[bpaf(short('f'), long)]
    hard_fail: bool,

    /// Headless mode.
    #[bpaf(short('z'), long)]
    headless: bool,

    ///
Repository: servo/servo
File: components/devtools/actors/inspector.rs (Rust)

Code before:
        };

        let walker = WalkerActor {
            name: registry.new_name::<WalkerActor>(),
            mutations: AtomicRefCell::new(vec![]),
            script_chan,
            pipeline,
        };

        let actor = Self {
            name: registry.new_name::<InspectorActor>(),
            highlighter: highlighter.name(),
            page_style: page_style.name(),
            walker: walker.name(),
        };
        let name = actor.name();

        registry.register(highlighter);
        registry.register(page_style);
        registry.register(walker);
        registry.register(actor);

        name
    }

    pub(crate) fn walker(&self) -> &str {
        &self.walker
    }
}


Review comment (suggestion):
It might be better to use `pub(crate)` on the attribute instead like you did on the browsing context

Code after:
        };

        let walker = WalkerActor {
            name: registry.new_name::<WalkerActor>(),
            mutations: AtomicRefCell::new(vec![]),
            script_chan,
            pipeline,
        };

        let actor = Self {
            name: registry.new_name::<InspectorActor>(),
            highlighter: highlighter.name(),
            page_style: page_style.name(),
            walker: walker.name(),
        };
        let name = actor.name();

        registry.register(highlighter);
        registry.register(page_style);
        registry.register(walker);
        registry.register(actor);

        name
    }
}
Repository: servo/servo
File: components/layout/table/construct.rs (Rust)

Code before:
    // The HTML specification clamps value of `span` for `<col>` to [1, 1000].
    assert!((1..=1000).contains(&span));

    let column = old_column.unwrap_or_else(|| {
        ArcRefCell::new(TableTrack {
            base: LayoutBoxBase::new(column_info.into(), column_info.style.clone()),
            group_index,
            is_anonymous,
            shared_background_style: SharedStyle::new(column_info.style.clone()),
        })
    });
    collection.extend(repeat_n(column.clone(), span as usize));
    column
}

impl TableSlotCell {
    pub(crate) fn rebuild(
        &mut self,
        layout_context: &LayoutContext,
        node_and_style_info: &NodeAndStyleInfo,
    ) {
        self.contents = BlockFormattingContext::construct(
            layout_context,
            node_and_style_info,
            NonReplacedContents::OfElement,
            // TODO: This only works because currently PropagatedBoxTreeDataonly stores
            // this single bit of information. If we ever add more information to
            // PropagatedBoxTreeDataonly, this will need to be stored on the table cell
            // and reused.
            PropagatedBoxTreeData::default().disallowing_percentage_table_columns(),
            false, /* is_list_item */
        );
        self.base.clear_fragments_and_fragment_cache();
        *self.base.cached_inline_content_size.borrow_mut() = None;
        self.base.repair_style(&node_and_style_info.style);
    }
}


Review comment (suggestion):
```suggestion
            // TODO: This only works because currently PropagatedBoxTreeData only stores
```

Code after:
    // The HTML specification clamps value of `span` for `<col>` to [1, 1000].
    assert!((1..=1000).contains(&span));

    let column = old_column.unwrap_or_else(|| {
        ArcRefCell::new(TableTrack {
            base: LayoutBoxBase::new(column_info.into(), column_info.style.clone()),
            group_index,
            is_anonymous,
            shared_background_style: SharedStyle::new(column_info.style.clone()),
        })
    });
    collection.extend(repeat_n(column.clone(), span as usize));
    column
}

impl TableSlotCell {
    pub(crate) fn rebuild(
        &mut self,
        layout_context: &LayoutContext,
        node_and_style_info: &NodeAndStyleInfo,
    ) {
        self.contents = BlockFormattingContext::construct(
            layout_context,
            node_and_style_info,
            NonReplacedContents::OfElement,
            // TODO: This only works because currently PropagatedBoxTreeData only stores
            // this single bit of information. If we ever add more information to
            // PropagatedBoxTreeData, this will need to be stored on the table cell
            // and reused.
            PropagatedBoxTreeData::default().disallowing_percentage_table_columns(),
            false, /* is_list_item */
        );
        self.base.clear_fragments_and_fragment_cache();
        *self.base.cached_inline_content_size.borrow_mut() = None;
        self.base.repair_style(&node_and_style_info.style);
    }
}