Code review 2025-07-18 #12
Conversation
| /// another `Gtk::Window`, so we need to implement a pseudo-embed by tracking | ||
| /// UI changes and then natively pass them to pseudo-children. | ||
| /// | ||
| /// For this purpose, We hook into the GTK event loop with `Glib::signal_idle` |
There was a problem hiding this comment.
@Zacharymk1213
layout_update_cb should become a pure virtual method in gbw::gtk::Window and then OS/browser specific logic should inherit hierarchically. I just initially implemented this here for MsWebview2 as a quick and dirty and have not yet got back to making it portable.
|
|
||
| browser_ready_signal.emit(); | ||
| } | ||
|
|
There was a problem hiding this comment.
@Zacharymk1213
As before, this method should be implemented as a virtual method from gbw::gtk::Browser_window.
This method is super critical as it is responsible for positioning the gbw::gtk::Browser_window over the gbw::Browser_widget. Without it the whole product falls down.
It is called on the GTK event loop at Glib::signal_idle, so can be resource intensive and impact the GUI in bad ways. Things to look at:
- Is there work we can offload to a worker thread without negatively impacting GTK?
- Do we need to call it on every loop? Maybe a small timeout between 10 -100ms can still give the illusion of a smooth UI update?
src/gtk/Browser_window.cc
Outdated
| present(); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
@Zacharymk1213
This is another super critical method that needs some thought and attention. The gbw::gtk::Browser_window is held above the underlying gtk::Application here: set_transient_for(*top_level_window);
This however does not control focus, which (in Windows at least) appears to be delegated to the underlying native window of the browser instance. The method here ensures that the application always regains focus if lost: top_level_window->present().
Everything appears to mostly work as expected within the browser, however, I am observing the following anomaly:
- Hover events appear to reliably register on actioned DOM elements
- About 1/10 times, even though the DOM has responded to the hover event, the click event only registers on the second click...
I am just making you aware here. I have not invested much time in debugging this until I have reproduced it in other OS/browser contexts. It may have nothing to do with focus. Maybe it is a bug on the GTK website where I tested it? Just keep an eye on it as you proceed because, if it is an issue, it can be a production release deal-breaker.
| HWND browser_hWnd; | ||
| HWND top_level_hWnd; | ||
|
|
||
| // Dummy trigger for code review 2025-07-18 |
There was a problem hiding this comment.
@Zacharymk1213
I am learning as I go regarding various browser engine API's and I started with MsWebview2. The three methods below represent the control flow for the MsWebview2 bootstrap:
- Get the environment API
- Use the environment API to generate the controller API
- Get the core (window DOM) API from the controller
I am hoping that other browser API's have similar control flow bootstrap logic.
A common logic should migrate to the gbw::gtk::Browser_window virtual methods so that the naming and control flow conventions remain consistent. Supposing that we find compatible logic across browser engines, here is what I think should happen (and does NOT yet in this code):
- The engine creates an environment API only once at the first
gbw::Browser_widgetinstance initialisation. This is made statically available. - The controller API I think is also common the all child browser windows, so should only be initialised once and made statically available.
- The first, and each subsequent
gbw::Browser_widgetinstance gets an instance API handle to a core DOM api (the equivalent of a new browser tab). What we do not want to do is spin up a new browser engine process for each widget instance.
This also needs some thought as to how we present the API to users in the public gbw::Browser_widget API. They will want to get a handle on the environment to do stuff like security and sandboxing, the controller for loading of resources and whatever other engine specific tasks it delegates and then finally the business of interacting with the instance DOM and javascript. This needs to be uncomplicated and user friendly.
None of the above is by any means guaranteed to be correct or set in stone. At the moment I am making educated guesses from what I have gleaned from MsWebview2. The user experience bit is important however.
| Browser_window(ebw_widget_t &ebw_widget); | ||
|
|
||
| ready_signal_t &ready_signal() { return browser_ready_impl(); } | ||
| // Dummy trigger for code review 2025-07-18 |
There was a problem hiding this comment.
I have named these terribly...
inner and outer definitely do not describe the intent. We need to find user-friendly names that suit all browser engine API categories.
No description provided.