Bug 110463

Summary: [WK2][EFL] WebView: Add callbacks to the WKViewClient to handle page viewport update
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, andersca, ap, benjamin, cmarcelo, commit-queue, gyuyoung.kim, hugo.lima, jesus, kenneth, kling, lucas.de.marchi, luciano.wolf, noam, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 114593    
Bug Blocks: 115871, 107657, 107662, 110741, 111543    
Attachments:
Description Flags
WIP
none
patch
none
rebased none

Description Mikhail Pozdnyakov 2013-02-21 06:25:15 PST
SSIA.
Comment 1 Mikhail Pozdnyakov 2013-02-22 05:13:30 PST
Created attachment 189748 [details]
WIP
Comment 2 Kenneth Rohde Christiansen 2013-02-22 05:39:26 PST
Comment on attachment 189748 [details]
WIP

Looks nice... im not sure whether we should use these names or the naming from css device adapt spec... scale seems nicer though than zoom as the spec uses
Comment 3 Mikhail Pozdnyakov 2013-02-22 05:57:22 PST
(In reply to comment #2)
> (From update of attachment 189748 [details])
> Looks nice... im not sure whether we should use these names or the naming from css device adapt spec... scale seems nicer though than zoom as the spec uses

good point, I would align names in public apis with specs, maybe we should rename WebCore types accordingly.
Comment 4 Mikhail Pozdnyakov 2013-02-22 06:28:53 PST
Created attachment 189765 [details]
patch

Decided not to include WKViewportAttributes functions so far, as they are not used within this patch (will be used
when Page Viewport Controller is based on C API). In the interim we can figure out appropriate naming for these functions.
Comment 5 Kenneth Rohde Christiansen 2013-02-22 06:31:37 PST
Comment on attachment 189765 [details]
patch

LGTM
Comment 6 Benjamin Poulain 2013-02-27 10:43:20 PST
Comment on attachment 189765 [details]
patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:52
>      WKViewPageDidChangeContentsSizeCallback          didChangeContentsSize;
> +    WKViewPageDidChangeViewportAttributesCallback    didChangeViewportAttributes;

I am more and more confused by what is going on in EFL :(

What goes in view? What goes in page? Why are those two suddenly view callbacks?
Comment 7 Mikhail Pozdnyakov 2013-02-28 07:48:16 PST
(In reply to comment #6)
> (From update of attachment 189765 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189765&action=review
> 
> > Source/WebKit2/UIProcess/API/C/efl/WKView.h:52
> >      WKViewPageDidChangeContentsSizeCallback          didChangeContentsSize;
> > +    WKViewPageDidChangeViewportAttributesCallback    didChangeViewportAttributes;
> 
> I am more and more confused by what is going on in EFL :(
> 
> What goes in view? What goes in page? Why are those two suddenly view callbacks?

We want to keep WebView toolkit-agnostic and WKViewClient in general is needed
to provide a way for enabling EwkView handlers.

ViewportAttributes are calculated based on view size and view in many ways controls the viewport, so I think having didChangeViewportAttributes callback in WKViewClient is quite natural.
Comment 8 Benjamin Poulain 2013-02-28 11:55:57 PST
Wait what? What do you think a WebPage is then? And the PageUIClient?
Comment 9 Mikhail Pozdnyakov 2013-03-01 04:22:32 PST
(In reply to comment #8)
> Wait what? What do you think a WebPage is then? And the PageUIClient?

Please correct me if I'm wrong, in my understanding WebPage is what to be shown by WebView and PageUIClient is to provide UI-related activities required by  WebPage (like showing message box or setting a focus).
Comment 10 Mikhail Pozdnyakov 2013-04-10 08:10:50 PDT
Created attachment 197269 [details]
rebased
Comment 11 Jesus Sanchez-Palencia 2013-04-12 05:33:55 PDT
(In reply to comment #6)
> I am more and more confused by what is going on in EFL :(
> 
> What goes in view? What goes in page? Why are those two suddenly view callbacks?

Benjamin, this idea came from the way Nix decided to handle this sort of information. It is not only about being "toolkit-agnostic", but also about being "UX-agnostic". NIXView also gets the same callbacks (https://github.com/WebKitNix/webkitnix/blob/master/Source/WebKit2/UIProcess/API/nix/NIXView.h#L82) so the application can decide how it wants to handle viewport changes.

In our Minibrowser, for instance, we use this so we can handle "gesture"-scaling properly, etc. https://github.com/WebKitNix/webkitnix/blob/master/Tools/MiniBrowser/nix/main.cpp#L689

I'm more than interested to hear your feedback on this.

(In reply to comment #9)
> (In reply to comment #8)
> > Wait what? What do you think a WebPage is then? And the PageUIClient?
> 
> Please correct me if I'm wrong, in my understanding WebPage is what to be shown by WebView and PageUIClient is to provide UI-related activities required by  WebPage (like showing message box or setting a focus).

I'm also interested to check if this is accurate or not. :)
Comment 12 WebKit Commit Bot 2013-04-12 06:09:02 PDT
Comment on attachment 197269 [details]
rebased

Clearing flags on attachment: 197269

Committed r148274: <http://trac.webkit.org/changeset/148274>
Comment 13 WebKit Commit Bot 2013-04-12 06:09:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Benjamin Poulain 2013-05-10 14:36:55 PDT
This was a bad idea.

I am in favor of reverting this change. RAW viewport attribute should not be exposed. What should be exposed is the behavior computed with those attributes.
Comment 15 Jesus Sanchez-Palencia 2013-05-10 14:58:14 PDT
So far, Nix needs such viewport information on its API layer so the application can use that information to implement whatever is the way it wants to interact with the page. We don't use the PageViewportController as the other CoordinatedGraphics ports do.

However, currently WKViewClient is exposing a WKViewportAttribute which is useless for us without something like what I'm proposing in https://bugs.webkit.org/show_bug.cgi?id=115871 .
Comment 16 Benjamin Poulain 2013-05-10 15:03:52 PDT
(In reply to comment #15)
> So far, Nix needs such viewport information on its API layer so the application can use that information to implement whatever is the way it wants to interact with the page. We don't use the PageViewportController as the other CoordinatedGraphics ports do.
> 
> However, currently WKViewClient is exposing a WKViewportAttribute which is useless for us without something like what I'm proposing in https://bugs.webkit.org/show_bug.cgi?id=115871 .

Let say I were to expose the way Transforms are applied to the API. That way each embedded could decide how to apply matrices to coordinates. Would you say it is a good idea?

This is how I see exposing viewport attribute. The input and computation should not matter for embedders and WebKit should be able to rely on deterministic properties given a certain input. Thus we should exposed the computed results, not a raw input.
Comment 17 Jesus Sanchez-Palencia 2013-05-10 15:08:08 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > So far, Nix needs such viewport information on its API layer so the application can use that information to implement whatever is the way it wants to interact with the page. We don't use the PageViewportController as the other CoordinatedGraphics ports do.
> > 
> > However, currently WKViewClient is exposing a WKViewportAttribute which is useless for us without something like what I'm proposing in https://bugs.webkit.org/show_bug.cgi?id=115871 .
> 
> Let say I were to expose the way Transforms are applied to the API. That way each embedded could decide how to apply matrices to coordinates. Would you say it is a good idea?

Well, funny enough this is something that we also do indeed: https://github.com/WebKitNix/webkitnix/blob/master/Source/WebKit2/UIProcess/API/nix/NIXView.h#L103