ronantakizawa/codereview-bench huggingface dataset:
Repository: servo/servo
File: components/shared/embedder/input_events.rs (Rust)

Code before:
impl From<MouseButton> for i16 {
    fn from(value: MouseButton) -> Self {
        match value {
            MouseButton::Left => 0,
            MouseButton::Middle => 1,
            MouseButton::Right => 2,
            MouseButton::Back => 3,
            MouseButton::Forward => 4,
            MouseButton::Other(value) => value as i16,
        }
    }
}

/// The types of mouse events
#[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)]
pub enum MouseButtonAction {
    /// Mouse button down
    Down,
    /// Mouse button up
    Up,
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub struct MouseMoveEvent {
    pub point: WebViewPoint,
    is_compatibility_for_touoch: bool,
}

impl MouseMoveEvent {
    pub fn new(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_for_touoch: false,
        }
    }

    pub fn new_compatibility_for_touch(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_for_touoch: true,
        }
    }

    pub fn is_compatibility_for_touch(&self) -> bool {
        self.is_compatibility_for_touoch
    }
}

#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize)]
pub struct MouseLeftViewportEvent {
    pub focus_moving_to_another_iframe: bool,

Review comment (suggestion):
```suggestion
    is_compatibility_for_touch: bool,
```

Code after:
impl From<MouseButton> for i16 {
    fn from(value: MouseButton) -> Self {
        match value {
            MouseButton::Left => 0,
            MouseButton::Middle => 1,
            MouseButton::Right => 2,
            MouseButton::Back => 3,
            MouseButton::Forward => 4,
            MouseButton::Other(value) => value as i16,
        }
    }
}

/// The types of mouse events
#[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)]
pub enum MouseButtonAction {
    /// Mouse button down
    Down,
    /// Mouse button up
    Up,
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub struct MouseMoveEvent {
    pub point: WebViewPoint,
    pub is_compatibility_event_for_touch: bool,
}

impl MouseMoveEvent {
    pub fn new(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_event_for_touch: false,
        }
    }

    pub fn new_compatibility_for_touch(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_event_for_touch: true,
        }
    }
}

#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize)]
pub struct MouseLeftViewportEvent {
    pub focus_moving_to_another_iframe: bool,
}

/// The type of input represented by a multi-touch event.
#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
Repository: servo/servo
File: components/shared/embedder/input_events.rs (Rust)

Code before:
impl From<MouseButton> for i16 {
    fn from(value: MouseButton) -> Self {
        match value {
            MouseButton::Left => 0,
            MouseButton::Middle => 1,
            MouseButton::Right => 2,
            MouseButton::Back => 3,
            MouseButton::Forward => 4,
            MouseButton::Other(value) => value as i16,
        }
    }
}

/// The types of mouse events
#[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)]
pub enum MouseButtonAction {
    /// Mouse button down
    Down,
    /// Mouse button up
    Up,
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub struct MouseMoveEvent {
    pub point: WebViewPoint,
    is_compatibility_for_touoch: bool,
}

impl MouseMoveEvent {
    pub fn new(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_for_touoch: false,
        }
    }

    pub fn new_compatibility_for_touch(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_for_touoch: true,
        }
    }

    pub fn is_compatibility_for_touch(&self) -> bool {
        self.is_compatibility_for_touoch
    }
}

#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize)]
pub struct MouseLeftViewportEvent {
    pub focus_moving_to_another_iframe: bool,

Review comment (suggestion):
```suggestion
    pub is_compatibility_for_touoch: bool,
```

Code after:
impl From<MouseButton> for i16 {
    fn from(value: MouseButton) -> Self {
        match value {
            MouseButton::Left => 0,
            MouseButton::Middle => 1,
            MouseButton::Right => 2,
            MouseButton::Back => 3,
            MouseButton::Forward => 4,
            MouseButton::Other(value) => value as i16,
        }
    }
}

/// The types of mouse events
#[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)]
pub enum MouseButtonAction {
    /// Mouse button down
    Down,
    /// Mouse button up
    Up,
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub struct MouseMoveEvent {
    pub point: WebViewPoint,
    pub is_compatibility_event_for_touch: bool,
}

impl MouseMoveEvent {
    pub fn new(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_event_for_touch: false,
        }
    }

