Bug 92676 - [WK2] Kill the concept of secondary shared process
Summary: [WK2] Kill the concept of secondary shared process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-30 14:02 PDT by Alexey Proskuryakov
Modified: 2012-07-31 10:50 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 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!
Comment 1 Alexey Proskuryakov 2012-07-30 14:06:07 PDT
Created attachment 155358 [details]
proposed patch
Comment 2 Alexey Proskuryakov 2012-07-30 14:31:28 PDT
Committed <http://trac.webkit.org/changeset/124092>.
Comment 3 Rafael Brandao 2012-07-30 16:14:58 PDT
Reopening to attach new patch.
Comment 4 Rafael Brandao 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?
Comment 5 Alexey Proskuryakov 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.
Comment 6 Rafael Brandao 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". :-(
Comment 7 Ryosuke Niwa 2012-07-30 19:44:05 PDT
Windows build fix landed in http://trac.webkit.org/changeset/124153.
Comment 8 Alexey Proskuryakov 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.
Comment 9 Ryosuke Niwa 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.
Comment 10 Alexey Proskuryakov 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.
Comment 11 Ryosuke Niwa 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?
Comment 12 Alexey Proskuryakov 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.
Comment 13 Alexey Proskuryakov 2012-07-30 21:32:57 PDT
Re-opening to re-fix Windows MiniBrowser.
Comment 14 Alexey Proskuryakov 2012-07-30 21:33:47 PDT
Created attachment 155435 [details]
MiniBrowser fix
Comment 15 Rafael Brandao 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.
Comment 16 WebKit Review Bot 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>