New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

winit: initial minibrowser #29976

Merged

mrobinson merged 44 commits into servo:master from delan:minibrowser

Aug 15, 2023

Member

delan commented Jul 6, 2023 •

edited

This patch introduces a new minibrowser mode, enabled by running the winit port with --minibrowser.

The minibrowser has a location bar and a go button. The location bar updates to reflect the URL of the top-level browsing context, unless the user has started editing the location without clicking go.

It has been tested on Linux with X11, but should also work with Wayland and on Windows and macOS.

Some changes to the compositor were necessary to allow Servo and our minibrowser code to cooperatively draw to the same OpenGL framebuffer. Normally the compositor “presents” each new frame (swapping buffers) as soon as WebRender has drawn to it, but we need to make sure that both WebRender and the minibrowser are repainted on every frame. Failing to do so leads to strange glitches where new frames get overwritten with stale framebuffer contents.

We add an external_present mode that instead notifies the embedder (via the constellation) that a new frame is ready, allowing the embedder to draw other things to the framebuffer. In external_present mode, the compositor does not run again until the embedder calls present, to ensure consistency of WebRender output, and IOCompositor::clear_background no longer clears the whole framebuffer, only the viewport area.

Some optimisations were also necessary to make this performant:

we add a Browser field that tracks the current top-level URL as a string, so we can avoid repeated serialisation
we vendor and modify EguiGlow to retain its shapes, so we can repaint the minibrowser without an egui update
when egui needs an update, we defer it through a winit redraw request, so we can coalesce updates over events

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Servo embedding: Create a mini-browser #29930
These changes partially involve [meta] Minibrowser #30049

There are tests for these changes OR
These changes do not require tests because it would be impractical to test the minibrowser directly, but we may be able to unify the codepaths so the minibrowser is indirectly tested with WPT

delan added 4 commits

July 6, 2023 16:04


          winit: add minibrowser feature that depends on egui{,-winit}

557183c


          winit: carve out some space at the top of headed windows

7bb3db5


          winit: minimal toolbar and egui/winit integration (but no painting)


          winit: try to paint with egui_glow (doesn’t work yet)

f9460af

delan force-pushed the minibrowser branch from ed2fb0b to f9460af Compare

July 6, 2023 09:50

delan and others added 7 commits

July 6, 2023 18:05


          winit: add comment about toolbar size

4e01fde


          Add framebuffer object, set it as glow's target

7103b4b


          compositing: clear only the viewport, not the whole framebuffer

3be6a17


          plumb the actual size of the egui toolbar to webrender

9eb92a2


          fix formatting

034a831


          winit: fix crash when fbo is zero

e495886


          winit: don’t bother binding the framebuffer object

e9f8c3c

delan commented

View reviewed changes

ports/winit/app.rs Outdated Show resolved Hide resolved

ports/winit/app.rs Outdated Show resolved Hide resolved

atbrakhi mentioned this pull request

Minibrowser: Introduce minibrowser.enabled prefs #29995

Merged

2 tasks

bors-servo added a commit that referenced this pull request


          Auto merge of #29995 - servo:minibrowser_prefs, r=<try>

8d58cc3

Minibrowser: Introduce minibrowser.enabled prefs

Introducing minibrowser.enabled prefs in preparation of #29976

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

bors-servo added a commit that referenced this pull request


          Auto merge of #29995 - servo:minibrowser_prefs, r=delan

19590bf

Minibrowser: Introduce minibrowser.enabled prefs

Introducing minibrowser.enabled prefs in preparation of #29976

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

atbrakhi added 2 commits

July 16, 2023 23:32


          winit: remove unsafe and get toolbar_height

a8e1e5b


          winit: location field should reflect the current top-level url

aac08c7

atbrakhi force-pushed the minibrowser branch from fa522bf to aac08c7 Compare

July 23, 2023 20:03


          merge from master

8da7cf8

Collaborator

servo-wpt-sync commented Jul 24, 2023

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.

delan added 2 commits

July 26, 2023 14:39


          [NFC] winit: move Minibrowser out of App::run

