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

Code before:
            add_origins(public_origins, StorageType::Local);

            let private_origins = self
                .private_storage_threads
                .webstorage_origins(WebStorageType::Local);
            add_origins(private_origins, StorageType::Local);
        }

        if storage_types.contains(StorageType::Session) {
            let public_origins = self
                .public_storage_threads
                .webstorage_origins(WebStorageType::Session);
            add_origins(public_origins, StorageType::Session);

            let private_origins = self
                .private_storage_threads
                .webstorage_origins(WebStorageType::Session);
            add_origins(private_origins, StorageType::Session);
        }

        let mut result: Vec<SiteData> = sites
            .into_iter()
            .map(|(name, storage_types)| SiteData::new(name, storage_types))
            .collect();

        result.sort_by_key(|a| a.name());

        result
    }

    pub fn clear_cookies(&self) {
        self.public_resource_threads.clear_cookies();
        self.private_resource_threads.clear_cookies();
    }
}


Review comment (suggestion):
```suggestion
        result.sort_by_key(SiteData::name);
```

Code after:
            }
        };

        if storage_types.contains(StorageType::Local) {
            let public_origins = self
                .public_storage_threads
                .webstorage_origins(WebStorageType::Local);
            add_origins(public_origins, StorageType::Local);

            let private_origins = self
                .private_storage_threads
                .webstorage_origins(WebStorageType::Local);
            add_origins(private_origins, StorageType::Local);
        }

        if storage_types.contains(StorageType::Session) {
            let public_origins = self
                .public_storage_threads
                .webstorage_origins(WebStorageType::Session);
            add_origins(public_origins, StorageType::Session);

            let private_origins = self
                .private_storage_threads
                .webstorage_origins(WebStorageType::Session);
            add_origins(private_origins, StorageType::Session);
        }

        let mut result: Vec<SiteData> = sites
            .into_iter()
            .map(|(name, storage_types)| SiteData::new(name, storage_types))
            .collect();

        result.sort_by_key(SiteData::name);

        result
    }

    pub fn clear_cookies(&self) {
        self.public_resource_threads.clear_cookies();
        self.private_resource_threads.clear_cookies();
    }
}
Repository: servo/servo
File: ports/servoshell/desktop/gamepad.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 std::collections::HashMap;

use gilrs::ff::{BaseEffect, BaseEffectType, Effect, EffectBuilder, Repeat, Replay, Ticks};
use gilrs::{EventType, Gilrs};
use log::{debug, warn};
use servo::{
    GamepadEvent, GamepadHapticEffectType, GamepadIndex, GamepadInputBounds,
    GamepadSupportedHapticEffects, GamepadUpdateType, GenericSender, InputEvent, WebView,
};

pub struct HapticEffect {
    pub effect: Effect,
    pub sender: GenericSender<bool>,
}

pub(crate) struct GamepadSupport {
    handle: Gilrs,
    haptic_effects: HashMap<usize, HapticEffect>,
}

impl GamepadSupport {
    pub(crate) fn maybe_new() -> Option<Self> {
        let handle = match Gilrs::new() {
            Ok(handle) => handle,
            Err(error) => {
                warn!("Error creating gamepad input connection ({error})");
                return None;
            },
        };
        Some(Self {
            handle,
            haptic_effects: Default::default(),
        })
    }

