Bug 51871 – [Qt][WK2] QWKContext should offer the available process models

• Component: WebKit2
• Status: RESOLVED INVALID

[Balazs Kelemen]

QWKContext currently forces using the process model where a view has it's own web process. There is also an available model where all view
in a UI process has one web process and furthermore this was the default before introducing QWKContext and this is still the default on
other platforms. The separated thread model is not supported on Qt right now but it could be in the future. I would add API that makes the
user be able to decide which model to use.

[Balazs Kelemen]

Created attachment 77884 [details]
Patch

[Balazs Kelemen]

Created attachment 78820 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 78820 [details]
Patch

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

> WebKit2/UIProcess/API/qt/qwkcontext.cpp:56
> +    case QWKContext::DedicatedWebprocess:

Dedicated? you mean that we have one per page?  also here you call it Webprocess and not WebProcess. why not just SharedProcess, DedicatedProcessPerPage or so?

> WebKit2/UIProcess/API/qt/qwkcontext.h:37


What is most important here? The parent of the proessmodel? I guess the parent and if so it should be first.

[Balazs Kelemen]

Created attachment 78924 [details]
Patch

[Balazs Kelemen]

(In reply to comment #3)
> (From update of attachment 78820 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78820&action=review
> 
> > WebKit2/UIProcess/API/qt/qwkcontext.cpp:56
> > +    case QWKContext::DedicatedWebprocess:
> 
> Dedicated? you mean that we have one per page?  also here you call it Webprocess and not WebProcess. why not just SharedProcess, DedicatedProcessPerPage or so?

I don't find DedicatedProcessPerPage really good. The process is dedicated to the context actually.
Just DedicatedProcess seems to be better.

> 
> > WebKit2/UIProcess/API/qt/qwkcontext.h:37
> 
> 
> What is most important here? The parent of the proessmodel? I guess the parent and if so it should be first.

Done.

[Balazs Kelemen]

Tried apply to trunk, still applies.

[Andreas Kling]

Comment on attachment 78924 [details]
Patch

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

> Tools/MiniBrowser/qt/BrowserView.cpp:36
> -    , m_context(context ? context : new QWKContext(this))
> +    , m_context(context ? context : new QWKContext(this, QWKContext::SharedProcess))

I think we should test the shared context mode a lot more before making it the default.
The rest of this patch is fine, but you should add something like a CLI option to MiniBrowser to choose the mode.

[Balazs Kelemen]

Created attachment 79731 [details]
Patch

[Andreas Kling]

Comment on attachment 79731 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qwkcontext.h:38
> +    QWKContext(const QWKContext&);

QObject subclasses should never have copy constructors.
You'll need to accomplish this in some other way.

[Balazs Kelemen]

Discussed on IRC that this API change is not correct and not necessary 
at this time. With the current API the user can take care about sharing
the web process between pages by instantiating the pages with the same QWKContext.

[Balazs Kelemen]

New bug with patch that affects only MiniBrowser to be able to use and test
the shared process model: https://bugs.webkit.org/show_bug.cgi?id=53090