Bug 53090

Summary: [Qt][WK2] Add a way to use shared process model in MiniBrowser
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, eric, ossy, s.mathur
Priority: P5 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch v2 none

Description Balazs Kelemen 2011-01-25 09:14:16 PST
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.
Comment 1 Balazs Kelemen 2011-01-25 09:37:56 PST
Created attachment 80070 [details]
Patch
Comment 2 Benjamin Poulain 2011-01-28 18:11:00 PST
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().
Comment 3 Benjamin Poulain 2011-01-28 18:11:13 PST
(hum, and do not forget the Qt keyword :))
Comment 4 Balazs Kelemen 2011-01-30 14:53:20 PST
> 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.
Comment 5 Benjamin Poulain 2011-01-30 15:00:24 PST
(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.
Comment 6 Balazs Kelemen 2011-01-31 06:03:27 PST
> 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.
Comment 7 Balazs Kelemen 2011-01-31 06:12:14 PST
Created attachment 80636 [details]
Patch v2
Comment 8 Benjamin Poulain 2011-01-31 08:10:00 PST
(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.
Comment 9 Balazs Kelemen 2011-01-31 08:31:40 PST
(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.
Comment 10 Andreas Kling 2011-02-01 08:23:23 PST
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.
Comment 11 Balazs Kelemen 2011-02-01 09:00:12 PST
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 :)
Comment 12 Balazs Kelemen 2011-02-01 09:09:43 PST
Committed r77253: <http://trac.webkit.org/changeset/77253>
Comment 13 Balazs Kelemen 2011-02-03 01:35:13 PST
(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