From f9f160fa82c3d7fd20a26c2617962b78b670ed1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 21 Oct 2016 01:33:04 +0200 Subject: [PATCH 1/3] Ensure that script animation callbacks are run after layout animations Per https://html.spec.whatwg.org/multipage/webappapis.html#run-css-animations-and-send-events --- components/compositing/compositor.rs | 19 +++------ components/constellation/constellation.rs | 31 ++++++-------- components/layout_thread/lib.rs | 42 ++++++++++++++----- components/script/dom/document.rs | 6 ++- components/script/dom/window.rs | 4 +- components/script_layout_interface/message.rs | 9 ++-- components/script_traits/lib.rs | 15 ++----- 7 files changed, 65 insertions(+), 61 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 35e8a9340efb..02d225c828a2 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -15,7 +15,7 @@ use ipc_channel::ipc::{self, IpcSharedMemory}; use msg::constellation_msg::{PipelineId, PipelineIndex, PipelineNamespaceId}; use net_traits::image::base::{Image, PixelFormat}; use profile_traits::time::{self, ProfilerCategory, profile}; -use script_traits::{AnimationState, AnimationTickType, ConstellationControlMsg}; +use script_traits::{AnimationState, ConstellationControlMsg}; use script_traits::{ConstellationMsg, LayoutControlMsg, MouseButton}; use script_traits::{MouseEventType, ScrollState}; use script_traits::{TouchpadPressurePhase, TouchEventType, TouchId, WindowSizeData, WindowSizeType}; @@ -1058,20 +1058,11 @@ impl IOCompositor { fn tick_animations_for_pipeline(&mut self, pipeline_id: PipelineId) { let animation_callbacks_running = self.pipeline_details(pipeline_id).animation_callbacks_running; - if animation_callbacks_running { - let msg = ConstellationMsg::TickAnimation(pipeline_id, AnimationTickType::Script); - if let Err(e) = self.constellation_chan.send(msg) { - warn!("Sending tick to constellation failed ({}).", e); - } - } - // We may need to tick animations in layout. (See #12749.) - let animations_running = self.pipeline_details(pipeline_id).animations_running; - if animations_running { - let msg = ConstellationMsg::TickAnimation(pipeline_id, AnimationTickType::Layout); - if let Err(e) = self.constellation_chan.send(msg) { - warn!("Sending tick to constellation failed ({}).", e); - } + // We still need to tick layout unfortunately, see things like #12749. + let msg = ConstellationMsg::TickAnimation(pipeline_id, animation_callbacks_running); + if let Err(e) = self.constellation_chan.send(msg) { + warn!("Sending tick to constellation failed ({}).", e); } } diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 4141e04d8032..34c5812471ec 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -99,7 +99,7 @@ use network_listener::NetworkListener; use pipeline::{InitialPipelineState, Pipeline}; use profile_traits::mem; use profile_traits::time; -use script_traits::{AnimationState, AnimationTickType, CompositorEvent}; +use script_traits::{AnimationState, CompositorEvent}; use script_traits::{ConstellationControlMsg, ConstellationMsg as FromCompositorMsg, DiscardBrowsingContext}; use script_traits::{DocumentActivity, DocumentState, LayoutControlMsg, LoadData}; use script_traits::{IFrameLoadInfo, IFrameLoadInfoWithData, IFrameSandboxState, TimerSchedulerMsg}; @@ -1002,8 +1002,8 @@ impl Constellation debug!("constellation got window resize message"); self.handle_window_size_msg(top_level_browsing_context_id, new_size, size_type); } - FromCompositorMsg::TickAnimation(pipeline_id, tick_type) => { - self.handle_tick_animation(pipeline_id, tick_type) + FromCompositorMsg::TickAnimation(pipeline_id, script_callbacks_present) => { + self.handle_tick_animation(pipeline_id, script_callbacks_present) } FromCompositorMsg::WebDriverCommand(command) => { debug!("constellation got webdriver command message"); @@ -1740,23 +1740,16 @@ impl Constellation animation_state)) } - fn handle_tick_animation(&mut self, pipeline_id: PipelineId, tick_type: AnimationTickType) { - let result = match tick_type { - AnimationTickType::Script => { - let msg = ConstellationControlMsg::TickAllAnimations(pipeline_id); - match self.pipelines.get(&pipeline_id) { - Some(pipeline) => pipeline.event_loop.send(msg), - None => return warn!("Pipeline {:?} got script tick after closure.", pipeline_id), - } - } - AnimationTickType::Layout => { - let msg = LayoutControlMsg::TickAnimations; - match self.pipelines.get(&pipeline_id) { - Some(pipeline) => pipeline.layout_chan.send(msg), - None => return warn!("Pipeline {:?} got layout tick after closure.", pipeline_id), - } - } + fn handle_tick_animation(&mut self, + pipeline_id: PipelineId, + script_callbacks_running: bool) { + let msg = LayoutControlMsg::TickAnimations(script_callbacks_running); + + let result = match self.pipelines.get(&pipeline_id) { + Some(pipeline) => pipeline.layout_chan.send(msg), + None => return warn!("Pipeline {:?} got layout tick after closure.", pipeline_id), }; + if let Err(e) = result { self.handle_send_error(pipeline_id, e); } diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 64ef126b79e0..9e774555b792 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -623,8 +623,10 @@ impl LayoutThread { self.handle_request_helper(Msg::SetScrollStates(new_scroll_states), possibly_locked_rw_data) }, - Request::FromPipeline(LayoutControlMsg::TickAnimations) => { - self.handle_request_helper(Msg::TickAnimations, possibly_locked_rw_data) + Request::FromPipeline(LayoutControlMsg::TickAnimations( + script_animation_callbacks_present)) => { + self.handle_request_helper(Msg::TickAnimations(script_animation_callbacks_present), + possibly_locked_rw_data) }, Request::FromPipeline(LayoutControlMsg::GetCurrentEpoch(sender)) => { self.handle_request_helper(Msg::GetCurrentEpoch(sender), possibly_locked_rw_data) @@ -699,7 +701,9 @@ impl LayoutThread { self.time_profiler_chan.clone(), || self.handle_reflow(&mut data, possibly_locked_rw_data)); }, - Msg::TickAnimations => self.tick_all_animations(possibly_locked_rw_data), + Msg::TickAnimations(script_animation_callbacks_present) => { + self.tick_all_animations(possibly_locked_rw_data, script_animation_callbacks_present) + }, Msg::SetScrollStates(new_scroll_states) => { self.set_scroll_states(new_scroll_states, possibly_locked_rw_data); } @@ -719,8 +723,11 @@ impl LayoutThread { let _rw_data = possibly_locked_rw_data.lock(); sender.send(self.epoch.get()).unwrap(); }, - Msg::AdvanceClockMs(how_many, do_tick) => { - self.handle_advance_clock_ms(how_many, possibly_locked_rw_data, do_tick); + Msg::AdvanceClockMs(how_many, do_tick, script_callbacks) => { + self.handle_advance_clock_ms(how_many, + possibly_locked_rw_data, + do_tick, + script_callbacks); } Msg::GetWebFontLoadState(sender) => { let _rw_data = possibly_locked_rw_data.lock(); @@ -873,10 +880,11 @@ impl LayoutThread { fn handle_advance_clock_ms<'a, 'b>(&mut self, how_many_ms: i32, possibly_locked_rw_data: &mut RwData<'a, 'b>, - tick_animations: bool) { + tick_animations: bool, + script_animation_callbacks_present: bool) { self.timer.increment(how_many_ms as f64 / 1000.0); if tick_animations { - self.tick_all_animations(possibly_locked_rw_data); + self.tick_all_animations(possibly_locked_rw_data, script_animation_callbacks_present); } } @@ -1471,12 +1479,16 @@ impl LayoutThread { rw_data.scroll_offsets = layout_scroll_states } - fn tick_all_animations<'a, 'b>(&mut self, possibly_locked_rw_data: &mut RwData<'a, 'b>) { + fn tick_all_animations<'a, 'b>(&mut self, + possibly_locked_rw_data: &mut RwData<'a, 'b>, + script_animation_callbacks_present: bool) { let mut rw_data = possibly_locked_rw_data.lock(); - self.tick_animations(&mut rw_data); + self.tick_animations(&mut rw_data, script_animation_callbacks_present); } - fn tick_animations(&mut self, rw_data: &mut LayoutThreadData) { + fn tick_animations(&mut self, + rw_data: &mut LayoutThreadData, + script_animation_callbacks_present: bool) { if opts::get().relayout_event { println!("**** pipeline={}\tForDisplay\tSpecial\tAnimationTick", self.id); } @@ -1520,6 +1532,16 @@ impl LayoutThread { &mut layout_context); assert!(layout_context.pending_images.is_none()); assert!(layout_context.newly_transitioning_nodes.is_none()); + + if script_animation_callbacks_present { + // Now that we're done sending transition events, etc, tell script + // it can run the animation callbacks. + // + // This order is defined by: + // https://html.spec.whatwg.org/multipage/#run-css-animations-and-send-events + let msg = ConstellationControlMsg::TickAllAnimations(self.id); + self.script_chan.send(msg).unwrap(); + } } } diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index c5940f02b48d..43e21f531e21 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -1577,6 +1577,10 @@ impl Document { } } + pub fn has_animation_callbacks(&self) -> bool { + !self.animation_frame_list.borrow().is_empty() + } + /// https://html.spec.whatwg.org/multipage/#run-the-animation-frame-callbacks pub fn run_the_animation_frame_callbacks(&self) { rooted_vec!(let mut animation_frame_list); @@ -1625,7 +1629,7 @@ impl Document { // constellation to stop giving us video refresh callbacks, to save energy. (A spurious // animation frame is one in which the callback did not mutate the DOM—that is, an // animation frame that wasn't actually used for animation.) - if self.animation_frame_list.borrow().is_empty() || + if !self.has_animation_callbacks() || (!was_faking_animation_frames && self.is_faking_animation_frames()) { mem::swap(&mut *self.animation_frame_list.borrow_mut(), &mut *animation_frame_list); diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 7ad5d0c02ace..08831d13721f 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -1185,7 +1185,9 @@ impl Window { /// Advances the layout animation clock by `delta` milliseconds, and then /// forces a reflow if `tick` is true. pub fn advance_animation_clock(&self, delta: i32, tick: bool) { - self.layout_chan.send(Msg::AdvanceClockMs(delta, tick)).unwrap(); + let has_animation_callbacks = self.Document().has_animation_callbacks(); + self.layout_chan + .send(Msg::AdvanceClockMs(delta, tick, has_animation_callbacks)).unwrap(); } /// Reflows the page unconditionally if possible and not suppressed. This diff --git a/components/script_layout_interface/message.rs b/components/script_layout_interface/message.rs index 91963bce226d..d815c312962a 100644 --- a/components/script_layout_interface/message.rs +++ b/components/script_layout_interface/message.rs @@ -45,13 +45,14 @@ pub enum Msg { GetRPC(Sender>), /// Requests that the layout thread render the next frame of all animations. - TickAnimations, + TickAnimations(bool /* script_callbacks_running */), /// Updates layout's timer for animation testing from script. /// - /// The inner field is the number of *milliseconds* to advance, and the bool - /// field is whether animations should be force-ticked. - AdvanceClockMs(i32, bool), + /// The inner field is the number of *milliseconds* to advance, the first + /// bool field is whether animations should be force-ticked, and the second + /// one whether there are script animation callbacks or not. + AdvanceClockMs(i32, bool /* force_tick */, bool /* script_callbacks_present */), /// Destroys layout data associated with a DOM node. /// diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index 8b2c98b1eb62..2affbece41b8 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -117,8 +117,8 @@ pub enum LayoutControlMsg { ExitNow, /// Requests the current epoch (layout counter) from this layout. GetCurrentEpoch(IpcSender), - /// Asks layout to run another step in its animation. - TickAnimations, + /// Asks layout to run another step in its animations. + TickAnimations(bool /* script_animation_callbacks_present */), /// Tells layout about the new scrolling offsets of each scrollable stacking context. SetScrollStates(Vec), /// Requests the current load state of Web fonts. `true` is returned if fonts are still loading @@ -698,15 +698,6 @@ impl MozBrowserErrorType { } } -/// Specifies whether the script or layout thread needs to be ticked for animation. -#[derive(Deserialize, Serialize)] -pub enum AnimationTickType { - /// The script thread. - Script, - /// The layout thread. - Layout, -} - /// The scroll state of a stacking context. #[derive(Clone, Copy, Debug, Deserialize, Serialize)] pub struct ScrollState { @@ -781,7 +772,7 @@ pub enum ConstellationMsg { /// Inform the constellation of a window being resized. WindowSize(TopLevelBrowsingContextId, WindowSizeData, WindowSizeType), /// Requests that the constellation instruct layout to begin a new tick of the animation. - TickAnimation(PipelineId, AnimationTickType), + TickAnimation(PipelineId, bool /* script_animation_callbacks_present */), /// Dispatch a webdriver command WebDriverCommand(WebDriverCommandMsg), /// Reload a top-level browsing context. From 1b8b0b6536ab80b4c6291211417c269d2002241c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 21 Oct 2016 01:37:23 +0200 Subject: [PATCH 2/3] Make basic-transition.html deterministic and report better errors --- tests/wpt/mozilla/meta/MANIFEST.json | 2 +- .../mozilla/tests/css/animations/basic-transition.html | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 75e8b9a0a4ae..1937946d8dcb 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -22726,7 +22726,7 @@ "testharness" ], "css/animations/basic-transition.html": [ - "6f4319854de92d7aee5ef425e17e80f9c39675df", + "4c1d67d8c180880c7e11c95979ef0f0346e8ce31", "testharness" ], "css/animations/mixed-units.html": [ diff --git a/tests/wpt/mozilla/tests/css/animations/basic-transition.html b/tests/wpt/mozilla/tests/css/animations/basic-transition.html index b80e8a666a6e..32a168ee65fb 100644 --- a/tests/wpt/mozilla/tests/css/animations/basic-transition.html +++ b/tests/wpt/mozilla/tests/css/animations/basic-transition.html @@ -19,16 +19,16 @@ var div = document.getElementById('check-me'); var span = div.childNodes[0]; async_test(function(t) { - window.addEventListener('load', function() { + window.addEventListener('load', t.step_func(function() { assert_equals(getComputedStyle(div).getPropertyValue('background-color'), 'rgb(255, 0, 0)'); div.id = "check-me-2"; - requestAnimationFrame(function() { + requestAnimationFrame(t.step_func(function() { var test = new window.TestBinding(); test.advanceClock(2000); span.innerHTML = "a"; assert_equals(getComputedStyle(div).getPropertyValue('background-color'), 'rgb(0, 0, 0)'); t.done(); - }); - }) -}) + })); + })); +}); From 75b7bf8ae4e8e635dc4ee9a0c708c5de9afcce31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 21 Oct 2016 03:36:39 +0200 Subject: [PATCH 3/3] Try to conform a bit more to "The event loop processing model" This removes a set of hacks that we'd been piling up. --- components/script/dom/window.rs | 16 +----- components/script/script_thread.rs | 83 +++++++++++++++--------------- 2 files changed, 43 insertions(+), 56 deletions(-) diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 08831d13721f..d81a5a2cb7fb 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -80,7 +80,7 @@ use script_thread::{ImageCacheMsg, MainThreadScriptChan, MainThreadScriptMsg}; use script_thread::{ScriptThread, SendableMainThreadScriptChan}; use script_traits::{ConstellationControlMsg, DocumentState, LoadData, MozBrowserEvent}; use script_traits::{ScriptToConstellationChan, ScriptMsg, ScrollState, TimerEvent, TimerEventId}; -use script_traits::{TimerSchedulerMsg, UntrustedNodeAddress, WindowSizeData, WindowSizeType}; +use script_traits::{TimerSchedulerMsg, UntrustedNodeAddress, WindowSizeData}; use script_traits::webdriver_msg::{WebDriverJSError, WebDriverJSResult}; use selectors::attr::CaseSensitivity; use servo_config::opts; @@ -200,9 +200,6 @@ pub struct Window { #[ignore_heap_size_of = "channels are hard"] devtools_marker_sender: DOMRefCell>>>, - /// Pending resize event, if any. - resize_event: Cell>, - /// Parent id associated with this page, if any. parent_info: Option<(PipelineId, FrameType)>, @@ -1625,16 +1622,6 @@ impl Window { self.pending_reflow_count.set(self.pending_reflow_count.get() + 1); } - pub fn set_resize_event(&self, event: WindowSizeData, event_type: WindowSizeType) { - self.resize_event.set(Some((event, event_type))); - } - - pub fn steal_resize_event(&self) -> Option<(WindowSizeData, WindowSizeType)> { - let event = self.resize_event.get(); - self.resize_event.set(None); - event - } - pub fn set_page_clip_rect_with_new_viewport(&self, viewport: Rect) -> bool { let rect = f32_rect_to_au_rect(viewport.clone()); self.current_viewport.set(rect); @@ -1875,7 +1862,6 @@ impl Window { bluetooth_thread, bluetooth_extra_permission_data: BluetoothExtraPermissionData::new(), page_clip_rect: Cell::new(max_rect()), - resize_event: Default::default(), layout_chan, layout_rpc, window_size: Cell::new(window_size), diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index b9bcf5be7c88..104c66f17413 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -896,24 +896,6 @@ impl ScriptThread { use self::MixedMessage::{FromConstellation, FromDevtools, FromImageCache}; use self::MixedMessage::{FromScheduler, FromScript}; - // Handle pending resize events. - // Gather them first to avoid a double mut borrow on self. - let mut resizes = vec!(); - - for (id, document) in self.documents.borrow().iter() { - // Only process a resize if layout is idle. - if let Some((size, size_type)) = document.window().steal_resize_event() { - resizes.push((id, size, size_type)); - } - } - - for (id, size, size_type) in resizes { - self.handle_event(id, ResizeEvent(size, size_type)); - } - - // Store new resizes, and gather all other events. - let mut sequential = vec![]; - // Receive at least one message so we don't spinloop. debug!("Waiting for event."); let mut event = { @@ -949,9 +931,18 @@ impl ScriptThread { }; debug!("Got event."); - // Squash any pending resize, reflow, animation tick, and mouse-move events in the queue. + // Sort any pending resize, reflow, animation tick, transition event, + // and mouse-move events in the queue. + // + // Dispatch them in order per: + // https://html.spec.whatwg.org/multipage/#run-css-animations-and-send-events + // + // FIXME: Lots to do here. + let mut sequential = vec![]; let mut mouse_move_event_index = None; - let mut animation_ticks = HashSet::new(); + let mut pipelines_to_tick = HashSet::new(); + let mut resizes = HashMap::new(); + let mut transition_events = vec![]; loop { // https://html.spec.whatwg.org/multipage/#event-loop-processing-model step 7 match event { @@ -981,10 +972,7 @@ impl ScriptThread { }) } FromConstellation(ConstellationControlMsg::Resize(id, size, size_type)) => { - // step 7.7 - self.profile_event(ScriptThreadEventCategory::Resize, || { - self.handle_resize(id, size, size_type); - }) + resizes.insert(id, (size, size_type)); } FromConstellation(ConstellationControlMsg::Viewport(id, rect)) => { self.profile_event(ScriptThreadEventCategory::SetViewport, || { @@ -998,11 +986,11 @@ impl ScriptThread { } FromConstellation(ConstellationControlMsg::TickAllAnimations( pipeline_id)) => { - // step 7.8 - if !animation_ticks.contains(&pipeline_id) { - animation_ticks.insert(pipeline_id); - sequential.push(event); - } + pipelines_to_tick.insert(pipeline_id); + } + FromConstellation(ConstellationControlMsg::TransitionEnd( + unsafe_node, name, duration)) => { + transition_events.push((unsafe_node, name, duration)); } FromConstellation(ConstellationControlMsg::SendEvent( _, @@ -1043,6 +1031,26 @@ impl ScriptThread { } } + // Step 5. Resize steps. + for (id, (size, size_type)) in resizes { + self.handle_resize(id, size, size_type); + } + + // FIXME Step 6. Scroll steps. + // FIXME Step 7. Media queries. + + // Step 8. CSS animation events. + for (unsafe_node, name, duration) in transition_events { + self.handle_transition_event(unsafe_node, name, duration); + } + + // FIXME Step 8: Fullscreen. + + // Step 9: animation frame callbacks. + for pipeline_id in pipelines_to_tick { + self.handle_tick_all_animations(pipeline_id); + } + // Process the gathered events. debug!("Processing events."); for msg in sequential { @@ -1219,10 +1227,6 @@ impl ScriptThread { self.handle_focus_iframe_msg(parent_pipeline_id, frame_id), ConstellationControlMsg::WebDriverScriptCommand(pipeline_id, msg) => self.handle_webdriver_msg(pipeline_id, msg), - ConstellationControlMsg::TickAllAnimations(pipeline_id) => - self.handle_tick_all_animations(pipeline_id), - ConstellationControlMsg::TransitionEnd(unsafe_node, name, duration) => - self.handle_transition_event(unsafe_node, name, duration), ConstellationControlMsg::WebFontLoaded(pipeline_id) => self.handle_web_font_loaded(pipeline_id), ConstellationControlMsg::DispatchIFrameLoadEvent { @@ -1244,8 +1248,10 @@ impl ScriptThread { msg @ ConstellationControlMsg::Viewport(..) | msg @ ConstellationControlMsg::SetScrollState(..) | msg @ ConstellationControlMsg::Resize(..) | + msg @ ConstellationControlMsg::TickAllAnimations(..) | + msg @ ConstellationControlMsg::TransitionEnd(..) | msg @ ConstellationControlMsg::ExitScriptThread => - panic!("should have handled {:?} already", msg), + panic!("should have handled {:?} already", msg), } } @@ -1392,17 +1398,12 @@ impl ScriptThread { } fn handle_resize(&self, id: PipelineId, size: WindowSizeData, size_type: WindowSizeType) { - let window = self.documents.borrow().find_window(id); - if let Some(ref window) = window { - window.set_resize_event(size, size_type); - return; - } let mut loads = self.incomplete_loads.borrow_mut(); if let Some(ref mut load) = loads.iter_mut().find(|load| load.pipeline_id == id) { load.window_size = Some(size); - return; } - warn!("resize sent to nonexistent pipeline"); + + self.handle_event(id, ResizeEvent(size, size_type)); } fn handle_viewport(&self, id: PipelineId, rect: Rect) {