Bug 92676

Summary:

[WK2] Kill the concept of secondary shared process

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

WebKit2

Assignee:

Alexey Proskuryakov <ap>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, cdumez, cgarcia, cmarcelo, menard, rafael.lobo, rniwa, ryuan.choi, sam, webkit.review.bot, zoltan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
proposed patch	sam: review+
Patch	ap: review+
MiniBrowser fix	none

Alexey Proskuryakov

Reported 2012-07-30 14:02:52 PDT

A client can build a singleton just as easily on top of WKCreateContext. In fact, both clients that use WKContextGetSharedProcessContext already wrap it in their own singleton!

Attachments
proposed patch (5.05 KB, patch) 2012-07-30 14:06 PDT, Alexey Proskuryakov	sam: review+	Details Formatted Diff Diff
Patch (1.57 KB, patch) 2012-07-30 16:15 PDT, Rafael Brandao	ap: review+	Details Formatted Diff Diff
MiniBrowser fix (1.30 KB, patch) 2012-07-30 21:33 PDT, Alexey Proskuryakov	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2012-07-30 14:06:07 PDT

Created attachment 155358 [details] proposed patch

Alexey Proskuryakov

Comment 2 2012-07-30 14:31:28 PDT

Committed <http://trac.webkit.org/changeset/124092>.

Rafael Brandao

Comment 3 2012-07-30 16:14:58 PDT

Reopening to attach new patch.

Rafael Brandao

Comment 4 2012-07-30 16:15:04 PDT

Created attachment 155393 [details] Patch This patch broke qt build. Could you double check if this is the proper fix?

Alexey Proskuryakov

Comment 5 2012-07-30 16:25:44 PDT

Comment on attachment 155393 [details] Patch Yes. Apologies for breaking the build - I grepped for API usage, but didn't realize that Qt used WK2 internals directly.

Rafael Brandao

Comment 6 2012-07-30 16:40:37 PDT

This patch was committed in <http://trac.webkit.org/changeset/124114>, yet that link gives me a "No changeset 124114 in the repository". :-(

Ryosuke Niwa

Comment 7 2012-07-30 19:44:05 PDT

Windows build fix landed in http://trac.webkit.org/changeset/124153.

Alexey Proskuryakov

Comment 8 2012-07-30 20:18:35 PDT

The Windows fix is not correct - WKContextGetSharedProcessContext should be replaced with WKContextCreate. Sorry, I cannot land an updated one right now. WKContextGetSharedThreadContext may or may not work at all, but it's definitely not the regular mode for running MiniBrowser.

Ryosuke Niwa

Comment 9 2012-07-30 20:22:34 PDT

(In reply to comment #8) > The Windows fix is not correct - WKContextGetSharedProcessContext should be replaced with WKContextCreate. Sorry, I cannot land an updated one right now. Okay, fixed in http://trac.webkit.org/changeset/124165.

Alexey Proskuryakov

Comment 10 2012-07-30 20:31:19 PDT

I looked at this function more carefully, and while it's correct that WKContextGetSharedProcessContext should be replaced with WKContextCreate, it's not quite that easy. We need to return a singleton object, so the result of WKContextCreate should be put into a static. Creating a new context for every view will likely work at the first glance, but wouldn't be the normal mode either. My apologies for an incomplete advice.

Ryosuke Niwa

Comment 11 2012-07-30 20:33:27 PDT

(In reply to comment #10) > I looked at this function more carefully, and while it's correct that WKContextGetSharedProcessContext should be replaced with WKContextCreate, it's not quite that easy. We need to return a singleton object, so the result of WKContextCreate should be put into a static. > > Creating a new context for every view will likely work at the first glance, but wouldn't be the normal mode either. Okay. Would you be able to fix sometime soon or should we roll patches out?

Alexey Proskuryakov

Comment 12 2012-07-30 20:35:18 PDT

Since the build is not broken any more, it seems fine to leave as is for a little while.

Alexey Proskuryakov

Comment 13 2012-07-30 21:32:57 PDT

Re-opening to re-fix Windows MiniBrowser.

Alexey Proskuryakov

Comment 14 2012-07-30 21:33:47 PDT

Created attachment 155435 [details] MiniBrowser fix

Rafael Brandao

Comment 15 2012-07-30 22:39:50 PDT

Comment on attachment 155435 [details] MiniBrowser fix I think we can apply the same logic for Qt's previous usage of WebContext::sharedProcessContext. I'm wondering if it would be a good idea to modify this API and use something like WebContext::instance() instead like we usually do for singletons.

WebKit Review Bot

Comment 16 2012-07-31 10:43:01 PDT

Comment on attachment 155435 [details] MiniBrowser fix Clearing flags on attachment: 155435 Committed r124220: <http://trac.webkit.org/changeset/124220>

Note You need to log in before you can comment on or make changes to this bug.