    pub fn new_compatibility_for_touch(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_event_for_touch: true,
        }
    }
}

#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize)]
pub struct MouseLeftViewportEvent {
    pub focus_moving_to_another_iframe: bool,
}

/// The type of input represented by a multi-touch event.
#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
Repository: servo/servo
File: components/shared/embedder/input_events.rs (Rust)

Code before:
            MouseButton::Forward => 4,
            MouseButton::Other(value) => value as i16,
        }
    }
}

/// The types of mouse events
#[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)]
pub enum MouseButtonAction {
    /// Mouse button down
    Down,
    /// Mouse button up
    Up,
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub struct MouseMoveEvent {
    pub point: WebViewPoint,
    pub is_compatibility_for_touch: bool,
}

impl MouseMoveEvent {
    pub fn new(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_for_touch: false,
        }
    }

    pub fn new_compatibility_for_touch(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_for_touch: true,
        }
    }
}

#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize)]
pub struct MouseLeftViewportEvent {
    pub focus_moving_to_another_iframe: bool,
}

/// The type of input represented by a multi-touch event.
#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub enum TouchEventType {
    /// A new touch point came in contact with the screen.
    Down,
    /// An existing touch point changed location.
    Move,
    /// A touch point was removed from the screen.
    Up,

Review comment (suggestion):
This reads a bit odd to me, so I suggest the following name:

```suggestion
            is_compatibility_event_for_touch: false,
```

Code after:
            MouseButton::Forward => 4,
            MouseButton::Other(value) => value as i16,
        }
    }
}

/// The types of mouse events
#[derive(Clone, Copy, Debug, Deserialize, MallocSizeOf, PartialEq, Serialize)]
pub enum MouseButtonAction {
    /// Mouse button down
    Down,
    /// Mouse button up
    Up,
}

#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub struct MouseMoveEvent {
    pub point: WebViewPoint,
    pub is_compatibility_event_for_touch: bool,
}

impl MouseMoveEvent {
    pub fn new(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_event_for_touch: false,
        }
    }

    pub fn new_compatibility_for_touch(point: WebViewPoint) -> Self {
        Self {
            point,
            is_compatibility_event_for_touch: true,
        }
    }
}

#[derive(Clone, Copy, Debug, Default, Deserialize, Serialize)]
pub struct MouseLeftViewportEvent {
    pub focus_moving_to_another_iframe: bool,
}

/// The type of input represented by a multi-touch event.
#[derive(Clone, Copy, Debug, Deserialize, Serialize)]
pub enum TouchEventType {
    /// A new touch point came in contact with the screen.
    Down,
    /// An existing touch point changed location.
    Move,
    /// A touch point was removed from the screen.
    Up,
Repository: servo/servo
File: components/shared/storage/client_storage.rs (Rust)

Code before:
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at https://mozilla.org/MPL/2.0/. */

use base::generic_channel::GenericSender;
use serde::{Deserialize, Serialize};

#[derive(Debug, Deserialize, Serialize)]
pub enum ClientStorageThreadMsg {
    /// Send a reply when done cleaning up thread resources and then shut it down
    Exit(GenericSender<()>),
}


Review comment (suggestion):
Some messages use "Msg" and some use "Message" in the name so let's go with "Message":


```suggestion
pub enum ClientStorageThreadMessage {
```

Code after:
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at https://mozilla.org/MPL/2.0/. */

use base::generic_channel::GenericSender;
use serde::{Deserialize, Serialize};

#[derive(Debug, Deserialize, Serialize)]
pub enum ClientStorageThreadMessage {
    /// Send a reply when done cleaning up thread resources and then shut it down
    Exit(GenericSender<()>),
}
Repository: servo/servo
File: components/storage/indexeddb/mod.rs (Rust)

Code before:
                } else {
                    let _ = sender.send(Err(BackendError::DbNotFound));
                }
            },
            SyncOperation::StartTransaction(sender, origin, db_name, txn) => {
                if let Some(db) = self.get_database_mut(origin, db_name) {
                    db.start_transaction(txn, Some(sender));
                } else {
                    let _ = sender.send(Err(BackendError::DbNotFound));
                }
            },
            SyncOperation::Version(sender, origin, db_name) => {
                if let Some(db) = self.get_database(origin, db_name) {
                    let _ = sender.send(db.version().map_err(BackendError::from));
                } else {
                    let _ = sender.send(Err(BackendError::DbNotFound));
                }
            },
            SyncOperation::RegisterNewTxn(sender, origin, db_name) => {
                if let Some(db) = self.get_database_mut(origin, db_name) {
                    db.serial_number_counter += 1;
                    let _ = sender.send(db.serial_number_counter);
                }
            },
            SyncOperation::Exit(_) => {
                unreachable!();
            },
        }
    }
}


