IFrame elements now manage FrameIds rather than the constellation.

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,

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);

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),

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.

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);

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);