Fix the location url that reverts to the old value while loading (#30518)

* add PumpResult::HistoryChanged

* Fix the location url that reverts to the old value while loading

* fix spellings, undo self.location changes

* fmt

* Rework PumpResult

* fmt and update trace message as per PumpResult

* remove comment

ports/servoshell/app.rs

@@ -39,18 +39,20 @@ pub struct App {
     minibrowser: Option<RefCell<Minibrowser>>,
 }
 
+enum Present {
+    Immediate,
+    Deferred,
+    None,
+}
+
 /// Action to be taken by the caller of [`App::handle_events`].
 enum PumpResult {
     /// The caller should shut down Servo and its related context.
     Shutdown,
-    /// A new frame is ready to present. The caller can paint other things themselves during this
+    Continue {
-    /// period, but has to call [`Servo::present`] to perform page flip and tell Servo compositor
+        history_changed: bool,
-    /// to continue rendering.
+        present: Present,
-    ReadyToPresent,
+    },
-    /// The size has changed. The caller can paint other things themselves during this
-    /// period, but has to call [`Servo::present`] to perform page flip and tell Servo compositor
-    /// to continue rendering.
-    Resize,
 }
 
 impl App {
@@ -108,8 +110,15 @@ impl App {
             webrender_surfman.make_gl_context_current().unwrap();
             debug_assert_eq!(webrender_gl.get_error(), gleam::gl::NO_ERROR);
 
-            app.minibrowser =
+            app.minibrowser = Some(
-                Some(Minibrowser::new(&webrender_surfman, &events_loop, window.as_ref()).into());
+                Minibrowser::new(
+                    &webrender_surfman,
+                    &events_loop,
+                    window.as_ref(),
+                    initial_url.clone(),
+                )
+                .into(),
+            );
         }
 
         if let Some(mut minibrowser) = app.minibrowser() {
@@ -271,29 +280,58 @@ impl App {
             }
 
             // Consume and handle any events from the Minibrowser.
-            if let Some(mut minibrowser) = app.minibrowser() {
+            if let Some(minibrowser) = app.minibrowser() {
                 let browser = &mut app.browser.borrow_mut();
                 let app_event_queue = &mut app.event_queue.borrow_mut();
                 minibrowser.queue_embedder_events_for_minibrowser_events(browser, app_event_queue);
-                if minibrowser.update_location_in_toolbar(browser) {
-                    // Update the minibrowser immediately. While we could update by requesting a
-                    // redraw, doing so would delay the location update by two frames.
-                    minibrowser
-                        .update(window.winit_window().unwrap(), "update_location_in_toolbar");
-                }
             }
 
             match app.handle_events() {
-                Some(PumpResult::Shutdown) => {
+                PumpResult::Shutdown => {
                     *control_flow = winit::event_loop::ControlFlow::Exit;
                     app.servo.take().unwrap().deinit();
                     if let Some(mut minibrowser) = app.minibrowser() {
                         minibrowser.context.destroy();
                     }
                 },
-                Some(PumpResult::ReadyToPresent) => {
+                PumpResult::Continue {
+                    history_changed,
+                    present,
+                } => {
+                    if history_changed {
+                        if let Some(mut minibrowser) = app.minibrowser() {
+                            let browser = &mut app.browser.borrow_mut();
+                            if minibrowser.update_location_in_toolbar(browser) {
+                                // Update the minibrowser immediately. While we could update by requesting a
+                                // redraw, doing so would delay the location update by two frames.
+                                minibrowser.update(
+                                    window.winit_window().unwrap(),
+                                    "update_location_in_toolbar",
+                                );
+                            }
+                        }
+                    }
+                    match present {
+                        Present::Immediate => {
+                            // The window was resized.
+                            trace!("PumpResult::Present::Immediate");
+
+                            // Resizes are unusual in that we need to repaint synchronously.
+                            // TODO(servo#30049) can we replace this with the simpler Servo::recomposite?
+                            app.servo.as_mut().unwrap().repaint_synchronously();
+
+                            if let Some(mut minibrowser) = app.minibrowser() {
+                                minibrowser.update(
+                                    window.winit_window().unwrap(),
+                                    "PumpResult::Present::Immediate",
+                                );
+                                minibrowser.paint(window.winit_window().unwrap());
+                            }
+                            app.servo.as_mut().unwrap().present();
+                        },
+                        Present::Deferred => {
                             // The compositor has painted to this frame.
-                    trace!("PumpResult::ReadyToPresent");
+                            trace!("PumpResult::Present::Deferred");
 
                             // Request a winit redraw event, so we can paint the minibrowser and present.
                             // Otherwise, it's in headless mode and we present directly.
@@ -307,21 +345,9 @@ impl App {
                             // TODO(servo#30331) broken on macOS?
                             // need_recomposite = false;
                         },
-                Some(PumpResult::Resize) => {
+                        Present::None => {},
-                    // The window was resized.
-                    trace!("PumpResult::Resize");
-
-                    // Resizes are unusual in that we need to repaint synchronously.
-                    // TODO(servo#30049) can we replace this with the simpler Servo::recomposite?
-                    app.servo.as_mut().unwrap().repaint_synchronously();
-
-                    if let Some(mut minibrowser) = app.minibrowser() {
-                        minibrowser.update(window.winit_window().unwrap(), "PumpResult::Resize");
-                        minibrowser.paint(window.winit_window().unwrap());
                     }
-                    app.servo.as_mut().unwrap().present();
                 },
-                None => {},
             }
         });
     }
@@ -381,7 +407,7 @@ impl App {
     /// Window queues to the Browser queue, and from the Browser queue to Servo. We receive and
     /// collect embedder messages from the various Servo components, then take them out of the
     /// Servo interface so that the Browser can handle them.
-    fn handle_events(&mut self) -> Option<PumpResult> {
+    fn handle_events(&mut self) -> PumpResult {
         let mut browser = self.browser.borrow_mut();
 
         // FIXME:
@@ -404,9 +430,12 @@ impl App {
         let mut embedder_messages = self.servo.as_mut().unwrap().get_events();
         let mut need_resize = false;
         let mut need_present = false;
+        let mut history_changed = false;
         loop {
             // Consume and handle those embedder messages.
-            need_present |= browser.handle_servo_events(embedder_messages);
+            let servo_event_response = browser.handle_servo_events(embedder_messages);
+            need_present |= servo_event_response.need_present;
+            history_changed |= servo_event_response.history_changed;
 
             // Route embedder events from the Browser to the relevant Servo components,
             // receives and collects embedder messages from various Servo components,
@@ -417,7 +446,7 @@ impl App {
                 .unwrap()
                 .handle_events(browser.get_events());
             if browser.shutdown_requested() {
-                return Some(PumpResult::Shutdown);
+                return PumpResult::Shutdown;
             }
 
             // Take any new embedder messages from Servo itself.
@@ -427,12 +456,17 @@ impl App {
             }
         }
 
-        if need_resize {
+        let present = if need_resize {
-            Some(PumpResult::Resize)
+            Present::Immediate
         } else if need_present {
-            Some(PumpResult::ReadyToPresent)
+            Present::Deferred
         } else {
-            None
+            Present::None
+        };
+
+        PumpResult::Continue {
+            history_changed,
+            present,
         }
     }
 

ports/servoshell/browser.rs

@@ -50,6 +50,11 @@ pub struct Browser<Window: WindowPortsMethods + ?Sized> {
     shutdown_requested: bool,
 }
 
+pub struct ServoEventResponse {
+    pub need_present: bool,
+    pub history_changed: bool,
+}
+
 impl<Window> Browser<Window>
 where
     Window: WindowPortsMethods + ?Sized,
@@ -277,9 +282,13 @@ where
         self.event_queue.push(event);
     }
 
-    /// Returns true iff the caller needs to manually present a new frame.
+    /// Returns true if the caller needs to manually present a new frame.
-    pub fn handle_servo_events(&mut self, events: Vec<(Option<BrowserId>, EmbedderMsg)>) -> bool {
+    pub fn handle_servo_events(
+        &mut self,
+        events: Vec<(Option<BrowserId>, EmbedderMsg)>,
+    ) -> ServoEventResponse {
         let mut need_present = false;
+        let mut history_changed = false;
         for (browser_id, msg) in events {
             trace!(
                 "embedder <- servo EmbedderMsg ({:?}, {:?})",
@@ -456,6 +465,7 @@ where
                 EmbedderMsg::HistoryChanged(urls, current) => {
                     self.current_url = Some(urls[current].clone());
                     self.current_url_string = Some(urls[current].clone().into_string());
+                    history_changed = true;
                 },
                 EmbedderMsg::SetFullscreenState(state) => {
                     self.window.set_fullscreen(state);
@@ -538,7 +548,10 @@ where
             }
         }
 
-        need_present
+        ServoEventResponse {
+            need_present,
+            history_changed,
+        }
     }
 }
 

ports/servoshell/minibrowser.rs

@@ -11,6 +11,7 @@ use euclid::Length;
 use log::{trace, warn};
 use servo::compositing::windowing::EmbedderEvent;
 use servo::servo_geometry::DeviceIndependentPixel;
+use servo::servo_url::ServoUrl;
 use servo::webrender_surfman::WebrenderSurfman;
 
 use crate::browser::Browser;
@@ -40,6 +41,7 @@ impl Minibrowser {
         webrender_surfman: &WebrenderSurfman,
         events_loop: &EventsLoop,
         window: &dyn WindowPortsMethods,
+        initial_url: ServoUrl,
     ) -> Self {
         let gl = unsafe {
             glow::Context::from_loader_function(|s| webrender_surfman.get_proc_address(s))
@@ -56,7 +58,7 @@ impl Minibrowser {
             event_queue: RefCell::new(vec![]),
             toolbar_height: Default::default(),
             last_update: Instant::now(),
-            location: RefCell::new(String::default()),
+            location: RefCell::new(initial_url.to_string()),
             location_dirty: false.into(),
         }
     }