Review comment (bug):
```suggestion
                unreachable!("We must've already broken out of event loop.");
```

Code after:
                } else {
                    let _ = sender.send(Err(BackendError::DbNotFound));
                }
            },
            SyncOperation::StartTransaction(sender, origin, db_name, txn) => {
                if let Some(db) = self.get_database_mut(origin, db_name) {
                    db.start_transaction(txn, Some(sender));
                } else {
                    let _ = sender.send(Err(BackendError::DbNotFound));
                }
            },
            SyncOperation::Version(sender, origin, db_name) => {
                if let Some(db) = self.get_database(origin, db_name) {
                    let _ = sender.send(db.version().map_err(BackendError::from));
                } else {
                    let _ = sender.send(Err(BackendError::DbNotFound));
                }
            },
            SyncOperation::RegisterNewTxn(sender, origin, db_name) => {
                if let Some(db) = self.get_database_mut(origin, db_name) {
                    db.serial_number_counter += 1;
                    let _ = sender.send(db.serial_number_counter);
                }
            },
            SyncOperation::Exit(_) => {
                unreachable!("We must've already broken out of event loop.");
            },
        }
    }
}
Repository: servo/servo
File: components/devtools/actors/object.rs (Rust)

Code before:
        // TODO: Handle enumSymbols for console object inspection
        Err(ActorError::UnrecognizedPacketType)
    }
}

impl ObjectActor {
    pub fn register(registry: &ActorRegistry, uuid: String) -> String {
        if !registry.script_actor_registered(uuid.clone()) {
            let name = registry.new_name("object");
            let actor = ObjectActor {
                name: name.clone(),
                _uuid: uuid.clone(),
            };

            registry.register_script_actor(uuid, name.clone());
            registry.register_later(Box::new(actor));

            name
        } else {
            registry.script_to_actor(uuid)
        }
    }
}

// TODO: Remove once the message is used.
#[expect(dead_code)]
pub trait ObjectToProtocol {
    fn encode(self) -> ObjectActorMsg;
}

impl ObjectToProtocol for ObjectActor {
    fn encode(self) -> ObjectActorMsg {
        // TODO: Review hardcoded values here
        ObjectActorMsg {
            actor: self.name,
            type_: "object".into(),
            class: "Window".into(),
            own_property_length: 0,
            extensible: true,
            frozen: false,
            sealed: false,
            is_error: false,
            preview: ObjectPreview {
                kind: "ObjectWithURL".into(),
                url: "".into(),
            },
        }
    }
}


Review comment (suggestion):
Please don't use `expect(dead_code)`, see https://github.com/servo/servo/issues/40882 for the reasoning

Code after:
        // TODO: Handle enumSymbols for console object inspection
        Err(ActorError::UnrecognizedPacketType)
    }
}

impl ObjectActor {
    pub fn register(registry: &ActorRegistry, uuid: String) -> String {
        if !registry.script_actor_registered(uuid.clone()) {
            let name = registry.new_name("object");
            let actor = ObjectActor {
                name: name.clone(),
                _uuid: uuid.clone(),
            };

            registry.register_script_actor(uuid, name.clone());
            registry.register_later(Box::new(actor));

            name
        } else {
            registry.script_to_actor(uuid)
        }
    }
}

// TODO: Remove once the message is used.
#[allow(dead_code)]
pub trait ObjectToProtocol {
    fn encode(self) -> ObjectActorMsg;
}