    /// Handle updates to connected gamepads from GilRs
    pub(crate) fn handle_gamepad_events(&mut self, active_webview: WebView) {
        while let Some(event) = self.handle.next_event() {

Review comment (suggestion):
We should not expose `GenericSender` to the public API as it is internal. Instead I think you can store the new `GamepadHapticEffectRequest` type here.

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 std::cell::RefCell;
use std::collections::HashMap;

use gilrs::ff::{BaseEffect, BaseEffectType, Effect, EffectBuilder, Repeat, Replay, Ticks};
use gilrs::{EventType, Gilrs};
use log::{debug, warn};
use servo::{
    GamepadEvent, GamepadHapticEffectRequest, GamepadHapticEffectRequestType,
    GamepadHapticEffectType, GamepadIndex, GamepadInputBounds, GamepadProvider,
    GamepadSupportedHapticEffects, GamepadUpdateType, InputEvent, WebView,
};

pub struct HapticEffect {
    pub effect: Effect,
    pub request: GamepadHapticEffectRequest,
}

pub(crate) struct ServoshellGamepadProvider {
    handle: RefCell<Gilrs>,
    haptic_effects: RefCell<HashMap<usize, HapticEffect>>,
}

impl ServoshellGamepadProvider {
    pub(crate) fn maybe_new() -> Option<Self> {
        let handle = match Gilrs::new() {
            Ok(handle) => handle,
            Err(error) => {
                warn!("Error creating gamepad input connection ({error})");
                return None;
            },
        };
        Some(Self {
            handle: RefCell::new(handle),
            haptic_effects: RefCell::new(Default::default()),
        })
    }

    /// Handle updates to connected gamepads from GilRs
Repository: servo/servo
File: components/servo/gamepad_provider.rs (Rust)

Code before:
    pub fn gamepad_index(&self) -> usize {
        self.gamepad_index
    }

    pub fn request_type(&self) -> &GamepadHapticEffectRequestType {
        &self.request_type
    }

    pub fn effect_type(&self) -> Option<&GamepadHapticEffectType> {
        match &self.request_type {
            GamepadHapticEffectRequestType::Play(effect_type) => Some(effect_type),
            GamepadHapticEffectRequestType::Stop => None,
        }
    }

    pub fn failed(self) {
        if let Some(callback) = self.callback {
            callback(false);
        }
    }

    pub fn succeeded(self) {
        if let Some(callback) = self.callback {
            callback(true);
        }
    }
}

pub trait GamepadProvider {
    /// Handle a request to play or stop a haptic effect on a connected gamepad.
    fn handle_haptic_effect_request(&self, _request: GamepadHapticEffectRequest) {}
}

pub(crate) struct DefaultGamepadProvider;

impl GamepadProvider for DefaultGamepadProvider {}


Review comment (bug):
Nice. You'll just need to add a `Drop` implementation for `GamepadHapticEffectRequest` that defaults to sending a failed message when the message wasn't sent before. This way embedders do not have to handle the request for it to work properly and not block forever:


```suggestion
    pub fn failed(self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }

    pub fn succeeded(self) {
        if let Some(callback) = self.callback.take() {
            callback(true);
        }
    }
}
    
impl GamepadHapticEffectRequest {
    fn drop(&mut self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }
}
```

Note that we `take()` on the callback in the other message senders so that the message is not sent twice.

Code after:
    pub fn gamepad_index(&self) -> usize {
        self.gamepad_index
    }

    pub fn request_type(&self) -> &GamepadHapticEffectRequestType {
        &self.request_type
    }

    pub fn failed(mut self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }

    pub fn succeeded(mut self) {
        if let Some(callback) = self.callback.take() {
            callback(true);
        }
    }
}

impl Drop for GamepadHapticEffectRequest {
    fn drop(&mut self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }
}

pub trait GamepadProvider {
    /// Handle a request to play or stop a haptic effect on a connected gamepad.
    fn handle_haptic_effect_request(&self, _request: GamepadHapticEffectRequest) {}
}

pub(crate) struct DefaultGamepadProvider;

impl GamepadProvider for DefaultGamepadProvider {}
Repository: servo/servo
File: components/servo/gamepad_provider.rs (Rust)

Code before:
    pub(crate) fn new(
        gamepad_index: usize,
        request_type: GamepadHapticEffectRequestType,
        callback: Box<dyn FnOnce(bool)>,
    ) -> Self {
        Self {
            gamepad_index,
            request_type,
            callback: Some(callback),
        }
    }

    pub fn gamepad_index(&self) -> usize {
        self.gamepad_index
    }

    pub fn request_type(&self) -> &GamepadHapticEffectRequestType {
        &self.request_type
    }

    pub fn effect_type(&self) -> Option<&GamepadHapticEffectType> {
        match &self.request_type {
            GamepadHapticEffectRequestType::Play(effect_type) => Some(effect_type),
            GamepadHapticEffectRequestType::Stop => None,
        }
    }

    pub fn failed(mut self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }

    pub fn succeeded(mut self) {
        if let Some(callback) = self.callback.take() {
            callback(true);
        }
    }
}

impl Drop for GamepadHapticEffectRequest {
    fn drop(&mut self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }
}

pub trait GamepadProvider {
    /// Handle a request to play or stop a haptic effect on a connected gamepad.
    fn handle_haptic_effect_request(&self, _request: GamepadHapticEffectRequest) {}

Review comment (refactor):
I think you can just remove this as it's in the `GamepadHapticEffectRequestType`.

Code after:
    pub(crate) fn new(
        gamepad_index: usize,
        request_type: GamepadHapticEffectRequestType,
        callback: Box<dyn FnOnce(bool)>,
    ) -> Self {
        Self {
            gamepad_index,
            request_type,
            callback: Some(callback),
        }
    }

    pub fn gamepad_index(&self) -> usize {
        self.gamepad_index
    }

    pub fn request_type(&self) -> &GamepadHapticEffectRequestType {
        &self.request_type
    }

    pub fn failed(mut self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }

    pub fn succeeded(mut self) {
        if let Some(callback) = self.callback.take() {
            callback(true);
        }
    }
}

impl Drop for GamepadHapticEffectRequest {
    fn drop(&mut self) {
        if let Some(callback) = self.callback.take() {
            callback(false);
        }
    }
}

pub trait GamepadProvider {
    /// Handle a request to play or stop a haptic effect on a connected gamepad.
    fn handle_haptic_effect_request(&self, _request: GamepadHapticEffectRequest) {}
}

pub(crate) struct DefaultGamepadProvider;

impl GamepadProvider for DefaultGamepadProvider {}
Repository: servo/servo
File: ports/servoshell/desktop/gamepad.rs (Rust)

Code before:
            request.failed();
            return;
        };

        let stopped_successfully = match haptic_effect.effect.stop() {
            Ok(()) => true,
            Err(e) => {
                debug!("Failed to stop haptic effect: {:?}", e);
                false
            },
        };
        haptic_effects.remove(&index);

        if stopped_successfully {
            request.succeeded();
        } else {
            request.failed();
        }
    }
}

impl GamepadProvider for ServoshellGamepadProvider {
    fn handle_haptic_effect_request(&self, request: GamepadHapticEffectRequest) {
        match request.request_type() {
            GamepadHapticEffectRequestType::Play(_) => {
                self.play_haptic_effect(request);
            },
            GamepadHapticEffectRequestType::Stop => {
                self.stop_haptic_effect(request);
            },
        }
    }
}


Review comment (suggestion):
Passing the effect type directly can make `play_haptic_effect` a little simpler:

```suggestion
            GamepadHapticEffectRequestType::Play(effect_type) => {
                self.play_haptic_effect(request, effect_type);
```

Code after:
            request.failed();
            return;
        };

        let stopped_successfully = match haptic_effect.effect.stop() {
            Ok(()) => true,
            Err(e) => {
                debug!("Failed to stop haptic effect: {:?}", e);
                false
            },
        };
        haptic_effects.remove(&index);

        if stopped_successfully {
            request.succeeded();
        } else {
            request.failed();
        }
    }
}

impl GamepadProvider for ServoshellGamepadProvider {
    fn handle_haptic_effect_request(&self, request: GamepadHapticEffectRequest) {
        match request.request_type() {
            GamepadHapticEffectRequestType::Play(effect_type) => {
                self.play_haptic_effect(&effect_type.clone(), request);
            },
            GamepadHapticEffectRequestType::Stop => {
                self.stop_haptic_effect(request);
            },
        }
    }
}
Repository: servo/servo
File: ports/servoshell/egl/app.rs (Rust)

Code before:

    pub fn resume_painting(
        &self,
        window_handle: RawWindowHandle,
        viewport_rect: Rect<i32, DevicePixel>,
    ) {
        let window_handle = unsafe { WindowHandle::borrow_raw(window_handle) };
        let size = viewport_rect.size.to_u32();
        if let Err(error) = self
            .platform_window
            .rendering_context
            .set_window(window_handle, PhysicalSize::new(size.width, size.height))
        {
            warn!("Binding native surface to context failed ({error:?})");
        }
        self.spin_event_loop();
    }
}

#[cfg(feature = "webxr")]
pub(crate) struct XrDiscoveryWebXrRegistry {
    xr_discovery: RefCell<Option<servo::webxr::Discovery>>,
}

#[cfg(feature = "webxr")]
#[cfg_attr(target_env = "ohos", expect(dead_code))]
impl XrDiscoveryWebXrRegistry {
    pub(crate) fn new(xr_discovery: Option<servo::webxr::Discovery>) -> Self {
        Self {
            xr_discovery: RefCell::new(xr_discovery),
        }
    }
}

#[cfg(feature = "webxr")]
impl servo::webxr::WebXrRegistry for XrDiscoveryWebXrRegistry {
    fn register(&self, registry: &mut servo::webxr::MainThreadRegistry) {
        log::debug!("XrDiscoveryWebXrRegistry::register");
        if let Some(discovery) = self.xr_discovery.take() {
            registry.register(discovery);
        }
    }
}


Review comment (suggestion):
I think this whole line can be removed, since it's only available when built with `#[cfg(feature = "webxr")]` anyways.

Code after:

    pub fn resume_painting(
        &self,
        window_handle: RawWindowHandle,
        viewport_rect: Rect<i32, DevicePixel>,
    ) {
        let window_handle = unsafe { WindowHandle::borrow_raw(window_handle) };
        let size = viewport_rect.size.to_u32();
        if let Err(error) = self
            .platform_window
            .rendering_context
            .set_window(window_handle, PhysicalSize::new(size.width, size.height))
        {
            warn!("Binding native surface to context failed ({error:?})");
        }
        self.spin_event_loop();
    }
}

#[cfg(feature = "webxr")]
pub(crate) struct XrDiscoveryWebXrRegistry {
    xr_discovery: RefCell<Option<servo::webxr::Discovery>>,
}

#[cfg(feature = "webxr")]
impl XrDiscoveryWebXrRegistry {
    pub(crate) fn new(xr_discovery: Option<servo::webxr::Discovery>) -> Self {
        Self {
            xr_discovery: RefCell::new(xr_discovery),
        }
    }
}

#[cfg(feature = "webxr")]
impl servo::webxr::WebXrRegistry for XrDiscoveryWebXrRegistry {
    fn register(&self, registry: &mut servo::webxr::MainThreadRegistry) {
        log::debug!("XrDiscoveryWebXrRegistry::register");
        if let Some(discovery) = self.xr_discovery.take() {
            registry.register(discovery);
        }
    }
}
Repository: servo/servo
File: components/shared/devtools/lib.rs (Rust)

Code before:
#[serde(rename_all = "camelCase")]
pub struct AttrModification {
    pub attribute_name: String,
    pub new_value: Option<String>,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct RuleModification {
    #[serde(rename = "type")]
    pub type_: String,
    pub index: u32,
    pub name: String,
    pub value: String,
    pub priority: String,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct StackFrame {
    pub filename: String,
    pub function_name: String,
    pub column_number: u32,
    pub line_number: u32,
    // Not implemented in Servo
    // source_id: String,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(untagged)]
pub enum ConsoleMessageArgument {
    String(String),
    Integer(i32),
    Number(f64),
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct ConsoleMessage {
    pub level: ConsoleLogLevel,
    pub filename: String,
    pub line_number: u32,
    pub column_number: u32,
    pub time_stamp: u64,
    pub arguments: Vec<ConsoleMessageArgument>,
    #[serde(skip_serializing_if = "Option::is_none")]
    pub stacktrace: Option<Vec<StackFrame>>,
    // Not implemented in Servo
    // inner_window_id: u32,
    // source_id: String,
}

Review comment (suggestion):
I would avoid commented out code here:

```suggestion
    // `source_id` is not not implemented in Servo
```

Code after:
#[serde(rename_all = "camelCase")]
pub struct AttrModification {
    pub attribute_name: String,
    pub new_value: Option<String>,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct RuleModification {
    #[serde(rename = "type")]
    pub type_: String,
    pub index: u32,
    pub name: String,
    pub value: String,
    pub priority: String,
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct StackFrame {
    pub filename: String,
    pub function_name: String,
    pub column_number: u32,
    pub line_number: u32,
    // Not implemented in Servo
    // source_id
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(untagged)]
pub enum ConsoleMessageArgument {
    String(String),
    Integer(i32),
    Number(f64),
}

#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct ConsoleMessage {
    pub level: ConsoleLogLevel,
    pub filename: String,
    pub line_number: u32,
    pub column_number: u32,
    pub time_stamp: u64,
    pub arguments: Vec<ConsoleMessageArgument>,
    #[serde(skip_serializing_if = "Option::is_none")]
    pub stacktrace: Option<Vec<StackFrame>>,
    // Not implemented in Servo
    // inner_window_id
    // source_id
}
Repository: servo/servo
File: components/shared/net/lib.rs (Rust)

Code before:
        .add(b' ')
        .add(b'"')
        .add(b'%')
        .add(b'\'')
        .add(b'(')
        .add(b')')
        .add(b'*')
        .add(b',')
        .add(b'/')
        .add(b':')
        .add(b';')
        .add(b'<')
        .add(b'-')
        .add(b'>')
        .add(b'?')
        .add(b'[')
        .add(b'\\')
        .add(b']')
        .add(b'{')
        .add(b'}');

    percent_encoding::percent_encode(bytes, HTTP_VALUE).to_string()
}

/// Returns the current system locale, or en-US by default.
pub fn get_current_locale() -> String {
    sys_locale::get_locale().unwrap_or_else(|| "en-US".into())
}

/// Step 12 of <https://fetch.spec.whatwg.org/#concept-fetch>
pub fn set_default_accept_language(headers: &mut HeaderMap) {
    // If request’s header list does not contain `Accept-Language`,
    // then user agents should append (`Accept-Language, an appropriate header value) to request’s header list.
    if headers.contains_key(header::ACCEPT_LANGUAGE) {
        return;
    }

    // To reduce fingerprinting we set only a single language.
    headers.insert(
        header::ACCEPT_LANGUAGE,
        HeaderValue::from_str(&get_current_locale())
            .unwrap_or_else(|_| HeaderValue::from_static("en-US")),
    );
}

pub static PRIVILEGED_SECRET: LazyLock<u32> = LazyLock::new(|| rng().next_u32());


Review comment (suggestion):
This should not be public:

```suggestion
fn get_current_locale() -> String {
```

Code after:
        .add(b' ')
        .add(b'"')
        .add(b'%')
        .add(b'\'')
        .add(b'(')
        .add(b')')
        .add(b'*')
        .add(b',')
        .add(b'/')
        .add(b':')
        .add(b';')
        .add(b'<')
        .add(b'-')
        .add(b'>')
        .add(b'?')
        .add(b'[')
        .add(b'\\')
        .add(b']')
        .add(b'{')
        .add(b'}');

    percent_encoding::percent_encode(bytes, HTTP_VALUE).to_string()
}

/// Returns the cached current system locale, or en-US by default.
pub fn get_current_locale() -> &'static (String, HeaderValue) {
    static CURRENT_LOCALE: OnceLock<(String, HeaderValue)> = OnceLock::new();

    CURRENT_LOCALE.get_or_init(|| {
        let locale_override = servo_config::pref!(intl_locale_override);
        let locale = if locale_override.is_empty() {
            sys_locale::get_locale().unwrap_or_else(|| "en-US".into())
        } else {
            locale_override
        };
        let header_value = HeaderValue::from_str(&locale)
            .ok()
            .unwrap_or_else(|| HeaderValue::from_static("en-US"));
        (locale, header_value)
    })
}

/// Step 12 of <https://fetch.spec.whatwg.org/#concept-fetch>
pub fn set_default_accept_language(headers: &mut HeaderMap) {
    // If request’s header list does not contain `Accept-Language`,
    // then user agents should append (`Accept-Language, an appropriate header value) to request’s header list.
    if headers.contains_key(header::ACCEPT_LANGUAGE) {
        return;
    }

    // To reduce fingerprinting we set only a single language.