Auto merge of #13498 - asajeffrey:script-iframe-stores-frameid, r=glennw

bors-servo · web-flow · commit d01a86621549 · 2016-10-07T14:54:31.000-05:00
IFrame elements now manage FrameIds rather than the constellation.  This PR stores the FrameId as well as the PipelineId in an html iframe. The iframes are now responsible for creating frame ids, not the constellation. This is the first step in fixing #633, because it means we know the frame id of each script thread when it is created. It also means we can share the frame id, for example using it in the debugger. cc @jdm, @ConnorGBrewster and @ejpbruel. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because it's a refactoring.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13498)
diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs
diff --git a/components/msg/constellation_msg.rs b/components/msg/constellation_msg.rs
@@ -221,9 +221,6 @@ pub enum TraversalDirection {
     Back(usize),
 }
 
-#[derive(Clone, PartialEq, Eq, Copy, Hash, Debug, Deserialize, Serialize, PartialOrd, Ord)]
-pub struct FrameId(pub u32);
-
 /// Each pipeline ID needs to be unique. However, it also needs to be possible to
 /// generate the pipeline ID from an iframe element (this simplifies a lot of other
 /// code that makes use of pipeline IDs).
@@ -242,7 +239,7 @@ pub struct FrameId(pub u32);
 #[derive(Clone, Copy)]
 pub struct PipelineNamespace {
     id: PipelineNamespaceId,
-    next_index: PipelineIndex,
+    index: u32,
 }
 
 impl PipelineNamespace {
@@ -251,21 +248,29 @@ impl PipelineNamespace {
             assert!(tls.get().is_none());
             tls.set(Some(PipelineNamespace {
                 id: namespace_id,
-                next_index: PipelineIndex(0),
+                index: 0,
             }));
         });
     }
 
-    fn next(&mut self) -> PipelineId {
-        let pipeline_id = PipelineId {
-            namespace_id: self.id,
-            index: self.next_index,
-        };
+    fn next_index(&mut self) -> u32 {
+        let result = self.index;
+        self.index = result + 1;
+        result
+    }
 
-        let PipelineIndex(current_index) = self.next_index;
-        self.next_index = PipelineIndex(current_index + 1);
+    fn next_pipeline_id(&mut self) -> PipelineId {
+        PipelineId {
+            namespace_id: self.id,
+            index: PipelineIndex(self.next_index()),
+        }
+    }
 
-        pipeline_id
+    fn next_frame_id(&mut self) -> FrameId {
+        FrameId {
+            namespace_id: self.id,
+            index: FrameIndex(self.next_index()),
+        }
     }
 }
 
