RESOLVED FIXED 110741
[WK2][EFL] WebView: Add callbacks to the WKViewClient to remove direct access to page viewport controller 
https://bugs.webkit.org/show_bug.cgi?id=110741
Summary [WK2][EFL] WebView: Add callbacks to the WKViewClient to remove direct access...
Mikhail Pozdnyakov
Reported 2013-02-25 02:13:07 PST
SSIA. WKViewClient interface shall be extended.
Attachments
patch (11.42 KB, patch)
2013-02-25 04:47 PST, Mikhail Pozdnyakov 		buildbot: commit-queue-
DetailsFormatted DiffDiff
patch v2 (11.40 KB, patch)
2013-02-26 00:23 PST, Mikhail Pozdnyakov 		no flags
DetailsFormatted DiffDiff
patch v3 (11.96 KB, patch)
2013-04-08 07:39 PDT, Mikhail Pozdnyakov 		andersca: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mikhail Pozdnyakov
Comment 1 2013-02-25 04:47:24 PST
Created attachment 190036 [details] patch
Kenneth Rohde Christiansen
Comment 2 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?
Build Bot
Comment 3 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
Mikhail Pozdnyakov
Comment 4 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".
Mikhail Pozdnyakov
Comment 5 2013-02-26 00:23:14 PST
Created attachment 190219 [details] patch v2 WebViewClient pageTransitionViewportReady -> didCompletePageTransition.
Kenneth Rohde Christiansen
Comment 6 2013-02-26 03:41:07 PST
LGTM
Kenneth Rohde Christiansen
Comment 7 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?
Caio Marcelo de Oliveira Filho
Comment 8 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.
Kenneth Rohde Christiansen
Comment 9 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.
Caio Marcelo de Oliveira Filho
Comment 10 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?
Mikhail Pozdnyakov
Comment 11 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.
Mikhail Pozdnyakov
Comment 12 2013-04-08 07:39:13 PDT
Created attachment 196856 [details] patch v3 Rebased. didRequestScroll -> didChangeContentsPosition
Mikhail Pozdnyakov
Comment 13 2013-04-08 07:48:34 PDT
Committed r147913: <http://trac.webkit.org/changeset/147913>
Note You need to log in before you can comment on or make changes to this bug.