Bug 89392 – [Chromium] Notify the embedder when the page scale changes

• Status: RESOLVED
• Resolution: LATER
• Component: New Bugs

[Adam Barth]

[Chromium] Notify the embedder when the page scale changes

[Adam Barth]

Created attachment 148188 [details]
Patch

[Adam Barth]

I'm not really sure what this is used for, but it's in the chromium-android branch.  trchen might know.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 148188 [details]
Patch

What's this for?  I wouldn't expect that the embedder needs to know about the page scale factor, generally speaking.

[Adam Barth]

If trchen doesn't know off-hand, I can do some more digging in the chromium-android branch to try to find out.

[James Robinson]

If you can search for callers on the chromium and figure out what they are doing that would be super useful.

[Adam Barth]

> If you can search for callers on the chromium and figure out what they are doing that would be super useful.

I gets routed up to the browser process and then out to Java.  Once in Java, it looks like it's used to update some UI controls for zooming.  Maybe there's a scale (like on a map) that shows you how zoomed in you are, possibly relative to what min and max zooms are available?  I don't read Java UI code very well.

[James Robinson]

Comment on attachment 148188 [details]
Patch

That sounds like a UI that was abandoned a while ago for page zoom - let's get some confirmation about whether this code is still alive or not before adding it.

[Adam Barth]

@trchen: Do we still need these notifications?

[Adam Barth]

I will attempt to remove this notification from the chromium-android branch to see if folks are still using it.

[Adam Barth]

Comment on attachment 148188 [details]
Patch

I put on my Java thinking cap and figure out what this actually does.  It lets the UI keep track of how much page scaling is possible so that it can properly react to various pinch-like gestures.  It's also used by the browser UI to adjust the size of various overly elements to scale them commensurate with the page.

[Adam Barth]

Comment on attachment 148188 [details]
Patch

jamesr and I spoke about this patch at length.  The approach in this patch isn't great because this patch essentially creates to notification paths for the page scale information to reach the browser's UI thread:

1) These callbacks.
2) The images drawn by the compositor.

Having two asynchronous notification paths is problematic because the messages arriving on these two paths will be inconsistent and lead to janky UI.  At a better approach is to send this information along the compositor's notification path so that it is synchronized with the images actually being displayed by the compositor.

James recommends looking at WebCompositorInputHandler and WebCompositorInputHandlerClient to see how we might follow that approach.

[Tien-Ren Chen]

(In reply to comment #12)
> (From update of attachment 148188 [details])
> jamesr and I spoke about this patch at length.  The approach in this patch isn't great because this patch essentially creates to notification paths for the page scale information to reach the browser's UI thread:
> 
> 1) These callbacks.
> 2) The images drawn by the compositor.
> 
> Having two asynchronous notification paths is problematic because the messages arriving on these two paths will be inconsistent and lead to janky UI.  At a better approach is to send this information along the compositor's notification path so that it is synchronized with the images actually being displayed by the compositor.
> 
> James recommends looking at WebCompositorInputHandler and WebCompositorInputHandlerClient to see how we might follow that approach.

In the case of Android, even if we send all these notification from the compositor thread it still won't be synchronous with the rendered images, as the GL commands go through the command buffer pipeline... In our downstream code we used to add a GL extension to pass frame information along with swap buffers, which was essential for glitch-free mixed-mode scrolling. It was later removed with the deprecation of mixed-mode. We deemed that async message is good enough for our use (and synchronous message is expensive).

For the long term I think we want to move all the rendering logic of extra decorations (mainly text selection handles) to the compositor so we don't have to deal with scale factor mess from the browser side.

Do we still need these IPCs? I'm not very sure. They might still be useful to handle WebView properties from 3rd-party WebView users, but don't quote me on that. Let me add aelias@ and klobag@ to the thread for more inputs.

[Adam Barth]

I believe they're needed for some of the WebView APIs, such as canZoomIn().

[Alexandre Elias]

The latest plan is to bundle this data with ubercompositor frames.  Clank is moving to ubercompositor and we recently figured out that to support WebView child views in particular, we'll need accurate, frame-synchronized scroll offset and page scale information for the root layer.  So we can delete these lines.