Bug 108952 – [Qt][WK2] Replace use of WebPreferences with use of WKPrefences C API

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

[Michael Brüning]

Most of this is used in QWebPreferences and QQuickWebView*...

[Michael Brüning]

Created attachment 186865 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 186865 [details]
Patch

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

I have a couple of comments.

Would "int(x)" syntax work as good as "static_cast<int>(x)" but with less noise?

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:187
> +        WKPreferencesSetStandardFontFamily(preferencesRef(), WKStringCreateWithQString(family));

I think these WKStringRefs leaking here and below.

[Michael Brüning]

(In reply to comment #2)
> (From update of attachment 186865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186865&action=review

Thanks for the comments, Caio!
> 
> I have a couple of comments.
> 
> Would "int(x)" syntax work as good as "static_cast<int>(x)" but with less noise?
I'd rather not use the function-style cast since it is not explicitly defined which cast type will be used and static_cast does not have this ambiguity.

> 
> > Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:187
> > +        WKPreferencesSetStandardFontFamily(preferencesRef(), WKStringCreateWithQString(family));
> 
> I think these WKStringRefs leaking here and below.
True, good catch, thanks :).

[Michael Brüning]

Created attachment 187098 [details]
Patch

[Early Warning System Bot]

Comment on attachment 187098 [details]
Patch

Attachment 187098 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16430185

[Michael Brüning]

Created attachment 187107 [details]
Patch

Fixed leaks by using adoptWK(xyz).get().

[Caio Marcelo de Oliveira Filho]

LGTM.

To add those WK2 C APIs you need feedback from Apple developers.

[Michael Brüning]

Dear Benjamin, could I ask you to please have a look once again?

[Benjamin Poulain]

Can you please split the patch in two. One using the existing APIs, and create a second bug for the new C APIs?

The change from WebKit::WebPreferences to WKPreferencesRef is great. I can review that quickly for all the existing APIs.

Adding the API is against our goal of diminishing the current mess. I'll have to look carefully at that part and possibly ask it to be Qt specific.

[Michael Brüning]

Created attachment 187275 [details]
Patch

[Michael Brüning]

Created attachment 187279 [details]
Patch

[Simon Hausmann]

Comment on attachment 187107 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPreferences.h:242
> +// Defaults to false.
> +WK_EXPORT bool WKPreferencesGetForceCompositingMode(WKPreferencesRef preferencesRef);
> +WK_EXPORT void WKPreferencesSetForceCompositingMode(WKPreferencesRef preferencesRef, bool enabled);
> +
> +// Defaults to false.
> +WK_EXPORT bool WKPreferencesGetScrollAnimatorEnabled(WKPreferencesRef preferencesRef);
> +WK_EXPORT void WKPreferencesSetScrollAnimatorEnabled(WKPreferencesRef preferencesRef, bool enabled);
> +

Let's just not expose these in the "QML" preferences API and then we also don't need them here at all.

> Source/WebKit2/UIProcess/API/C/WKPreferences.h:253
> +WK_EXPORT uint32_t WKPreferencesGetLayoutFallbackWidth(WKPreferencesRef preferencesRef);
> +WK_EXPORT void WKPreferencesSetLayoutFallbackWidth(WKPreferencesRef preferencesRef, uint32_t width);
> +
> +// Defaults to 0.
> +WK_EXPORT uint32_t WKPreferencesGetDeviceWidth(WKPreferencesRef preferencesRef);
> +WK_EXPORT void WKPreferencesSetDeviceWidth(WKPreferencesRef preferencesRef, uint32_t width);
> +
> +// Defaults to 0.
> +WK_EXPORT uint32_t WKPreferencesGetDeviceHeight(WKPreferencesRef preferencesRef);
> +WK_EXPORT void WKPreferencesSetDeviceHeight(WKPreferencesRef preferencesRef, uint32_t height);

We might not need those public either if instead it's just the view that uses them (the rawwebview in the current design).

[Michael Brüning]

(In reply to comment #12)
> (From update of attachment 187107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187107&action=review
> 
> > +// Defaults to false.
> > +WK_EXPORT bool WKPreferencesGetScrollAnimatorEnabled(WKPreferencesRef preferencesRef);
> > +WK_EXPORT void WKPreferencesSetScrollAnimatorEnabled(WKPreferencesRef preferencesRef, bool enabled);
> > +
At least this one is currently exposed to QWebPreferences - should we remove it then?
> 
> Let's just not expose these in the "QML" preferences API and then we also don't need them here at all.
> 
> > Source/WebKit2/UIProcess/API/C/WKPreferences.h:253
> > +WK_EXPORT uint32_t WKPreferencesGetLayoutFallbackWidth(WKPreferencesRef preferencesRef);
> > +WK_EXPORT void WKPreferencesSetLayoutFallbackWidth(WKPreferencesRef preferencesRef, uint32_t width);
> > +
> > +// Defaults to 0.
> > +WK_EXPORT uint32_t WKPreferencesGetDeviceWidth(WKPreferencesRef preferencesRef);
> > +WK_EXPORT void WKPreferencesSetDeviceWidth(WKPreferencesRef preferencesRef, uint32_t width);
> > +
> > +// Defaults to 0.
> > +WK_EXPORT uint32_t WKPreferencesGetDeviceHeight(WKPreferencesRef preferencesRef);
> > +WK_EXPORT void WKPreferencesSetDeviceHeight(WKPreferencesRef preferencesRef, uint32_t height);
> 
> We might not need those public either if instead it's just the view that uses them (the rawwebview in the current design).
True, they are only used by the view afaics. So let's leave them for now and see what we do with them as part of the move to QRawWebView.

[Kenneth Rohde Christiansen]

Comment on attachment 187107 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:1040
> +void WKPreferencesSetForceCompositingMode(WKPreferencesRef preferencesRef, bool enabled)
> +{
> +    toImpl(preferencesRef)->setForceCompositingMode(enabled);
> +}

These are not generic settings, thus we should not add C API for them. Instead these could either be set when you construct your view or as part of your view API. I would prefer the former. It is basically the same for all these settings you are exposing here.

They are either view related or not something you would want to expose to a app developer.

[Michael Brüning]

Created attachment 187314 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 187314 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:48
>  {
>      switch (attr) {
>      case AutoLoadImages:
> -        return preferences()->loadsImagesAutomatically();
> +        return WKPreferencesGetLoadsImagesAutomatically(preferencesRef());
>  #if ENABLE(FULLSCREEN_API)
>      case FullScreenEnabled:
> -        return preferences()->fullScreenEnabled();
> +        return WKPreferencesGetFullScreenEnabled(preferencesRef());
>  #endif
>      case JavascriptEnabled:

Why don't you not just get the preferencesRef once? then I guess you could avoid that weird getter

[Michael Brüning]

(In reply to comment #16)
> (From update of attachment 187314 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187314&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:48
> >  {
> >      switch (attr) {
> >      case AutoLoadImages:
> > -        return preferences()->loadsImagesAutomatically();
> > +        return WKPreferencesGetLoadsImagesAutomatically(preferencesRef());
> >  #if ENABLE(FULLSCREEN_API)
> >      case FullScreenEnabled:
> > -        return preferences()->fullScreenEnabled();
> > +        return WKPreferencesGetFullScreenEnabled(preferencesRef());
> >  #endif
> >      case JavascriptEnabled:
> 
> Why don't you not just get the preferencesRef once? then I guess you could avoid that weird getter

Could you please elaborate why this getter is "weird"? It is how the preferences reference was retrieved before we moved to the C++ API and changed the getter to the return a WebPreferences pointer. I can change it of course and save a little bit in the binary, I just would like to understand if there is anything else that I have overlooked.

Or did you mean saving the preferences reference in a member variable? I would rather not do that as the C API offers a way to set another preferences object as well.

[Michael Brüning]

Created attachment 187581 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 187581 [details]
Patch

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

I lot nicer

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:573
> -WebKit::WebPreferences* QWebPreferencesPrivate::preferences() const
> +WKPreferencesRef QWebPreferencesPrivate::getPreferencesRef() const
>  {
> -    return webViewPrivate->webPageProxy->pageGroup()->preferences();
> +    return WKPageGroupGetPreferences(WKPageGetPageGroup(webViewPrivate->webPage.get()));

you still really need this?

If you need it outside of this class, isnt it better using it direclty?

[Michael Brüning]

(In reply to comment #19)
> (From update of attachment 187581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187581&action=review
> 
> I lot nicer
> 
> > Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:573
> > -WebKit::WebPreferences* QWebPreferencesPrivate::preferences() const
> > +WKPreferencesRef QWebPreferencesPrivate::getPreferencesRef() const
> >  {
> > -    return webViewPrivate->webPageProxy->pageGroup()->preferences();
> > +    return WKPageGroupGetPreferences(WKPageGetPageGroup(webViewPrivate->webPage.get()));
> 
> you still really need this?
> 
> If you need it outside of this class, isnt it better using it direclty?

Okay, now I see your point. Why didn't you mention that you wanted to use the pageGroup directly ? :)

[Michael Brüning]

Created attachment 187598 [details]
Patch

[Allan Sandfeld Jensen]

Looks good to me. Though if ScrollAnimator is removed as a preference, I personally think we should change it to default enabled to match the other WK2 ports. We have it special cased in WebPreferenceStore.h.

[Simon Hausmann]

Comment on attachment 187598 [details]
Patch

LGTM. Benjamin, can you sign off on this? :)

[Simon Hausmann]

Ping, seeking sign-off from WK2 owners :). This is a fairly straight-forward port from internal preferences to WKPreferences usage.

[Simon Hausmann]

WK2-sign-off-ping :)

[Benjamin Poulain]

Comment on attachment 187598 [details]
Patch

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

I sign off on this, feel free to review for WK2.

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:197
> +    WKPreferencesRef preferencesRef = WKPageGroupGetPreferences(webViewPrivate->pageGroup.get());
>      switch (which) {
>      case StandardFont:
> -        preferences()->setStandardFontFamily(family);
> +        WKPreferencesSetStandardFontFamily(preferencesRef, adoptWK(WKStringCreateWithQString(family)).get());
>          break;
>      case FixedFont:
> -        preferences()->setFixedFontFamily(family);
> +        WKPreferencesSetFixedFontFamily(preferencesRef, adoptWK(WKStringCreateWithQString(family)).get());
>          break;
>      case SerifFont:
> -        preferences()->setSerifFontFamily(family);
> +        WKPreferencesSetSerifFontFamily(preferencesRef, adoptWK(WKStringCreateWithQString(family)).get());
>          break;
>      case SansSerifFont:
> -        preferences()->setSansSerifFontFamily(family);
> +        WKPreferencesSetSansSerifFontFamily(preferencesRef, adoptWK(WKStringCreateWithQString(family)).get());
>          break;
>      case CursiveFont:
> -        preferences()->setCursiveFontFamily(family);
> +        WKPreferencesSetCursiveFontFamily(preferencesRef, adoptWK(WKStringCreateWithQString(family)).get());
>          break;
>      case FantasyFont:
> -        preferences()->setFantasyFontFamily(family);
> +        WKPreferencesSetFantasyFontFamily(preferencesRef, adoptWK(WKStringCreateWithQString(family)).get());

Please move adoptWK(WKStringCreateWithQString(family)) above the switch. Something like:
    WKRetain<WKStringRef> familyRef = adoptWK(WKStringCreateWithQString(family).

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:209
> -        return preferences()->standardFontFamily();
> +        return WKStringCopyQString(adoptWK(WKPreferencesCopyStandardFontFamily(preferencesRef)).get());

A new function adoptToQString(WKStringRef) would be useful here:
    return adoptToQString(WKPreferencesCopyStandardFontFamily(preferencesRef))

[Michael Brüning]

Created attachment 190479 [details]
Patch

Added the adoptToQString method Benjamin suggested.

[Michael Brüning]

Created attachment 190484 [details]
Patch

Moved adoptToQString into the WebKit namespace.

[Michael Brüning]

Committed r144165: <http://trac.webkit.org/changeset/144165>