Bug 71559 – [Qt] Setting QWebPreferences affect multiple WebViews

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

[Caio Marcelo de Oliveira Filho]

Created attachment 113653 [details]
Example where autoloadimages preference is set in one webview but applies to the other too

The QWebPreferences are per WebView, but the actual implementation in WebKit2 port make them per page group and currently we only use one page group for all the views.

[Jesus Sanchez-Palencia]

Perhaps what we need here is to have one pagegroup per view.

[Jesus Sanchez-Palencia]

Created attachment 119039 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 119039 [details]
Patch

At least without exposing the concept of page group and context in the API(that we haven't yet figure out, and most probably won't before 5.0) it seems to me that it is the most sensible approach to take. Hopefully there is no dragon here.

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (From update of attachment 119039 [details])
> At least without exposing the concept of page group and context in the API(that we haven't yet figure out, and most probably won't before 5.0) it seems to me that it is the most sensible approach to take. Hopefully there is no dragon here.

I think we need to actually do that as well as security origin

[Jesus Sanchez-Palencia]

(In reply to comment #4)
> I think we need to actually do that as well as security origin

What do you mean, Kenneth?

[Simon Hausmann]

Comment on attachment 119039 [details]
Patch

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

Looks good, but needs a rebase :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:69
> +    if (!pageGroupRef)
> +        pageGroupRef = toAPI(WebPageGroup::create().leakRef());

I think that's okay, but this patch is going to need a rebase, and that will make it nicer :) . I suspect that after a rebase the code will look like this:

RefPtr<WebPageGroup> pageGroup;
if (pageGroupRef)
    pageGroup = toImpl(pageGroupRef);
else
    pageGroup = WebPageGroup::create();

... foo(pageGroup.get());

[Jesus Sanchez-Palencia]

Created attachment 119906 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 119906 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [Qt] QWebPreferences are leaking to other WebViews

leaking sounds like a mem leak. I would really like you do use different wording.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_preferences.qml:79
> +            function test_preferencesNotLeakingBetweenViews() {

I wouldnt really call that leaking. That is not the definition that people are used to. test_preferencesAffectsCurrentViewOnly()

[Jesus Sanchez-Palencia]

Created attachment 119914 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119914 [details]
Patch

Clearing flags on attachment: 119914

Committed r103275: <http://trac.webkit.org/changeset/103275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.