Bug 84527 – [Qt][WK2] Define clear split between QtWebPageLoadClient and QQuickWebViewPrivate for loading tasks.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[zalan]

Right now QtWebPageLoadClient does 4 different things
1, emits signals on webview: emit m_webView->loadingChanged(&loadRequest);
2, calls into QQuickWebViewPrivate. m_webView->d_func()->loadDidCommit(); where there's no implementation whatsoever.
3, calls into QQuickWebViewPrivate. m_webView->d_func()->loadDidSucceed(); where the implementation could be completely omitted.
4, calls into QQuickWebViewPrivate: m_webView->d_func()->didChangeBackForwardList(); where there's some proxy implementation.

They should be harmonized in some ways, either push back more implementation to QtWebPageLoadClient so that it needs no access to QQuickWebViewPrivate, or just use it truly as a proxy class. Right now it's a mixture.

(case 3, loadDidSucceed() implements some deferred loading success dispatching due to the delayed interaction engine construction. I looked at the original patch (bug 77111) and don't see any reasoning why success() is special in that case, and for example failed() is not. Commit log also states that page is suspended while I don't think it is, it's just that the suspend flag is turned on by default, so not sure why success() is treated differently here, unless I miss something, which could very well be the case. If this delaying logic could be omitted, we would end up even less d_func() calls in QtWebPageLoadClient(), provided that's the preferred direction.)

[zalan]

Simon, do you know something about case#3? (as Andras is being on vacation for a week)

[Simon Hausmann]

(In reply to comment #0)
[...]
> 3, calls into QQuickWebViewPrivate. m_webView->d_func()->loadDidSucceed(); where the implementation could be completely omitted.
[...] 
> (case 3, loadDidSucceed() implements some deferred loading success dispatching due to the delayed interaction engine construction. I looked at the original patch (bug 77111) and don't see any reasoning why success() is special in that case, and for example failed() is not. Commit log also states that page is suspended while I don't think it is, it's just that the suspend flag is turned on by default, so not sure why success() is treated differently here, unless I miss something, which could very well be the case. If this delaying logic could be omitted, we would end up even less d_func() calls in QtWebPageLoadClient(), provided that's the preferred direction.)

I don't see much magic in loadDidSucceed apart from a straight signal emission.

[Andras Becsi]

(In reply to comment #0) 
> (case 3, loadDidSucceed() implements some deferred loading success dispatching due to the delayed interaction engine construction. I looked at the original patch (bug 77111) and don't see any reasoning why success() is special in that case, and for example failed() is not. Commit log also states that page is suspended while I don't think it is, it's just that the suspend flag is turned on by default, so not sure why success() is treated differently here, unless I miss something, which could very well be the case. If this delaying logic could be omitted, we would end up even less d_func() calls in QtWebPageLoadClient(), provided that's the preferred direction.)

This delay was needed before the WebView became a subclass of Flickable because the instantiation of the internal Flickable happened in onComponentComplete and we needed to defer the emission of the success signal after the construction finished to prevent crashes in API tests, since almost all the unit tests depend on loadSuccess.

This delay can be omitted since WebView is a direct subclass of Flickable now.

[zalan]

09:58:23 AM) simon: but I for one like the idea of removing unnecessary layers :)
(09:59:00 AM) zalan: simon: yea, it's the balance of having a bloated webview vs. layering.
(09:59:27 AM) simon: I suppose we could do it in two steps, make it a pure proxy first, and then move the rest

as discussed with Simon, as the first step, QtWebPageLoadClient will be transformed into a pure proxy class.

[zalan]

Created attachment 140717 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 140717 [details]
Patch

Question : why? We separated this stuff back then to avoid QQuickWebView private to increase too much. The idea of QtWebPageLoadClient was to contain all the loading logic. Plus your changelog doesn't say the motivation of this move.

[Alexis Menard (darktears)]

