Auto merge of #7132 - jdm:docenum, r=ms2ger

Document the use and meaning of the devtools control messages. Fixes …

…#6922.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7132)
<!-- Reviewable:end -->

components/devtools/actors/framerate.rs

@@ -2,27 +2,24 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use ipc_channel::ipc::{self, IpcSender};
+use ipc_channel::ipc::IpcSender;
-use ipc_channel::router::ROUTER;
 use rustc_serialize::json;
 use std::mem;
 use std::net::TcpStream;
 use std::sync::{Arc, Mutex};
-use std::sync::mpsc::Sender;
 use time::precise_time_ns;
 
 use msg::constellation_msg::PipelineId;
 use actor::{Actor, ActorRegistry};
 use actors::timeline::HighResolutionStamp;
-use devtools_traits::{ChromeToDevtoolsControlMsg, DevtoolsControlMsg, DevtoolScriptControlMsg};
+use devtools_traits::DevtoolScriptControlMsg;
 
 pub struct FramerateActor {
     name: String,
     pipeline: PipelineId,
     script_sender: IpcSender<DevtoolScriptControlMsg>,
-    devtools_sender: Sender<DevtoolsControlMsg>,
     start_time: Option<u64>,
-    is_recording: Arc<Mutex<bool>>,
+    is_recording: bool,
     ticks: Arc<Mutex<Vec<HighResolutionStamp>>>,
 }
 