@@ -289,24 +294,12 @@ impl PipelineId {
     pub fn new() -> PipelineId {
         PIPELINE_NAMESPACE.with(|tls| {
             let mut namespace = tls.get().expect("No namespace set for this thread!");
-            let new_pipeline_id = namespace.next();
+            let new_pipeline_id = namespace.next_pipeline_id();
             tls.set(Some(namespace));
             new_pipeline_id
         })
     }
 
-    // TODO(gw): This should be removed. It's only required because of the code
-    // that uses it in the devtools lib.rs file (which itself is a TODO). Once
-    // that is fixed, this should be removed. It also relies on the first
-    // call to PipelineId::new() returning (0,0), which is checked with an
-    // assert in handle_init_load().
-    pub fn fake_root_pipeline_id() -> PipelineId {
-        PipelineId {
-            namespace_id: PipelineNamespaceId(0),
-            index: PipelineIndex(0),
-        }
-    }
-
     pub fn to_webrender(&self) -> webrender_traits::PipelineId {
         let PipelineNamespaceId(namespace_id) = self.namespace_id;
         let PipelineIndex(index) = self.index;
@@ -331,6 +324,41 @@ impl fmt::Display for PipelineId {
     }
 }
 
+#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Copy, Hash, Debug, Deserialize, Serialize, HeapSizeOf)]
+pub struct FrameIndex(pub u32);
+
+#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Copy, Hash, Debug, Deserialize, Serialize, HeapSizeOf)]
+pub struct FrameId {
+    pub namespace_id: PipelineNamespaceId,
+    pub index: FrameIndex
+}
+
+impl FrameId {
+    pub fn new() -> FrameId {
+        PIPELINE_NAMESPACE.with(|tls| {
+            let mut namespace = tls.get().expect("No namespace set for this thread!");
+            let new_frame_id = namespace.next_frame_id();
+            tls.set(Some(namespace));
+            new_frame_id
+        })
+    }
+}
+
+impl fmt::Display for FrameId {
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+        let PipelineNamespaceId(namespace_id) = self.namespace_id;
+        let FrameIndex(index) = self.index;
+        write!(fmt, "({},{})", namespace_id, index)
+    }
+}
+
+// We provide ids just for unit testing.
+pub const TEST_NAMESPACE: PipelineNamespaceId = PipelineNamespaceId(1234);
+pub const TEST_PIPELINE_INDEX: PipelineIndex = PipelineIndex(5678);
+pub const TEST_PIPELINE_ID: PipelineId = PipelineId { namespace_id: TEST_NAMESPACE, index: TEST_PIPELINE_INDEX };
+pub const TEST_FRAME_INDEX: FrameIndex = FrameIndex(8765);
+pub const TEST_FRAME_ID: FrameId = FrameId { namespace_id: TEST_NAMESPACE, index: TEST_FRAME_INDEX };
+
 #[derive(Clone, PartialEq, Eq, Copy, Hash, Debug, Deserialize, Serialize, HeapSizeOf)]
 pub enum FrameType {
     IFrame,
diff --git a/components/script/dom/bindings/trace.rs b/components/script/dom/bindings/trace.rs
@@ -57,7 +57,7 @@ use js::jsapi::{GCTraceKindToAscii, Heap, JSObject, JSTracer, TraceKind};
 use js::jsval::JSVal;
 use js::rust::Runtime;
 use libc;
-use msg::constellation_msg::{FrameType, PipelineId, ReferrerPolicy, WindowSizeType};
+use msg::constellation_msg::{FrameId, FrameType, PipelineId, ReferrerPolicy, WindowSizeType};
 use net_traits::{Metadata, NetworkError, ResourceThreads};
 use net_traits::filemanager_thread::RelativePos;
 use net_traits::image::base::{Image, ImageMetadata};
@@ -308,7 +308,7 @@ no_jsmanaged_fields!(PropertyDeclarationBlock);
 no_jsmanaged_fields!(HashSet<T>);
 // These three are interdependent, if you plan to put jsmanaged data
 // in one of these make sure it is propagated properly to containing structs
-no_jsmanaged_fields!(FrameType, WindowSizeData, WindowSizeType, PipelineId);
+no_jsmanaged_fields!(FrameId, FrameType, WindowSizeData, WindowSizeType, PipelineId);
 no_jsmanaged_fields!(TimerEventId, TimerSource);
 no_jsmanaged_fields!(WorkerId);
 no_jsmanaged_fields!(QuirksMode);
diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs
@@ -38,7 +38,7 @@ use dom::window::{ReflowReason, Window};
 use ipc_channel::ipc;
 use js::jsapi::{JSAutoCompartment, JSContext, MutableHandleValue};
 use js::jsval::{NullValue, UndefinedValue};
-use msg::constellation_msg::{FrameType, LoadData, PipelineId, TraversalDirection};
+use msg::constellation_msg::{FrameType, FrameId, LoadData, PipelineId, TraversalDirection};
 use net_traits::response::HttpsState;
 use script_layout_interface::message::ReflowQueryType;
 use script_traits::{IFrameLoadInfo, MozBrowserEvent, ScriptMsg as ConstellationMsg};
@@ -67,6 +67,7 @@ bitflags! {
 #[dom_struct]
 pub struct HTMLIFrameElement {
     htmlelement: HTMLElement,
+    frame_id: FrameId,
     pipeline_id: Cell<Option<PipelineId>>,
     sandbox: MutNullableHeap<JS<DOMTokenList>>,
     sandbox_allowance: Cell<Option<SandboxAllowance>>,
@@ -130,6 +131,7 @@ impl HTMLIFrameElement {
         let load_info = IFrameLoadInfo {
             load_data: load_data,
             parent_pipeline_id: global_scope.pipeline_id(),
+            frame_id: self.frame_id,
             old_pipeline_id: old_pipeline_id,
             new_pipeline_id: new_pipeline_id,
             sandbox: sandboxed,
@@ -181,6 +183,7 @@ impl HTMLIFrameElement {
                      document: &Document) -> HTMLIFrameElement {
         HTMLIFrameElement {
             htmlelement: HTMLElement::new_inherited(local_name, prefix, document),
+            frame_id: FrameId::new(),
             pipeline_id: Cell::new(None),
             sandbox: Default::default(),
             sandbox_allowance: Cell::new(None),
diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs
@@ -449,6 +449,8 @@ pub struct IFrameLoadInfo {
     pub load_data: Option<LoadData>,
     /// Pipeline ID of the parent of this iframe
     pub parent_pipeline_id: PipelineId,
+    /// The ID for this iframe.
+    pub frame_id: FrameId,
     /// The old pipeline ID for this iframe, if a page was previously loaded.
     pub old_pipeline_id: Option<PipelineId>,
     /// The new pipeline ID that the iframe has generated.
diff --git a/tests/unit/net/fetch.rs b/tests/unit/net/fetch.rs
@@ -19,7 +19,7 @@ use hyper::server::{Handler, Listening, Server};
 use hyper::server::{Request as HyperRequest, Response as HyperResponse};
 use hyper::status::StatusCode;
 use hyper::uri::RequestUri;
-use msg::constellation_msg::{PipelineId, ReferrerPolicy};
+use msg::constellation_msg::{ReferrerPolicy, TEST_PIPELINE_ID};
 use net::fetch::cors_cache::CORSCache;
 use net::fetch::methods::{FetchContext, fetch, fetch_with_cors_cache};
 use net::http_loader::HttpState;
@@ -776,8 +776,7 @@ fn test_fetch_with_devtools() {
     let (mut server, url) = make_server(handler);
 
     let origin = Origin::Origin(url.origin());
-    let pipeline_id = PipelineId::fake_root_pipeline_id();
-    let request = Request::new(url.clone(), Some(origin), false, Some(pipeline_id));
+    let request = Request::new(url.clone(), Some(origin), false, Some(TEST_PIPELINE_ID));
     *request.referrer.borrow_mut() = Referrer::NoReferrer;
 
     let (devtools_chan, devtools_port) = channel::<DevtoolsControlMsg>();
@@ -815,7 +814,7 @@ fn test_fetch_with_devtools() {
         method: Method::Get,
         headers: headers,
         body: None,
-        pipeline_id: pipeline_id,
+        pipeline_id: TEST_PIPELINE_ID,
         startedDateTime: devhttprequest.startedDateTime,
         timeStamp: devhttprequest.timeStamp,
         connect_time: devhttprequest.connect_time,
@@ -832,7 +831,7 @@ fn test_fetch_with_devtools() {
         headers: Some(response_headers),
         status: Some((200, b"OK".to_vec())),
         body: None,
-        pipeline_id: pipeline_id,
+        pipeline_id: TEST_PIPELINE_ID,
     };
 
     assert_eq!(devhttprequest, httprequest);
diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs
@@ -18,7 +18,7 @@ use hyper::http::RawStatus;
 use hyper::method::Method;
 use hyper::mime::{Mime, SubLevel, TopLevel};
 use hyper::status::StatusCode;
-use msg::constellation_msg::{PipelineId, ReferrerPolicy};
+use msg::constellation_msg::{PipelineId, ReferrerPolicy, TEST_PIPELINE_ID};
 use net::cookie::Cookie;
 use net::cookie_storage::CookieStorage;
 use net::hsts::HstsEntry;
@@ -47,7 +47,7 @@ impl LoadOrigin for HttpTest {
         None
     }
     fn pipeline_id(&self) -> Option<PipelineId> {
-        Some(PipelineId::fake_root_pipeline_id())
+        Some(TEST_PIPELINE_ID)
     }
 }
 
@@ -472,8 +472,6 @@ fn test_request_and_response_data_with_network_messages() {
 
     let url = Url::parse("https://mozilla.com").unwrap();
     let (devtools_chan, devtools_port) = mpsc::channel::<DevtoolsControlMsg>();
-    // This will probably have to be changed as it uses fake_root_pipeline_id which is marked for removal.
-    let pipeline_id = PipelineId::fake_root_pipeline_id();
     let mut load_data = LoadData::new(LoadContext::Browsing, url.clone(), &HttpTest);
     let mut request_headers = Headers::new();
     request_headers.set(Host { hostname: "bar.foo".to_owned(), port: None });
@@ -521,7 +519,7 @@ fn test_request_and_response_data_with_network_messages() {
         method: Method::Get,
         headers: headers,
         body: None,
-        pipeline_id: pipeline_id,
+        pipeline_id: TEST_PIPELINE_ID,
         startedDateTime: devhttprequest.startedDateTime,
         timeStamp: devhttprequest.timeStamp,
         connect_time: devhttprequest.connect_time,
@@ -538,7 +536,7 @@ fn test_request_and_response_data_with_network_messages() {
         headers: Some(response_headers),
         status: Some((200, b"OK".to_vec())),
         body: None,
-        pipeline_id: pipeline_id,
+        pipeline_id: TEST_PIPELINE_ID,
     };
 
     assert_eq!(devhttprequest, httprequest);
