Bug 108473 – [Qt][WK2] Fold QtWebPageLoadClient into QQuickWebViewPrivate and move to C API.

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

[Michael Brüning]

SSIA, this is part of cleaning up the Qt classes to not use WK2 internals directly.

[Michael Brüning]

This is blocked a bit at the moment by the strong dependency on QQuickWebView / QQuickWebViewPrivate, i.e., it needs to include files from the UIProcess/API/qt, but we remove those in WebKit2QML.pri...

[Simon Hausmann]

(In reply to comment #1)
> This is blocked a bit at the moment by the strong dependency on QQuickWebView / QQuickWebViewPrivate, i.e., it needs to include files from the UIProcess/API/qt, but we remove those in WebKit2QML.pri...

We may have to clean up qquickwebview_p.h/_p_p.h to allow for inclusion from there, i.e. use forward declarations, etc.

An alternate approach would be to fold things back, like in bug #108920.

[Michael Brüning]

Created attachment 186619 [details]
Patch

[Michael Brüning]

Adding Benjamin and Andreas to check for WK2.

[Simon Hausmann]

Comment on attachment 186619 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:411
> +void QQuickWebViewPrivate::didStartProvisionalLoadForFrame(WKPageRef, WKFrameRef frame, WKTypeRef, const void* clientInfo)
> +{
> +    if (!WKFrameIsMainFrame(frame))
> +        return;
> +
> +    WKRetainPtr<WKURLRef> url  = adoptWK(WKFrameCopyProvisionalURL(frame));
> +    toQQuickWebViewPrivate(clientInfo)->provisionalLoadDidStart(toWTFString(url.get()));
> +}

I think the idea was to avoid the forwarding calls altogether and instead make the actual handling functions in QQuickWebViewPrivate static. Like in bug #108920

[Michael Brüning]

Created attachment 186633 [details]
Patch

[Michael Brüning]

Comment on attachment 186633 [details]
Patch

Just noticed a cut and paste bug.

[Michael Brüning]

Created attachment 186636 [details]
Patch

[Benjamin Poulain]

Comment on attachment 186636 [details]
Patch

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

I sign off on this for WebKit2.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:317
> -    pageFindClient.reset(new QtWebPageFindClient(webPage.get(), q_ptr));
> -    pageLoadClient.reset(new QtWebPageLoadClient(webPage.get(), q_ptr));
> +    registerPageLoadClient();
> +    pageFindClient.reset(new QtWebPageFindClient(webPage.get(), q_ptr));    

Some extra spaces here.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:391
> +    // We set the unreachable url unconditionally so that the current
> +    // active url of the webview when the loadingChanged signal is
> +    // emitted reflects the failed url, not the previously committed
> +    // url. This also ensures that if the user does not do a loadHtml
> +    // with an error page and and unreachable url as a reponse to the
> +    // failed load, we can still detect the failed url for reloads.
> +
> +    // FIXME: Find a way to do this with the C API / add a function to do this to the C API
> +    toImpl(frame)->setUnreachableURL(error.url());

I don't understand why any of this is needed :(

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:440
> +    PageViewportController* pvc = webViewPrivate->viewportController();
> +    if (pvc)
> +        pvc->didCommitLoad();

Let's use a better variable name here: pageViewportController

[Michael Brüning]

Thanks for the sign-off :).

(In reply to comment #9)
> (From update of attachment 186636 [details])
[...]
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:391
> > +    // We set the unreachable url unconditionally so that the current
> > +    // active url of the webview when the loadingChanged signal is
> > +    // emitted reflects the failed url, not the previously committed
> > +    // url. This also ensures that if the user does not do a loadHtml
> > +    // with an error page and and unreachable url as a reponse to the
> > +    // failed load, we can still detect the failed url for reloads.
> > +
> > +    // FIXME: Find a way to do this with the C API / add a function to do this to the C API
> > +    toImpl(frame)->setUnreachableURL(error.url());
> 
> I don't understand why any of this is needed :(

It's used to return the URL that has failed to load to the QQuickWebView / it's clients. Since the load fails while it was still not committed, the url that we get from WebPageProxy::activeURL at the time this load fail is dispatched is still the previous one, unless we set the unreachableURL. I'll look into getting this solved so we can see if it can be solved a different way as we progress with the cleanup.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:440
> > +    PageViewportController* pvc = webViewPrivate->viewportController();
> > +    if (pvc)
> > +        pvc->didCommitLoad();
> 
> Let's use a better variable name here: pageViewportController
True :) I copied this from the old PageLoadClient, but better variable names never hurt anyone :).

[Michael Brüning]

Created attachment 186817 [details]
Patch

[Simon Hausmann]

Comment on attachment 186817 [details]
Patch

r=me

Looks like didFailLoadWithErrorForFrame, didFailProvisionalLoadWithErrorForFrame and dispatchLoadFailed could be merged into one static function though.

[Jocelyn Turcotte]

Comment on attachment 186817 [details]
Patch

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

> Source/WebKit2/ChangeLog:15
> +        the current C API and adds FIXMEs to the places that need to be fixed in this part
> +        of the API still.

I would use FIXMEs here since it would be pretty straightforward to just look for references to internal Web* types.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:316
> +    registerPageLoadClient();

Please follow the same style as bug #108920, or convince Simon to use yours :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:397
> +static QQuickWebViewPrivate* toQQuickWebViewPrivate(const void* clientInfo)

static inline please

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:436
> +    QQuickWebViewPrivate* webViewPrivate = toQQuickWebViewPrivate(clientInfo);

This could be called "d"

[Jocelyn Turcotte]

(In reply to comment #13)
> I would use FIXMEs here since it would be pretty straightforward to just look for references to internal Web* types.
*wouldn't*

[Simon Hausmann]

Comment on attachment 186817 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:397
>> +static QQuickWebViewPrivate* toQQuickWebViewPrivate(const void* clientInfo)
> 
> static inline please

Or remove the function altogether when landing because I just landed http://trac.webkit.org/changeset/142073 :)

[Michael Brüning]

(In reply to comment #14)
> (In reply to comment #13)
> > I would use FIXMEs here since it would be pretty straightforward to just look for references to internal Web* types.
> *wouldn't*

I can remove them of course, I just thought it would be a good reminder as in both cases we either need to add C API to keep the functionality and apart from those two, the classes are already cleaned up. But it's true that we can just as well grep the API for the remaining occurences of toImpl ...

[Michael Brüning]

Created attachment 187052 [details]
Patch

[Michael Brüning]

Committed r142095: <http://trac.webkit.org/changeset/142095>