37ea3f0


          winit: clean up toolbar height code

d717962

Collaborator

servo-wpt-sync commented Jul 26, 2023

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.


          winit: make App own the Minibrowser if any

dda07d9

Collaborator

servo-wpt-sync commented Jul 26, 2023

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.


          winit: make the go button work

6717cf0

Collaborator

servo-wpt-sync commented Jul 26, 2023

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.


          winit:make the location field reflect the current top-level url

8cd288e

Collaborator

servo-wpt-sync commented Jul 27, 2023

🛠 These changes could not be applied onto the latest upstream WPT. Servo's copy of the Web Platform Tests may be out of sync.


          winit: take toolbar height into account in hit testing

3e56ccb

wusyong reviewed

View reviewed changes

ports/winit/app.rs Outdated Show resolved Hide resolved

delan added 10 commits

August 8, 2023 17:31


          winit: pass egui only relevant CursorMoved events

ffc971d


          winit: scratch that, coalesce minibrowser updates instead

62ec187


          ensure both minibrowser and WR are repainted on every frame

d1ea041


          compositing: only skip framebuffer clear in external present mode

f7a13a7


          winit: destroy egui glow Painter when shutting down

8b8a201


          winit: clean up and fix license lint

2a31b39


          fix duplicate versions lint by downgrading bytemuck_derive

dac5b14

was egui_glow ^0.22.0 (0.22.0)
→ egui/bytemuck ^0.22.0 (0.22.0)
→ epaint/bytemuck ^0.22.0 (0.22.0)
→ bytemuck ^1.7.2 (1.13.1)
→ bytemuck_derive ^1.4 (1.4.1)
→ syn ^2.0.1 (2.0.28)

now lock has bytemuck_derive 1.4.0
→ syn ^1.0.99 (1.0.103)


          fix duplicate versions lint by disabling egui-winit/links

1a4ce1f

(we don’t need support for hyperlinks in our use of egui)


          squelch duplicate versions lint by excluding clipboard-win

573820c


          winit: fix compile warnings

95dabae

delan marked this pull request as ready for review

August 10, 2023 06:56


          winit: make gleam an optional dependency under /minibrowser

bd114b5

mrobinson requested changes

View reviewed changes

Member

mrobinson left a comment

Thanks for walking through this with us. I'm excited about this functionality! I just have a few suggestions.

ports/winit/Cargo.toml Outdated Show resolved Hide resolved

ports/winit/app.rs Outdated Show resolved Hide resolved

ports/winit/app.rs Outdated Show resolved Hide resolved

ports/winit/app.rs Outdated Show resolved Hide resolved

ports/winit/browser.rs Outdated Show resolved Hide resolved

ports/winit/app.rs Outdated Show resolved Hide resolved

ports/winit/app.rs Outdated Show resolved Hide resolved

ports/winit/browser.rs Outdated Show resolved Hide resolved

delan added 4 commits

August 14, 2023 16:33


          winit: remove cargo feature, since it’s not really optional

00d6187


          winit: extract Minibrowser and related code to separate module

06e0d29


          winit: remove unnecessary trailing comma

ebdb007


          winit: simplify the ServoUrl serialisation optimisation

b01b10c

delan requested a review from mrobinson

August 14, 2023 09:27

mrobinson approved these changes

View reviewed changes

Member

mrobinson left a comment

Thanks for these updates. This looks great!

mrobinson added this pull request to the merge queue

github-merge-queue bot removed this pull request from the merge queue due to failed status checks

mrobinson added this pull request to the merge queue

Merged via the queue into servo:master with commit 2778bee

9 of 10 checks passed

delan deleted the minibrowser branch

August 15, 2023 09:45

atbrakhi mentioned this pull request

Fix libsimpleservo build #30109

Merged

3 tasks

delan mentioned this pull request

minibrowser: fix spurious update when location has not changed #30309

Merged

5 tasks

delan mentioned this pull request

Fix servoshell can't respond when there's no minibrowser #30598

Merged

4 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment