Repository: servo/servo
File: components/script/dom/webgl/webglprogram.rs (Rust)

Code before:
impl DroppableWebGLProgram {
    fn new(
        id: WebGLProgramId,
        is_in_use: Cell<bool>,
        marked_for_deletion: Cell<bool>,
        fragment_shader: RefCell<Option<WeakRef<WebGLShader>>>,
        vertex_shader: RefCell<Option<WeakRef<WebGLShader>>>,
        context: WeakRef<WebGLRenderingContext>,
    ) -> Self {
        Self {
            id,
            is_in_use,
            marked_for_deletion,
            fragment_shader,
            vertex_shader,
            context,
        }
    }
}

impl DroppableWebGLProgram {
    pub(crate) fn id(&self) -> WebGLProgramId {
        self.id
    }

    pub(crate) fn is_in_use(&self) -> bool {
        self.is_in_use.get()
    }

    pub(crate) fn set_in_use(&self, value: bool) {
        self.is_in_use.set(value);
    }

    pub(crate) fn is_marked_for_deletion(&self) -> bool {
        self.marked_for_deletion.get()
    }

    pub(crate) fn set_marked_for_deletion(&self, value: bool) {
        self.marked_for_deletion.set(value);
    }

    pub(crate) fn get_fragment_shader(&self) -> Option<DomRoot<WebGLShader>> {
        match self.fragment_shader.borrow().clone() {
            Some(ref weak_shader) => weak_shader.root(),
            None => None,
        }
    }

    pub(crate) fn set_fragment_shader(&self, shader: Option<WeakRef<WebGLShader>>) {
        self.fragment_shader.replace(shader);
    }

Review comment (suggestion):
I think the code that calls simple getters/setters like this should just use the members directly. This will avoid confusion like `is_in_use()` vs. `in_use()` which do very different checks.

Code after:
impl DroppableWebGLProgram {
    fn new(id: WebGLProgramId, context: &WebGLRenderingContext) -> Self {
        Self {
            id,
            context: WeakRef::new(context),
            fragment_shader: None,
            vertex_shader: None,
            marked_for_deletion: Default::default(),
            is_in_use: Default::default(),
        }
    }
}

impl DroppableWebGLProgram {
    fn attach_shader<'a>(&mut self, shader: &'a WebGLShader) -> WebGLResult<&'a WebGLShader> {
        if self.is_deleted() || shader.is_deleted() {
            return Err(WebGLError::InvalidOperation);
        }
        let shader_slot = match shader.gl_type() {
            constants::FRAGMENT_SHADER => &mut self.fragment_shader,
            constants::VERTEX_SHADER => &mut self.vertex_shader,
            _ => {
                error!("detachShader: Unexpected shader type");
                return Err(WebGLError::InvalidValue);
            },
        };

        if shader_slot.is_some() {
            return Err(WebGLError::InvalidOperation);
        }

        *shader_slot = Some(WeakRef::new(shader));
        shader.increment_attached_counter();

        self.send_command(WebGLCommand::AttachShader(self.id, shader.id()));

        Ok(shader)
    }

    fn detach_shader<'a>(&mut self, shader: &'a WebGLShader) -> WebGLResult<&'a WebGLShader> {
        if self.is_deleted() {
            return Err(WebGLError::InvalidOperation);
        }
        let shader_slot = match shader.gl_type() {
            constants::FRAGMENT_SHADER => &mut self.fragment_shader,
            constants::VERTEX_SHADER => &mut self.vertex_shader,
            _ => return Err(WebGLError::InvalidValue),
        };

        match shader_slot {
            Some(attached_shader) => match attached_shader.root() {