minibrowser: Add loading spinner

[frereit]

I've started to implement a simple spinner that indicates when a page is loading. This PR is not complete yet but I'd like to get some feedback and discuss some implementation details if that's possible.

For this "MVP", I've simply added a new bool loading_changed analogous to the history_changed field in PumpResult::Continue. The WebView then keeps track of wether or not it is loading and exposes this via WebView::loading(), like WebView::shutdown_requested. The Minibrowser then gets a notification from the App when loading changed and updates the spinner accordingly.

A few questions arose while implementing this:

• The comments in the code and issue indicate that EmbedderMsg::HeadParsed should also "influence" the loading state, but I do not understand how this should surface. In my understanding, the spinner simply shows up when the document starts loading and stops once it is complete. What is the intended "intermediary" step here?

servo/ports/servoshell/webview.rs

Lines 596 to 598 in 8cfc6a1

	EmbedderMsg::HeadParsed => {
	// FIXME: surface the loading state in the UI somehow
	},

• It seems a bit clunky to add a xyz_changed for everything about the WebView that could be required to bubble up to the Minibrowser. For example, showing the Favicon might also require a favicon_changed: bool in the future. Perhaps it would make sense to combine all these into a toolbar_data_changed or metadata_changed or something and let the Minibrowser figure out what it has to do (or just refresh the entire toolbar when anything changed, that shouldn't really impact performance). Is this something I should implement in this PR?

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix servoshell: add a spinner that indicates when the page is loading #31689

• These changes do not require tests because it is purely visual in the minibrowser.

[jdm]

What is the intended "intermediary" step here?

One common pattern I've seen is a slow spinner in one direction while the initial network request is in flight, then the spinner changes direction or speeds up when the beginning of the response is received. This corresponds to the HeadParsed event.

[delan]

What is the intended "intermediary" step here?

One common pattern I've seen is a slow spinner in one direction while the initial network request is in flight, then the spinner changes direction or speeds up when the beginning of the response is received. This corresponds to the HeadParsed event.

Yeah, HeadParsed happens somewhere between LoadStart and LoadComplete. It would be good to show these two periods differently, but if we can’t, then we should treat HeadParsed like LoadStart.

It seems a bit clunky to add a xyz_changed for everything about the WebView that could be required to bubble up to the Minibrowser. For example, showing the Favicon might also require a favicon_changed: bool in the future. Perhaps it would make sense to combine all these into a toolbar_data_changed or metadata_changed or something and let the Minibrowser figure out what it has to do (or just refresh the entire toolbar when anything changed, that shouldn't really impact performance). Is this something I should implement in this PR?

Good point! Since we only use our history_changed flag to check if we should run an egui update (and maybe update some Minibrowser fields), I agree that it would be good to combine them.

[frereit]

What is the intended "intermediary" step here?

One common pattern I've seen is a slow spinner in one direction while the initial network request is in flight, then the spinner changes direction or speeds up when the beginning of the response is received. This corresponds to the HeadParsed event.

Yeah, HeadParsed happens somewhere between LoadStart and LoadComplete. It would be good to show these two periods differently, but if we can’t, then we should treat HeadParsed like LoadStart.

Done. During the LoadStart -> HeadParsed period the spinner is now Color32::GRAY and the default white otherwise.

It seems a bit clunky to add a xyz_changed for everything about the WebView that could be required to bubble up to the Minibrowser. For example, showing the Favicon might also require a favicon_changed: bool in the future. Perhaps it would make sense to combine all these into a toolbar_data_changed or metadata_changed or something and let the Minibrowser figure out what it has to do (or just refresh the entire toolbar when anything changed, that shouldn't really impact performance). Is this something I should implement in this PR?

Good point! Since we only use our history_changed flag to check if we should run an egui update (and maybe update some Minibrowser fields), I agree that it would be good to combine them.

Done.

I've also changed the webview to always return need_present: true when the spinner should be active because the spinner is animated and requires regular redraws to not be choppy. Is this the idiomatic way to achieve this or should the redraw be requested some other way?

[delan]

Excellent work! One question…

[delan]

I've also changed the webview to always return need_present: true when the spinner should be active because the spinner is animated and requires regular redraws to not be choppy. Is this the idiomatic way to achieve this or should the redraw be requested some other way?

This makes sense and seems good enough to me.

[delan]

Great, thanks!

[frereit]

Apologies for the format errors. Fixed.