servo: Rename `WebView::pinch_zoom` to `adjust_pinch_zoom` and add pinch zoom getter by chrisduerr · Pull Request #43228 · servo/servo

chrisduerr · 2026-03-13T04:23:48Z

Previously the pinch_zoom method on WebView would be used to update the page's pinch zoom level, however there was no way for the embedder to retrieve the current pinch zoom.

This patch renames the existing pinch_zoom method to adjust_pinch_zoom and changes the pinch_zoom method to instead return the current pinch zoom level.

Decided to go for the breaking change since I'd find pinch_zoom combined with get_pinch_zoom to be confusing, however I'm happy to change that if a non-breaking change is preferred.

Motivation for this patch is just that I'd like to display the zoom level in my UI while rendering.

Testing: Since this change adds a simple getter to the API, we can probably avoid a unit test in this case.

chrisduerr · 2026-03-13T04:24:21Z

            .get(&webview_id)
            .map(|webview_renderer| webview_renderer.page_zoom.get())
-            .unwrap_or_default()
+            .unwrap_or(1.)


I changed this because it looks wrong to me. From what I understand page zoom of 0 would be invalid, so defaulting to 0 doesn't make sense, right?

It's an error for this code to receive a request for a WebView that does not exist. Still, it's a recoverable error. I think that 0 is better return value given this.

In which way is 0 better here? If the goal is to indicate an invalid value, None should be returned (imo). If the goal is to have a value that is as uncontroversial as possible, then 1 should be picked because 0 as a scale is likely to cause issues in the embedder if not handled explicitly.

If you're confident that 0 is the best value here please let me know, but I'm struggling to fathom why one would think that using a value that is technically the right type but likely to cause issues (like div by zero) would be better than a return value that is clearly indicating an issue None or one that is just using the default and avoiding issues (1).

If this fails and 0 is returned, it's quite likely that something is really wrong with the internals of Servo.

Well the web view ID just might not exist anymore, so I don't think something is necessarily 'very wrong with servo' (which might justify a panic).

A return value of 0 isn't standing out more than a value of 1, it's just going to cause confusing behavior when encountered.

If you insist I can change that back, but I feel like this should either be None or 1. A value of 0 is like None, but the consumer isn't forced to deal with it, thus likely to cause issues.

WebViewId is not exposed to the the embedding API and as long as a WebView is alive, it should have a valid WebViewRenderer in the Painter. That means that the situation where we return the default value shouldn't happen and represents a logic error in the implementation of Servo. Likely these should all be expect("...") but we opted for not panicking in this case. Perhaps that was the wrong choice.

I've sent this to the merge queue anyway.

I'd be happy to panic too, but I don't think an error here is important enough to fail here. Trying to recover as well as possible for an extremely unlikely scenario by returning 1 seems fine to me. Chances are things will likely fail in a different place anyway.

mrobinson

Looks good, but with two small changes:

mrobinson · 2026-03-13T17:26:02Z

            .get(&webview_id)
            .map(|webview_renderer| webview_renderer.page_zoom.get())
-            .unwrap_or_default()
+            .unwrap_or(1.)


It's an error for this code to receive a request for a WebView that does not exist. Still, it's a recoverable error. I think that 0 is better return value given this.

mrobinson · 2026-03-13T17:26:30Z

+        self.webview_renderers
+            .get(&webview_id)
+            .map(|webview_renderer| webview_renderer.pinch_zoom().zoom_factor().0)
+            .unwrap_or(1.)


For consistency with the page_zoom getter:

Suggested change

.unwrap_or(1.)

.unwrap_or_default()

codecov-commenter · 2026-03-17T16:09:44Z

⚠️ JUnit XML file not found

The CLI was unable to find any JUnit XML files to upload.
For more help, visit our troubleshooting guide.

Previously the `pinch_zoom` method on `WebView` would be used to update the page's pinch zoom level, however there was no way for the embedder to retrieve the current pinch zoom. This patch renames the existing `pinch_zoom` method to `adjust_pinch_zoom` and changes the `pinch_zoom` method to instead return the current pinch zoom level. Signed-off-by: Christian Duerr <contact@christianduerr.com>

Signed-off-by: Christian Duerr <contact@christianduerr.com>

chrisduerr · 2026-03-17T22:45:14Z

I'm uncertain why this failed? CI runs all tests automatically, right? So if CI is green then merge queue should work?

I rebased it just to make sure.

jdm · 2026-03-18T00:57:23Z

Build failure in the OpenHarmony port: https://github.com/servo/servo/actions/runs/23214339818/job/67471126130

jdm · 2026-03-18T00:58:39Z

And no, the checks that run by default in the PR are a subset of the full checks that run when attempting to merge.

Signed-off-by: Christian Duerr <contact@christianduerr.com>

chrisduerr · 2026-03-18T01:03:58Z

@jdm Is there a way to manually trigger these builds on PRs?

jdm · 2026-03-18T03:50:01Z

By applying the T- labels to trigger "try" builds: https://book.servo.org/contributing/making-a-pull-request.html#running-tests-in-pull-requests . They can be applied by anybody in the Contributors or Maintainers groups.

