address review comments

components/canvas_traits/lib.rs

@@ -131,7 +131,6 @@ pub enum CanvasWebGLMsg {
 
 #[derive(Clone, Copy, PartialEq)]
 pub enum WebGLError {
-    NoError,
     InvalidEnum,
     InvalidOperation,
     InvalidValue,

components/script/dom/webglbuffer.rs

@@ -34,8 +34,9 @@ impl WebGLBuffer {
     pub fn maybe_new(global: GlobalRef, renderer: Sender<CanvasMsg>) -> Option<Root<WebGLBuffer>> {
         let (sender, receiver) = channel();
         renderer.send(CanvasMsg::WebGL(CanvasWebGLMsg::CreateBuffer(sender))).unwrap();
-        receiver.recv().unwrap()
-            .map(|buffer_id| WebGLBuffer::new(global, renderer, *buffer_id))
+
+        let result = receiver.recv().unwrap();
+        result.map(|buffer_id| WebGLBuffer::new(global, renderer, *buffer_id))
     }
 
     pub fn new(global: GlobalRef, renderer: Sender<CanvasMsg>, id: u32) -> Root<WebGLBuffer> {

components/script/dom/webglframebuffer.rs

@@ -34,8 +34,9 @@ impl WebGLFramebuffer {
     pub fn maybe_new(global: GlobalRef, renderer: Sender<CanvasMsg>) -> Option<Root<WebGLFramebuffer>> {
         let (sender, receiver) = channel();
         renderer.send(CanvasMsg::WebGL(CanvasWebGLMsg::CreateFramebuffer(sender))).unwrap();
-        receiver.recv().unwrap()
-            .map(|fb_id| WebGLFramebuffer::new(global, renderer, *fb_id))
+
+        let result = receiver.recv().unwrap();
+        result.map(|fb_id| WebGLFramebuffer::new(global, renderer, *fb_id))
     }
 
     pub fn new(global: GlobalRef, renderer: Sender<CanvasMsg>, id: u32) -> Root<WebGLFramebuffer> {
@@ -55,9 +56,8 @@ impl<'a> WebGLFramebufferHelpers for &'a WebGLFramebuffer {
     }
 
     fn bind(self, target: u32) {
-        self.renderer.send(
-            CanvasMsg::WebGL(
-                CanvasWebGLMsg::BindFramebuffer(target, WebGLFramebufferBindingRequest::Explicit(self.id)))).unwrap();
+        let cmd = CanvasWebGLMsg::BindFramebuffer(target, WebGLFramebufferBindingRequest::Explicit(self.id));
+        self.renderer.send(CanvasMsg::WebGL(cmd)).unwrap();
     }
 
     fn delete(self) {

components/script/dom/webglprogram.rs

@@ -42,8 +42,9 @@ impl WebGLProgram {
     pub fn maybe_new(global: GlobalRef, renderer: Sender<CanvasMsg>) -> Option<Root<WebGLProgram>> {
         let (sender, receiver) = channel();
         renderer.send(CanvasMsg::WebGL(CanvasWebGLMsg::CreateProgram(sender))).unwrap();
-        receiver.recv().unwrap()
-            .map(|program_id| WebGLProgram::new(global, renderer, *program_id))
+
+        let result = receiver.recv().unwrap();
+        result.map(|program_id| WebGLProgram::new(global, renderer, *program_id))
     }
 
     pub fn new(global: GlobalRef, renderer: Sender<CanvasMsg>, id: u32) -> Root<WebGLProgram> {
@@ -87,7 +88,7 @@ impl<'a> WebGLProgramHelpers for &'a WebGLProgram {
             _ => return Err(WebGLError::InvalidOperation),
         };
 
-        // TODO(ecoal95): Differenciate between same shader already assigned and other previous
+        // TODO(ecoal95): Differentiate between same shader already assigned and other previous
         // shader.
         if shader_slot.get().is_some() {
             return Err(WebGLError::InvalidOperation);

components/script/dom/webglrenderbuffer.rs

@@ -34,8 +34,9 @@ impl WebGLRenderbuffer {
     pub fn maybe_new(global: GlobalRef, renderer: Sender<CanvasMsg>) -> Option<Root<WebGLRenderbuffer>> {
         let (sender, receiver) = channel();
         renderer.send(CanvasMsg::WebGL(CanvasWebGLMsg::CreateRenderbuffer(sender))).unwrap();
-        receiver.recv().unwrap()
-            .map(|renderbuffer_id| WebGLRenderbuffer::new(global, renderer, *renderbuffer_id))
+
+        let result = receiver.recv().unwrap();
+        result.map(|renderbuffer_id| WebGLRenderbuffer::new(global, renderer, *renderbuffer_id))
     }
 
     pub fn new(global: GlobalRef, renderer: Sender<CanvasMsg>, id: u32) -> Root<WebGLRenderbuffer> {

components/script/dom/webglrenderingcontext.rs

@@ -34,7 +34,7 @@ use std::sync::mpsc::{channel, Sender};
 use util::str::DOMString;
 use offscreen_gl_context::GLContextAttributes;
 
-pub const MAX_UNIFORM_AND_ATTRIBUTE_LEN : usize = 256;
+pub const MAX_UNIFORM_AND_ATTRIBUTE_LEN: usize = 256;
 
 macro_rules! handle_potential_webgl_error {
     ($context:ident, $call:expr, $return_on_error:expr) => {
@@ -54,7 +54,7 @@ pub struct WebGLRenderingContext {
     global: GlobalField,
     renderer: Sender<CanvasMsg>,
     canvas: JS<HTMLCanvasElement>,
-    last_error: Cell<WebGLError>,
+    last_error: Cell<Option<WebGLError>>,
 }
 
 impl WebGLRenderingContext {
@@ -69,7 +69,7 @@ impl WebGLRenderingContext {
             reflector_: Reflector::new(),
             global: GlobalField::from_rooted(&global),
             renderer: chan,
-            last_error: Cell::new(WebGLError::NoError),
+            last_error: Cell::new(None),
             canvas: JS::from_ref(canvas),
         })
     }
@@ -134,15 +134,18 @@ impl<'a> WebGLRenderingContextMethods for &'a WebGLRenderingContext {
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.3
     fn GetError(self) -> u32 {
-        let error_code = match self.last_error.get() {
-            WebGLError::NoError => constants::NO_ERROR,
-            WebGLError::InvalidEnum => constants::INVALID_ENUM,
-            WebGLError::InvalidValue => constants::INVALID_VALUE,
-            WebGLError::InvalidOperation => constants::INVALID_OPERATION,
-            WebGLError::OutOfMemory => constants::OUT_OF_MEMORY,
-            WebGLError::ContextLost => constants::CONTEXT_LOST_WEBGL,
+        let error_code = if let Some(error) = self.last_error.get() {
+            match error {
+                WebGLError::InvalidEnum => constants::INVALID_ENUM,
+                WebGLError::InvalidValue => constants::INVALID_VALUE,
+                WebGLError::InvalidOperation => constants::INVALID_OPERATION,
+                WebGLError::OutOfMemory => constants::OUT_OF_MEMORY,
+                WebGLError::ContextLost => constants::CONTEXT_LOST_WEBGL,
+            }
+        } else {
+            constants::NO_ERROR
         };
-        self.last_error.set(WebGLError::NoError);
+        self.last_error.set(None);
         error_code
     }
 
@@ -231,9 +234,8 @@ impl<'a> WebGLRenderingContextMethods for &'a WebGLRenderingContext {
             framebuffer.bind(target)
         } else {
             // Bind the default framebuffer
-            self.renderer.send(
-                CanvasMsg::WebGL(
-                    CanvasWebGLMsg::BindFramebuffer(target, WebGLFramebufferBindingRequest::Default))).unwrap()
+            let cmd = CanvasWebGLMsg::BindFramebuffer(target, WebGLFramebufferBindingRequest::Default);
+            self.renderer.send(CanvasMsg::WebGL(cmd)).unwrap();
         }
     }
 
@@ -502,8 +504,8 @@ impl<'a> WebGLRenderingContextHelpers for &'a WebGLRenderingContext {
     fn handle_webgl_error(&self, err: WebGLError) {
         // If an error has been detected no further errors must be
         // recorded until `getError` has been called
-        if self.last_error.get() == WebGLError::NoError {
-            self.last_error.set(err);
+        if self.last_error.get().is_none() {
+            self.last_error.set(Some(err));
         }
     }
 }

components/script/dom/webglshader.rs

@@ -21,10 +21,9 @@ pub struct WebGLShader {
     webgl_object: WebGLObject,
     id: u32,
     gl_type: u32,
-    // TODO(ecoal95): is RefCell ok?
     source: RefCell<Option<String>>,
     is_deleted: Cell<bool>,
-    // TODO(ecoal95): Valorate moving this to `WebGLObject`
+    // TODO(ecoal95): Evaluate moving this to `WebGLObject`
     renderer: Sender<CanvasMsg>,
 }
 
@@ -45,8 +44,9 @@ impl WebGLShader {
                      shader_type: u32) -> Option<Root<WebGLShader>> {
         let (sender, receiver) = channel();
         renderer.send(CanvasMsg::WebGL(CanvasWebGLMsg::CreateShader(shader_type, sender))).unwrap();
-        receiver.recv().unwrap()
-            .map(|shader_id| WebGLShader::new(global, renderer, *shader_id, shader_type))
+
+        let result = receiver.recv().unwrap();
+        result.map(|shader_id| WebGLShader::new(global, renderer, *shader_id, shader_type))
     }
 
     pub fn new(global: GlobalRef,
@@ -81,13 +81,11 @@ impl<'a> WebGLShaderHelpers for &'a WebGLShader {
     // TODO(ecoal95): Validate shaders to be conforming to the WebGL spec
     /// glCompileShader
     fn compile(self) {
-        // NB(ecoal95): We intentionally don't check for source, we don't wan't
-        // to change gl error behavior
         self.renderer.send(CanvasMsg::WebGL(CanvasWebGLMsg::CompileShader(self.id))).unwrap()
     }
 
     /// Mark this shader as deleted (if it wasn't previously)
-    /// and delete it as if calling tr glDeleteShader.
+    /// and delete it as if calling glDeleteShader.
     fn delete(self) {
         if !self.is_deleted.get() {
             self.is_deleted.set(true);
@@ -116,7 +114,7 @@ impl<'a> WebGLShaderHelpers for &'a WebGLShader {
 
     /// Get the shader source
     fn source(self) -> Option<String> {
-        (*self.source.borrow()).clone()
+        self.source.borrow().clone()
     }
 
     /// glShaderSource

components/script/dom/webgltexture.rs

@@ -34,8 +34,9 @@ impl WebGLTexture {
     pub fn maybe_new(global: GlobalRef, renderer: Sender<CanvasMsg>) -> Option<Root<WebGLTexture>> {
         let (sender, receiver) = channel();
         renderer.send(CanvasMsg::WebGL(CanvasWebGLMsg::CreateTexture(sender))).unwrap();
-        receiver.recv().unwrap()
-            .map(|texture_id| WebGLTexture::new(global, renderer, *texture_id))
+
+        let result = receiver.recv().unwrap();
+        result.map(|texture_id| WebGLTexture::new(global, renderer, *texture_id))
     }
 
     pub fn new(global: GlobalRef, renderer: Sender<CanvasMsg>, id: u32) -> Root<WebGLTexture> {