Bug 115871

Created attachment 201269 [details] Patch

Comment on attachment 201269 [details] Patch Common C APIs should have tests.

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'

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.

Created attachment 201367 [details] Patch

(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.

(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.

(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.

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.

(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?

(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?

(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.

> (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.

> > 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.

(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.

(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.

So?

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.

(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.

(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.

(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.