github-actions · 2026-03-18T03:50:36Z

🔨 Triggering try run (#23228214389) for OpenHarmony

github-actions · 2026-03-18T03:50:52Z

🔨 Triggering try run (#23228216497) for Android

github-actions · 2026-03-18T04:11:21Z

Bencher Report

Branch	43228/PR
Testbed	HUAWEI Mate 60 Pro

⚠️ WARNING: Truncated view!
The full continuous benchmarking report exceeds the maximum length allowed on this platform.

⚠️ WARNING: No Threshold found!
Without a Threshold, no Alerts will ever be generated.

🐰 View full continuous benchmarking report in Bencher

github-actions · 2026-03-18T04:18:28Z

✨ Try run (#23228214389) succeeded.

github-actions · 2026-03-18T04:20:44Z

✨ Try run (#23228216497) succeeded.

…nch zoom getter (servo#43228) Previously the `pinch_zoom` method on `WebView` would be used to update the page's pinch zoom level, however there was no way for the embedder to retrieve the current pinch zoom. This patch renames the existing `pinch_zoom` method to `adjust_pinch_zoom` and changes the `pinch_zoom` method to instead return the current pinch zoom level. --- Decided to go for the breaking change since I'd find `pinch_zoom` combined with `get_pinch_zoom` to be confusing, however I'm happy to change that if a non-breaking change is preferred. Motivation for this patch is just that I'd like to display the zoom level in my UI while rendering. Testing: Since this change adds a simple getter to the API, we can probably avoid a unit test in this case. --------- Signed-off-by: Christian Duerr <contact@christianduerr.com>

chrisduerr requested a review from atbrakhi as a code owner March 13, 2026 04:23

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 13, 2026

chrisduerr commented Mar 13, 2026

View reviewed changes

chrisduerr force-pushed the pinch_zoom branch 3 times, most recently from ca20b5f to 4a46dd2 Compare March 13, 2026 16:11

mrobinson requested changes Mar 13, 2026

View reviewed changes

servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 13, 2026

mrobinson changed the title ~~Add getter for pinch zoom~~ servo: Rename WebView::pinch_zoom to adjust_pinch_zoom and add pinch zoom getter Mar 13, 2026

chrisduerr requested a review from mrobinson March 13, 2026 19:21

mrobinson approved these changes Mar 17, 2026

View reviewed changes

mrobinson enabled auto-merge March 17, 2026 13:47

auto-merge was automatically disabled March 17, 2026 16:03
Head branch was pushed to by a user without write access

chrisduerr force-pushed the pinch_zoom branch from 4a46dd2 to 8361e11 Compare March 17, 2026 16:03

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 17, 2026

chrisduerr requested a review from mrobinson March 17, 2026 18:59

mrobinson added this pull request to the merge queue Mar 17, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 17, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 17, 2026

servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 17, 2026

chrisduerr force-pushed the pinch_zoom branch from f75a6ad to ff07f69 Compare March 17, 2026 22:24

servo-highfive removed the S-tests-failed The changes caused existing tests to fail. label Mar 17, 2026

chrisduerr added 2 commits March 17, 2026 23:24

Fix tests

ff07f69

Signed-off-by: Christian Duerr <contact@christianduerr.com>

Fix openharmony build

939d157

Signed-off-by: Christian Duerr <contact@christianduerr.com>

chrisduerr requested a review from jschwe as a code owner March 18, 2026 01:03

jdm added T-ohos Do a try run on OpenHarmony T-android Do a try run on Android labels Mar 18, 2026

github-actions Bot removed the T-ohos Do a try run on OpenHarmony label Mar 18, 2026

github-actions Bot removed the T-android Do a try run on Android label Mar 18, 2026

jdm added this pull request to the merge queue Mar 18, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 18, 2026

Merged via the queue into servo:main with commit 3c9221c Mar 18, 2026
57 checks passed

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 18, 2026

chrisduerr deleted the pinch_zoom branch March 18, 2026 06:40

Uh oh!

Conversation

chrisduerr commented Mar 13, 2026 • edited by mrobinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrobinson Mar 13, 2026 • edited by jdm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 17, 2026

⚠️ JUnit XML file not found

Uh oh!

Uh oh!

chrisduerr commented Mar 17, 2026

Uh oh!

jdm commented Mar 18, 2026

Uh oh!

jdm commented Mar 18, 2026

Uh oh!

chrisduerr commented Mar 18, 2026

Uh oh!

jdm commented Mar 18, 2026

Uh oh!

github-actions Bot commented Mar 18, 2026

Uh oh!

github-actions Bot commented Mar 18, 2026

Uh oh!

github-actions Bot commented Mar 18, 2026

⚠️ WARNING: Truncated view!

⚠️ WARNING: No Threshold found!

Uh oh!

github-actions Bot commented Mar 18, 2026

Uh oh!

github-actions Bot commented Mar 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chrisduerr commented Mar 13, 2026 •

edited by mrobinson

Loading

mrobinson Mar 13, 2026 •

edited by jdm

Loading