M1453: addressed review comments r/3291

components/devtools/actors/inspector.rs

@@ -6,7 +6,6 @@
 
 use devtools_traits::{GetRootNode, GetDocumentElement, GetChildren, DevtoolScriptControlMsg};
 use devtools_traits::{GetLayout, NodeInfo, ModifyAttribute};
-use devtools_traits::Modification;
 
 use actor::{Actor, ActorRegistry};
 use protocol::JsonPacketSender;
@@ -59,7 +58,7 @@ struct HideBoxModelReply {
 }
 
 impl Actor for HighlighterActor {
-        fn name(&self) -> String {
+    fn name(&self) -> String {
         self.name.clone()
     }
 
@@ -92,7 +91,7 @@ impl Actor for HighlighterActor {
 
 #[deriving(Encodable)]
 struct ModifyAttributeReply{
-    to: String,
+    from: String,
 }
 
 impl Actor for NodeActor {
@@ -101,27 +100,25 @@ impl Actor for NodeActor {
     }
 
     fn handle_message(&self,
-                      _registry: &ActorRegistry,
+                      registry: &ActorRegistry,
                       msg_type: &String,
-                      _msg: &json::JsonObject,
+                      msg: &json::JsonObject,
                       stream: &mut TcpStream) -> bool {
         match msg_type.as_slice() {
             "modifyAttributes" => {
-                let target = _msg.find(&"to".to_string()).unwrap().as_string().unwrap();
-                let _mods = _msg.find(&"modifications".to_string()).unwrap().as_list().unwrap();
-                let mut modifications: Vec<Modification> = Vec::new();
-                for json_mod in _mods.iter() {
-                    let modification: Modification = json::decode(json_mod.to_string().as_slice()).unwrap();
-                    modifications.push(modification);
-                }
+                let target = msg.find(&"to".to_string()).unwrap().as_string().unwrap();
+                let mods = msg.find(&"modifications".to_string()).unwrap().as_list().unwrap();
+                let modifications = mods.iter().map(|json_mod| {
+                    json::decode(json_mod.to_string().as_slice()).unwrap()
+                }).collect();
 
                 self.script_chan.send(ModifyAttribute(self.pipeline,
-                                                _registry.actor_to_script(target.to_string()),
-                                                modifications));
-                let msg = ModifyAttributeReply{
-                    to:target.to_string(),
+                                                      registry.actor_to_script(target.to_string()),
+                                                      modifications));
+                let reply = ModifyAttributeReply{
+                    from:self.name(),
                 };
-                stream.write_json_packet(&msg);
+                stream.write_json_packet(&reply);
                 true
             }
 
@@ -178,13 +175,19 @@ struct NodeActorMsg {
 }
 
 trait NodeInfoToProtocol {
-    fn encode(self, actors: &ActorRegistry, display: bool,
-        script_chan: Sender<DevtoolScriptControlMsg>, pipeline: PipelineId) -> NodeActorMsg;
+    fn encode(self,
+              actors: &ActorRegistry,
+              display: bool,
+              script_chan: Sender<DevtoolScriptControlMsg>,
+              pipeline: PipelineId) -> NodeActorMsg;
 }
 
 impl NodeInfoToProtocol for NodeInfo {
-    fn encode(self, actors: &ActorRegistry, display: bool,
-        script_chan: Sender<DevtoolScriptControlMsg>, pipeline: PipelineId) -> NodeActorMsg {
+    fn encode(self,
+              actors: &ActorRegistry,
+              display: bool,
+              script_chan: Sender<DevtoolScriptControlMsg>,
+              pipeline: PipelineId) -> NodeActorMsg {
         let actor_name = if !actors.script_actor_registered(self.uniqueId.clone()) {
             let name = actors.new_name("node");
             let node_actor = NodeActor {

components/devtools/actors/tab.rs

@@ -75,7 +75,7 @@ impl Actor for TabActor {
     }
 
     fn handle_message(&self,
-                      _registry: &ActorRegistry,
+                      registry: &ActorRegistry,
                       msg_type: &String,
                       _msg: &json::JsonObject,
                       stream: &mut TcpStream) -> bool {
@@ -96,19 +96,21 @@ impl Actor for TabActor {
                     javascriptEnabled: true,
                     traits: TabTraits,
                 };
-                let console_actor = _registry.find::<ConsoleActor>(self.console.clone().as_slice());
-                console_actor.streams.borrow_mut().push((*stream).clone());
+                let console_actor = registry.find::<ConsoleActor>(self.console.as_slice());
+                console_actor.streams.borrow_mut().push(stream.clone());
                 stream.write_json_packet(&msg);
                 true
             }
 
+            //FIXME: The current implemnetation won't work for multiple connections. Need to ensure
+            //       that the correct stream is removed.
             "detach" => {
                 let msg = TabDetachedReply {
                     from: self.name(),
                     __type__: "detached".to_string(),
                 };
-                let console_actor = _registry.find::<ConsoleActor>(self.console.clone().as_slice());
-                console_actor.streams.borrow_mut().pop( );
+                let console_actor = registry.find::<ConsoleActor>(self.console.as_slice());
+                console_actor.streams.borrow_mut().pop();
                 stream.write_json_packet(&msg);
                 true
             }

components/devtools/lib.rs

@@ -91,7 +91,7 @@ fn run_server(receiver: Receiver<DevtoolsControlMsg>, port: u16) {
 
     let mut accepted_connections: Vec<TcpStream> = Vec::new();
 
-    let mut _actor_pipelines: HashMap<PipelineId, String> = HashMap::new();
+    let mut actor_pipelines: HashMap<PipelineId, String> = HashMap::new();
 
     /// Process the input from a single devtools client until EOF.
     fn handle_client(actors: Arc<Mutex<ActorRegistry>>, mut stream: TcpStream) {
@@ -142,7 +142,7 @@ fn run_server(receiver: Receiver<DevtoolsControlMsg>, port: u16) {
     fn handle_new_global(actors: Arc<Mutex<ActorRegistry>>,
                          pipeline: PipelineId,
                          sender: Sender<DevtoolScriptControlMsg>,
-                         mut actor_pipelines: HashMap<PipelineId, String>) {
+                         actor_pipelines: &mut HashMap<PipelineId, String>) {
         let mut actors = actors.lock();
         //TODO: move all this actor creation into a constructor method on TabActor
         let (tab, console, inspector) = {
@@ -191,7 +191,7 @@ fn run_server(receiver: Receiver<DevtoolsControlMsg>, port: u16) {
             Err(ref e) if e.kind == TimedOut => {
                 match receiver.try_recv() {
                     Ok(ServerExitMsg) | Err(Disconnected) => break,
-                    Ok(NewGlobal(id, sender)) => handle_new_global(actors.clone(), id, sender, _actor_pipelines.clone()),
+                    Ok(NewGlobal(id, sender)) => handle_new_global(actors.clone(), id, sender, &mut actor_pipelines),
                     Err(Empty) => acceptor.set_timeout(Some(POLL_TIMEOUT)),
                 }
             }

components/script/script_task.rs

@@ -13,7 +13,6 @@ use dom::bindings::codegen::Bindings::EventTargetBinding::EventTargetMethods;
 use dom::bindings::codegen::InheritTypes::{EventTargetCast, NodeCast, EventCast, ElementCast};
 use dom::bindings::conversions;
 use dom::bindings::conversions::{FromJSValConvertible, Empty};
-use dom::bindings::error::Error;
 use dom::bindings::global;
 use dom::bindings::js::{JS, JSRef, RootCollection, Temporary, OptionalRootable};
 use dom::bindings::trace::JSTraceable;
@@ -630,18 +629,12 @@ impl ScriptTask {
         let node = self.find_node_by_unique_id(pipeline, node_id).root();
         let elem: JSRef<Element> = ElementCast::to_ref(*node).expect("should be getting layout of element");
 
-        for _modification in modifications.iter(){
-            match _modification.newValue {
+        for modification in modifications.iter(){
+            match modification.newValue {
                 Some(ref string) => {
-                    let result: Result<(), Error> = elem.SetAttribute(_modification.attributeName.clone(), string.clone());
-                    let result = match result {
-                        Ok(result) => result,
-                        Err(e) => {
-                          //TBD: Error handling
-                        },
-                    };
+                    let _ = elem.SetAttribute(modification.attributeName.clone(), string.clone());
                 },
-                None => elem.RemoveAttribute(_modification.attributeName.clone()),
+                None => elem.RemoveAttribute(modification.attributeName.clone()),
             }
         }
     }