Bug 84527 - [Qt][WK2] Define clear split between QtWebPageLoadClient and QQuickWebViewPrivate for loading tasks.
Summary: [Qt][WK2] Define clear split between QtWebPageLoadClient and QQuickWebViewPri...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Minor
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-21 03:51 PDT by zalan
Modified: 2012-05-09 05:10 PDT (History)
7 users (show)

See Also:


Attachments
Patch (17.66 KB, patch)
2012-05-08 07:27 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (17.84 KB, patch)
2012-05-09 04:13 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2012-04-21 03:51:38 PDT
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.)
Comment 1 zalan 2012-04-27 09:15:48 PDT
Simon, do you know something about case#3? (as Andras is being on vacation for a week)
Comment 2 Simon Hausmann 2012-04-30 01:53:21 PDT
(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.
Comment 3 Andras Becsi 2012-05-03 01:24:10 PDT
(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.
Comment 4 zalan 2012-05-04 02:06:36 PDT
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.
Comment 5 zalan 2012-05-08 07:27:26 PDT
Created attachment 140717 [details]
Patch
Comment 6 Alexis Menard (darktears) 2012-05-08 07:36:08 PDT
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.
Comment 7 Alexis Menard (darktears) 2012-05-08 07:41:05 PDT
(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.
Comment 8 zalan 2012-05-08 07:55:59 PDT
(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.
Comment 9 Caio Marcelo de Oliveira Filho 2012-05-08 13:02:46 PDT
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.
Comment 10 Simon Hausmann 2012-05-09 00:35:33 PDT
(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.
Comment 11 Simon Hausmann 2012-05-09 00:37:46 PDT
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).
Comment 12 zalan 2012-05-09 04:13:24 PDT
Created attachment 140908 [details]
Patch
Comment 13 WebKit Review Bot 2012-05-09 05:10:32 PDT
Comment on attachment 140908 [details]
Patch

Clearing flags on attachment: 140908

Committed r116516: <http://trac.webkit.org/changeset/116516>
Comment 14 WebKit Review Bot 2012-05-09 05:10:38 PDT
All reviewed patches have been landed.  Closing bug.