@@ -45,16 +42,14 @@ impl FramerateActor {
     /// return name of actor
     pub fn create(registry: &ActorRegistry,
                   pipeline_id: PipelineId,
-                  script_sender: IpcSender<DevtoolScriptControlMsg>,
+                  script_sender: IpcSender<DevtoolScriptControlMsg>) -> String {
-                  devtools_sender: Sender<DevtoolsControlMsg>) -> String {
         let actor_name = registry.new_name("framerate");
         let mut actor = FramerateActor {
             name: actor_name.clone(),
             pipeline: pipeline_id,
             script_sender: script_sender,
-            devtools_sender: devtools_sender,
             start_time: None,
-            is_recording: Arc::new(Mutex::new(false)),
+            is_recording: false,
             ticks: Arc::new(Mutex::new(Vec::new())),
         };
 
@@ -67,6 +62,12 @@ impl FramerateActor {
         let mut lock = self.ticks.lock();
         let mut ticks = lock.as_mut().unwrap();
         ticks.push(HighResolutionStamp::wrap(tick));
+
+        if self.is_recording {
+            let msg = DevtoolScriptControlMsg::RequestAnimationFrame(self.pipeline,
+                                                                     self.name());
+            self.script_sender.send(msg).unwrap();
+        }
     }
 
     pub fn take_pending_ticks(&self) -> Vec<HighResolutionStamp> {
@@ -76,74 +77,23 @@ impl FramerateActor {
     }
 
     fn start_recording(&mut self) {
-        let mut lock = self.is_recording.lock();
+        if self.is_recording {
-        if **lock.as_ref().unwrap() {
             return;
         }
 
         self.start_time = Some(precise_time_ns());
-        let is_recording = lock.as_mut();
+        self.is_recording = true;
-        **is_recording.unwrap() = true;
 
-        fn get_closure(is_recording: Arc<Mutex<bool>>,
-                       name: String,
-                       pipeline: PipelineId,
-                       script_sender: IpcSender<DevtoolScriptControlMsg>,
-                       devtools_sender: Sender<DevtoolsControlMsg>)
-                          -> Box<Fn(f64, ) + Send> {
-
-            let closure = move |now: f64| {
-                let msg = DevtoolsControlMsg::FromChrome(ChromeToDevtoolsControlMsg::FramerateTick(
-                        name.clone(), now));
-                devtools_sender.send(msg).unwrap();
-
-                if !*is_recording.lock().unwrap() {
-                    return;
-                }
-
-                let closure = get_closure(is_recording.clone(),
-                                          name.clone(),
-                                          pipeline.clone(),
-                                          script_sender.clone(),
-                                          devtools_sender.clone());
-                let (request_animation_frame_sender, request_animation_frame_receiver) =
-                    ipc::channel().unwrap();
-                ROUTER.add_route(request_animation_frame_receiver.to_opaque(), box move |message| {
-                    let value: f64 = message.to().unwrap();
-                    closure(value);
-                });
-                let msg = DevtoolScriptControlMsg::RequestAnimationFrame(
-                    pipeline,
-                    request_animation_frame_sender);
-                script_sender.send(msg).unwrap();
-            };
-            Box::new(closure)
-        };
-
-        let closure = get_closure(self.is_recording.clone(),
-                                  self.name(),
-                                  self.pipeline.clone(),
-                                  self.script_sender.clone(),
-                                  self.devtools_sender.clone());
-        let (request_animation_frame_sender, request_animation_frame_receiver) =
-            ipc::channel().unwrap();
-        ROUTER.add_route(request_animation_frame_receiver.to_opaque(), box move |message| {
-            let value: f64 = message.to().unwrap();
-            closure(value);
-        });
         let msg = DevtoolScriptControlMsg::RequestAnimationFrame(self.pipeline,
-                                                                 request_animation_frame_sender);
+                                                                 self.name());
         self.script_sender.send(msg).unwrap();
     }
 
     fn stop_recording(&mut self) {
-        let mut lock = self.is_recording.lock();
+        if !self.is_recording {
-        if !**lock.as_ref().unwrap() {
             return;
         }
-
+        self.is_recording = false;
-        let is_recording = lock.as_mut();
-        **is_recording.unwrap() = false;
         self.start_time = None;
     }
 

components/devtools/actors/inspector.rs

@@ -5,7 +5,7 @@
 //! Liberally derived from the [Firefox JS implementation]
 //! (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js).
 
-use devtools_traits::{DevtoolScriptControlMsg, NodeInfo};
+use devtools_traits::{DevtoolScriptControlMsg, NodeInfo, ComputedNodeLayout};
 use devtools_traits::DevtoolScriptControlMsg::{GetRootNode, GetDocumentElement, GetChildren};
 use devtools_traits::DevtoolScriptControlMsg::{GetLayout, ModifyAttribute};
 
@@ -458,7 +458,7 @@ impl Actor for PageStyleActor {
                                       registry.actor_to_script(target.to_string()),
                                       tx))
                                 .unwrap();
-                let (width, height) = rx.recv().unwrap();
+                let ComputedNodeLayout { width, height } = rx.recv().unwrap();
 
                 let auto_margins = msg.get(&"autoMargins".to_string())
                     .and_then(&Json::as_boolean).unwrap_or(false);

components/devtools/actors/timeline.rs

@@ -11,12 +11,12 @@ use std::mem;
 use std::net::TcpStream;
 use std::thread::sleep_ms;
 use std::sync::{Arc, Mutex};
-use std::sync::mpsc::{channel, Sender};
+use std::sync::mpsc::channel;
 
 use actor::{Actor, ActorRegistry};
 use actors::memory::{MemoryActor, TimelineMemoryReply};
 use actors::framerate::FramerateActor;
-use devtools_traits::{DevtoolsControlMsg, DevtoolScriptControlMsg};
+use devtools_traits::DevtoolScriptControlMsg;
 use devtools_traits::DevtoolScriptControlMsg::{SetTimelineMarkers, DropTimelineMarkers};
 use devtools_traits::{PreciseTime, TimelineMarker, TracingMetadata, TimelineMarkerType};
 use protocol::JsonPacketStream;
@@ -25,7 +25,6 @@ use util::task;
 pub struct TimelineActor {
     name: String,
     script_sender: IpcSender<DevtoolScriptControlMsg>,
-    devtools_sender: Sender<DevtoolsControlMsg>,
     marker_types: Vec<TimelineMarkerType>,
     pipeline: PipelineId,
     is_recording: Arc<Mutex<bool>>,
@@ -126,8 +125,7 @@ static DEFAULT_TIMELINE_DATA_PULL_TIMEOUT: u32 = 200; //ms
 impl TimelineActor {
     pub fn new(name: String,
                pipeline: PipelineId,
-               script_sender: IpcSender<DevtoolScriptControlMsg>,
+               script_sender: IpcSender<DevtoolScriptControlMsg>) -> TimelineActor {
-               devtools_sender: Sender<DevtoolsControlMsg>) -> TimelineActor {
 
         let marker_types = vec!(TimelineMarkerType::Reflow,
                                 TimelineMarkerType::DOMEvent);
@@ -137,7 +135,6 @@ impl TimelineActor {
             pipeline: pipeline,
             marker_types: marker_types,
             script_sender: script_sender,
-            devtools_sender: devtools_sender,
             is_recording: Arc::new(Mutex::new(false)),
             stream: RefCell::new(None),
 
@@ -260,8 +257,7 @@ impl Actor for TimelineActor {
                         let framerate_actor = Some(FramerateActor::create(
                                 registry,
                                 self.pipeline.clone(),
-                                self.script_sender.clone(),
+                                self.script_sender.clone()));
-                                self.devtools_sender.clone()));
                         *self.framerate_actor.borrow_mut() = framerate_actor;
                     }
                 }

components/devtools/lib.rs

@@ -194,7 +194,6 @@ fn run_server(sender: Sender<DevtoolsControlMsg>,
     fn handle_new_global(actors: Arc<Mutex<ActorRegistry>>,
                          ids: (PipelineId, Option<WorkerId>),
                          script_sender: IpcSender<DevtoolScriptControlMsg>,
-                         devtools_sender: Sender<DevtoolsControlMsg>,
                          actor_pipelines: &mut HashMap<PipelineId, String>,
                          actor_workers: &mut HashMap<(PipelineId, WorkerId), String>,
                          page_info: DevtoolsPageInfo) {
@@ -221,8 +220,7 @@ fn run_server(sender: Sender<DevtoolsControlMsg>,
 
             let timeline = TimelineActor::new(actors.new_name("timeline"),
                                               pipeline,
-                                              script_sender,
+                                              script_sender);
-                                              devtools_sender);
 
             let DevtoolsPageInfo { title, url } = page_info;
             let tab = TabActor {
@@ -407,12 +405,12 @@ fn run_server(sender: Sender<DevtoolsControlMsg>,
                     handle_client(actors, stream.try_clone().unwrap())
                 })
             }
-            Ok(DevtoolsControlMsg::FromChrome(ChromeToDevtoolsControlMsg::FramerateTick(
+            Ok(DevtoolsControlMsg::FromScript(ScriptToDevtoolsControlMsg::FramerateTick(
                         actor_name, tick))) =>
                 handle_framerate_tick(actors.clone(), actor_name, tick),
             Ok(DevtoolsControlMsg::FromScript(ScriptToDevtoolsControlMsg::NewGlobal(
                         ids, script_sender, pageinfo))) =>
-                handle_new_global(actors.clone(), ids, script_sender, sender.clone(), &mut actor_pipelines,
+                handle_new_global(actors.clone(), ids, script_sender, &mut actor_pipelines,
                                   &mut actor_workers, pageinfo),
             Ok(DevtoolsControlMsg::FromScript(ScriptToDevtoolsControlMsg::ConsoleAPI(
                         id,

components/devtools_traits/lib.rs

@@ -48,26 +48,39 @@ pub struct DevtoolsPageInfo {
     pub url: Url
 }
 
-/// Messages to the instruct the devtools server to update its known actors/state
+/// Messages to instruct the devtools server to update its known actors/state
 /// according to changes in the browser.
 pub enum DevtoolsControlMsg {
+    /// Messages from tasks in the chrome process (resource/constellation/devtools)
     FromChrome(ChromeToDevtoolsControlMsg),
+    /// Messages from script tasks
     FromScript(ScriptToDevtoolsControlMsg),
 }
 
+/// Events that the devtools server must act upon.
 pub enum ChromeToDevtoolsControlMsg {
+    /// A new client has connected to the server.
     AddClient(TcpStream),
-    FramerateTick(String, f64),
+    /// The browser is shutting down.
     ServerExitMsg,
+    /// A network event occurred (request, reply, etc.). The actor with the
+    /// provided name should be notified.
     NetworkEvent(String, NetworkEvent),
 }
 
 #[derive(Deserialize, Serialize)]
+/// Events that the devtools server must act upon.
 pub enum ScriptToDevtoolsControlMsg {
+    /// A new global object was created, associated with a particular pipeline.
+    /// The means of communicating directly with it are provided.
     NewGlobal((PipelineId, Option<WorkerId>),
               IpcSender<DevtoolScriptControlMsg>,
               DevtoolsPageInfo),
+    /// A particular page has invoked the console API.
     ConsoleAPI(PipelineId, ConsoleMessage, Option<WorkerId>),
+    /// An animation frame with the given timestamp was processed in a script task.
+    /// The actor with the provided name should be notified.
+    FramerateTick(String, f64),
 }
 
 /// Serialized JS return values
@@ -134,20 +147,39 @@ pub enum TimelineMarkerType {
     DOMEvent,
 }
 
+/// The properties of a DOM node as computed by layout.
+#[derive(Deserialize, Serialize)]
+pub struct ComputedNodeLayout {
+    pub width: f32,
+    pub height: f32,
+}
+
 /// Messages to process in a particular script task, as instructed by a devtools client.
 #[derive(Deserialize, Serialize)]
 pub enum DevtoolScriptControlMsg {
+    /// Evaluate a JS snippet in the context of the global for the given pipeline.
     EvaluateJS(PipelineId, String, IpcSender<EvaluateJSReply>),
+    /// Retrieve the details of the root node (ie. the document) for the given pipeline.
     GetRootNode(PipelineId, IpcSender<NodeInfo>),
+    /// Retrieve the details of the document element for the given pipeline.
     GetDocumentElement(PipelineId, IpcSender<NodeInfo>),
+    /// Retrieve the details of the child nodes of the given node in the given pipeline.
     GetChildren(PipelineId, String, IpcSender<Vec<NodeInfo>>),
-    GetLayout(PipelineId, String, IpcSender<(f32, f32)>),
+    /// Retrieve the computed layout properties of the given node in the given pipeline.
+    GetLayout(PipelineId, String, IpcSender<ComputedNodeLayout>),
+    /// Retrieve all stored console messages for the given pipeline.
     GetCachedMessages(PipelineId, CachedConsoleMessageTypes, IpcSender<Vec<CachedConsoleMessage>>),
+    /// Update a given node's attributes with a list of modifications.
     ModifyAttribute(PipelineId, String, Vec<Modification>),
+    /// Request live console messages for a given pipeline (true if desired, false otherwise).
     WantsLiveNotifications(PipelineId, bool),
+    /// Request live notifications for a given set of timeline events for a given pipeline.
     SetTimelineMarkers(PipelineId, Vec<TimelineMarkerType>, IpcSender<TimelineMarker>),
+    /// Withdraw request for live timeline notifications for a given pipeline.
     DropTimelineMarkers(PipelineId, Vec<TimelineMarkerType>),
-    RequestAnimationFrame(PipelineId, IpcSender<f64>),
+    /// Request a callback directed at the given actor name from the next animation frame
+    /// executed in the given pipeline.
+    RequestAnimationFrame(PipelineId, String),
 }
 
 #[derive(RustcEncodable, Deserialize, Serialize)]

components/script/devtools.rs

@@ -4,7 +4,7 @@
 
 use devtools_traits::{CachedConsoleMessage, CachedConsoleMessageTypes, PAGE_ERROR, CONSOLE_API};
 use devtools_traits::{EvaluateJSReply, NodeInfo, Modification, TimelineMarker, TimelineMarkerType};
-use devtools_traits::{ConsoleAPI, PageError};
+use devtools_traits::{ConsoleAPI, PageError, ScriptToDevtoolsControlMsg, ComputedNodeLayout};
 use dom::bindings::conversions::jsstring_to_str;
 use dom::bindings::conversions::FromJSValConvertible;
 use dom::bindings::js::Root;
@@ -97,13 +97,16 @@ pub fn handle_get_children(page: &Rc<Page>, pipeline: PipelineId, node_id: Strin
     reply.send(children).unwrap();
 }
 
-pub fn handle_get_layout(page: &Rc<Page>, pipeline: PipelineId, node_id: String, reply: IpcSender<(f32, f32)>) {
+pub fn handle_get_layout(page: &Rc<Page>,
+                         pipeline: PipelineId,
+                         node_id: String,
+                         reply: IpcSender<ComputedNodeLayout>) {
     let node = find_node_by_unique_id(&*page, pipeline, node_id);
     let elem = ElementCast::to_ref(node.r()).expect("should be getting layout of element");
     let rect = elem.GetBoundingClientRect();
     let width = *rect.r().Width();
     let height = *rect.r().Height();
-    reply.send((width, height)).unwrap();
+    reply.send(ComputedNodeLayout { width: width, height: height }).unwrap();
 }
 
 pub fn handle_get_cached_messages(_pipeline_id: PipelineId,
@@ -202,10 +205,12 @@ pub fn handle_drop_timeline_markers(page: &Rc<Page>,
     }
 }
 
-pub fn handle_request_animation_frame(page: &Rc<Page>, id: PipelineId, callback: IpcSender<f64>) {
+pub fn handle_request_animation_frame(page: &Rc<Page>, id: PipelineId, actor_name: String) {
     let page = page.find(id).expect("There is no such page");
     let doc = page.document();
+    let devtools_sender = page.window().devtools_chan().unwrap();
     doc.r().request_animation_frame(box move |time| {
-        callback.send(time).unwrap()
+        let msg = ScriptToDevtoolsControlMsg::FramerateTick(actor_name, time);
+        devtools_sender.send(msg).unwrap();
     });
 }

components/script/dom/bindings/trace.rs

@@ -64,6 +64,7 @@ use net_traits::image::base::Image;
 use profile_traits::mem::ProfilerChan;
 use util::str::{LengthOrPercentageOrAuto};
 use selectors::parser::PseudoElement;
+use std::boxed::FnBox;
 use std::cell::{Cell, UnsafeCell, RefCell};
 use std::collections::{HashMap, HashSet};
 use std::collections::hash_state::HashState;
@@ -313,7 +314,7 @@ impl JSTraceable for Box<ScriptChan+Send> {
     }
 }
 
-impl JSTraceable for Box<Fn(f64, )> {
+impl JSTraceable for Box<FnBox(f64, )> {
     #[inline]
     fn trace(&self, _trc: *mut JSTracer) {
         // Do nothing

components/script/dom/document.rs

@@ -94,6 +94,7 @@ use js::jsapi::{JSContext, JSObject, JSRuntime};
 use num::ToPrimitive;
 use std::iter::FromIterator;
 use std::borrow::ToOwned;
+use std::boxed::FnBox;
 use std::collections::HashMap;
 use std::collections::hash_map::Entry::{Occupied, Vacant};
 use std::ascii::AsciiExt;
@@ -148,7 +149,7 @@ pub struct Document {
     /// https://html.spec.whatwg.org/multipage/#list-of-animation-frame-callbacks
     /// List of animation frame callbacks
     #[ignore_heap_size_of = "closures are hard"]
-    animation_frame_list: RefCell<HashMap<i32, Box<Fn(f64)>>>,
+    animation_frame_list: RefCell<HashMap<i32, Box<FnBox(f64)>>>,
     /// Tracks all outstanding loads related to this document.
     loader: DOMRefCell<DocumentLoader>,
     /// The current active HTML parser, to allow resuming after interruptions.
@@ -292,7 +293,7 @@ pub trait DocumentHelpers<'a> {
     fn set_current_script(self, script: Option<&HTMLScriptElement>);
     fn trigger_mozbrowser_event(self, event: MozBrowserEvent);
     /// https://html.spec.whatwg.org/multipage/#dom-window-requestanimationframe
-    fn request_animation_frame(self, callback: Box<Fn(f64, )>) -> i32;
+    fn request_animation_frame(self, callback: Box<FnBox(f64, )>) -> i32;
     /// https://html.spec.whatwg.org/multipage/#dom-window-cancelanimationframe
     fn cancel_animation_frame(self, ident: i32);
     /// https://html.spec.whatwg.org/multipage/#run-the-animation-frame-callbacks
@@ -949,7 +950,7 @@ impl<'a> DocumentHelpers<'a> for &'a Document {
     }
 
     /// https://html.spec.whatwg.org/multipage/#dom-window-requestanimationframe
-    fn request_animation_frame(self, callback: Box<Fn(f64, )>) -> i32 {
+    fn request_animation_frame(self, callback: Box<FnBox(f64)>) -> i32 {
         let window = self.window.root();
         let window = window.r();
         let ident = self.animation_frame_ident.get() + 1;

components/script/lib.rs

@@ -16,6 +16,7 @@
 #![feature(custom_attribute)]
 #![feature(custom_derive)]
 #![feature(drain)]
+#![feature(fnbox)]
 #![feature(hashmap_hasher)]
 #![feature(mpsc_select)]
 #![feature(nonzero)]

components/script/script_task.rs

@@ -909,8 +909,8 @@ impl ScriptTask {
                 devtools::handle_set_timeline_markers(&page, self, marker_types, reply),
             DevtoolScriptControlMsg::DropTimelineMarkers(_pipeline_id, marker_types) =>
                 devtools::handle_drop_timeline_markers(&page, self, marker_types),
-            DevtoolScriptControlMsg::RequestAnimationFrame(pipeline_id, callback) =>
+            DevtoolScriptControlMsg::RequestAnimationFrame(pipeline_id, name) =>
-                devtools::handle_request_animation_frame(&page, pipeline_id, callback),
+                devtools::handle_request_animation_frame(&page, pipeline_id, name),
         }
     }