impl ObjectToProtocol for ObjectActor {
    fn encode(self) -> ObjectActorMsg {
        // TODO: Review hardcoded values here
        ObjectActorMsg {
            actor: self.name,
            type_: "object".into(),
            class: "Window".into(),
            own_property_length: 0,
            extensible: true,
            frozen: false,
            sealed: false,
            is_error: false,
            preview: ObjectPreview {
                kind: "ObjectWithURL".into(),
                url: "".into(),
            },
        }
    }
}
Repository: servo/servo
File: components/fonts/platform/freetype/android/xml.rs (Rust)

Code before:
    let mut nodes = Vec::new();
    for result in xml::EventReader::new(bytes) {
        match result? {
            StartElement {
                name, attributes, ..
            } => {
                stack.push((name, attributes, nodes));
                nodes = Vec::new();
            },
            EndElement { .. } => {
                let (name, attributes, mut parent_nodes) = stack.pop().unwrap();
                parent_nodes.push(Node::Element {
                    name,
                    attributes,
                    children: nodes,
                });
                nodes = parent_nodes;
            },
            CData(s) | Characters(s) | Whitespace(s) => {
                if let Some(Node::Text(text)) = nodes.last_mut() {
                    text.push_str(&s)
                } else {
                    nodes.push(Node::Text(s))
                }
            },
            StartDocument { .. } | EndDocument | ProcessingInstruction { .. } | Comment(..) => {}, _ => {},
        }
    }
    assert!(stack.is_empty());
    Ok(nodes)
}


Review comment (suggestion):
Instead of listing half of the variants and then a wildcard, please add the new variant here. So instead of `_`, write `Doctype`.

Code after:
    let mut nodes = Vec::new();
    for result in xml::EventReader::new(bytes) {
        match result? {
            StartElement {
                name, attributes, ..
            } => {
                stack.push((name, attributes, nodes));
                nodes = Vec::new();
            },
            EndElement { .. } => {
                let (name, attributes, mut parent_nodes) = stack.pop().unwrap();
                parent_nodes.push(Node::Element {
                    name,
                    attributes,
                    children: nodes,
                });
                nodes = parent_nodes;
            },
            CData(s) | Characters(s) | Whitespace(s) => {
                if let Some(Node::Text(text)) = nodes.last_mut() {
                    text.push_str(&s)
                } else {
                    nodes.push(Node::Text(s))
                }
            },
            StartDocument { .. } | EndDocument | ProcessingInstruction { .. } | Comment(..) => {},
            Doctype => {},
        }
    }
    assert!(stack.is_empty());
    Ok(nodes)
}
Repository: servo/servo
File: components/script/dom/html/htmlformelement.rs (Rust)

Code before:
                    )
                };

                let content_disposition = format!("form-data; name=\"{}\"; {}", entry.name, extra);
                // https://tools.ietf.org/html/rfc7578#section-4.4
                let content_type: Mime = f
                    .upcast::<Blob>()
                    .Type()
                    .parse()
                    .unwrap_or(mime::TEXT_PLAIN);
                let mut type_bytes = format!(
                    "Content-Disposition: {}\r\ncontent-type: {}\r\n\r\n",
                    content_disposition, content_type
                )
                .into_bytes();
                result.append(&mut type_bytes);

                let mut bytes = f.upcast::<Blob>().get_bytes().unwrap_or(vec![]);

                result.append(&mut bytes);
                result.extend(b"\r\n");
            },
        }
    }

    let mut boundary_bytes = format!("--{}--\r\n", boundary).into_bytes();
    result.append(&mut boundary_bytes);

    result
}

// https://tools.ietf.org/html/rfc7578#section-4.1
pub(crate) fn generate_boundary() -> String {
    let i1 = random::<u32>();
    let i2 = random::<u32>();

    format!("---------------------------{0}{1}", i1, i2)
}


Review comment (suggestion):
```suggestion
    let mut boundary_bytes = format!("--{boundary}--\r\n").into_bytes();
```

