Bug 115871 – [WK2] Make it possible to read Viewport information from WKViewportAttributes

• Status: ASSIGNED
• Component: WebKit2

[Jesus Sanchez-Palencia]

WKViewClient (from WKView.h) has a callback defined as:

typedef void (*WKViewPageDidChangeViewportAttributesCallback)(WKViewRef view, WKViewportAttributesRef, const void* clientInfo);

However, currently it isn't possible to read any information from WKViewportAttributes.

[Jesus Sanchez-Palencia]

Created attachment 201269 [details]
Patch

[Benjamin Poulain]

Comment on attachment 201269 [details]
Patch

Common C APIs should have tests.

[Mikhail Pozdnyakov]

Comment on attachment 201269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201269&action=review

> Source/WebKit2/UIProcess/API/C/WKViewportAttributes.h:40
> +WK_EXPORT float WKViewportAttributesGetHeight(WKViewportAttributesRef);

maybe WKViewportAttributesGetLayoutSize instead of 2 functions?

> Source/WebKit2/UIProcess/API/C/WKViewportAttributes.h:44
> +WK_EXPORT bool WKViewportAttributesGetIsUserScalable(WKViewportAttributesRef);

I remember we had a discussion about whether spec-compliant 'zoom' shall be used rather than 'scale'

[Jesus Sanchez-Palencia]

Comment on attachment 201269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=201269&action=review

>> Source/WebKit2/UIProcess/API/C/WKViewportAttributes.h:40
>> +WK_EXPORT float WKViewportAttributesGetHeight(WKViewportAttributesRef);
> 
> maybe WKViewportAttributesGetLayoutSize instead of 2 functions?

Having both avoid us creating a WKSize, but this is not a hot path anyway.

>> Source/WebKit2/UIProcess/API/C/WKViewportAttributes.h:44
>> +WK_EXPORT bool WKViewportAttributesGetIsUserScalable(WKViewportAttributesRef);
> 
> I remember we had a discussion about whether spec-compliant 'zoom' shall be used rather than 'scale'

The spec says user-scalable, so does ViewportAttributes.h. I would rather keep it like this.

[Jesus Sanchez-Palencia]

Created attachment 201367 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #2)
> (From update of attachment 201269 [details])
> Common C APIs should have tests.

Yes, the problem about this one is that WKViewportAttributes has no "setters" and the only API using it is didChangeViewportAttributes from WKViewClient (API/C/CoordinatedGraphics/WKView.h). There is no other API using a WKViewportAttributesRef.

[Mikhail Pozdnyakov]

