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.
Created attachment 80070 [details] Patch
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().
(hum, and do not forget the Qt keyword :))
> 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.
(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.
> 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.
Created attachment 80636 [details] Patch v2
(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.
(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 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.
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 :)
Committed r77253: <http://trac.webkit.org/changeset/77253>
(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