(In reply to comment #6)
> (From update of attachment 140717 [details])
> Question : why? We separated this stuff back then to avoid QQuickWebView private to increase too much. The idea of QtWebPageLoadClient was to contain all the loading logic. Plus your changelog doesn't say the motivation of this move.

Ok got a bit of info on the bug history.

I would advocate to push the logic into QtWebPageLoadClient and keep the QQuickWebView private for stuff related to QQuickItem usage that have no other place to go but the private pointer QQuickWebViewPrivate.

[zalan]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 140717 [details] [details])
> > Question : why? We separated this stuff back then to avoid QQuickWebView private to increase too much. The idea of QtWebPageLoadClient was to contain all the loading logic. Plus your changelog doesn't say the motivation of this move.
> 
> Ok got a bit of info on the bug history.
> 
> I would advocate to push the logic into QtWebPageLoadClient and keep the QQuickWebView private for stuff related to QQuickItem usage that have no other place to go but the private pointer QQuickWebViewPrivate.

That was my original intention (as it can been seen here bug 84445), but discussing with Simon and Tor Arne, it ended up the other way around. As quoted here https://bugs.webkit.org/show_bug.cgi?id=84527#c4
I can see both directions' advantages and disadvantages.

[Caio Marcelo de Oliveira Filho]

I've made some of the commits that split that code. The main motivation was to break QtWebPageProxy class into smaller pieces. The QtWebPageProxy class handled all the C clients plus the C++ client code (like implementing PageClient interface) and was pretty confusing.

One drawback of making those classes just proxies to QQuickWebViewPrivate is the danger of recreating the big blob we had before. At the time the split was done I was imagining that we could make use of it to separate the Loader code in one place, Policy code in another, etc. Maybe the existing division isn't perfect, but we could try.

One example of this way of thinking: I wonder if url()/setUrl()/related could be moved inside the small class and make QQuickWebViewPrivate free of the complexity of handling those things (if now looks simple, remember we have bug 77554).

Maybe making the boring C client code (static methods that call the instance methods) less visible with macros or something else. Then handle everything on the instance methods (of the smaller classes) can make things less confusing in those small classes.

[Simon Hausmann]

(In reply to comment #9)
> I've made some of the commits that split that code. The main motivation was to break QtWebPageProxy class into smaller pieces. The QtWebPageProxy class handled all the C clients plus the C++ client code (like implementing PageClient interface) and was pretty confusing.
> 
> One drawback of making those classes just proxies to QQuickWebViewPrivate is the danger of recreating the big blob we had before. At the time the split was done I was imagining that we could make use of it to separate the Loader code in one place, Policy code in another, etc. Maybe the existing division isn't perfect, but we could try.
> 
> One example of this way of thinking: I wonder if url()/setUrl()/related could be moved inside the small class and make QQuickWebViewPrivate free of the complexity of handling those things (if now looks simple, remember we have bug 77554).

So there's a fear of bloating QQuickWebView, but I have to ask: What is that bloat exactly? What is that complexity that is required?


I believe we should strive towards keeping our API layer as thin as possible with as little logic and complexit as possible on top of what the internal WebKit2 API provides. If there is an actual logic, and actual behavior or algorithm for us to implement, then I think it's fine to have that in a separate encapsulated class. But that's not the case here, it's not the case for any of the loading state business nor the url() API, where we want to merely expose WebPageProxy::activeUrl().


Note: In WK1 we actually implemented a lot of things in QWebPage that for us are already implemented in WebPageProxy. That made it seem like it's bloated, because it implemented API _and_ semantics that are required between  ChromeClient/FrameLoaderClient and any sensible public API layer.


> Maybe making the boring C client code (static methods that call the instance methods) less visible with macros or something else. Then handle everything on the instance methods (of the smaller classes) can make things less confusing in those small classes.

The C to C++ code is something that indeed can be improved with macros or templates. Tor Arne is looking into that. Right now there are cases where the C callback actually contains _logic_ before it calls the C++ function, and that doesn't feel right.

[Simon Hausmann]

Comment on attachment 140717 [details]
Patch

I'm r+'ing this patch because it moves code that implements an API right where the API is defined (signal emission as well as translation of WebPageProxy internals to public Qt API with correct types).

[zalan]

Created attachment 140908 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140908 [details]
Patch

Clearing flags on attachment: 140908

Committed r116516: <http://trac.webkit.org/changeset/116516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.