Bug 110741 - [WK2][EFL] WebView: Add callbacks to the WKViewClient to remove direct access to page viewport controller
Summary: [WK2][EFL] WebView: Add callbacks to the WKViewClient to remove direct access...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 110463
Blocks: 107657 107662 111401 111543
  Show dependency treegraph
 
Reported: 2013-02-25 02:13 PST by Mikhail Pozdnyakov
Modified: 2013-04-08 07:48 PDT (History)
10 users (show)

See Also:


Attachments
patch (11.42 KB, patch)
2013-02-25 04:47 PST, Mikhail Pozdnyakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch v2 (11.40 KB, patch)
2013-02-26 00:23 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v3 (11.96 KB, patch)
2013-04-08 07:39 PDT, Mikhail Pozdnyakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2013-02-25 02:13:07 PST
SSIA. WKViewClient interface shall be extended.
Comment 1 Mikhail Pozdnyakov 2013-02-25 04:47:24 PST
Created attachment 190036 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2013-02-25 05:59:14 PST
Comment on attachment 190036 [details]
patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:56
> +    WKViewCallback                                   pageTransitionViewportReady;

Maybe we could find a better name that users could actually understand :-) I have no suggestion though.

> Source/WebKit2/UIProcess/efl/WebViewClient.h:48
> +    void pageTransitionViewportReady(WebView*);

When exactly does this happen? Isnt it when the new page is ready to render something?

Something with did?
Comment 3 Build Bot 2013-02-25 06:35:35 PST
Comment on attachment 190036 [details]
patch

Attachment 190036 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16745259

New failing tests:
svg/zoom/page/zoom-mask-with-percentages.svg
Comment 4 Mikhail Pozdnyakov 2013-02-26 00:13:19 PST
(In reply to comment #2)
> (From update of attachment 190036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190036&action=review
> 
> > Source/WebKit2/UIProcess/API/C/efl/WKView.h:56
> > +    WKViewCallback                                   pageTransitionViewportReady;
> 
> Maybe we could find a better name that users could actually understand :-) I have no suggestion though.
> 
> > Source/WebKit2/UIProcess/efl/WebViewClient.h:48
> > +    void pageTransitionViewportReady(WebView*);
> 
> When exactly does this happen? Isnt it when the new page is ready to render something?
yeah
> 
> Something with did?
I guess we could take after WebPage and use "didCompletePageTransition".
Comment 5 Mikhail Pozdnyakov 2013-02-26 00:23:14 PST
Created attachment 190219 [details]
patch v2

WebViewClient pageTransitionViewportReady -> didCompletePageTransition.
Comment 6 Kenneth Rohde Christiansen 2013-02-26 03:41:07 PST
LGTM
Comment 7 Kenneth Rohde Christiansen 2013-03-20 04:53:29 PDT
Comment on attachment 190219 [details]
patch v2

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:44
> +typedef void (*WKViewPageDidRequestScrollCallback)(WKViewRef view, WKPoint position, const void* clientInfo);

Wouldn't something like didChangePosition not make more sense?
Comment 8 Caio Marcelo de Oliveira Filho 2013-03-20 11:05:23 PDT
Comment on attachment 190219 [details]
patch v2

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

>> Source/WebKit2/UIProcess/API/C/efl/WKView.h:44
>> +typedef void (*WKViewPageDidRequestScrollCallback)(WKViewRef view, WKPoint position, const void* clientInfo);
> 
> Wouldn't something like didChangePosition not make more sense?

I think the DidRequestScroll is fine. The scroll was requested, the view user may decide to do different things besides setting the position like not scrolling at all, or scroll the page in an animated fashion. While WKView shouldn't automatically handle those things, it's fine for the EWK specific view to handle them.
Comment 9 Kenneth Rohde Christiansen 2013-03-20 11:07:31 PDT
(In reply to comment #8)
> (From update of attachment 190219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190219&action=review
> 
> >> Source/WebKit2/UIProcess/API/C/efl/WKView.h:44
> >> +typedef void (*WKViewPageDidRequestScrollCallback)(WKViewRef view, WKPoint position, const void* clientInfo);
> > 
> > Wouldn't something like didChangePosition not make more sense?
> 
> I think the DidRequestScroll is fine. The scroll was requested, the view user may decide to do different things besides setting the position like not scrolling at all, or scroll the page in an animated fashion. While WKView shouldn't automatically handle those things, it's fine for the EWK specific view to handle them.

The problem is that in some cases we automatically adjust the position on the web process site, so it is not really a request.
Comment 10 Caio Marcelo de Oliveira Filho 2013-03-20 11:27:55 PDT
> The problem is that in some cases we automatically adjust the position on the web process site, so it is not really a request.

Which cases?
Comment 11 Mikhail Pozdnyakov 2013-04-08 06:42:05 PDT
(In reply to comment #10)
> > The problem is that in some cases we automatically adjust the position on the web process site, so it is not really a request.
> 
> Which cases?

For instance when viewport attributes are changing, please take a look at WebPage::sendViewportAttributesChanged.
Comment 12 Mikhail Pozdnyakov 2013-04-08 07:39:13 PDT
Created attachment 196856 [details]
patch v3

Rebased. didRequestScroll -> didChangeContentsPosition
Comment 13 Mikhail Pozdnyakov 2013-04-08 07:48:34 PDT
Committed r147913: <http://trac.webkit.org/changeset/147913>