(In reply to comment #6)
> (In reply to comment #2)
> > (From update of attachment 201269 [details] [details])
> > Common C APIs should have tests.
> 
> Yes, the problem about this one is that WKViewportAttributes has no "setters" and the only API using it is didChangeViewportAttributes from WKViewClient (API/C/CoordinatedGraphics/WKView.h). There is no other API using a WKViewportAttributesRef.

yeah, but it is still possible to test the API if you load a test page having viewport meta tag (or css viewport descriptors) and handle WKViewClient callback.

[Jesus Sanchez-Palencia]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #2)
> > > (From update of attachment 201269 [details] [details] [details])
> > > Common C APIs should have tests.
> > 
> > Yes, the problem about this one is that WKViewportAttributes has no "setters" and the only API using it is didChangeViewportAttributes from WKViewClient (API/C/CoordinatedGraphics/WKView.h). There is no other API using a WKViewportAttributesRef.
> 
> yeah, but it is still possible to test the API if you load a test page having viewport meta tag (or css viewport descriptors) and handle WKViewClient callback.

Sure, but that would be testing the API only for ports using CoordinatedGraphics WKView. That means, on trunk, testing it only for EFL. And, by the way, Tests/WebKit2/efl/WKViewClientWebProcessCallbacks.cpp is only testing 2 of the WKViewClient callbacks (webProcessCrashed and webProcessDidRelaunch) anyway.

What I mean is that there is no way to test this as a Common C API test.

[Benjamin Poulain]

Comment on attachment 201367 [details]
Patch

I discussed this a bit with Jesus.

Exposing raw viewport attribute is a bad idea. The computation of the view parameter depending on the page parameter is complex and it should be common to all the ports.
It is really easy to fuck that up. The design of this should be such that mistakes are difficult to make.

[Noam Rosenthal]

(In reply to comment #9)
> (From update of attachment 201367 [details])
> I discussed this a bit with Jesus.
> 
> Exposing raw viewport attribute is a bad idea. The computation of the view parameter depending on the page parameter is complex and it should be common to all the ports.
> It is really easy to fuck that up. The design of this should be such that mistakes are difficult to make.

Is the handling of viewport attributes completely deterministic?
when I look at PageViewportController.cpp there are a lot of decisions there that seem heuristic/experience driven. It would be nice to share them but it doesn't seem absolutely mandatory.
The idea of externalizing this is not about what code could be shared, it is that some of this is more of a browser-space experience decision than a library decision.
btw isn't Apple doing all of this in browser space and not webkit space?

[Jesus Sanchez-Palencia]

(In reply to comment #10)
> Is the handling of viewport attributes completely deterministic?
> when I look at PageViewportController.cpp there are a lot of decisions there that seem heuristic/experience driven. It would be nice to share them but it doesn't seem absolutely mandatory.
> The idea of externalizing this is not about what code could be shared, it is that some of this is more of a browser-space experience decision than a library decision.
> btw isn't Apple doing all of this in browser space and not webkit space?

Good point...

If the problem here is adding the APIs to WKViewportAttributes.h, which is a common header, then I will propose that we modify WKViewPageDidChangeViewportAttributesCallback to receive:

(WKViewRef view, float width, float height, float minimumScale, float maximumScale, float initialScale, int userScalable, const void* clientInfo)

instead of:

(WKViewRef view, WKViewportAttributesRef, const void* clientInfo).

This will be affecting only a CoordinatedGraphics specific API. Furthermore, by doing this we can also remove WKViewportAttributes.h from the WK2 C API entirely, as no one seems to be using this.

What do you reviewers prefer?

[Benjamin Poulain]

(In reply to comment #10)
> Is the handling of viewport attributes completely deterministic?
> when I look at PageViewportController.cpp there are a lot of decisions there that seem heuristic/experience driven. It would be nice to share them but it doesn't seem absolutely mandatory.
> The idea of externalizing this is not about what code could be shared, it is that some of this is more of a browser-space experience decision than a library decision.

Is the behavior completely defined by specifications? Unfortunately no.
Would it be better for the Web to have a single behavior? yes.

I don't want to limit the freedom you have to tweak the UX. But when it comes to computing the standard parameters and the layout size, WebKit should really align with iOS and Android.

> btw isn't Apple doing all of this in browser space and not webkit space?

No.

[Benjamin Poulain]

> (WKViewRef view, float width, float height, float minimumScale, float maximumScale, float initialScale, int userScalable, const void* clientInfo)
> 
> instead of:
> 
> (WKViewRef view, WKViewportAttributesRef, const void* clientInfo).
> 
> This will be affecting only a CoordinatedGraphics specific API. Furthermore, by doing this we can also remove WKViewportAttributes.h from the WK2 C API entirely, as no one seems to be using this.

That was not my point at all.

Sweeping stuff under the rug is not okay.

[Noam Rosenthal]

> > This will be affecting only a CoordinatedGraphics specific API. Furthermore, by doing this we can also remove WKViewportAttributes.h from the WK2 C API entirely, as no one seems to be using this.
> 
> That was not my point at all.
> 
> Sweeping stuff under the rug is not okay.
Nobody wants to sweep stuff under the rug. The discussion here is about whether this is browser space or webkit space.

[Jesus Sanchez-Palencia]

(In reply to comment #14)
> Nobody wants to sweep stuff under the rug. The discussion here is about whether this is browser space or webkit space.

I had another chat about this with Benjamin last night. The summary, at least the way I see it, is that even for their use case (i.e.g. iOS does some sort of "computation" and then let WebKit know about the computed values, EFL uses PageViewportController, etc) a callback is missing in WK2. But, even after the computation this callback would need to expose:

(WKViewRef view, float minimumScale, float maximumScale, float initialScale, int userScalable, const void* clientInfo);

That is exposing everything related to Viewport but the layout size.

What Benjamin insisted is that the mistake here is exposing "raw" viewport values and not the "computed" ones. The computed ones would avoid a conflicting set of inputs or unrealistic values (e.g. infinite initial scale), besides dealing with all the viewport hardcore computation.

My point here persists: no matter if a port is doing some computation between its PageClient and its WebView, the C API is still missing a callback that gives the necessary information to the upper level. For example, the application needs to know if that content is user scalable or not so it can prevent the user from zooming in/out.

[Noam Rosenthal]

(In reply to comment #15)
> (In reply to comment #14)
> My point here persists: no matter if a port is doing some computation between its PageClient and its WebView, the C API is still missing a callback that gives the necessary information to the upper level. For example, the application needs to know if that content is user scalable or not so it can prevent the user from zooming in/out.

I tend to agree. This API is necessary for implementing e.g. pinch-to-zoom outside WebKit code.
I don't see how you would implement pinch-to-zoom without knowing the minimum scale, max scale, user-scalable attributes and the viewport size.

Benjamin, I think that this approach is right, though if it doesn't fit with Apple's API approach, I suggest that we either put it in API/CoordinatedGraphics or if you have more concrete details to share about how this could be done in a different way we'd love to see.

[Marcelo Lira]

So?

[Benjamin Poulain]

You should go forward if that blocks you, but not make APIs. Just use the internal objects directly.

I also think WKViewportAttributesRef should go away.

[Marcelo Lira]

(In reply to comment #18)
> You should go forward if that blocks you, but not make APIs. Just use the internal objects directly.
> 
> I also think WKViewportAttributesRef should go away.

I wanted to access those values from an application, a MiniBrowser, outside WebKit. The exact case Noam mentioned: I want to know if zoom is allowed and how much, so that I know how the UI will handle pinching.

[Benjamin Poulain]

(In reply to comment #19)
> I wanted to access those values from an application, a MiniBrowser, outside WebKit. The exact case Noam mentioned: I want to know if zoom is allowed and how much, so that I know how the UI will handle pinching.

Then expose a private API for your particular port.

[Marcelo Lira]

(In reply to comment #20)
> (In reply to comment #19)
> > I wanted to access those values from an application, a MiniBrowser, outside WebKit. The exact case Noam mentioned: I want to know if zoom is allowed and how much, so that I know how the UI will handle pinching.
> 
> Then expose a private API for your particular port.

Ok. Works for me.