Code after:

                let content_disposition = format!("form-data; name=\"{}\"; {}", entry.name, extra);
                // https://tools.ietf.org/html/rfc7578#section-4.4
                let content_type: Mime = f
                    .upcast::<Blob>()
                    .Type()
                    .parse()
                    .unwrap_or(mime::TEXT_PLAIN);
                let mut type_bytes = format!(
                    "Content-Disposition: {}\r\ncontent-type: {}\r\n\r\n",
                    content_disposition, content_type
                )
                .into_bytes();
                result.append(&mut type_bytes);

                let mut bytes = f.upcast::<Blob>().get_bytes().unwrap_or(vec![]);

                result.append(&mut bytes);
                result.extend(b"\r\n");
            },
        }
    }

    let mut boundary_bytes = format!("--{boundary}--\r\n").into_bytes();
    result.append(&mut boundary_bytes);

    result
}

// https://tools.ietf.org/html/rfc7578#section-4.1
pub(crate) fn generate_boundary() -> String {
    let i1 = random::<u32>();
    let i2 = random::<u32>();

    format!("---------------------------{0}{1}", i1, i2)
}
Repository: servo/servo
File: components/storage/webstorage/mod.rs (Rust)

Code before:
}

#[derive(Deserialize, Serialize)]
pub struct StorageDescriptor {
    origin_descriptors: FxHashMap<String, OriginDescriptor>,
}

impl StorageDescriptor {
    fn new() -> Self {
        StorageDescriptor {
            origin_descriptors: FxHashMap::default(),
        }
    }

    fn ensure_origin_descriptor(&mut self, origin: &ImmutableOrigin) -> bool {
        let origin = origin.ascii_serialization();
        match self.origin_descriptors.entry(origin.clone()) {
            Entry::Occupied(_) => false,
            Entry::Vacant(entry) => {
                entry.insert(OriginDescriptor::new(origin));
                true
            },
        }
    }

    fn get_origin_descriptors(&self) -> Vec<OriginDescriptor> {
        self.origin_descriptors.values().cloned().collect()
    }
}

#[derive(Clone, Default, MallocSizeOf)]
pub struct OriginEntry {
    tree: BTreeMap<String, String>,
    size: usize,
}

impl OriginEntry {
    pub fn inner(&self) -> &BTreeMap<String, String> {
        &self.tree
    }

    pub fn insert(&mut self, key: String, value: String) -> Option<String> {
        let old_value = self.tree.insert(key.clone(), value.clone());
        let size_change = match &old_value {
            Some(old) => value.len() as isize - old.len() as isize,
            None => (key.len() + value.len()) as isize,
        };
        self.size = (self.size as isize + size_change) as usize;
        old_value
    }


Review comment (suggestion):
```suggestion
    fn origin_descriptors(&self) -> Vec<OriginDescriptor> {
```

Code after:
}

#[derive(Deserialize, Serialize)]
pub struct StorageOrigins {
    origin_descriptors: FxHashMap<String, OriginDescriptor>,
}

impl StorageOrigins {
    fn new() -> Self {
        StorageOrigins {
            origin_descriptors: FxHashMap::default(),
        }
    }

    /// Ensures that an origin descriptor exists for the given origin.
    ///
    /// Returns `true` if a new origin descriptor was created, or `false` if
    /// one already existed.
    fn ensure_origin_descriptor(&mut self, origin: &ImmutableOrigin) -> bool {
        let origin = origin.ascii_serialization();
        match self.origin_descriptors.entry(origin.clone()) {
            Entry::Occupied(_) => false,
            Entry::Vacant(entry) => {
                entry.insert(OriginDescriptor::new(origin));
                true
            },
        }
    }

    fn origin_descriptors(&self) -> Vec<OriginDescriptor> {
        self.origin_descriptors.values().cloned().collect()
    }
}

#[derive(Clone, Default, MallocSizeOf)]
pub struct OriginEntry {
    tree: BTreeMap<String, String>,
    size: usize,
}

impl OriginEntry {
    pub fn inner(&self) -> &BTreeMap<String, String> {
        &self.tree
    }

    pub fn insert(&mut self, key: String, value: String) -> Option<String> {
        let old_value = self.tree.insert(key.clone(), value.clone());
        let size_change = match &old_value {
            Some(old) => value.len() as isize - old.len() as isize,
            None => (key.len() + value.len()) as isize,
        };