From 483bf245dff23a4bfcb5f698549236aec534ca41 Mon Sep 17 00:00:00 2001 From: Gregory Terzian Date: Tue, 6 Nov 2018 18:43:21 +0800 Subject: [PATCH] constellation: restructure navigation, remove sync comm --- components/compositing/windowing.rs | 5 +- components/constellation/constellation.rs | 86 +++++++++++++++++------ components/embedder_traits/lib.rs | 8 +-- components/script_traits/lib.rs | 3 + components/servo/lib.rs | 10 +++ ports/libmlservo/src/lib.rs | 9 ++- ports/libsimpleservo/src/api.rs | 11 +-- ports/servo/browser.rs | 7 +- 8 files changed, 100 insertions(+), 39 deletions(-) diff --git a/components/compositing/windowing.rs b/components/compositing/windowing.rs index 7e95ca02161..c2575640975 100644 --- a/components/compositing/windowing.rs +++ b/components/compositing/windowing.rs @@ -9,7 +9,7 @@ use euclid::TypedScale; #[cfg(feature = "gl")] use gleam::gl; use keyboard_types::KeyboardEvent; -use msg::constellation_msg::{TopLevelBrowsingContextId, TraversalDirection}; +use msg::constellation_msg::{PipelineId, TopLevelBrowsingContextId, TraversalDirection}; use script_traits::{MouseButton, TouchEventType, TouchId}; use servo_geometry::DeviceIndependentPixel; use servo_url::ServoUrl; @@ -49,6 +49,8 @@ pub enum WindowEvent { Refresh, /// Sent when the window is resized. Resize, + /// Sent when a navigation request from script is allowed/refused. + AllowNavigationResponse(PipelineId, bool), /// Sent when a new URL is to be loaded. LoadUrl(TopLevelBrowsingContextId, ServoUrl), /// Sent when a mouse hit test is to be performed. @@ -96,6 +98,7 @@ impl Debug for WindowEvent { WindowEvent::Refresh => write!(f, "Refresh"), WindowEvent::Resize => write!(f, "Resize"), WindowEvent::Keyboard(..) => write!(f, "Keyboard"), + WindowEvent::AllowNavigationResponse(..) => write!(f, "AllowNavigationResponse"), WindowEvent::LoadUrl(..) => write!(f, "LoadUrl"), WindowEvent::MouseWindowEventClass(..) => write!(f, "Mouse"), WindowEvent::MouseWindowMoveEventClass(..) => write!(f, "MouseMove"), diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index a54169d66db..c3a178d9f49 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -156,6 +156,7 @@ use servo_rand::{random, Rng, SeedableRng, ServoRng}; use servo_remutex::ReentrantMutex; use servo_url::{Host, ImmutableOrigin, ServoUrl}; use std::borrow::ToOwned; +use std::collections::hash_map::Entry; use std::collections::{HashMap, VecDeque}; use std::marker::PhantomData; use std::mem::replace; @@ -168,6 +169,8 @@ use style_traits::viewport::ViewportConstraints; use style_traits::CSSPixel; use webvr_traits::{WebVREvent, WebVRMsg}; +type PendingApprovalNavigations = HashMap; + /// Servo supports tabs (referred to as browsers), so `Constellation` needs to /// store browser specific data for bookkeeping. struct Browser { @@ -366,6 +369,9 @@ pub struct Constellation { /// A channel through which messages can be sent to the canvas paint thread. canvas_chan: IpcSender, + + /// Navigation requests from script awaiting approval from the embedder. + pending_approval_navigations: PendingApprovalNavigations, } /// State needed to construct a constellation. @@ -698,6 +704,7 @@ where webgl_threads: state.webgl_threads, webvr_chan: state.webvr_chan, canvas_chan: CanvasPaintThread::start(), + pending_approval_navigations: HashMap::new(), }; constellation.run(); @@ -1044,6 +1051,36 @@ where FromCompositorMsg::Keyboard(key_event) => { self.handle_key_msg(key_event); }, + // Perform a navigation previously requested by script, if approved by the embedder. + // If there is already a pending page (self.pending_changes), it will not be overridden; + // However, if the id is not encompassed by another change, it will be. + FromCompositorMsg::AllowNavigationResponse(pipeline_id, allowed) => { + let pending = self.pending_approval_navigations.remove(&pipeline_id); + + let top_level_browsing_context_id = match self.pipelines.get(&pipeline_id) { + Some(pipeline) => pipeline.top_level_browsing_context_id, + None => return warn!("Attempted to navigate {} after closure.", pipeline_id), + }; + + match pending { + Some((load_data, replace)) => { + if allowed { + self.load_url( + top_level_browsing_context_id, + pipeline_id, + load_data, + replace, + ); + } + }, + None => { + return warn!( + "AllowNavigationReqsponse for unknow request: {:?}", + pipeline_id + ) + }, + }; + }, // Load a new page from a typed url // If there is already a pending page (self.pending_changes), it will not be overridden; // However, if the id is not encompassed by another change, it will be. @@ -1059,12 +1096,9 @@ where ) }, }; - self.handle_load_url_msg( - top_level_browsing_context_id, - pipeline_id, - load_data, - false, - ); + // Since this is a top-level load, initiated by the embedder, go straight to load_url, + // bypassing schedule_navigation. + self.load_url(top_level_browsing_context_id, pipeline_id, load_data, false); }, FromCompositorMsg::IsReadyToSaveImage(pipeline_states) => { let is_ready = self.handle_is_ready_to_save_image(pipeline_states); @@ -1176,11 +1210,9 @@ where FromScriptMsg::ChangeRunningAnimationsState(animation_state) => { self.handle_change_running_animations_state(source_pipeline_id, animation_state) }, - // Load a new page from a mouse click - // If there is already a pending page (self.pending_changes), it will not be overridden; - // However, if the id is not encompassed by another change, it will be. + // Ask the embedder for permission to load a new page. FromScriptMsg::LoadUrl(load_data, replace) => { - self.handle_load_url_msg(source_top_ctx_id, source_pipeline_id, load_data, replace); + self.schedule_navigation(source_top_ctx_id, source_pipeline_id, load_data, replace); }, FromScriptMsg::AbortLoadUrl => { self.handle_abort_load_url_msg(source_pipeline_id); @@ -2115,14 +2147,33 @@ where } } - fn handle_load_url_msg( + /// Schedule a navigation(via load_url). + /// 1: Ask the embedder for permission. + /// 2: Store the details of the navigation, pending approval from the embedder. + fn schedule_navigation( &mut self, top_level_browsing_context_id: TopLevelBrowsingContextId, source_id: PipelineId, load_data: LoadData, replace: bool, ) { - self.load_url(top_level_browsing_context_id, source_id, load_data, replace); + match self.pending_approval_navigations.entry(source_id) { + Entry::Occupied(_) => { + return warn!( + "Pipeline {:?} tried to schedule a navigation while one is already pending.", + source_id + ) + }, + Entry::Vacant(entry) => { + let _ = entry.insert((load_data.clone(), replace)); + }, + }; + // Allow the embedder to handle the url itself + let msg = ( + Some(top_level_browsing_context_id), + EmbedderMsg::AllowNavigationRequest(source_id, load_data.url.clone()), + ); + self.embedder_proxy.send(msg); } fn load_url( @@ -2132,17 +2183,6 @@ where load_data: LoadData, replace: bool, ) -> Option { - // Allow the embedder to handle the url itself - let (chan, port) = ipc::channel().expect("Failed to create IPC channel!"); - let msg = ( - Some(top_level_browsing_context_id), - EmbedderMsg::AllowNavigation(load_data.url.clone(), chan), - ); - self.embedder_proxy.send(msg); - if let Ok(false) = port.recv() { - return None; - } - debug!("Loading {} in pipeline {}.", load_data.url, source_id); // If this load targets an iframe, its framing element may exist // in a separate script thread than the framed document that initiated diff --git a/components/embedder_traits/lib.rs b/components/embedder_traits/lib.rs index 93c3275522d..e25ad11d13e 100644 --- a/components/embedder_traits/lib.rs +++ b/components/embedder_traits/lib.rs @@ -14,7 +14,7 @@ pub mod resources; use crossbeam_channel::{Receiver, Sender}; use ipc_channel::ipc::IpcSender; use keyboard_types::KeyboardEvent; -use msg::constellation_msg::{InputMethodType, TopLevelBrowsingContextId}; +use msg::constellation_msg::{InputMethodType, PipelineId, TopLevelBrowsingContextId}; use servo_url::ServoUrl; use std::fmt::{Debug, Error, Formatter}; use style_traits::cursor::CursorKind; @@ -79,8 +79,8 @@ pub enum EmbedderMsg { ResizeTo(DeviceIntSize), // Show an alert message. Alert(String, IpcSender<()>), - /// Wether or not to follow a link - AllowNavigation(ServoUrl, IpcSender), + /// Wether or not to allow a pipeline to load a url. + AllowNavigationRequest(PipelineId, ServoUrl), /// Whether or not to allow script to open a new tab/browser AllowOpeningBrowser(IpcSender), /// A new browser was created by script @@ -128,7 +128,7 @@ impl Debug for EmbedderMsg { EmbedderMsg::ResizeTo(..) => write!(f, "ResizeTo"), EmbedderMsg::Alert(..) => write!(f, "Alert"), EmbedderMsg::AllowUnload(..) => write!(f, "AllowUnload"), - EmbedderMsg::AllowNavigation(..) => write!(f, "AllowNavigation"), + EmbedderMsg::AllowNavigationRequest(..) => write!(f, "AllowNavigationRequest"), EmbedderMsg::Keyboard(..) => write!(f, "Keyboard"), EmbedderMsg::SetCursor(..) => write!(f, "SetCursor"), EmbedderMsg::NewFavicon(..) => write!(f, "NewFavicon"), diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index 2dfc2378d2f..4443bee3919 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -726,6 +726,8 @@ pub enum ConstellationMsg { IsReadyToSaveImage(HashMap), /// Inform the constellation of a key event. Keyboard(KeyboardEvent), + /// Whether to allow script to navigate. + AllowNavigationResponse(PipelineId, bool), /// Request to load a page. LoadUrl(TopLevelBrowsingContextId, ServoUrl), /// Request to traverse the joint session history of the provided browsing context. @@ -770,6 +772,7 @@ impl fmt::Debug for ConstellationMsg { GetFocusTopLevelBrowsingContext(..) => "GetFocusTopLevelBrowsingContext", IsReadyToSaveImage(..) => "IsReadyToSaveImage", Keyboard(..) => "Keyboard", + AllowNavigationResponse(..) => "AllowNavigationResponse", LoadUrl(..) => "LoadUrl", TraverseHistory(..) => "TraverseHistory", WindowSize(..) => "WindowSize", diff --git a/components/servo/lib.rs b/components/servo/lib.rs index 93cda15c419..186eb59fd2e 100644 --- a/components/servo/lib.rs +++ b/components/servo/lib.rs @@ -267,6 +267,16 @@ where self.compositor.on_resize_window_event(); }, + WindowEvent::AllowNavigationResponse(pipeline_id, allowed) => { + let msg = ConstellationMsg::AllowNavigationResponse(pipeline_id, allowed); + if let Err(e) = self.constellation_chan.send(msg) { + warn!( + "Sending allow navigation to constellation failed ({:?}).", + e + ); + } + }, + WindowEvent::LoadUrl(top_level_browsing_context_id, url) => { let msg = ConstellationMsg::LoadUrl(top_level_browsing_context_id, url); if let Err(e) = self.constellation_chan.send(msg) { diff --git a/ports/libmlservo/src/lib.rs b/ports/libmlservo/src/lib.rs index e0a65744e55..6cf895b0aec 100644 --- a/ports/libmlservo/src/lib.rs +++ b/ports/libmlservo/src/lib.rs @@ -128,12 +128,15 @@ pub unsafe extern "C" fn heartbeat_servo(servo: *mut ServoInstance) { // Servo heartbeat goes here! if let Some(servo) = servo.as_mut() { servo.servo.handle_events(vec![]); - for ((_browser_id, event)) in servo.servo.get_events() { + for ((browser_id, event)) in servo.servo.get_events() { match event { // Respond to any messages with a response channel // to avoid deadlocking the constellation - EmbedderMsg::AllowNavigation(_url, sender) => { - let _ = sender.send(true); + EmbedderMsg::AllowNavigationRequest(pipeline_id, _url) => { + if let Some(_browser_id) = browser_id { + let window_event = WindowEvent::AllowNavigationResponse(pipeline_id, true); + servo.servo.handle_events(vec![window_event]); + } }, EmbedderMsg::GetSelectedBluetoothDevice(_, sender) => { let _ = sender.send(None); diff --git a/ports/libsimpleservo/src/api.rs b/ports/libsimpleservo/src/api.rs index 90c6abdb702..2d68bad07c3 100644 --- a/ports/libsimpleservo/src/api.rs +++ b/ports/libsimpleservo/src/api.rs @@ -388,7 +388,7 @@ impl ServoGlue { } fn handle_servo_events(&mut self) -> Result<(), &'static str> { - for (_browser_id, event) in self.servo.get_events() { + for (browser_id, event) in self.servo.get_events() { match event { EmbedderMsg::ChangePageTitle(title) => { let fallback_title: String = if let Some(ref current_url) = self.current_url { @@ -403,10 +403,11 @@ impl ServoGlue { let title = format!("{} - Servo", title); self.callbacks.host_callbacks.on_title_changed(title); }, - EmbedderMsg::AllowNavigation(_url, response_chan) => { - if let Err(e) = response_chan.send(true) { - warn!("Failed to send allow_navigation() response: {}", e); - }; + EmbedderMsg::AllowNavigationRequest(pipeline_id, _url) => { + if let Some(_browser_id) = browser_id { + let window_event = WindowEvent::AllowNavigationResponse(pipeline_id, true); + let _ = self.process_event(window_event); + } }, EmbedderMsg::HistoryChanged(entries, current) => { let can_go_back = current > 0; diff --git a/ports/servo/browser.rs b/ports/servo/browser.rs index 17135f33ebd..78417305401 100644 --- a/ports/servo/browser.rs +++ b/ports/servo/browser.rs @@ -290,9 +290,10 @@ impl Browser { .push(WindowEvent::SendError(browser_id, reason)); } }, - EmbedderMsg::AllowNavigation(_url, sender) => { - if let Err(e) = sender.send(true) { - warn!("Failed to send AllowNavigation response: {}", e); + EmbedderMsg::AllowNavigationRequest(pipeline_id, _url) => { + if let Some(_browser_id) = browser_id { + self.event_queue + .push(WindowEvent::AllowNavigationResponse(pipeline_id, true)); } }, EmbedderMsg::AllowOpeningBrowser(response_chan) => {