Bug 53090 – [Qt][WK2] Add a way to use shared process model in MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Balazs Kelemen]

Currently every new window creates a new QWKContext that creates and starts a new web process
for the window. We should be able to test the n-window/tab-1-process model as well especially as
this is the default on mac and win.

[Balazs Kelemen]

Created attachment 80070 [details]
Patch

[Benjamin Poulain]

I do not understand. (In reply to comment #0)
> Currently every new window creates a new QWKContext that creates and starts a new web process
> for the window. We should be able to test the n-window/tab-1-process model as well especially as
> this is the default on mac and win.

I do not understand. You are saying "Currently every new window creates a new QWKContext", which is actually what your patch does, not what I see from the previous code.

I think the patch is also incomplete, you do not include creating a window through newPageFunction().

[Benjamin Poulain]

(hum, and do not forget the Qt keyword :))

[Balazs Kelemen]

> I do not understand. You are saying "Currently every new window creates a new QWKContext", which is actually what your patch does, not what I see from the previous code.

Because it is so confusing :)
Default value of the QWKContext* argument of BrowserWindow's ctor is 0.
Currently every instantiation use the default value. The pointer is then 
passed to BrowserView's ctor that actually creates a new context if got 0
argument (and leaks it). So we are creating a new QWKContext on every 
BrowserWindow  instantiation. With the patch the context of the first window
is reused in the newWindow method by default so this context is shared between
all windows.

> 
> I think the patch is also incomplete, you do not include creating a window through newPageFunction().

The page (and window) that is created by this function must use the same context as it's creator.
This is called as a PageClient virtual method from inside the lib according to a window.open() js call
or something similar so the new page must be scriptable from the creator page.

[Benjamin Poulain]

(In reply to comment #4)
> > I think the patch is also incomplete, you do not include creating a window through newPageFunction().
> 
> The page (and window) that is created by this function must use the same context as it's creator.
> This is called as a PageClient virtual method from inside the lib according to a window.open() js call
> or something similar so the new page must be scriptable from the creator page.

It is also used by "open link in new window" in the context menu, in which case the page has no reference to the new window.

[Balazs Kelemen]

> It is also used by "open link in new window" in the context menu, in which case the page has no reference to the new window.

Yep. Seems like we need a way to differentiate between these types of new window creation.
Do you think it is necessary for this patch? Maybe we should rather fix that in a follow-up patch.
By the way there is another problem with the patch, namely the leaking of the first created context.
I am going to upload a new one that fixing that issue.

[Balazs Kelemen]

Created attachment 80636 [details]
Patch v2

[Benjamin Poulain]

(In reply to comment #6)
> Do you think it is necessary for this patch? Maybe we should rather fix that in a follow-up patch.

My comment was not against this change, I just mentioned another problem I know about. The best in my opinion would be to fix that other problem first, then do this patch on top so you have one consistent fix. But if it is easier for you to split that in 3 patches, no problem with me.

[Balazs Kelemen]

(In reply to comment #8)
> (In reply to comment #6)
> > Do you think it is necessary for this patch? Maybe we should rather fix that in a follow-up patch.
> 
> My comment was not against this change, I just mentioned another problem I know about. The best in my opinion would be to fix that other problem first, then do this patch on top so you have one consistent fix. But if it is easier for you to split that in 3 patches, no problem with me.

The fix of that other problem can be a longer story because that is platform independent and needs some change in the core.
So yes, I would better like to finish with this patch first.

[Andreas Kling]

Comment on attachment 80636 [details]
Patch v2

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

r=me

> Tools/MiniBrowser/qt/main.cpp:48
> +    if ((indexOfSeparateWebProcessOption = args.indexOf(QRegExp(QLatin1String("-separate-web-process-per-window")))) != -1) {

No need for the QRegExp() here.

[Balazs Kelemen]

Note: in -separate-web-process-per-window we are crashing when closing one of the windows (if there
are multiple windows). This is happening because of a known bug: 
https://bugs.webkit.org/show_bug.cgi?id=51115. Because the bug is not caused by this patch I am going
to land it. At least now we have a reliable way to reproduce the crash :)

[Balazs Kelemen]

Committed r77253: <http://trac.webkit.org/changeset/77253>

[Balazs Kelemen]

(In reply to comment #6)
> > It is also used by "open link in new window" in the context menu, in which case the page has no reference to the new window.
> 
> Yep. Seems like we need a way to differentiate between these types of new window creation.
> Do you think it is necessary for this patch? Maybe we should rather fix that in a follow-up patch.
> By the way there is another problem with the patch, namely the leaking of the first created context.
> I am going to upload a new one that fixing that issue.

Tried an approach to fix this but Sam pointed out that the non-shared
process model is not yet supported really: https://bugs.webkit.org/show_bug.cgi?id=53597