Bug 199917 - Add SPI _WKProcessPoolConfiguration.javaScriptConfigurationDirectory
Summary: Add SPI _WKProcessPoolConfiguration.javaScriptConfigurationDirectory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-18 14:53 PDT by Alex Christensen
Modified: 2019-07-22 11:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.79 KB, patch)
2019-07-18 14:55 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.38 KB, patch)
2019-07-18 15:57 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.87 KB, patch)
2019-07-19 09:42 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (7.82 KB, patch)
2019-07-19 17:11 PDT, Alex Christensen
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-07-18 14:53:08 PDT
Add SPI _WKWebsiteDataStoreConfiguration.javaScriptConfigurationDirectory
Comment 1 Alex Christensen 2019-07-18 14:55:11 PDT
Created attachment 374419 [details]
Patch
Comment 2 Alex Christensen 2019-07-18 15:57:17 PDT
Created attachment 374423 [details]
Patch
Comment 3 Sam Weinig 2019-07-18 16:33:13 PDT
Is it currently possible to have two WKWebView's with different WKWebSiteDataStores using the same WebProcess? If so, does this work in that case?
Comment 4 Alex Christensen 2019-07-18 16:34:57 PDT
I don't think it's possible any more, but even if it is, this is only used for some internal engineers so I don't think this SPI is a big deal.
Comment 5 Alex Christensen 2019-07-18 16:37:11 PDT
(I'm trying to keep the status quo as much as possible while giving a client the ability to move away from WKContextCreateWithInjectedBundlePath)
Comment 6 Sam Weinig 2019-07-19 07:20:13 PDT
(In reply to Alex Christensen from comment #4)
> I don't think it's possible any more, but even if it is, this is only used
> for some internal engineers so I don't think this SPI is a big deal.

Hm. Seems like it should be possible. What happens if you have a process pool created with _WKProcessPoolConfiguration.usesSingleWebProcess = YES, and then have two WKWebViews. 

Would it be acceptable to the client to have this be true for all processes in a process pool? If so, can it be moved to _WKProcessPoolConfiguration? Or would that break service workers?

Either way, seems worth testing what happens in the shared web process case.
Comment 7 Alex Christensen 2019-07-19 08:54:59 PDT
Those are existing problems. I agree that this SPI isn't in the perfect place, but that's not the problem I'm trying to solve right now. Right now I'm trying to solve the problem that we can't migrate from WKContextCreateWithInjectedBundlePath. I'm trying to make that transition as close to no change as possible to minimize risk of regressions when making the switch. Once the switch has been made, we can move the location of this SPI in a smaller, easier-to-review patch. I will add a FIXME comment indicating my intent.
Comment 8 Alex Christensen 2019-07-19 09:42:55 PDT
Created attachment 374470 [details]
Patch
Comment 9 Sam Weinig 2019-07-19 10:15:38 PDT
(In reply to Alex Christensen from comment #7)
> Those are existing problems. I agree that this SPI isn't in the perfect
> place, but that's not the problem I'm trying to solve right now. Right now
> I'm trying to solve the problem that we can't migrate from
> WKContextCreateWithInjectedBundlePath. I'm trying to make that transition as
> close to no change as possible to minimize risk of regressions when making
> the switch. Once the switch has been made, we can move the location of this
> SPI in a smaller, easier-to-review patch. I will add a FIXME comment
> indicating my intent.

Given this is SPI you don't want anyone to use except the clients you are trying to move off the C SPI, I suggest giving it a name that explains that and putting the FIXME comment in the SPI header. That said, if you are going to ask the clients to move to another SPI in the future, it might make sense to make the change now, so they only have to make the change once.

That said, I still think you should be testing the shared web process case.
Comment 10 Alex Christensen 2019-07-19 17:11:00 PDT
Created attachment 374526 [details]
Patch
Comment 11 Alex Christensen 2019-07-19 17:11:37 PDT
I moved the SPI to _WKProcessPoolConfiguration. Now the shared web process case is not relevant.
Comment 12 Alex Christensen 2019-07-22 10:19:52 PDT
http://trac.webkit.org/r247685
Comment 13 Radar WebKit Bug Importer 2019-07-22 10:24:46 PDT
<rdar://problem/53403898>
Comment 14 Alex Christensen 2019-07-22 11:12:44 PDT
All the tests were looking for (and not finding) the default JSC config file until http://trac.webkit.org/r247689